[CacheStorage] [DevTools] Added "Time Received" column to cache storage data grid

[CacheStorage] [DevTools] Added "Time Received" column to cache storage data grid

Screenshot of the change in the bug.

Since the entry response time is given as an internal UTC format, I added wrapper methods to Time.h to convert it to standard UTC format.

BUG=727922
Review-Url: https://codereview.chromium.org/2928833002
Cr-Commit-Position: refs/heads/master@{#478797}
Committed: https://chromium.googlesource.com/chromium/src/+/d3ec2cc58603cc0578e7f3f17ddba5ff768303a7

[kristipark, 2017-06-07 21:07:44]

Description was changed from

==========
[CacheStorage] [DevTools] Added "Time Received" column to cache storage data
grid

Screenshot of the change in the bug.

Since the entry response time is given as an internal UTC format, I added
wrapper methods to Time.h to convert it to standard UTC format.

BUG=727922
==========

to

==========
[CacheStorage] [DevTools] Added "Time Received" column to cache storage data
grid

Screenshot of the change in the bug.

Since the entry response time is given as an internal UTC format, I added
wrapper methods to Time.h to convert it to standard UTC format.

BUG=727922
==========

[kristipark, 2017-06-07 21:07:44]

kristipark@chromium.org changed reviewers:
+ dgozman@chromium.org, dmurph@chromium.org

[dgozman, 2017-06-07 21:32:07]

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1135: 
Why these empty lines?

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1957: { "name":
"responseTime", "type": "number", "description": "Time at which the response was
received from the server. UTC internal time in microseconds." }
Let's send the same as Network.Timestamp here.

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkerCacheViews.js
(right):

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkerCacheViews.js:40:
{id: 'responseTime', title: Common.UIString('Time Received')}
Should this be 'Time Cached' ?

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js
(right):

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:218:
* @this {SDK.ServiceWorkerCacheModel}
It's already an instance method, no need for @this annotation.

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:228:
dataEntry.request, dataEntry.response,
this._formatTimestamp(dataEntry.responseTime)));
new Date(dataEntry.responseTime * 1000).toLocaleString()

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/cachestorage/InspectorCacheStorageAgent.cpp
(right):

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:215:
.setResponseTime(request_response.responseTime)
We usually send a double which means "Number of seconds since epoch".

[kristipark, 2017-06-07 22:42:38]

Thanks for reviewing!

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1135: 
On 2017/06/07 21:32:06, dgozman wrote:
> Why these empty lines?

Sorry, those were added accidentally while rebasing. Removed!

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1957: { "name":
"responseTime", "type": "number", "description": "Time at which the response was
received from the server. UTC internal time in microseconds." }
On 2017/06/07 21:32:06, dgozman wrote:
> Let's send the same as Network.Timestamp here.

Done.

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkerCacheViews.js
(right):

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/resources/ServiceWorkerCacheViews.js:40:
{id: 'responseTime', title: Common.UIString('Time Received')}
On 2017/06/07 21:32:06, dgozman wrote:
> Should this be 'Time Cached' ?

Done.

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js
(right):

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:228:
dataEntry.request, dataEntry.response,
this._formatTimestamp(dataEntry.responseTime)));
On 2017/06/07 21:32:06, dgozman wrote:
> new Date(dataEntry.responseTime * 1000).toLocaleString()

Done.

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/cachestorage/InspectorCacheStorageAgent.cpp
(right):

https://codereview.chromium.org/2928833002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:215:
.setResponseTime(request_response.responseTime)
On 2017/06/07 21:32:06, dgozman wrote:
> We usually send a double which means "Number of seconds since epoch".

Done.

[dgozman, 2017-06-08 17:30:01]

dgozman@chromium.org changed reviewers:
+ yutak@chromium.org

[dgozman, 2017-06-08 17:30:01]

+yutak for Time.h

lgtm modulo test output, but please wait for Time.h review

https://codereview.chromium.org/2928833002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/cache-storage/cache-deletion-expected.txt
(right):

https://codereview.chromium.org/2928833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/cache-storage/cache-deletion-expected.txt:12:
0, http://fake.request.com/1, OK, 12/31/1969, 4:00:00 PM
Won't this flake? I think it depends on the current time zone? Let's drop them.

[kristipark, 2017-06-08 19:38:22]

https://codereview.chromium.org/2928833002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector/cache-storage/cache-deletion-expected.txt
(right):

https://codereview.chromium.org/2928833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector/cache-storage/cache-deletion-expected.txt:12:
0, http://fake.request.com/1, OK, 12/31/1969, 4:00:00 PM
On 2017/06/08 17:30:01, dgozman wrote:
> Won't this flake? I think it depends on the current time zone? Let's drop
them.

Good point, removed responseTime from cache tree dump

[Yuta Kitamura, 2017-06-09 06:20:02]

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

[Yuta Kitamura, 2017-06-09 06:20:03]

+horo

I'm not sure you expected WebServiceWorkerResponse's
ResponseTime is used this way. For me, it seems redundant
and weird to get the time value exposed as int64_t
(internal value) and convert it back to Time.

And also this patch somewhat breaks the contract that
WTF::TimeTicks' internal value is for testing purposes
only (as shown in WTF::TimeTicks::ToInternalValueForTesting).

horo, WDYT?

[Yuta Kitamura, 2017-06-09 06:26:03]

https://codereview.chromium.org/2928833002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/cachestorage/InspectorCacheStorageAgent.cpp
(right):

https://codereview.chromium.org/2928833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:169:
double responseTime;
We should spell this "response_time" now.

https://codereview.chromium.org/2928833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/wtf/Time.h (right):

https://codereview.chromium.org/2928833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/wtf/Time.h:43: time_t ToDoubleT() const {
return value_.ToDoubleT(); }
WTF::Time is now an alias of base::Time (see line 24), so you don't have to
touch this file at all, do you? I think the changes in this file are unused.

[kristipark, 2017-06-09 17:37:13]

Thanks for the review!

https://codereview.chromium.org/2928833002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/cachestorage/InspectorCacheStorageAgent.cpp
(right):

https://codereview.chromium.org/2928833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:169:
double responseTime;
On 2017/06/09 06:26:03, Yuta Kitamura wrote:
> We should spell this "response_time" now.

Done.

https://codereview.chromium.org/2928833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/wtf/Time.h (right):

https://codereview.chromium.org/2928833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/wtf/Time.h:43: time_t ToDoubleT() const {
return value_.ToDoubleT(); }
On 2017/06/09 06:26:03, Yuta Kitamura wrote:
> WTF::Time is now an alias of base::Time (see line 24), so you don't have to
> touch this file at all, do you? I think the changes in this file are unused.

You are correct, thanks for pointing that out. Removed!

[horo, 2017-06-12 02:37:34]

On 2017/06/09 06:20:03, Yuta Kitamura wrote:
> +horo
> 
> I'm not sure you expected WebServiceWorkerResponse's
> ResponseTime is used this way. For me, it seems redundant
> and weird to get the time value exposed as int64_t
> (internal value) and convert it back to Time.

I think it was because there was no way to keep the int64_t value of base::Time
in Blink.
But after https://codereview.chromium.org/2543663002, we have WTF::Time.
So we should use WTF::Time in FetchResponseData.response_time_.
Filed crbug.com/732214

> 
> And also this patch somewhat breaks the contract that
> WTF::TimeTicks' internal value is for testing purposes
> only (as shown in WTF::TimeTicks::ToInternalValueForTesting).
> 
> horo, WDYT?

[Yuta Kitamura, 2017-06-12 04:48:16]

On 2017/06/12 02:37:34, horo wrote:
> I think it was because there was no way to keep the int64_t value of
base::Time
> in Blink.
> But after https://codereview.chromium.org/2543663002, we have WTF::Time.
> So we should use WTF::Time in FetchResponseData.response_time_.
> Filed crbug.com/732214

Thanks for context. Given that there's a plan to change that,
I think this patch's approach is good to go, as we can remove
the redundant conversion later.

So LGTM (we no longer have wtf changes though :) )

[kristipark, 2017-06-12 17:25:52]

The CQ bit was checked by kristipark@chromium.org

[kristipark, 2017-06-12 17:25:52]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2928833002/#ps60001
(title: "Removed Time.h changes")

[commit-bot: I haz the power, 2017-06-12 17:26:19]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 22:42:26]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497288352080420,
"parent_rev": "0c983d19e007ba71e65931b4c86794c000e3f93d", "commit_rev":
"d3ec2cc58603cc0578e7f3f17ddba5ff768303a7"}

[commit-bot: I haz the power, 2017-06-12 22:42:36]

Description was changed from

==========
[CacheStorage] [DevTools] Added "Time Received" column to cache storage data
grid

Screenshot of the change in the bug.

Since the entry response time is given as an internal UTC format, I added
wrapper methods to Time.h to convert it to standard UTC format.

BUG=727922
==========

to

==========
[CacheStorage] [DevTools] Added "Time Received" column to cache storage data
grid

Screenshot of the change in the bug.

Since the entry response time is given as an internal UTC format, I added
wrapper methods to Time.h to convert it to standard UTC format.

BUG=727922

Review-Url: https://codereview.chromium.org/2928833002
Cr-Commit-Position: refs/heads/master@{#478797}
Committed:
https://chromium.googlesource.com/chromium/src/+/d3ec2cc58603cc0578e7f3f17ddb...
==========

[commit-bot: I haz the power, 2017-06-12 22:42:38]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d3ec2cc58603cc0578e7f3f17ddb...