Bug 33812 – Need way to be notified when a page changes its favicon

• Component: DOM
• Status: RESOLVED FIXED

[Dave Moore]

A page can change its favicon by setting link tag of this format:
<link rel="shortcut icon" type="image/x-icon" href="http://test.com/newfavicon.ico"/>

Javascript can be used to change this link. This will cause Document::setIconURL() to be called. But it never notifies the frame loader about it, as is done when the title is changed. There should be a notification sent that will allow a containing application to update it's UI when this happens.

[Dave Moore]

Created attachment 46853 [details]
Patch to implement

I'm aware that this patch only works on Windows. I don't have a Mac to test / implement on. I would like to get a review of what I have so that I can be confident that I'm not wasting someone's time when I ask them to help with the implementations on other platforms.

[WebKit Review Bot]

Attachment 46853 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/loader/DocumentLoader.cpp:612:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/loader/DocumentLoader.cpp:615:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/loader/DocumentLoader.cpp:619:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKitTools/WinLauncher/WinLauncher.h:64:  Extra space after ( in function call  [whitespace/parens] [4]
WebKitTools/DumpRenderTree/win/FrameLoadDelegate.cpp:192:  Extra space after ( in function call  [whitespace/parens] [4]
WebKitTools/DumpRenderTree/win/FrameLoadDelegate.cpp:193:  Declaration has space between type name and * in IWebView *webView  [whitespace/declaration] [3]
WebKitTools/DumpRenderTree/win/FrameLoadDelegate.cpp:195:  Declaration has space between type name and * in IWebFrame *frame  [whitespace/declaration] [3]
WebKitTools/DumpRenderTree/win/FrameLoadDelegate.h:75:  Extra space after ( in function call  [whitespace/parens] [4]
Total errors found: 8


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Moore]

Created attachment 46857 [details]
Patch

[Dimitri Glazkov (Google)]

Looks sane to me, but I ain't no expert in this area. Adding a few peeps who may be more experty.

[WebKit Review Bot]

Attachment 46857 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/200097

[WebKit Review Bot]

Attachment 46857 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/198362

[WebKit Review Bot]

Attachment 46857 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/200112

[Gustavo Noronha (kov)]

(In reply to comment #1)
> Created an attachment (id=46853) [details]
> Patch to implement
> 
> I'm aware that this patch only works on Windows. I don't have a Mac to test /
> implement on. I would like to get a review of what I have so that I can be
> confident that I'm not wasting someone's time when I ask them to help with the
> implementations on other platforms.

You don't need to implement the methods on all platforms, but you need to provide empty implementations for all FrameLoaderClient implementations, so that all ports keep building when your patch lands.

[Eric Seidel (no email)]

Comment on attachment 46857 [details]
Patch

We can't land a patch which breaks other builds.

[Dave Moore]

I'm not asking to have it landed yet. I wanted an initial opinion about the approach before I did the work (or got it done by someone else) to make it work on all platforms. Before it is to be landed it will at least have empty implementations that don't break the builds.

[Dave Moore]

Created attachment 47030 [details]
Fixes builds

[Dave Moore]

Created attachment 47043 [details]
Patch to implement

Removes the set executable on the new files

[Dave Moore]

I think this is ready to land now. I've implemented the notification in the same way as the title change notification is done...there is a willChange..., a didChange... and a didReceive... method in FrameLoaderClient which can be used to notify the wrapper. In chromium we do that. Doing it on other platforms is left to future work if it's ever needed.

I've implemented the new methods from the FrameLoaderClient interface in all places. I believe the builds will work. It's been tested on mac, win, chromium.

[Dave Moore]

Created attachment 47069 [details]
Patch to implement

Uploading again because buildbots were broken.

[WebKit Review Bot]

Attachment 47069 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:848:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:848:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 47069 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/201835

[WebKit Review Bot]

Attachment 47069 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/202647

[Dave Moore]

Created attachment 47169 [details]
Fixes style and gtk failures

[WebKit Review Bot]

Attachment 47169 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/204591

[Adam Barth]

> Build output: http://webkit-commit-queue.appspot.com/results/204591

I don't know why this results link is blank.  We've seen this bug a couple times.  I'll try to investigate it tomorrow.

[Adam Barth]

(In reply to comment #20)
> > Build output: http://webkit-commit-queue.appspot.com/results/204591
> 
> I don't know why this results link is blank.  We've seen this bug a couple
> times.  I'll try to investigate it tomorrow.

Interesting.  Both times it got a 500 from AppEngine and had to re-try the upload.  I bet the log was too big.

[Dave Moore]

Created attachment 47351 [details]
Should fix qt build failure

[Dimitri Glazkov (Google)]

Comment on attachment 47351 [details]
Should fix qt build failure

Lots of comments in the first pass. Welcome to WebKit-land :)

> +    if (Frame* f = frame())
> +        f->loader()->setIconURL(m_iconURL);

In WebKit, we try to use descriptive variable names: Frame* frame = this->frame()

> +    if (loader == m_documentLoader) {
> +        m_client->dispatchDidReceiveIconURL(loader->iconURL());
> +        // TODO[[e-mail]] How do we update the icons for history?

TODO[...] => FIXME:


> +void FrameLoaderClientImpl::willChangeIconURL(DocumentLoader*)
> +{
> +    // FIXME

A bit more comment here?

> +}
> +
> +void FrameLoaderClientImpl::didChangeIconURL(DocumentLoader*)
> +{
> +    // FIXME
> +}

Ditto.

> Index: WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp
> ===================================================================
> --- WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp	(revision 53780)
> +++ WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp	(working copy)
> @@ -749,6 +749,11 @@ void FrameLoaderClient::dispatchDidRecei
>      }
>  }
>  
> +void FrameLoaderClient::dispatchDidReceiveIconURL(const String& iconURL)
> +{
> +    notImplemented();

Why is it notImplemented() here and FIXME in other ports?

>  
> +void FrameLoaderClient::willChangeIconURL(WebCore::DocumentLoader*)
> +{
> +    notImplemented();

Ditto.

> +void FrameLoaderClient::didChangeIconURL(WebCore::DocumentLoader*)
> +{
> +    // FIXME implement to update favicon

Colon after FIXME and make it a sentence.

FIXME: Implement to update favicon.

> +void WebFrameLoaderClient::dispatchDidReceiveIconURL(const String& iconURL)

I think you should omit parameter name here -- otherwise, compiler will yell at you.

> +void WebFrameLoaderClient::willChangeIconURL(DocumentLoader* loader)

Ditto.

> +{
> +    // FIXME: Implement to send notification if icon url is about to change
> +}
> +
> +void WebFrameLoaderClient::didChangeIconURL(DocumentLoader* loader)

Ditto.

>  
>      /*!
> +        @method webView:didReceiveIconURL:forFrame:
> +        @abstract Notifies the delegate that the page icon URL for a frame has been received
> +        @param webView The WebView sending the message
> +        @param iconURL The new icon url
> +        @param frame The frame for which the icon URL has been received
> +        - (void)webView:(WebView *)sender didReceiveIconURL:(NSString *)iconURL forFrame:(WebFrame *)frame;
> +    */
> +    HRESULT didReceiveIconURL([in] IWebView* webView, [in] BSTR iconURL, [in] IWebFrame* frame);
> +
> +    /*!

I have no clue about this parts of WebKit. Need aroben to look.


Also, you should modify platform/(mac|qt|gtk)/Skipped, if you're not planning to implement DRT changes for them.

[Dimitri Glazkov (Google)]

+aroben, can you look over the WebKit/win parts? I know nothing about that neck of the woods.

[Adam Roben (:aroben)]

Comment on attachment 47351 [details]
Should fix qt build failure

> +++ WebKit/win/Interfaces/IWebFrameLoadDelegate.idl	(working copy)
> @@ -114,6 +114,16 @@ interface IWebFrameLoadDelegate : IUnkno
>      HRESULT didReceiveTitle([in] IWebView* webView, [in] BSTR title, [in] IWebFrame* frame);
>  
>      /*!
> +        @method webView:didReceiveIconURL:forFrame:
> +        @abstract Notifies the delegate that the page icon URL for a frame has been received
> +        @param webView The WebView sending the message
> +        @param iconURL The new icon url
> +        @param frame The frame for which the icon URL has been received
> +        - (void)webView:(WebView *)sender didReceiveIconURL:(NSString *)iconURL forFrame:(WebFrame *)frame;
> +    */
> +    HRESULT didReceiveIconURL([in] IWebView* webView, [in] BSTR iconURL, [in] IWebFrame* frame);
> +
> +    /*!
>          @method webView:didReceiveIcon:forFrame:
>          @abstract Notifies the delegate that a page icon image for a frame has been received
>          @param webView The WebView sending the message

This will break nightly builds of WebKit. (New delegate functions can't be added to interfaces that existed when Safari/iTunes last shipped.) You need to add this function to the end of the IWebFrameLoadDelegatePrivate2 interface.

[Adam Roben (:aroben)]

CCing Tim and Brady. Hopefully they can comment on the new API and its implementation.

[Brady Eidson]

There's a couple of shortcomings with the work that's been done here.

HTML 5 specifies support for multiple icons for a given document.  See http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#rel-icon for details and examples.

WebKit already supports this to some degree, and we need to make the support (much) better in the future.

The API probably needs to pass along size and mime-type information with the URL.

Therefore, the client method name "didReceiveIconURL" is a little too specific to the URL portion of the data.

I can't think of a fabulous alternative at the moment.  My gut instinct was "did ReceiveIconData" but I don't really like it for some reason.

The methods "willChangeIconURL" and "didChangeIconURL" also need to be rethought.  If the new <link rel=icon> isn't overriding a previous entry, then nothing "changed"

There might have to be smarts that determine - for the purposes of the client callback - whether the icon information is new, or overriding a previously specified link.

[Dave Moore]

Created attachment 50460 [details]
Patch to implement

I've modified the patch to not include the icon url in the notification. Instead there is just a notification that the icons for the document changed. Agents are free to ignore those notifications. If an agent is interested it can get the updated icon url from the document. This will allow more flexibility as webkit adds additional support for multiple favicons.

[WebKit Review Bot]

Attachment 50460 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/WinLauncher/WinLauncher.h:64:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebCore/loader/FrameLoader.cpp:4034:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 5 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 50460 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/597004

[Dave Moore]

Created attachment 50509 [details]
Fixed build failures

[WebKit Review Bot]

Attachment 50509 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/1614130

[Dave Moore]

Created attachment 53052 [details]
Patch to implement

[WebKit Review Bot]

Attachment 53052 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/WinLauncher/WinLauncher.h:64:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 4 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Moore]

Created attachment 53054 [details]
Removed stupid Ctrl-Ms

[WebKit Review Bot]

Attachment 53054 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/WinLauncher/WinLauncher.h:64:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 4 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 53052 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1663283

[WebKit Review Bot]

Attachment 53054 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1590423

[Dave Moore]

Created attachment 53059 [details]
Fix chromium error

[WebKit Review Bot]

Attachment 53059 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/WinLauncher/WinLauncher.h:64:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 4 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dimitri Glazkov (Google)]

Comment on attachment 53059 [details]
Fix chromium error

Dave is moving the call to IWebFrameLoadDelegatePrivate2.idl.

[Dave Moore]

Created attachment 53534 [details]
Patch to fix idl issue

[WebKit Review Bot]

Attachment 53534 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/WinLauncher/WinLauncher.h:64:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 4 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Moore]

Created attachment 53536 [details]
Skip test on other platforms

[WebKit Review Bot]

Attachment 53536 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/WinLauncher/WinLauncher.h:64:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 4 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 53534 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1731012

[Dave Moore]

Created attachment 53545 [details]
Fix expectations syntax error

[WebKit Review Bot]

Attachment 53545 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/WinLauncher/WinLauncher.h:64:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 4 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 53536 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1710150

[Dave Moore]

Created attachment 53559 [details]
Fix chromium error

[WebKit Review Bot]

Attachment 53545 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1658442

[WebKit Review Bot]

Attachment 53559 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/WinLauncher/WinLauncher.h:64:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 4 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dimitri Glazkov (Google)]

Comment on attachment 53559 [details]
Fix chromium error

ok.

[WebKit Commit Bot]

Comment on attachment 53559 [details]
Fix chromium error

Rejecting patch 53559 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Dimitri Glazkov', '--force']" exit_code: 1
Last 500 characters of output:
eLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/dom/icon-url-property-expected.txt
patching file LayoutTests/fast/dom/icon-url-property.html
patching file LayoutTests/platform/chromium/test_expectations.txt
Hunk #1 FAILED at 2792.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej
patching file LayoutTests/platform/gtk/Skipped
patching file LayoutTests/platform/mac/Skipped
patching file LayoutTests/platform/qt/Skipped

Full output: http://webkit-commit-queue.appspot.com/results/1586512

[Dimitri Glazkov (Google)]

Dave, it looks like test_expectations.txt got stale. Can you rebase the patch and re-upload?

[Dave Moore]

Created attachment 53628 [details]
Fixed test_expectations collision

[WebKit Review Bot]

Attachment 53628 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/WinLauncher/WinLauncher.h:64:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 4 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 53628 [details]
Fixed test_expectations collision

Rejecting patch 53628 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Dimitri Glazkov', '--force']" exit_code: 1
Last 500 characters of output:
s to file WebKitTools/WinLauncher/WinLauncher.h.rej
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/dom/icon-url-property-expected.txt
patching file LayoutTests/fast/dom/icon-url-property.html
patching file LayoutTests/platform/chromium/test_expectations.txt
patching file LayoutTests/platform/gtk/Skipped
patching file LayoutTests/platform/mac/Skipped
patching file LayoutTests/platform/qt/Skipped
Hunk #1 succeeded at 2788 (offset -10 lines).

Full output: http://webkit-commit-queue.appspot.com/results/1614616

[Dimitri Glazkov (Google)]

:(

[Dave Moore]

Created attachment 53636 [details]
Another collision

[WebKit Review Bot]

Attachment 53636 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/WinLauncher/WinLauncher.h:64:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 4 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dimitri Glazkov (Google)]

Comment on attachment 53636 [details]
Another collision

pretty, please, cq?!

[Eric Seidel (no email)]

Are the style errors false positives?  Do we need to fix the style checker?

[WebKit Commit Bot]

Comment on attachment 53636 [details]
Another collision

Rejecting patch 53636 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Dimitri Glazkov', '--force']" exit_code: 1
Last 500 characters of output:
uncher/WinLauncher.h
Hunk #1 FAILED at 61.
1 out of 1 hunk FAILED -- saving rejects to file WebKitTools/WinLauncher/WinLauncher.h.rej
patching file LayoutTests/ChangeLog
patching file LayoutTests/fast/dom/icon-url-property-expected.txt
patching file LayoutTests/fast/dom/icon-url-property.html
patching file LayoutTests/platform/chromium/test_expectations.txt
patching file LayoutTests/platform/gtk/Skipped
patching file LayoutTests/platform/mac/Skipped
patching file LayoutTests/platform/qt/Skipped

Full output: http://webkit-commit-queue.appspot.com/results/1583617

[Eric Seidel (no email)]

The tools are probably failing on the \r.

[Dimitri Glazkov (Google)]

I will land by hand tomorrow.

[Eric Seidel (no email)]

Comment on attachment 53559 [details]
Fix chromium error

Cleared Dimitri Glazkov's review+ from obsolete attachment 53559 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 53628 [details]
Fixed test_expectations collision

Cleared Dimitri Glazkov's review+ from obsolete attachment 53628 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/57823.

[Eric Seidel (no email)]

Looks like this broke Windows:

http://build.webkit.org/builders/Windows%20Debug%20%28Build%29/builds/13910/steps/compile-webkit/logs/stdio

..\WebCoreSupport\WebFrameLoaderClient.cpp(386) : error C2039: 'didChangeIcons' : is not a member of 'IWebFrameLoadDelegatePrivate2'

        C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Include\WebKit/WebKit.h(32233) : see declaration of 'IWebFrameLoadDelegatePrivate2'

[Dimitri Glazkov (Google)]

(In reply to comment #70)
> Looks like this broke Windows:
> 
> http://build.webkit.org/builders/Windows%20Debug%20%28Build%29/builds/13910/steps/compile-webkit/logs/stdio
> 
> ..\WebCoreSupport\WebFrameLoaderClient.cpp(386) : error C2039: 'didChangeIcons'
> : is not a member of 'IWebFrameLoadDelegatePrivate2'
> 
>        
> C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Include\WebKit/WebKit.h(32233)
> : see declaration of 'IWebFrameLoadDelegatePrivate2'

Just needed a force-rebuild. now cooking.

[Mark Rowe (bdash)]

The changes to the Windows IDL files here are incorrect.  You can’t just add methods to delegate interfaces like this.  WebKit determines which methods the delegate implements by querying for each version of the interface.  Existing clients that implement IWebFrameLoadDelegatePrivate2 (e.g., shipping versions of Safari and friends) but do not have this method, so attempts to call it will result in calling a bogus method.  A new IWebFrameLoadDelegatePrivate3 interface needs to be created that inherits from IWebFrameLoadDelegatePrivate2 and contains this extra method.  Then the WebKit code needs to look for that interface when determining whether it can call didChangeIcons.

I’m going to roll this change out in a moment since it causes crashes when WebKit is used with existing clients.

[Mark Rowe (bdash)]

Rolled out in r57856.  Reopening.

[Mark Rowe (bdash)]

Comment on attachment 53636 [details]
Another collision

Setting patch to r- for reasons noted above.

[Dimitri Glazkov (Google)]

(In reply to comment #72)
> The changes to the Windows IDL files here are incorrect.  You can’t just add
> methods to delegate interfaces like this.  WebKit determines which methods the
> delegate implements by querying for each version of the interface.  Existing
> clients that implement IWebFrameLoadDelegatePrivate2 (e.g., shipping versions
> of Safari and friends) but do not have this method, so attempts to call it will
> result in calling a bogus method.  A new IWebFrameLoadDelegatePrivate3
> interface needs to be created that inherits from IWebFrameLoadDelegatePrivate2
> and contains this extra method.  Then the WebKit code needs to look for that
> interface when determining whether it can call didChangeIcons.
> 
> I’m going to roll this change out in a moment since it causes crashes when
> WebKit is used with existing clients.

Weird. I asked Dave to do exactly what aroben suggested in comment https://bugs.webkit.org/show_bug.cgi?id=33812#c25.

[Adam Roben (:aroben)]

(In reply to comment #75)
> (In reply to comment #72)
> > The changes to the Windows IDL files here are incorrect.  You can’t just add
> > methods to delegate interfaces like this.  WebKit determines which methods the
> > delegate implements by querying for each version of the interface.  Existing
> > clients that implement IWebFrameLoadDelegatePrivate2 (e.g., shipping versions
> > of Safari and friends) but do not have this method, so attempts to call it will
> > result in calling a bogus method.  A new IWebFrameLoadDelegatePrivate3
> > interface needs to be created that inherits from IWebFrameLoadDelegatePrivate2
> > and contains this extra method.  Then the WebKit code needs to look for that
> > interface when determining whether it can call didChangeIcons.
> > 
> > I’m going to roll this change out in a moment since it causes crashes when
> > WebKit is used with existing clients.
> 
> Weird. I asked Dave to do exactly what aroben suggested in comment
> https://bugs.webkit.org/show_bug.cgi?id=33812#c25.

When I wrote comment 25, I was under the impression that no shipping WebKit clients used IWebFrameLoadDelegatePrivate2. It's possible that I was incorrect at the time. It's also possible that a client has shipped since comment 25 was written that uses IWebFrameLoadDelegatePrivate2.

[Dimitri Glazkov (Google)]

(In reply to comment #76)
> (In reply to comment #75)
> > (In reply to comment #72)
> > > The changes to the Windows IDL files here are incorrect.  You can’t just add
> > > methods to delegate interfaces like this.  WebKit determines which methods the
> > > delegate implements by querying for each version of the interface.  Existing
> > > clients that implement IWebFrameLoadDelegatePrivate2 (e.g., shipping versions
> > > of Safari and friends) but do not have this method, so attempts to call it will
> > > result in calling a bogus method.  A new IWebFrameLoadDelegatePrivate3
> > > interface needs to be created that inherits from IWebFrameLoadDelegatePrivate2
> > > and contains this extra method.  Then the WebKit code needs to look for that
> > > interface when determining whether it can call didChangeIcons.
> > > 
> > > I’m going to roll this change out in a moment since it causes crashes when
> > > WebKit is used with existing clients.
> > 
> > Weird. I asked Dave to do exactly what aroben suggested in comment
> > https://bugs.webkit.org/show_bug.cgi?id=33812#c25.
> 
> When I wrote comment 25, I was under the impression that no shipping WebKit
> clients used IWebFrameLoadDelegatePrivate2. It's possible that I was incorrect
> at the time. It's also possible that a client has shipped since comment 25 was
> written that uses IWebFrameLoadDelegatePrivate2.

I see. So -- the plan is to:

1) Add new file IWebFrameLoadDelegatePrivate3.idl (with ALL CAPS DON"T CONSUME STUFF FROM HERE, PROMOTE TO PUBLIC FIRST at the top) with this one method in it.
2) Add the idl to Interfaces.vcproj
3) resubmit the patch.

Does that sound about right?

[Adam Roben (:aroben)]

(In reply to comment #77)
> (In reply to comment #76)
> > When I wrote comment 25, I was under the impression that no shipping WebKit
> > clients used IWebFrameLoadDelegatePrivate2. It's possible that I was incorrect
> > at the time. It's also possible that a client has shipped since comment 25 was
> > written that uses IWebFrameLoadDelegatePrivate2.
> 
> I see. So -- the plan is to:
> 
> 1) Add new file IWebFrameLoadDelegatePrivate3.idl (with ALL CAPS DON"T CONSUME
> STUFF FROM HERE, PROMOTE TO PUBLIC FIRST at the top) with this one method in
> it.
> 2) Add the idl to Interfaces.vcproj
> 3) resubmit the patch.
> 
> Does that sound about right?

I just talked with Mark Rowe, and we determined that adding the new function to IWebFrameLoadDelegatePrivate2 should be safe. Mark was (understandably!) confused by a different incarnation of IWebFrameLoadDelegatePrivate2 that was added in r39385 (which has since been merged into IWebFrameLoadDelegate).

So, I think we can re-land the patch as-is. Sorry for the trouble!

[Dimitri Glazkov (Google)]

Comment on attachment 53636 [details]
Another collision

great news! thanks!

[Dimitri Glazkov (Google)]

Comment on attachment 53636 [details]
Another collision

I just remembered that it won't land as-is because of the missing description for WebKit/gtk ChangeLog. David, could you merge back the commit that I made into your local tree, sync it up to trunk and post it here?

[Dave Moore]

Created attachment 53967 [details]
Resubmitting because last version was reverted incorrectly.

[Dimitri Glazkov (Google)]

Comment on attachment 53967 [details]
Resubmitting because last version was reverted incorrectly.

cq should be able to handle this now.

[WebKit Commit Bot]

Comment on attachment 53967 [details]
Resubmitting because last version was reverted incorrectly.

Rejecting patch 53967 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12716 test cases.
fast/dom/icon-url-property.html -> failed

Exiting early after 1 failures. 5676 tests run.
92.86s total testing time

5675 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/1847026

[Dimitri Glazkov (Google)]

Boogers. I will land by hand tomorrow.

[Dimitri Glazkov (Google)]

(In reply to comment #84)
> Boogers. I will land by hand tomorrow.

Landed as http://trac.webkit.org/changeset/58111.