Bug 45954 – Web Inspector: [Extensions API] expose page load events timings

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Andrey Kosyakov]

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).

[Andrey Kosyakov]

Created attachment 69435 [details]
patch

[Ilya Tikhonovsky]

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.

[Andrey Kosyakov]

Created attachment 69453 [details]
patch

Rewritten page even time event logic as proposed by loislo.

[Ilya Tikhonovsky]

Comment on attachment 69453 [details]
patch

looks good to me

[Pavel Feldman]

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.

[Andrey Kosyakov]

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).

[Adam Roben (:aroben)]

These tests are failing on Windows now. See http://build.webkit.org/results/Windows%20Debug%20(Tests)/r68890%20(20586)/results.html

[Andrey Kosyakov]

(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).

[Adam Roben (:aroben)]

(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.

[Eric Seidel (no email)]

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.

[Andrey Kosyakov]

Manually committed r68887: http://trac.webkit.org/changeset/68887