Bug 42433 - [Qt] Enable Web Timing for Qt
Summary: [Qt] Enable Web Timing for Qt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Enhancement
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 30685
  Show dependency treegraph
 
Reported: 2010-07-15 17:56 PDT by Tony Gentilcore
Modified: 2010-12-03 09:32 PST (History)
7 users (show)

See Also:


Attachments
first try (8.45 KB, patch)
2010-11-11 16:17 PST, Laszlo Gombos
no flags Details | Formatted Diff | Diff
second-try (4.15 KB, patch)
2010-11-15 16:43 PST, Laszlo Gombos
kling: review+
kling: commit-queue-
Details | Formatted Diff | Diff
fixed ChangeLog, uploading for cq to land (4.06 KB, patch)
2010-12-02 17:22 PST, Laszlo Gombos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2010-07-15 17:56:28 PDT
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
Comment 1 Laszlo Gombos 2010-11-11 16:17:16 PST
Created attachment 73678 [details]
first try
Comment 2 Tony Gentilcore 2010-11-11 16:43:32 PST
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.
Comment 3 Laszlo Gombos 2010-11-15 16:43:42 PST
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.
Comment 4 Tony Gentilcore 2010-11-15 16:50:10 PST
LGTM, thanks! (I'm not a reviewer)
Comment 5 Andreas Kling 2010-11-15 16:58:03 PST
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"
Comment 6 Laszlo Gombos 2010-12-02 17:22:58 PST
Created attachment 75441 [details]
fixed ChangeLog, uploading for cq to land

Thanks for the review Andreas.
Comment 7 WebKit Commit Bot 2010-12-02 22:49:29 PST
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>
Comment 8 WebKit Commit Bot 2010-12-02 22:49:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Csaba Osztrogonác 2010-12-03 04:07:59 PST
(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.
Comment 10 Laszlo Gombos 2010-12-03 05:10:54 PST
(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 !
Comment 11 Eric Seidel (no email) 2010-12-03 08:34:32 PST
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.
Comment 12 Csaba Osztrogonác 2010-12-03 09:32:41 PST
(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