Bug 75922 – [chromium] Fix navigation start time on cross-renderer navigation

• Component: New Bugs
• Status: RESOLVED FIXED

[James Simonsen]

[chromium] Fix navigation start time on cross-renderer navigation

[James Simonsen]

Created attachment 121776 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Tony Gentilcore]

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.

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

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.

[James Simonsen]

Created attachment 121910 [details]
Patch

[Darin Fisher (:fishd, Google)]

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.

[James Simonsen]

Created attachment 122301 [details]
Patch

[Darin Fisher (:fishd, Google)]

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?

[James Simonsen]

Created attachment 123688 [details]
Patch

[WebKit Review Bot]

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

[Darin Fisher (:fishd, Google)]

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?

[James Simonsen]

Created attachment 129479 [details]
Patch for landing

[WebKit Review Bot]

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

[James Simonsen]

Created attachment 129516 [details]
Patch for landing

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 129516 [details]:

css3/filters/effect-hue-rotate-hw.html bug 79845 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 129516 [details]
Patch for landing

Clearing flags on attachment: 129516

Committed r109300: <http://trac.webkit.org/changeset/109300>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.