Summary: | Web Inspector: [Extensions API] expose page load events timings | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrey Kosyakov <caseq> | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Andrey Kosyakov <caseq> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aroben, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Andrey Kosyakov
2010-09-17 03:11:06 PDT
Created attachment 69435 [details]
patch
Comment on attachment 69435 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=69435&action=review I think it is not good that the manipulations with domContentTime and loadEventTime are not localized in the code. With all respect, I would suggest to write this little piece of code slightly different way. if (payload.didTimingChange) { if (payload.startTime) resource.startTime = payload.startTime; if (payload.responseReceivedTime) resource.responseReceivedTime = payload.responseReceivedTime; if (payload.endTime) resource.endTime = payload.endTime; if (resource.mainResource) { // This loadEventTime is for the main resource, and we want to show it // for all resources on this page. This means we want to set it as a member // of the resources panel instead of the individual resource. var loadEventTime = payload.loadEventTime ? payload.loadEventTime : -1; this.panels.resources.mainResourceLoadTime = loadEventTime; if (loadEventTime !== -1) this.panels.audits.mainResourceLoadTime = loadEventTime; if (this.panels.network) this.panels.network.mainResourceLoadTime = loadEventTime; // This domContentEventTime is for the main resource, so it should go in // the resources panel for the same reasons as above. var domContentEventTime = payload.domContentEventTime ? payload.domContentEventTime : -1; this.panels.resources.mainResourceDOMContentTime = domContentEventTime; this.panels.audits.mainResourceDOMContentTime = domContentEventTime; if (this.panels.network) this.panels.network.mainResourceDOMContentTime = domContentEventTime; } } Really audits panel is not using these times but starts the audit process when mainResourceLoadTime arrives. It'd be better to do that explicitly. > WebCore/inspector/front-end/inspector.js:1298 > + if (payload.startTime) { > resource.startTime = payload.startTime; > + if (resource.mainResource) { > + loadEventTime = loadEventTime || -1; > + domContentEventTime = domContentEventTime || -1; > + } > + } As I can see there is no dependency between startTime and loadEventTime at backend side. Created attachment 69453 [details]
patch
Rewritten page even time event logic as proposed by loislo.
Comment on attachment 69453 [details]
patch
looks good to me
Comment on attachment 69453 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=69453&action=review > WebCore/inspector/front-end/inspector.js:1297 > + if (resource.mainResource) { This looks confusing. Load time should be sent to the front-end after the main resource has been loaded. It is just that we are using a hack to pass page load timing along with the main resource data. Created attachment 69461 [details] patch to land Removed controversial fix for page event timings at all. Will fix later, along with better way to report page event times to front-end (see https://bugs.webkit.org/show_bug.cgi?id=46981). These tests are failing on Windows now. See http://build.webkit.org/results/Windows%20Debug%20(Tests)/r68890%20(20586)/results.html (In reply to comment #7) > These tests are failing on Windows now. See http://build.webkit.org/results/Windows%20Debug%20(Tests)/r68890%20(20586)/results.html Those but inspector/extensions-* appear totally unrelated. With regards to inspector/extenstions-*, the -api.html suggests that frontend is of an older version (i.e. lacks a method introduced in this patch; I don't think there's a space for any flakiness there). This also does not reproduce locally. Can we clobber the win bots? Also, I reckon that build-webkit does not copy inspector front-end files under windows, unless --inspector-frontend is specified or there's a larger change outside of inspector/front-end. If clobbering fixes the tests, I think we need to raise a bug for build to perform build-webkit --inspector-frontend at least on bots (it's cheap). (In reply to comment #8) > (In reply to comment #7) > > These tests are failing on Windows now. See http://build.webkit.org/results/Windows%20Debug%20(Tests)/r68890%20(20586)/results.html > > Those but inspector/extensions-* appear totally unrelated. Yes. I filed bug 46988 to cover the failures. Let's continue the discussion there. Comment on attachment 69453 [details] patch Cleared Pavel Feldman's review+ from obsolete attachment 69453 [details] so that this bug does not appear in http://webkit.org/pending-commit. Manually committed r68887: http://trac.webkit.org/changeset/68887 |