It looks like IconRecord::loadImageFromResource is not called. Either it should be called from IconDatabase to construct the IconRecord for the "iconUrl" or removed.
Created attachment 19022 [details] Propse to remove the method Propose the remove IconRecord::loadImageFromResource method as I assume the IconDatabase code is right and I couldn't find any other users.
Comment on attachment 19022 [details] Propse to remove the method r=me
Holger, do you intend to land this any time soon?
(In reply to comment #3) > Holger, do you intend to land this any time soon? > Yes I do, there is one thing I want to check first and didn't find the time. I'm flying to Taipei tomorrow so I plan to land it on thursday (after checking that one thing).
Okay, I checked this one thing and have one question. r25512 obsoleted the IconRecord::loadImageFromResource method because the icon got compiled into the binary. This might be a startup increase for OSX but with platforms with icon theme support we might want to ask the platform layer. Does this sound like a case for #ifdef PLATFORM?
Created attachment 19513 [details] Allow to use an external urlIcon In the Qt port we allow to set an external urlIcon and IconDatabase should use that. Inlining and forcing a specific icon is not appropriate for Qt and Gtk+ as both of these toolkits support themable platforms.
Comment on attachment 19513 [details] Allow to use an external urlIcon r=me The code seems fine. There's no ChangeLog here. The comment in the email seems less good. The mention of "closed" vs. "open" and "forcing a specific icon onto the user" seems wrong. The real issue is simply which platforms have a themeable URL icon that we want to use. We chose not to do that on Mac OS X or Windows, and but we'd like to do it for Qt and maybe later for GTK. That's all quite sensible, but as far as I can tell this has nothing to do with closed vs. open or forcing anyone to do anything.
(In reply to comment #7) > (From update of attachment 19513 [details] [edit]) > r=me > > The code seems fine. > > There's no ChangeLog here. The comment in the email seems less good I will come up with a more appropriate ChangeLog entry along the lines of allowing a themable urlIcon for the Qt port. Thanks for reviewing.
Landed in r30961.
Created attachment 19763 [details] Allow the urlIcon to be themable on some ports Okay the Windows code was not completely dead and adam had to back out my previous commit. Change the Windows code to ask the iconDatabase() for the urlIcon. I think this change makes sense as the urlIcon is not themable in the windows platform and there should be only one urlIcon, I opted for the one in the IconDatabase.cpp. It would be appreciated if some one with a windows machine could give it a try.
Reopening since the patch was rolled out.
Comment on attachment 19763 [details] Allow the urlIcon to be themable on some ports I think the #if could be done better. The default icon data could go inside the if statement. The only line that needs to be different is the setImageData/loadImageFromResource line. To make it clear that the tradeoff here is faster load time vs. themability, I think the #if should be named something like CAN_THEME_URL_ICON and be defined at the top of IconDatabase.cpp. #if PLATFORM(QT) #define CAN_THEME_URL_ICON #endif Then later: #ifdef CAN_THEME_URL_ICON Or something along those lines. r=me, as-is
Holger, are you planning on tweaking this and landing it?
Landed in r35970. I might need to change the windows code to use *size instead of size... I will take a look at the buildbot.
I assume the ChangeLog entry was appropriate and I tried to keep the #if at a minimum. One option would have been to move the static image data into the if(). I decided against doing this because this would have meant to re-indent the static data and I think this would have made the diff and the code less readable.