> 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. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
