Add ServiceWorker timing information on the popup panel in DevTools's Network tab (1/2)

Add ServiceWorker timing information on the popup panel in DevTools's Network tab (1/2)

2nd patch: https://codereview.chromium.org/515753003/

BUG=401389
TEST=N/A

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182231

[shimazu, 2014-08-28 06:04:07]

shimazu@chromium.org changed reviewers:
+ horo@chromium.org

[shimazu, 2014-08-28 06:16:19]

review this kindly:)

[eustas, 2014-08-28 06:23:31]

eustas@chromium.org changed reviewers:
+ eustas@chromium.org

[eustas, 2014-08-28 06:23:31]

https://codereview.chromium.org/515583005/diff/1/Source/devtools/front_end/ne...
File Source/devtools/front_end/network/RequestTimingView.js (right):

https://codereview.chromium.org/515583005/diff/1/Source/devtools/front_end/ne...
Source/devtools/front_end/network/RequestTimingView.js:186:
createServiceWorkerTimingTable();
Please inline create###TimingTable()

[horo, 2014-08-28 06:58:26]

https://codereview.chromium.org/515583005/diff/1/public/platform/WebURLLoadTi...
File public/platform/WebURLLoadTiming.h (right):

https://codereview.chromium.org/515583005/diff/1/public/platform/WebURLLoadTi...
public/platform/WebURLLoadTiming.h:81: BLINK_PLATFORM_EXPORT double fetchStart()
const;
"fetchStart/fetchEnd" is too general name.
I think
- serviceWorkerFetchStart
- serviceWorkerFetchReady
- serviceWorkerFetchEnd
is better.

[shimazu, 2014-08-28 07:11:19]

https://codereview.chromium.org/515583005/diff/1/Source/devtools/front_end/ne...
File Source/devtools/front_end/network/RequestTimingView.js (right):

https://codereview.chromium.org/515583005/diff/1/Source/devtools/front_end/ne...
Source/devtools/front_end/network/RequestTimingView.js:186:
createServiceWorkerTimingTable();
Sure, but I think this is more readable and extensible. Are there any rules?
On 2014/08/28 06:23:30, eustas wrote:
> Please inline create###TimingTable()

[eustas, 2014-08-28 07:32:01]

https://codereview.chromium.org/515583005/diff/1/Source/devtools/front_end/ne...
File Source/devtools/front_end/network/RequestTimingView.js (right):

https://codereview.chromium.org/515583005/diff/1/Source/devtools/front_end/ne...
Source/devtools/front_end/network/RequestTimingView.js:186:
createServiceWorkerTimingTable();
On 2014/08/28 07:11:19, makoto wrote:
> Sure, but I think this is more readable and extensible. Are there any rules?
> On 2014/08/28 06:23:30, eustas wrote:
> > Please inline create###TimingTable()
> 

We have no strict rules, but ve do not use var-functions until is is absolutely
necessary.

Alternatively, you can extract those functions as member functions.
Let them return array of objects describing rows.

[pfeldman, 2014-08-28 07:35:04]

pfeldman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2014-08-28 07:35:04]

https://codereview.chromium.org/515583005/diff/1/Source/devtools/front_end/ne...
File Source/devtools/front_end/network/RequestTimingView.js (right):

https://codereview.chromium.org/515583005/diff/1/Source/devtools/front_end/ne...
Source/devtools/front_end/network/RequestTimingView.js:186:
createServiceWorkerTimingTable();
We do have a strict rule to not use anonymous functions (which implies that var
is prohibited).

[shimazu, 2014-08-29 02:00:47]

https://codereview.chromium.org/515583005/diff/1/Source/devtools/front_end/ne...
File Source/devtools/front_end/network/RequestTimingView.js (right):

https://codereview.chromium.org/515583005/diff/1/Source/devtools/front_end/ne...
Source/devtools/front_end/network/RequestTimingView.js:186:
createServiceWorkerTimingTable();
On 2014/08/28 07:32:01, eustas wrote:
> On 2014/08/28 07:11:19, makoto wrote:
> > Sure, but I think this is more readable and extensible. Are there any rules?
> > On 2014/08/28 06:23:30, eustas wrote:
> > > Please inline create###TimingTable()
> > 
> 
> We have no strict rules, but ve do not use var-functions until is is
absolutely
> necessary.
> 
> Alternatively, you can extract those functions as member functions.
> Let them return array of objects describing rows.

Done.

https://codereview.chromium.org/515583005/diff/1/public/platform/WebURLLoadTi...
File public/platform/WebURLLoadTiming.h (right):

https://codereview.chromium.org/515583005/diff/1/public/platform/WebURLLoadTi...
public/platform/WebURLLoadTiming.h:81: BLINK_PLATFORM_EXPORT double fetchStart()
const;
On 2014/08/28 06:58:26, horo wrote:
> "fetchStart/fetchEnd" is too general name.
> I think
> - serviceWorkerFetchStart
> - serviceWorkerFetchReady
> - serviceWorkerFetchEnd
> is better.

Done.

[horo, 2014-08-29 04:33:22]

https://codereview.chromium.org/515583005/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/network/RequestTimingView.js (right):

https://codereview.chromium.org/515583005/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/network/RequestTimingView.js:169:
addRow(WebInspector.UIString("Launching ServiceWorker"), "ssl",
timing.serviceWorkerFetchStart, timing.serviceWorkerFetchReady);
- I think "Launching" is not appropriate. "Waiting for ServiceWorker" is better.
- Why "ssl"?

[shimazu, 2014-08-29 07:28:40]

https://codereview.chromium.org/515583005/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/network/RequestTimingView.js (right):

https://codereview.chromium.org/515583005/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/network/RequestTimingView.js:169:
addRow(WebInspector.UIString("Launching ServiceWorker"), "ssl",
timing.serviceWorkerFetchStart, timing.serviceWorkerFetchReady);
Done.
I think "ServiceWorker Perparation" is much meaningful and changed so, and what
do you think?
On 2014/08/29 04:33:22, horo wrote:
> - I think "Launching" is not appropriate. "Waiting for ServiceWorker" is
better.
> - Why "ssl"?
>

[horo, 2014-08-29 08:09:56]

lgtm

https://codereview.chromium.org/515583005/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/network/RequestTimingView.js (right):

https://codereview.chromium.org/515583005/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/network/RequestTimingView.js:169:
addRow(WebInspector.UIString("Launching ServiceWorker"), "ssl",
timing.serviceWorkerFetchStart, timing.serviceWorkerFetchReady);
On 2014/08/29 07:28:40, makoto wrote:
> Done.
> I think "ServiceWorker Perparation" is much meaningful and changed so, and
what
> do you think?

Sounds good.

[eustas, 2014-08-29 12:18:41]

https://codereview.chromium.org/515583005/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/network/RequestTimingView.js (right):

https://codereview.chromium.org/515583005/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/network/RequestTimingView.js:159: cell.colSpan = 2;
Why don't leave common case (!request.finished) inplace?

[shimazu, 2014-09-01 05:55:29]

https://codereview.chromium.org/515583005/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/network/RequestTimingView.js (right):

https://codereview.chromium.org/515583005/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/network/RequestTimingView.js:159: cell.colSpan = 2;
On 2014/08/29 12:18:41, eustas wrote:
> Why don't leave common case (!request.finished) inplace?

Done.

[shimazu, 2014-09-08 08:45:59]

Could you review the latest patch?

[shimazu, 2014-09-11 05:02:06]

eustas@, pfeldman@: Could you check this patch?

[pfeldman, 2014-09-11 18:00:29]

The code looks good. I asked a question on the bug on the actionability of the
revealed information though.

https://codereview.chromium.org/515583005/diff/100001/Source/devtools/front_e...
File Source/devtools/front_end/network/RequestTimingView.js (right):

https://codereview.chromium.org/515583005/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/network/RequestTimingView.js:137:
addRow(WebInspector.UIString("Stalled"), "blocking", 0, blocking);
blocking is defined below. does it closure-compile?

https://codereview.chromium.org/515583005/diff/100001/Source/devtools/protoco...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/515583005/diff/100001/Source/devtools/protoco...
Source/devtools/protocol.json:1102: { "name": "serviceWorkerFetchStart", "type":
"number", "description": "Started fetching via ServiceWorker." },
Please add hidden: true on these

[shimazu, 2014-09-12 03:11:39]

Thanks for your comment, and I wrote an explanation of the actionability on
https://crbug.com/401389

https://codereview.chromium.org/515583005/diff/100001/Source/devtools/front_e...
File Source/devtools/front_end/network/RequestTimingView.js (right):

https://codereview.chromium.org/515583005/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/network/RequestTimingView.js:137:
addRow(WebInspector.UIString("Stalled"), "blocking", 0, blocking);
On 2014/09/11 18:00:29, pfeldman wrote:
> blocking is defined below. does it closure-compile?

I didn't know about the closure-compile. 
I looked at some pages below.
-
https://developer.chrome.com/devtools/docs/contributing#step-5-pull-develop-b...
-
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-bugs/34WPn3Tscaw

Then I tried ./devtools/scripts/compile_frontend.py and got no error and
warning.
Does this mean okay?

https://codereview.chromium.org/515583005/diff/100001/Source/devtools/protoco...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/515583005/diff/100001/Source/devtools/protoco...
Source/devtools/protocol.json:1102: { "name": "serviceWorkerFetchStart", "type":
"number", "description": "Started fetching via ServiceWorker." },
On 2014/09/11 18:00:29, pfeldman wrote:
> Please add hidden: true on these

Done.
But I don't know the meaning of this "hidden" option. 
Could you tell me where this option enable to hide from?

[pfeldman, 2014-09-18 09:36:35]

lgtm

https://codereview.chromium.org/515583005/diff/100001/Source/devtools/front_e...
File Source/devtools/front_end/network/RequestTimingView.js (right):

https://codereview.chromium.org/515583005/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/network/RequestTimingView.js:137:
addRow(WebInspector.UIString("Stalled"), "blocking", 0, blocking);
Yep, then it is Ok.

https://codereview.chromium.org/515583005/diff/100001/Source/devtools/protoco...
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/515583005/diff/100001/Source/devtools/protoco...
Source/devtools/protocol.json:1102: { "name": "serviceWorkerFetchStart", "type":
"number", "description": "Started fetching via ServiceWorker." },
hidden means that this is not a part of the public remote debugging protocol.

[shimazu, 2014-09-18 09:40:21]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2014-09-18 09:40:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/515583005/140001

[commit-bot: I haz the power, 2014-09-18 10:46:29]

Committed patchset #8 (id:140001) as 182231