Bug 42433 – [Qt] Enable Web Timing for Qt

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Tony Gentilcore]

Web Timing should be enabled for the Qt port.

This involves the following:
1. Implement http://trac.webkit.org/browser/trunk/WebCore/platform/network/ResourceLoadTiming.h
2. Flip WEB_TIMING flag

[Laszlo Gombos]

Created attachment 73678 [details]
first try

[Tony Gentilcore]

I'm excited to see this getting enabled, but backfilling the times from the network stack means that these attributes report incorrect values with no hint to the user. It also isn't compliant w/ the spec.

Assuming modifying the network stack isn't a reasonable option, I'd recommend one of two approaches:
1. Keep zeroes for these times and just keep the failing tests in the Skipped file.
2. Do what is proposed in https://bugs.webkit.org/show_bug.cgi?id=48923 and make the webcore times enabled by default and just guard the network times with the enable web timing flag. This would result in those properties not being defined at all rather than backfilling or exposing zeroes.

[Laszlo Gombos]

Created attachment 73944 [details]
second-try

Revert changes in Timing.cpp and fail the corresponding test as suggested by Tony Gentilcore.

The intention for QtWebKit is to implement the network related timers as well in a separate patch.

[Tony Gentilcore]

LGTM, thanks! (I'm not a reviewer)

[Andreas Kling]

Comment on attachment 73944 [details]
second-try

View in context: https://bugs.webkit.org/attachment.cgi?id=73944&action=review

LGTM.

> LayoutTests/ChangeLog:9
> +        * fast/dom/webtiming-document-open-expected.txt: Add
> +          webkitPerformance property

Should be "Add an extra line to the expected results"

> LayoutTests/ChangeLog:14
> +        * platform/qt/fast/dom/Window/window-property-descriptors-expected.txt:
> +          Add an extra line to the expected results

Should be "Add webkitPerformance property"

[Laszlo Gombos]

Created attachment 75441 [details]
fixed ChangeLog, uploading for cq to land

Thanks for the review Andreas.

[WebKit Commit Bot]

Comment on attachment 75441 [details]
fixed ChangeLog, uploading for cq to land

Clearing flags on attachment: 75441

Committed r73241: <http://trac.webkit.org/changeset/73241>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #7)
> (From update of attachment 75441 [details])
> Clearing flags on attachment: 75441
> 
> Committed r73241: <http://trac.webkit.org/changeset/73241>

FYI: Unfortunately it broke many tests because modifying a define 
never trigger rebuild. :( I did a clean build on Qt bots manually.

[Laszlo Gombos]

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 75441 [details] [details])
> > Clearing flags on attachment: 75441
> > 
> > Committed r73241: <http://trac.webkit.org/changeset/73241>
> 
> FYI: Unfortunately it broke many tests because modifying a define 
> never trigger rebuild. :( I did a clean build on Qt bots manually.

Thanks Ossy !

[Eric Seidel (no email)]

Seems like we should have a bug on this dependency problem in the Qt build.  Dependency problems cause all sorts of badness since humans have to step in to fix bots, etc.

[Csaba Osztrogonác]

(In reply to comment #11)
> Seems like we should have a bug on this dependency problem in the Qt build.  Dependency problems cause all sorts of badness since humans have to step in to fix bots, etc.

We have a bug about it, but there isn't a good way to solve it:
https://bugs.webkit.org/show_bug.cgi?id=38054
details: https://bugs.webkit.org/show_bug.cgi?id=42027#c26