Summary: | [Qt] Enable Web Timing for Qt | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tony Gentilcore <tonyg> | ||||||||
Component: | Page Loading | Assignee: | 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
Tony Gentilcore
2010-07-15 17:56:28 PDT
Created attachment 73678 [details]
first try
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. 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.
LGTM, thanks! (I'm not a reviewer) 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" Created attachment 75441 [details]
fixed ChangeLog, uploading for cq to land
Thanks for the review Andreas.
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> All reviewed patches have been landed. Closing bug. (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. (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 ! 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. (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 |