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

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

In order to contain the newer timing information, net::LoadTimingInfo::ServiceWorkerTiming is defined and contained in net::LoadTimingInfo. Of course this struct is widely used, so is this okey? or is there some another solution?

1st patch: https://codereview.chromium.org/515583005/

BUG=401389
TEST=N/A

Committed: https://crrev.com/403d2d975696fa4288f7075733613f1a5e743b19
Cr-Commit-Position: refs/heads/master@{#295635}

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

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

[horo, 2014-08-28 07:00:08]

https://codereview.chromium.org/515753003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/515753003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_url_request_job.cc:342:
load_timing_info_.serviceworker_timing.launch_serviceworker =
"launch_serviceworker" is not appropriate.
ServiceWorker may be already launched before fetch_start.
I think fetch_ready is better.

[shimazu, 2014-08-29 02:16:33]

https://codereview.chromium.org/515753003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/515753003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_url_request_job.cc:342:
load_timing_info_.serviceworker_timing.launch_serviceworker =
On 2014/08/28 07:00:08, horo wrote:
> "launch_serviceworker" is not appropriate.
> ServiceWorker may be already launched before fetch_start.
> I think fetch_ready is better.

Done.

[horo, 2014-08-29 04:35:14]

lgtm

[shimazu, 2014-08-29 07:47:15]

shimazu@chromium.org changed reviewers:
+ jam@chromium.org, mmenke@chromium.org, tsepez@chromium.org

[shimazu, 2014-08-29 07:47:15]

tsepez@chromium.org: Please review changes in 

jam@chromium.org: Please review changes in 

mmenke@chromium.org: Please review changes in

[shimazu, 2014-08-29 07:57:31]

Please review kindly:)

mailto:tsepez@chromium.org: Please review changes in content/child/*

mailto:jam@chromium.org: Please review changes in
content/common/resource_messages.cc
 
mailto:mmenke@chromium.org: Please review changes in net/base/load_timing_info*


On 2014/08/29 07:47:15, makoto wrote:
> mailto:tsepez@chromium.org: Please review changes in 
> 
> mailto:jam@chromium.org: Please review changes in 
> 
> mailto:mmenke@chromium.org: Please review changes in

[Tom Sepez, 2014-08-29 17:52:26]

IPC lgtm.  

Ideally, these shouldn't be hand-rolled traits, but rather generated.  I'll take
a look at doing that after this lands.

[mmenke, 2014-09-02 19:10:44]

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:99:
net::LoadTimingInfo* load_timing_info) const {
+2 indent.

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:237:
load_timing_info_.request_start = base::TimeTicks::Now();
These two are populated by the URLRequest itself, so this isn't needed.

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:250:
base::TimeTicks::Now();
+2 indent

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:343:
base::TimeTicks::Now();
+2 indent.

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:404: const
std::map<std::string, std::string>& headers) {
+2 indent for the above 3 lines.

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.h (right):

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.h:61: const
OVERRIDE;
nit:  4 space indent.

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.h:61: const
OVERRIDE;
nit:  Close parentheses and const/OVERRIDE should generally go on the same line,
so this should be:

virtual void GetLoadTimingInfo(
    net::LoadTimingInfo* load_timing_info) const OVERRIDE;

https://codereview.chromium.org/515753003/diff/20001/net/base/load_timing_info.h
File net/base/load_timing_info.h (right):

https://codereview.chromium.org/515753003/diff/20001/net/base/load_timing_inf...
net/base/load_timing_info.h:96: struct NET_EXPORT_PRIVATE ServiceWorkerTiming {
It's a layering violation for net/ to know about ServiceWorkerURLRequestJobs, or
service workers in general.  Suggest adding the new times directly to
ResourceResponseInfo, and setting them in PopulateResourceResponse.

There are a couple ways to attach them to the request - if you keep track of
ServiceWorkerURLRequestJobs somewhere, could find the right one for a request
and grab them directly.  If not, could attach them to the URLRequest via the
SupportsUserData API.  Open to other ideas.

[michaeln1, 2014-09-03 00:30:32]

> or is there some another solution?

The timing info should probably be retrieved via
ServiceWorkerContextRequestHandler::GetExtraResponseInfo(). The 'handler' itself
is attached to the request.

[michaeln, 2014-09-03 00:56:03]

michaeln@chromium.org changed reviewers:
+ michaeln@chromium.org
- michaeln@google.com

[michaeln, 2014-09-03 00:56:04]

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:31: using
base::TimeTicks;
are these needed, looks like you fully qualify them when using Time and
TimeTicks?

[shimazu, 2014-09-04 03:29:37]

Patchset #3 (id:40001) has been deleted

[shimazu, 2014-09-04 03:32:41]

Could you review this patch again?

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:31: using
base::TimeTicks;
On 2014/09/03 00:56:04, michaeln wrote:
> are these needed, looks like you fully qualify them when using Time and
> TimeTicks?

Done.

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:99:
net::LoadTimingInfo* load_timing_info) const {
On 2014/09/02 19:10:43, mmenke wrote:
> +2 indent.

Done.

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:237:
load_timing_info_.request_start = base::TimeTicks::Now();
On 2014/09/02 19:10:43, mmenke wrote:
> These two are populated by the URLRequest itself, so this isn't needed.

Done.

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:250:
base::TimeTicks::Now();
On 2014/09/02 19:10:43, mmenke wrote:
> +2 indent

Done.

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:343:
base::TimeTicks::Now();
On 2014/09/02 19:10:43, mmenke wrote:
> +2 indent.

Done.

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:404: const
std::map<std::string, std::string>& headers) {
On 2014/09/02 19:10:43, mmenke wrote:
> +2 indent for the above 3 lines.

Done.

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.h (right):

https://codereview.chromium.org/515753003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.h:61: const
OVERRIDE;
On 2014/09/02 19:10:43, mmenke wrote:
> nit:  4 space indent.

Done.

https://codereview.chromium.org/515753003/diff/20001/net/base/load_timing_info.h
File net/base/load_timing_info.h (right):

https://codereview.chromium.org/515753003/diff/20001/net/base/load_timing_inf...
net/base/load_timing_info.h:96: struct NET_EXPORT_PRIVATE ServiceWorkerTiming {
Thanks for your comment, and I tried the implementation as you said.
ServiceWorker's timing info is contained by ServiceWorkerURLRequestJob directly
and passed to ResourceResponseInfo via GetExtraResponseInfo().
On 2014/09/02 19:10:43, mmenke wrote:
> It's a layering violation for net/ to know about ServiceWorkerURLRequestJobs,
or
> service workers in general.  Suggest adding the new times directly to
> ResourceResponseInfo, and setting them in PopulateResourceResponse.
> 
> There are a couple ways to attach them to the request - if you keep track of
> ServiceWorkerURLRequestJobs somewhere, could find the right one for a request
> and grab them directly.  If not, could attach them to the URLRequest via the
> SupportsUserData API.  Open to other ideas.

[mmenke, 2014-09-04 14:53:51]

Given any thought to testing this?

https://codereview.chromium.org/515753003/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/515753003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:185: const
net::HttpResponseInfo* ServiceWorkerURLRequestJob::http_info() const {
Note:  This should either be inlined in the header, or not named http_info.  No
need to worry about it in this CL, but it does violate the style guide.

https://codereview.chromium.org/515753003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:399:
load_timing_info_.receive_headers_end = base::TimeTicks::Now();
Currently, either all of send_start/send_end/receive_headers_end are set, or
none of them are (Requests served out of the cache set them to time the were
requested from the cache).

Per spec at
https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/NavigationTiming/Overview.html,
requestStart (i.e. send_start) "must return the time immediately before the user
agent starts requesting the current document from the server, or from relevant
application caches or from local resources.", which doesn't really give much
room for it to be null.

https://codereview.chromium.org/515753003/diff/60001/content/child/web_url_lo...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/515753003/diff/60001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:776: (info.service_worker_fetch_end -
kNullTicks).InSecondsF());
+2 indent (x3)

[jam, 2014-09-04 22:27:01]

lgtm

[shimazu, 2014-09-05 02:29:26]

Patchset #4 (id:80001) has been deleted

[shimazu, 2014-09-05 04:05:47]

Patchset #4 (id:100001) has been deleted

[shimazu, 2014-09-05 04:18:48]

I incorporated the mmenke@'s review.

mmenke@: I'm wondering if this fixing is correct. Could you take a look?

https://codereview.chromium.org/515753003/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/515753003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:399:
load_timing_info_.receive_headers_end = base::TimeTicks::Now();
On 2014/09/04 14:53:51, mmenke wrote:
> Currently, either all of send_start/send_end/receive_headers_end are set, or
> none of them are (Requests served out of the cache set them to time the were
> requested from the cache).
> 
> Per spec at
>
https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/NavigationTiming/Overview.html,
> requestStart (i.e. send_start) "must return the time immediately before the
user
> agent starts requesting the current document from the server, or from relevant
> application caches or from local resources.", which doesn't really give much
> room for it to be null.

Thanks, I didn't know the spec.
I set the send_start/send_end to load_timing_info.

https://codereview.chromium.org/515753003/diff/60001/content/child/web_url_lo...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/515753003/diff/60001/content/child/web_url_lo...
content/child/web_url_loader_impl.cc:776: (info.service_worker_fetch_end -
kNullTicks).InSecondsF());
On 2014/09/04 14:53:51, mmenke wrote:
> +2 indent (x3)

Done.

[mmenke, 2014-09-05 14:22:34]

LGTM, as-is.  Note that I only reviewed the RequestJob class, resource_loader,
and the content/child classes.

https://codereview.chromium.org/515753003/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/515753003/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:383:
load_timing_info_.send_end = fetch_end_time_;
Will fetch_end_time_ always be send_end, or might they be different in the
future?

Just wondering if we need both - if you'll be using load timing information from
the network stack, in the case that requests actually go over the network, then
they'll definitely be different in that case, and we should have both.  If the
long term plan is to always do what you're doing now, then they're redundant. 
Same goes for the start times.

https://codereview.chromium.org/515753003/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:401:
load_timing_info_.receive_headers_end = base::TimeTicks::Now();
Not exactly a huge cost in terms of performance or binary size, but I don't
think we really need another call to TimeTicks::Now() here, unless we're on a
file thread and CreateBlobRequest blocks on file I/O or something.

[michaeln, 2014-09-05 21:23:52]

https://codereview.chromium.org/515753003/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/515753003/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:348:
fetch_ready_time_ = base::TimeTicks::Now();
lgtm 2 

fetch_ready_time might be more analogous to send_end than fetch_end_time?

[shimazu, 2014-09-19 00:22:27]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2014-09-19 00:23:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/515753003/160001

[commit-bot: I haz the power, 2014-09-19 01:25:27]

Committed patchset #6 (id:160001) as c7be142ed9a13e23f16f72bca4742ce0b488066f

[commit-bot: I haz the power, 2014-09-19 01:26:03]

Patchset 6 (id:??) landed as
https://crrev.com/403d2d975696fa4288f7075733613f1a5e743b19
Cr-Commit-Position: refs/heads/master@{#295635}