Bug 45954 - Web Inspector: [Extensions API] expose page load events timings
Summary: Web Inspector: [Extensions API] expose page load events timings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Kosyakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-17 03:11 PDT by Andrey Kosyakov
Modified: 2010-10-02 06:05 PDT (History)
10 users (show)

See Also:


Attachments
patch (10.07 KB, patch)
2010-10-01 01:50 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
patch (9.69 KB, patch)
2010-10-01 05:05 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
patch to land (7.58 KB, patch)
2010-10-01 06:35 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
patch
Comment 2 Ilya Tikhonovsky 2010-10-01 03:19:45 PDT
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.
Comment 3 Andrey Kosyakov 2010-10-01 05:05:07 PDT
Created attachment 69453 [details]
patch

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]
patch

looks good to me
Comment 5 Pavel Feldman 2010-10-01 06:04:28 PDT
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.
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.

Yes.

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]
patch

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