Bug 17261 - IconRecord::loadImageFromResource is not called for urlIcon
Summary: IconRecord::loadImageFromResource is not called for urlIcon
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Holger Freyther
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-02-09 13:33 PST by Holger Freyther
Modified: 2008-08-28 09:08 PDT (History)
0 users

See Also:


Attachments
Propse to remove the method (1.50 KB, patch)
2008-02-09 13:35 PST, Holger Freyther
no flags Details | Formatted Diff | Diff
Allow to use an external urlIcon (6.31 KB, patch)
2008-03-03 18:14 PST, Holger Freyther
no flags Details | Formatted Diff | Diff
Allow the urlIcon to be themable on some ports (7.22 KB, patch)
2008-03-14 09:56 PDT, Holger Freyther
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Holger Freyther 2008-02-09 13:33:33 PST
It looks like IconRecord::loadImageFromResource is not called. Either it should be called from IconDatabase to construct the IconRecord for the "iconUrl" or removed.
Comment 1 Holger Freyther 2008-02-09 13:35:05 PST
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 2 Darin Adler 2008-02-09 22:04:02 PST
Comment on attachment 19022 [details]
Propse to remove the method

r=me
Comment 3 Mark Rowe (bdash) 2008-02-24 22:33:13 PST
Holger, do you intend to land this any time soon?
Comment 4 Holger Freyther 2008-02-25 14:36:27 PST
(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).

Comment 5 Holger Freyther 2008-02-28 20:30:43 PST
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?
Comment 6 Holger Freyther 2008-03-03 18:14:23 PST
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 7 Darin Adler 2008-03-07 12:50:00 PST
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.
Comment 8 Holger Freyther 2008-03-08 03:31:08 PST
(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.
Comment 9 Holger Freyther 2008-03-11 08:40:54 PDT
Landed in r30961.
Comment 10 Holger Freyther 2008-03-14 09:56:57 PDT
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.
Comment 11 Darin Adler 2008-03-16 15:25:51 PDT
Reopening since the patch was rolled out.
Comment 12 Darin Adler 2008-06-08 18:05:30 PDT
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
Comment 13 Mark Rowe (bdash) 2008-07-26 23:05:39 PDT
Holger, are you planning on tweaking this and landing it?
Comment 14 Holger Freyther 2008-08-28 08:57:47 PDT
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.
Comment 15 Holger Freyther 2008-08-28 09:08:18 PDT
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.