Bug 33812 - Need way to be notified when a page changes its favicon
Summary: Need way to be notified when a page changes its favicon
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Dave Moore
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-18 14:20 PST by Dave Moore
Modified: 2010-04-22 13:04 PDT (History)
12 users (show)

See Also:


Attachments
Patch to implement (24.01 KB, patch)
2010-01-18 14:40 PST, Dave Moore
no flags Details | Formatted Diff | Diff
Patch (24.02 KB, patch)
2010-01-18 14:58 PST, Dave Moore
eric: review-
Details | Formatted Diff | Diff
Fixes builds (33.51 KB, patch)
2010-01-20 07:38 PST, Dave Moore
no flags Details | Formatted Diff | Diff
Patch to implement (33.19 KB, patch)
2010-01-20 09:53 PST, Dave Moore
no flags Details | Formatted Diff | Diff
Patch to implement (33.19 KB, patch)
2010-01-20 15:22 PST, Dave Moore
no flags Details | Formatted Diff | Diff
Fixes style and gtk failures (33.51 KB, patch)
2010-01-21 18:34 PST, Dave Moore
no flags Details | Formatted Diff | Diff
Should fix qt build failure (37.33 KB, patch)
2010-01-25 09:31 PST, Dave Moore
dglazkov: review-
Details | Formatted Diff | Diff
Patch to implement (30.05 KB, patch)
2010-03-10 17:48 PST, Dave Moore
no flags Details | Formatted Diff | Diff
Fixed build failures (39.58 KB, patch)
2010-03-11 08:53 PST, Dave Moore
no flags Details | Formatted Diff | Diff
Patch to implement (29.06 KB, patch)
2010-04-10 14:06 PDT, Dave Moore
no flags Details | Formatted Diff | Diff
Removed stupid Ctrl-Ms (29.06 KB, patch)
2010-04-10 14:23 PDT, Dave Moore
no flags Details | Formatted Diff | Diff
Fix chromium error (30.37 KB, patch)
2010-04-10 16:41 PDT, Dave Moore
dglazkov: review-
Details | Formatted Diff | Diff
Patch to fix idl issue (30.26 KB, patch)
2010-04-16 10:22 PDT, Dave Moore
no flags Details | Formatted Diff | Diff
Skip test on other platforms (32.35 KB, patch)
2010-04-16 10:45 PDT, Dave Moore
no flags Details | Formatted Diff | Diff
Fix expectations syntax error (32.36 KB, patch)
2010-04-16 11:34 PDT, Dave Moore
no flags Details | Formatted Diff | Diff
Fix chromium error (31.62 KB, patch)
2010-04-16 13:41 PDT, Dave Moore
no flags Details | Formatted Diff | Diff
Fixed test_expectations collision (31.81 KB, patch)
2010-04-18 08:07 PDT, Dave Moore
no flags Details | Formatted Diff | Diff
Another collision (31.80 KB, patch)
2010-04-18 14:20 PDT, Dave Moore
no flags Details | Formatted Diff | Diff
Resubmitting because last version was reverted incorrectly. (31.39 KB, patch)
2010-04-21 10:05 PDT, Dave Moore
dglazkov: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Moore 2010-01-18 14:20:29 PST
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.
Comment 1 Dave Moore 2010-01-18 14:40:40 PST
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.
Comment 2 WebKit Review Bot 2010-01-18 14:52:12 PST
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.
Comment 3 Dave Moore 2010-01-18 14:58:23 PST
Created attachment 46857 [details]
Patch
Comment 4 Dimitri Glazkov (Google) 2010-01-18 20:41:14 PST
Looks sane to me, but I ain't no expert in this area. Adding a few peeps who may be more experty.
Comment 5 WebKit Review Bot 2010-01-18 20:41:28 PST
Attachment 46857 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/200097
Comment 6 WebKit Review Bot 2010-01-18 20:48:59 PST
Attachment 46857 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/198362
Comment 7 WebKit Review Bot 2010-01-18 21:17:21 PST
Attachment 46857 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/200112
Comment 8 Gustavo Noronha (kov) 2010-01-19 11:06:17 PST
(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.
Comment 9 Eric Seidel (no email) 2010-01-19 14:25:08 PST
Comment on attachment 46857 [details]
Patch

We can't land a patch which breaks other builds.
Comment 10 Dave Moore 2010-01-19 14:28:35 PST
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.
Comment 11 Dave Moore 2010-01-20 07:38:51 PST
Created attachment 47030 [details]
Fixes builds
Comment 12 Dave Moore 2010-01-20 09:53:46 PST
Created attachment 47043 [details]
Patch to implement

Removes the set executable on the new files
Comment 13 Dave Moore 2010-01-20 09:58:05 PST
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.
Comment 14 Dave Moore 2010-01-20 15:22:07 PST
Created attachment 47069 [details]
Patch to implement

Uploading again because buildbots were broken.
Comment 15 WebKit Review Bot 2010-01-20 15:24:49 PST
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.
Comment 16 WebKit Review Bot 2010-01-20 20:07:22 PST
Attachment 47069 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/201835
Comment 17 WebKit Review Bot 2010-01-20 20:29:08 PST
Attachment 47069 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/202647
Comment 18 Dave Moore 2010-01-21 18:34:43 PST
Created attachment 47169 [details]
Fixes style and gtk failures
Comment 19 WebKit Review Bot 2010-01-22 01:07:20 PST
Attachment 47169 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/204591
Comment 20 Adam Barth 2010-01-22 01:47:10 PST
> 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.
Comment 21 Adam Barth 2010-01-22 01:58:16 PST
(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.
Comment 22 Dave Moore 2010-01-25 09:31:06 PST
Created attachment 47351 [details]
Should fix qt build failure
Comment 23 Dimitri Glazkov (Google) 2010-01-25 12:35:09 PST
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[davemoore@chromium.org] 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.
Comment 24 Dimitri Glazkov (Google) 2010-01-25 12:35:58 PST
+aroben, can you look over the WebKit/win parts? I know nothing about that neck of the woods.
Comment 25 Adam Roben (:aroben) 2010-01-25 15:21:47 PST
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.
Comment 26 Adam Roben (:aroben) 2010-01-25 15:22:30 PST
CCing Tim and Brady. Hopefully they can comment on the new API and its implementation.
Comment 27 Brady Eidson 2010-01-25 15:41:14 PST
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.
Comment 28 Dave Moore 2010-03-10 17:48:25 PST
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.
Comment 29 WebKit Review Bot 2010-03-10 17:52:18 PST
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.
Comment 30 WebKit Review Bot 2010-03-10 18:28:40 PST
Attachment 50460 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/597004
Comment 31 Dave Moore 2010-03-11 08:53:57 PST
Created attachment 50509 [details]
Fixed build failures
Comment 32 WebKit Review Bot 2010-03-31 16:16:48 PDT
Attachment 50509 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/1614130
Comment 33 Dave Moore 2010-04-10 14:06:03 PDT
Created attachment 53052 [details]
Patch to implement
Comment 34 WebKit Review Bot 2010-04-10 14:12:28 PDT
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.
Comment 35 Dave Moore 2010-04-10 14:23:58 PDT
Created attachment 53054 [details]
Removed stupid Ctrl-Ms
Comment 36 WebKit Review Bot 2010-04-10 14:28:40 PDT
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.
Comment 37 WebKit Review Bot 2010-04-10 14:47:06 PDT
Attachment 53052 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1663283
Comment 38 WebKit Review Bot 2010-04-10 15:11:53 PDT
Attachment 53054 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1590423
Comment 39 Dave Moore 2010-04-10 16:41:04 PDT
Created attachment 53059 [details]
Fix chromium error
Comment 40 WebKit Review Bot 2010-04-10 16:45:22 PDT
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.
Comment 41 Dimitri Glazkov (Google) 2010-04-16 09:12:42 PDT
Comment on attachment 53059 [details]
Fix chromium error

Dave is moving the call to IWebFrameLoadDelegatePrivate2.idl.
Comment 42 Dave Moore 2010-04-16 10:22:37 PDT
Created attachment 53534 [details]
Patch to fix idl issue
Comment 43 WebKit Review Bot 2010-04-16 10:25:59 PDT
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.
Comment 44 Dave Moore 2010-04-16 10:45:35 PDT
Created attachment 53536 [details]
Skip test on other platforms
Comment 45 WebKit Review Bot 2010-04-16 10:48:17 PDT
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.
Comment 46 WebKit Review Bot 2010-04-16 11:18:41 PDT
Attachment 53534 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1731012
Comment 47 Dave Moore 2010-04-16 11:34:55 PDT
Created attachment 53545 [details]
Fix expectations syntax error
Comment 48 WebKit Review Bot 2010-04-16 11:39:50 PDT
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.
Comment 49 WebKit Review Bot 2010-04-16 12:09:18 PDT
Attachment 53536 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1710150
Comment 50 Dave Moore 2010-04-16 13:41:36 PDT
Created attachment 53559 [details]
Fix chromium error
Comment 51 WebKit Review Bot 2010-04-16 13:43:24 PDT
Attachment 53545 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1658442
Comment 52 WebKit Review Bot 2010-04-16 13:48:15 PDT
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.
Comment 53 Dimitri Glazkov (Google) 2010-04-16 15:56:38 PDT
Comment on attachment 53559 [details]
Fix chromium error

ok.
Comment 54 WebKit Commit Bot 2010-04-16 18:48:30 PDT
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
Comment 55 Dimitri Glazkov (Google) 2010-04-16 19:11:19 PDT
Dave, it looks like test_expectations.txt got stale. Can you rebase the patch and re-upload?
Comment 56 Dave Moore 2010-04-18 08:07:54 PDT
Created attachment 53628 [details]
Fixed test_expectations collision
Comment 57 WebKit Review Bot 2010-04-18 08:09:51 PDT
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.
Comment 58 WebKit Commit Bot 2010-04-18 08:34:06 PDT
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
Comment 59 Dimitri Glazkov (Google) 2010-04-18 08:45:47 PDT
:(
Comment 60 Dave Moore 2010-04-18 14:20:55 PDT
Created attachment 53636 [details]
Another collision
Comment 61 WebKit Review Bot 2010-04-18 14:29:55 PDT
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.
Comment 62 Dimitri Glazkov (Google) 2010-04-18 15:07:48 PDT
Comment on attachment 53636 [details]
Another collision

pretty, please, cq?!
Comment 63 Eric Seidel (no email) 2010-04-18 15:15:52 PDT
Are the style errors false positives?  Do we need to fix the style checker?
Comment 64 WebKit Commit Bot 2010-04-18 15:25:44 PDT
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
Comment 65 Eric Seidel (no email) 2010-04-18 15:27:35 PDT
The tools are probably failing on the \r.
Comment 66 Dimitri Glazkov (Google) 2010-04-18 16:01:50 PDT
I will land by hand tomorrow.
Comment 67 Eric Seidel (no email) 2010-04-18 16:27:36 PDT
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.
Comment 68 Eric Seidel (no email) 2010-04-18 16:27:44 PDT
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.
Comment 69 Dimitri Glazkov (Google) 2010-04-19 11:50:51 PDT
Landed as http://trac.webkit.org/changeset/57823.
Comment 70 Eric Seidel (no email) 2010-04-19 12:00:24 PDT
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'
Comment 71 Dimitri Glazkov (Google) 2010-04-19 12:15:55 PDT
(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.
Comment 72 Mark Rowe (bdash) 2010-04-19 17:10:38 PDT
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.
Comment 73 Mark Rowe (bdash) 2010-04-19 17:17:00 PDT
Rolled out in r57856.  Reopening.
Comment 74 Mark Rowe (bdash) 2010-04-19 17:18:55 PDT
Comment on attachment 53636 [details]
Another collision

Setting patch to r- for reasons noted above.
Comment 75 Dimitri Glazkov (Google) 2010-04-20 08:42:16 PDT
(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.
Comment 76 Adam Roben (:aroben) 2010-04-20 08:45:27 PDT
(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.
Comment 77 Dimitri Glazkov (Google) 2010-04-20 08:55:39 PDT
(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?
Comment 78 Adam Roben (:aroben) 2010-04-20 14:34:57 PDT
(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!
Comment 79 Dimitri Glazkov (Google) 2010-04-20 14:41:24 PDT
Comment on attachment 53636 [details]
Another collision

great news! thanks!
Comment 80 Dimitri Glazkov (Google) 2010-04-20 14:43:41 PDT
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?
Comment 81 Dave Moore 2010-04-21 10:05:17 PDT
Created attachment 53967 [details]
Resubmitting because last version was reverted incorrectly.
Comment 82 Dimitri Glazkov (Google) 2010-04-21 10:08:49 PDT
Comment on attachment 53967 [details]
Resubmitting because last version was reverted incorrectly.

cq should be able to handle this now.
Comment 83 WebKit Commit Bot 2010-04-21 19:28:21 PDT
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
Comment 84 Dimitri Glazkov (Google) 2010-04-21 19:31:21 PDT
Boogers. I will land by hand tomorrow.
Comment 85 Dimitri Glazkov (Google) 2010-04-22 13:04:48 PDT
(In reply to comment #84)
> Boogers. I will land by hand tomorrow.

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