Bug 42433

Summary: [Qt] Enable Web Timing for Qt
Product: WebKit Reporter: Tony Gentilcore <tonyg>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: benjamin, commit-queue, eric, jchaffraix, laszlo.gombos, ossy, simonjam
Priority: P3 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 30685    
Attachments:
Description Flags
first try
none
second-try
kling: review+, kling: commit-queue-
fixed ChangeLog, uploading for cq to land none

Tony Gentilcore
Reported 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
Attachments
first try (8.45 KB, patch)
2010-11-11 16:17 PST, Laszlo Gombos 		no flags
DetailsFormatted DiffDiff
second-try (4.15 KB, patch)
2010-11-15 16:43 PST, Laszlo Gombos 		kling: review+
kling: commit-queue-
DetailsFormatted DiffDiff
fixed ChangeLog, uploading for cq to land (4.06 KB, patch)
2010-12-02 17:22 PST, Laszlo Gombos 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Laszlo Gombos
Comment 1 2010-11-11 16:17:16 PST
Created attachment 73678 [details] first try
Tony Gentilcore
Comment 2 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.
Laszlo Gombos
Comment 3 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.
Tony Gentilcore
Comment 4 2010-11-15 16:50:10 PST
LGTM, thanks! (I'm not a reviewer)
Andreas Kling
Comment 5 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"
Laszlo Gombos
Comment 6 2010-12-02 17:22:58 PST
Created attachment 75441 [details] fixed ChangeLog, uploading for cq to land Thanks for the review Andreas.
WebKit Commit Bot
Comment 7 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>
WebKit Commit Bot
Comment 8 2010-12-02 22:49:37 PST
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 9 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.
Laszlo Gombos
Comment 10 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 !
Eric Seidel (no email)
Comment 11 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.
Csaba Osztrogonác
Comment 12 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
Note You need to log in before you can comment on or make changes to this bug.