Summary: | [chromium] Fix navigation start time on cross-renderer navigation | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Simonsen <simonjam> | ||||||||||||||
Component: | New Bugs | Assignee: | James Simonsen <simonjam> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | dglazkov, fishd, japhet, tonyg, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
James Simonsen
2012-01-09 18:18:28 PST
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. |