yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

> Hi Gary,

Hi Brad.  Thank you for the review.  Replies inline.

> 
> Thanks for the clean up to the Makefile and the addition of the favicon.  I
> hadn't missed it before but think it'll be nice to have.
> 
> I took the liberty of making a card for this task assuming you just forgot.
> Also I couldn't find a Rietveld proposal for this work so I'm reviewing old
> school.

Yes, thank you. Sorry, it was an evening hack that got bigger than I intended.

> 
> Unfortunately, running 'make server' locally I do not see the favicon show up
> in Chromium (and FF is hopelessly broken ATM).  Looking at the downloaded
> resources I see it was served up but it does not show in the address bar.  Are
> you actually seeing it?

Yes.  As I mentioned on IRC, it was not working in debug mode, and I fixed that.  Firefox seemed happier when I was explicit about the ico file also, though that should not be necessary.  I tested it on another computer as well and it seemed ok to me.

> 
> I'm a bit confused by the NODE_TARGETS / EXPECTED_NODE_TARGETS /
> FOUND_NODE_TARGETS dance.  I assume we can't just compute FOUND_NODE_TARGETS
> and use it directly.  If that is really the case could you document exactly
> what is going on lest someone like the Future Me try to undo your work?

Good call.  I added a comment and got your approval on IRC.

> 
> The 'test -d' before 'mkdir -p' is nice but unnecessary ask 'mkdir -p' will
> happily do a no-op and exit with 0 if the directory already exists.  I see it
> used in a few places.

Nice improvement, yes.  Changed.

> 
> Otherwise it looks good and is easier to follow.  Thanks.

Thank you!

Gary
-- 
https://code.launchpad.net/~gary/juju-gui/favicon/+merge/135045
Your team Juju GUI Hackers is subscribed to branch lp:juju-gui.