[webkit-reviews] review denied: [Bug 89244] Web Inspector: Add progress events to the timeline to keep track of how much the main frame has loaded : [Attachment 147893] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied hanma at rim.com's request for
review:
Bug 89244: Web Inspector: Add progress events to the timeline to keep track of
how much the main frame has loaded
https://bugs.webkit.org/show_bug.cgi?id=89244

Attachment 147893: Patch
https://bugs.webkit.org/attachment.cgi?id=147893&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.