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
Summary: Web Inspector: Add data length to resource events on timeline to keep track o...
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-15 13:50 PDT by Hanna
Modified: 2012-06-29 14:57 PDT (History)
16 users (show)

See Also:


Attachments
Patch (15.39 KB, patch)
2012-06-15 14:00 PDT, Hanna
no flags Details | Formatted Diff | Diff
Patch (14.76 KB, patch)
2012-06-21 07:41 PDT, Hanna
no flags Details | Formatted Diff | Diff
Patch (19.72 KB, patch)
2012-06-21 11:40 PDT, Hanna
no flags Details | Formatted Diff | Diff
Patch (19.81 KB, patch)
2012-06-21 13:29 PDT, Hanna
no flags Details | Formatted Diff | Diff
Patch (19.76 KB, patch)
2012-06-21 13:55 PDT, Hanna
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (682.65 KB, application/zip)
2012-06-21 18:03 PDT, WebKit Review Bot
no flags Details
Patch (16.24 KB, patch)
2012-06-22 07:52 PDT, Hanna
no flags Details | Formatted Diff | Diff
Patch (16.29 KB, patch)
2012-06-22 08:17 PDT, Hanna
no flags Details | Formatted Diff | Diff
Patch (16.94 KB, patch)
2012-06-25 08:01 PDT, Hanna
no flags Details | Formatted Diff | Diff
Patch (16.55 KB, patch)
2012-06-25 15:49 PDT, Hanna
no flags Details | Formatted Diff | Diff
Patch (16.57 KB, patch)
2012-06-25 15:57 PDT, Hanna
no flags Details | Formatted Diff | Diff
Patch (16.48 KB, patch)
2012-06-29 10:06 PDT, Hanna
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hanna 2012-06-15 13:50:37 PDT
Add progress events to the timeline to keep track of how much the main frame has loaded
Comment 1 Hanna 2012-06-15 14:00:01 PDT
Created attachment 147893 [details]
Patch
Comment 2 Rob Buis 2012-06-15 14:10:18 PDT
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 .
Comment 3 Pavel Feldman 2012-06-15 14:35:39 PDT
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.
Comment 4 Timothy Hatcher 2012-06-15 15:48:06 PDT
> 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.
Comment 5 Hanna 2012-06-18 11:28:25 PDT
(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?
Comment 6 Eli Fidler 2012-06-18 13:54:01 PDT
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.
Comment 7 Hanna 2012-06-21 07:41:40 PDT
Created attachment 148797 [details]
Patch
Comment 8 Hanna 2012-06-21 07:42:50 PDT
layout test coming up soon
Comment 9 Pavel Feldman 2012-06-21 08:01:47 PDT
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).
Comment 10 Hanna 2012-06-21 11:40:55 PDT
Created attachment 148850 [details]
Patch
Comment 11 Rob Buis 2012-06-21 12:39:43 PDT
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.
Comment 12 Hanna 2012-06-21 13:29:38 PDT
Created attachment 148872 [details]
Patch
Comment 13 Rob Buis 2012-06-21 13:34:59 PDT
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.
Comment 14 Hanna 2012-06-21 13:55:32 PDT
Created attachment 148881 [details]
Patch
Comment 15 Rob Buis 2012-06-21 14:01:18 PDT
Comment on attachment 148881 [details]
Patch

LGTM, no more nits, but I would feel more comfortable if Web Inspector person reviews. Pavel maybe?
Comment 16 WebKit Review Bot 2012-06-21 18:03:44 PDT
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
Comment 17 WebKit Review Bot 2012-06-21 18:03:48 PDT
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
Comment 18 Pavel Feldman 2012-06-21 18:24:32 PDT
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.
Comment 19 Hanna 2012-06-22 07:52:44 PDT
Created attachment 149027 [details]
Patch
Comment 20 Timothy Hatcher 2012-06-22 08:01:18 PDT
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.
Comment 21 Hanna 2012-06-22 08:17:04 PDT
Created attachment 149032 [details]
Patch
Comment 22 Hanna 2012-06-22 10:31:42 PDT
r? anyone?
Comment 23 Pavel Feldman 2012-06-22 15:54:09 PDT
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.
Comment 24 Hanna 2012-06-25 08:01:38 PDT
Created attachment 149294 [details]
Patch
Comment 25 Hanna 2012-06-25 08:03:27 PDT
(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.
Comment 26 Pavel Feldman 2012-06-25 09:58:14 PDT
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.
Comment 27 Hanna 2012-06-25 15:49:57 PDT
Created attachment 149375 [details]
Patch
Comment 28 Hanna 2012-06-25 15:57:45 PDT
Created attachment 149378 [details]
Patch
Comment 29 Hanna 2012-06-27 09:15:05 PDT
r?
Comment 30 Hanna 2012-06-29 08:11:31 PDT
Thank you!
Comment 31 Pavel Feldman 2012-06-29 08:24:26 PDT
(In reply to comment #30)
> Thank you!

Thank you for your efforts!
Comment 32 Hanna 2012-06-29 10:06:04 PDT
Created attachment 150203 [details]
Patch
Comment 33 WebKit Review Bot 2012-06-29 14:57:41 PDT
Comment on attachment 150203 [details]
Patch

Clearing flags on attachment: 150203

Committed r121592: <http://trac.webkit.org/changeset/121592>
Comment 34 WebKit Review Bot 2012-06-29 14:57:47 PDT
All reviewed patches have been landed.  Closing bug.