Bug 75922

Created attachment 121776 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

Comment on attachment 121776 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121776&action=review LGTM, but leaving to fishd to review. > Source/WebKit/chromium/public/WebFrame.h:362 > + // cross-renderer navigation, the value comes from the previous renderer. This makes the API kind of sad, but I guess it really is that complicated.

Comment on attachment 121776 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121776&action=review >> Source/WebKit/chromium/public/WebFrame.h:362 >> + // cross-renderer navigation, the value comes from the previous renderer. > > This makes the API kind of sad, but I guess it really is that complicated. It seems like this should be a method on WebDataSource instead as it specific to the creation of a document.

Comment on attachment 121776 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121776&action=review > Source/WebKit/chromium/src/WebFrameImpl.cpp:1093 > + FrameLoader* frameLoader = m_frame->loader(); yeah, since your implementation just pokes the DocumentLoader, this clearly needs to be a method on WebDataSource instead.

Created attachment 121910 [details] Patch

Comment on attachment 121910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121910&action=review > Source/WebKit/chromium/public/WebDataSource.h:107 > + // navigation start is determined in FrameLoader. But, in the case of nit: Please avoid referencing WebCore classes in the public APIs. The point of the API is to insulate users of the API from WebCore. Also, someone modifying WebCore (who renames a class in WebCore) will be unlikely to update such comments. Also, WebKit does not know anything about the concept of cross-renderer navigation. Please avoid adding anything to WebKit that is predicated on WebKit knowing about such things. APIs should make sense outside of that context too.

Created attachment 122301 [details] Patch

Comment on attachment 122301 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122301&action=review > Source/WebKit/chromium/public/WebDataSource.h:109 > + virtual void setNavigationStartTime(double) = 0; When is it OK to call this function? inside WebFrameClient::didCreateDataSource? or, what about later on? Will it cause problems to set this later on?

Created attachment 123688 [details] Patch

Comment on attachment 123688 [details] Patch Attachment 123688 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11117720 New failing tests: media/audio-garbage-collect.html

Comment on attachment 123688 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123688&action=review > Source/WebKit/chromium/public/WebDataSource.h:110 > + // Calling it later may confuse users, because Javascript may have run and nit: Javascript -> JavaScript do you want to assert this restriction?

Created attachment 129479 [details] Patch for landing

Comment on attachment 129479 [details] Patch for landing Rejecting attachment 129479 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/11769024

Created attachment 129516 [details] Patch for landing

The commit-queue encountered the following flaky tests while processing attachment 129516 [details]: css3/filters/effect-hue-rotate-hw.html bug 79845 (author: cmarrin@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 129516 [details] Patch for landing Clearing flags on attachment: 129516 Committed r109300: <http://trac.webkit.org/changeset/109300>

All reviewed patches have been landed. Closing bug.