Bug 45954

Summary: Web Inspector: [Extensions API] expose page load events timings
Product: WebKit Reporter: Andrey Kosyakov <caseq>
Component: Web Inspector (Deprecated)Assignee: Andrey Kosyakov <caseq>
Severity: Normal CC: aroben, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
patch to land none

Description Andrey Kosyakov 2010-09-17 03:11:06 PDT
Times of load and DOMContentLoaded events are part of HAR specs, but are not currently exposed, as there are under log.pages and we only offer entries found under log.entries in extensions API.
It is proposed to add webInspector.resources.getPageTimings() for extensions to retrieve page timings. Alternatively, we can offer page timings as part of response to webInspector.resources.getResource() for main resource (but outside of the HAREntry returned as part of response).
Comment 1 Andrey Kosyakov 2010-10-01 01:50:00 PDT
Created attachment 69435 [details]
Comment 2 Ilya Tikhonovsky 2010-10-01 03:19:45 PDT
Comment on attachment 69435 [details]

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.
Comment 3 Andrey Kosyakov 2010-10-01 05:05:07 PDT
Created attachment 69453 [details]

Rewritten page even time event logic as proposed by loislo.
Comment 4 Ilya Tikhonovsky 2010-10-01 05:31:00 PDT
Comment on attachment 69453 [details]

looks good to me
Comment 5 Pavel Feldman 2010-10-01 06:04:28 PDT
Comment on attachment 69453 [details]

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.
Comment 6 Andrey Kosyakov 2010-10-01 06:35:17 PDT
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).
Comment 7 Adam Roben (:aroben) 2010-10-01 09:14:15 PDT
These tests are failing on Windows now. See http://build.webkit.org/results/Windows%20Debug%20(Tests)/r68890%20(20586)/results.html
Comment 8 Andrey Kosyakov 2010-10-01 09:38:49 PDT
(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).
Comment 9 Adam Roben (:aroben) 2010-10-01 10:19:25 PDT
(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.


I filed bug 46988 to cover the failures. Let's continue the discussion there.
Comment 10 Eric Seidel (no email) 2010-10-02 05:35:04 PDT
Comment on attachment 69453 [details]

Cleared Pavel Feldman's review+ from obsolete attachment 69453 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 11 Andrey Kosyakov 2010-10-02 06:05:55 PDT
Manually committed r68887: http://trac.webkit.org/changeset/68887