Bug 17261 – IconRecord::loadImageFromResource is not called for urlIcon

• Component: New Bugs
• Status: RESOLVED FIXED

[Holger Freyther]

It looks like IconRecord::loadImageFromResource is not called. Either it should be called from IconDatabase to construct the IconRecord for the "iconUrl" or removed.

[Holger Freyther]

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.

[Darin Adler]

Comment on attachment 19022 [details]
Propse to remove the method

r=me

[Mark Rowe (bdash)]

Holger, do you intend to land this any time soon?

[Holger Freyther]

(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).

[Holger Freyther]

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?

[Holger Freyther]

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.

[Darin Adler]

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.

[Holger Freyther]

(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.

[Holger Freyther]

Landed in r30961.

[Holger Freyther]

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.

[Darin Adler]

Reopening since the patch was rolled out.

[Darin Adler]

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

[Mark Rowe (bdash)]

Holger, are you planning on tweaking this and landing it?

[Holger Freyther]

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.

[Holger Freyther]

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.