Bug 100379 - [Win] WebView didReceiveIcon delegate call doesn't have the icon in it
Summary: [Win] WebView didReceiveIcon delegate call doesn't have the icon in it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-10-25 07:27 PDT by peavo
Modified: 2013-07-23 12:00 PDT (History)
7 users (show)

See Also:


Attachments
Patch as diff, output from svn-create-patch script. (1.22 KB, patch)
2012-10-25 07:29 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (1.84 KB, patch)
2012-10-29 08:14 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (1.89 KB, patch)
2013-06-14 05:01 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (1.89 KB, patch)
2013-07-23 11:17 PDT, peavo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description peavo 2012-10-25 07:27:20 PDT
Pass HBITMAP favicon to load delegate.
Comment 1 peavo 2012-10-25 07:29:33 PDT
Created attachment 170645 [details]
Patch as diff, output from svn-create-patch script.
Comment 2 Alexey Proskuryakov 2012-10-25 10:54:51 PDT
<rdar://problem/5491010>
Comment 3 Alexey Proskuryakov 2012-10-25 10:55:54 PDT
Would you be willing to submit a patch for inclusion in WebKit? Please follow the steps in <http://www.webkit.org/coding/contributing.html> - in particular, every patch needs a ChangeLog, and needs to be marked for review to be in review queue.
Comment 4 peavo 2012-10-26 10:39:05 PDT
Yes, will submit a patch soon.
Comment 5 peavo 2012-10-29 08:14:31 PDT
Created attachment 171249 [details]
Patch
Comment 6 Alexey Proskuryakov 2012-11-05 09:53:36 PST
Comment on attachment 171249 [details]
Patch

Brady should probably review this.
Comment 7 peavo 2012-11-12 05:57:37 PST
Any takers? :)
Comment 8 Brent Fulgham 2013-06-13 10:16:31 PDT
Comment on attachment 171249 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171249&action=review

Looks good.  I think it would be simpler to create one IntSize and use it in both API calls that need it.  Can you make that small change and I'll r+ this?

> Source/WebKit/win/WebView.cpp:2842
> +        sz.cy = 16;

Why not just create the IntSize directly, then use it later?

> Source/WebKit/win/WebView.cpp:2844
> +        BitmapInfo bmInfo = BitmapInfo::create(IntSize(sz));

Use the IntSize here...

> Source/WebKit/win/WebView.cpp:2848
> +        Image* icon = iconDatabase().synchronousIconForPageURL(str, IntSize(sz));

... and here!
Comment 9 peavo 2013-06-14 05:01:16 PDT
Created attachment 204703 [details]
Patch
Comment 10 peavo 2013-06-14 05:02:48 PDT
Modifed patch after review.
Comment 11 peavo 2013-07-23 07:22:04 PDT
(In reply to comment #8)

> Looks good.  I think it would be simpler to create one IntSize and use it in both API calls that need it.  Can you make that small change and I'll r+ this?

r+? ;)
Comment 12 Brent Fulgham 2013-07-23 09:52:01 PDT
Comment on attachment 204703 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204703&action=review

Sorry -- I didn't notice that we would have to cast that sz variable from IntSize to SIZE. Please correct that and I'll r+ it (today!)

> Source/WebKit/win/WebView.cpp:2882
> +            icon->getHBITMAPOfSize(hBitmap, &(SIZE)sz);

This should be written as "&static_cast<SIZE>(sz)"
Comment 13 peavo 2013-07-23 11:17:24 PDT
Created attachment 207339 [details]
Patch
Comment 14 Brent Fulgham 2013-07-23 11:19:48 PDT
r=me
Comment 15 peavo 2013-07-23 11:20:41 PDT
(In reply to comment #14)
> r=me

Thanks for reviewing!
Comment 16 WebKit Commit Bot 2013-07-23 12:00:51 PDT
Comment on attachment 207339 [details]
Patch

Clearing flags on attachment: 207339

Committed r153059: <http://trac.webkit.org/changeset/153059>
Comment 17 WebKit Commit Bot 2013-07-23 12:00:55 PDT
All reviewed patches have been landed.  Closing bug.