Bug 89244 – Web Inspector: Add data length to resource events on timeline to keep track of the amount of data loaded and the total data length

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

[Hanna]

Add progress events to the timeline to keep track of how much the main frame has loaded

[Hanna]

Created attachment 147893 [details]
Patch

[Rob Buis]

Comment on attachment 147893 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147893&action=review

Looks good, but probably needs an inspector expert. Would also be nice to add PR (if any) and internal reviewer.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:212
> +

Better not change layout style for code we do not own. You might like this better but that is not a good enough reason.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:269
> +

Ditto.

> Source/WebCore/loader/ProgressTracker.cpp:221
> +    InspectorInstrumentation::updateProgress(frame.get(), m_progressValue*100);

Nit: better make it m_progressValue * 100 .

[Pavel Feldman]

Comment on attachment 147893 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147893&action=review

> Source/WebCore/ChangeLog:3
> +        Web Inspector: Add progress events to the timeline to keep track of how much the main frame has loaded

InspectorInstrumentation is only used to instrument what is actually happening in the engine. In contrast, progress events are only there for the progress indication in the UI (like the wait cursor). I don't think these marks are of any practical use. Users record the timeline first and analyse it later, it is not realistic to track events while they are being collected. And once it is all recorded, user would see the actual progress snapshot.

In fact, alternate timeline clients would be able to figure out the progress on their own based on the ResourceReceiveResponse / ResourceReceivedData events if we added expected content length value into the ResourceReceiveResponse's data.

Btw, you should declare your intent upfront, i.e. file a bug first and get the feedback prior to doing the actual coding.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

You should always explain what has changed in the code and why in the ChangeLog. You should also provide tests for your changes.

> Source/WebCore/inspector/front-end/TimelineModel.js:68
> +    MarkProgress: "MarkProgress",

LayoutTests/inspector/timeline/timeline-enum-stability.txt is now failing.

[Timothy Hatcher]

> In fact, alternate timeline clients would be able to figure out the progress on their own based on the ResourceReceiveResponse / ResourceReceivedData events if we added expected content length value into the ResourceReceiveResponse's data.

I agree, getting the load progress for resources would be much more useful. The progress update from ProgressTracker is a patently bogus number in many situations.

[Hanna]

(In reply to comment #3)

> In fact, alternate timeline clients would be able to figure out the progress on their own based on the ResourceReceiveResponse / ResourceReceivedData events if we added expected content length value into the ResourceReceiveResponse's data.


I have a question, how does clients figure out the progress based on ResourceReceiveResponse / ResourceReceivedData events? the expected content length most likely wouldn't correspond to the total size? I could put data length under resourcereceiveddata, but it doesn't do much without knowing the total size, or is it alright to expect clients to know how big their site is?

[Eli Fidler]

ProgressTracker may not give a totally-representative number, but it is often hooked up to an embedder's progress bar.

I'd say that correlating the ProgressTracker number with the timeline is still a useful thing to do.

[Hanna]

Created attachment 148797 [details]
Patch

[Hanna]

layout test coming up soon

[Pavel Feldman]

Comment on attachment 148797 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148797&action=review

> Source/WebCore/ChangeLog:3
> +        Added content length, data length, and percentage loaded to

Please prepent Web Inspector:

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:188
> +        this._progress = {};

These names should be more specific to the network.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:396
> +            presentationModel._progress[record.data["requestId"]] += parseInt(record.data["length"]);

I don't think progress makes sense in the default timeline view.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:523
> +                    contentHelper._appendTextRow(WebInspector.UIString("Data Length"), WebInspector.UIString("%d Bytes", this.data["length"]));

Please add this line to the English.lproj/localizedStrings.js.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:524
> +                if (contentLength) {

Please remove this branch, I don't think percentage loaded makes sense in timeline. Could you explain your scenario in greater detail? We don't focus on network requests in Timeline panel. We have a Network panel for that.

> Source/WebCore/loader/ResourceLoader.cpp:443
> +    InspectorInstrumentationCookie cookie = InspectorInstrumentation::willReceiveResourceData(m_frame.get(), identifier(), length);

You might want encodedDataLength here (the one that is actually transmitted over the network, not the decompressed chunk).

[Hanna]

Created attachment 148850 [details]
Patch

[Rob Buis]

Comment on attachment 148850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148850&action=review

Looks good but ChangeLog can be improved.

> Source/WebCore/ChangeLog:5
> +         keep track of the amount of data loaded.

This seems wrong, you should just put the bug title here.

> Source/WebCore/ChangeLog:9
> +

You can move the above description to here.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:543
> +                }

Inconsistent use of braces, I'd advise not using braces for this if section.

> LayoutTests/ChangeLog:4
> +        timeline network received data event.

Just state bug title here.

> LayoutTests/ChangeLog:8
> +

You can move the above description to here.

[Hanna]

Created attachment 148872 [details]
Patch

[Rob Buis]

Comment on attachment 148872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148872&action=review

Looks good, some small stuff to fix still.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:415
> +        if (record.type === recordTypes.ResourceFinish)

Could add else here.

> LayoutTests/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).

Reviewed by should be first, then empty line, then description.

> LayoutTests/inspector/timeline/timeline-network-received-data.html:38
> +            if (record.type === WebInspector.TimelineModel.RecordType.ResourceReceivedData){

Add space between ')' and '{'.

> LayoutTests/inspector/timeline/timeline-network-received-data.html:40
> +}

Please indent properly.

[Hanna]

Created attachment 148881 [details]
Patch

[Rob Buis]

Comment on attachment 148881 [details]
Patch

LGTM, no more nits, but I would feel more comfortable if Web Inspector person reviews. Pavel maybe?

[WebKit Review Bot]

Comment on attachment 148881 [details]
Patch

Attachment 148881 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13027310

New failing tests:
inspector/timeline/timeline-parse-html.html

[WebKit Review Bot]

Created attachment 148929 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Pavel Feldman]

Comment on attachment 148881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148881&action=review

> Source/WebCore/inspector/front-end/TimelinePanel.js:799
> +            var contentLength = this._presentationModel._contentLengthByRequestId[record.data["requestId"]];

You should not access private fields defined in other files.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:196
> +        this._networkLoadedDataLength = {};

I don't think we really need all these details in the Timeline.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:197
> +        this._contentLengthByRequestId = {};

ditto

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:537
> +                if (this.data["length"])

I would leave only these two lines that are providing the actual instrumented data.

> LayoutTests/inspector/timeline/timeline-network-received-data-expected.txt:6
> +ResourceReceivedData Properties:

This test might get flaky: what if there are multiple received data entries... You should check that there is at least one received data entry and that it has length instead.

[Hanna]

Created attachment 149027 [details]
Patch

[Timothy Hatcher]

Comment on attachment 149027 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149027&action=review

> Source/WebCore/inspector/TimelineRecordFactory.cpp:168
> +    data->setNumber("DataLength", length);

This should be "dataLength" (lowercase first letter). Perhaps it should even be "encodedDataLength"?

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:530
> +                if (this.data["DataLength"])

Ditto.

[Hanna]

Created attachment 149032 [details]
Patch

[Hanna]

r? anyone?

[Pavel Feldman]

Comment on attachment 149032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149032&action=review

> LayoutTests/inspector/timeline/timeline-network-received-data.html:29
> +        InspectorTest.printTimelineRecords("ResourceReceivedData");

I don't think that you've actually addressed my test comment. This test will be flaky.

[Hanna]

Created attachment 149294 [details]
Patch

[Hanna]

(In reply to comment #23)
> (From update of attachment 149032 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149032&action=review
> 
> > LayoutTests/inspector/timeline/timeline-network-received-data.html:29
> > +        InspectorTest.printTimelineRecords("ResourceReceivedData");
> 
> I don't think that you've actually addressed my test comment. This test will be flaky.

I am sorry, I do not get what you meant, do you mind explain a bit? I have added another patch which changes the test a little bit, but I don't really know if this is what you are looking for.

[Pavel Feldman]

Comment on attachment 149294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149294&action=review

> LayoutTests/inspector/timeline/timeline-network-received-data-expected.txt:4
> +ResourceReceivedData Properties:

Your test is fetching the script from the given URL and expects single received data entry here. Whereas the data can be sent in a number of chunks. You should change your test so that it makes sure that _at least one_ data chunk is received.

[Hanna]

Created attachment 149375 [details]
Patch

[Hanna]

Created attachment 149378 [details]
Patch

[Hanna]

r?

[Hanna]

Thank you!

[Pavel Feldman]

(In reply to comment #30)
> Thank you!

Thank you for your efforts!

[Hanna]

Created attachment 150203 [details]
Patch

[WebKit Review Bot]

Comment on attachment 150203 [details]
Patch

Clearing flags on attachment: 150203

Committed r121592: <http://trac.webkit.org/changeset/121592>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.