ServiceWorker: Make a way to retrieve a service worker version id (Blink)

ServiceWorker: Make a way to retrieve a service worker version id (Blink)

This is used to make a key of MemoryCache:
https://codereview.chromium.org/640463003/

Chromium-side: https://codereview.chromium.org/669303003/

BUG=423219
TEST=n/a

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

[nhiroki, 2014-10-24 02:16:08]

Patchset #1 (id:1) has been deleted

[nhiroki, 2014-10-24 07:13:22]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2014-10-27 12:56:32]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2014-10-28 00:57:29]

Patchset #1 (id:60001) has been deleted

[nhiroki, 2014-10-28 01:08:39]

nhiroki@chromium.org changed reviewers:
+ horo@chromium.org, mkwst@chromium.org

[nhiroki, 2014-10-28 01:10:14]

Hi mkwst@ and horo@, could you review this? Thanks!

[horo, 2014-10-28 01:20:21]

https://codereview.chromium.org/650033007/diff/80001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/650033007/diff/80001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:632: if (!m_documentLoader)
ResourceFetcher of the imported HTML doesn't have m_documentLoader.
So we should check localFrame->loader().documentLoader().

See: https://codereview.chromium.org/655373003/

[nhiroki, 2014-10-28 05:18:14]

Patchset #2 (id:100001) has been deleted

[nhiroki, 2014-10-28 05:21:23]

Updated.

https://codereview.chromium.org/650033007/diff/80001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/650033007/diff/80001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:632: if (!m_documentLoader)
On 2014/10/28 01:20:21, horo wrote:
> ResourceFetcher of the imported HTML doesn't have m_documentLoader.
> So we should check localFrame->loader().documentLoader().
> 
> See: https://codereview.chromium.org/655373003/

Thanks for your information! Done in the same way with you.

[Mike West, 2014-10-28 08:09:24]

https://codereview.chromium.org/650033007/diff/120001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/650033007/diff/120001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:631: return -1;
Why are you using -1 as the invalid value? Why not 0? 0 makes comparisons
simpler to understand, and would allow you to make this a `uint64_t` instead.
That would be less prone to breakage.

[nhiroki, 2014-10-28 08:34:01]

Thank you for reviewing.

https://codereview.chromium.org/650033007/diff/120001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/650033007/diff/120001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:631: return -1;
On 2014/10/28 08:09:24, Mike West wrote:
> Why are you using -1 as the invalid value? Why not 0? 0 makes comparisons
> simpler to understand, and would allow you to make this a `uint64_t` instead.
> That would be less prone to breakage.

This seviceWorkerID is corresponding to the ServiceWorkerVersionId in Chromium
and it's zero-based number (-1 is used as an invalid number[1]).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/ser...

[Mike West, 2014-10-28 08:35:22]

On 2014/10/28 08:34:01, nhiroki wrote:
> This seviceWorkerID is corresponding to the ServiceWorkerVersionId in Chromium
> and it's zero-based number (-1 is used as an invalid number[1]).

Why is it 0-based? If there's a good reason, brilliant! If not, I'd prefer to
change that.

[Mike West, 2014-10-28 08:36:58]

On 2014/10/28 08:35:22, Mike West wrote:
> On 2014/10/28 08:34:01, nhiroki wrote:
> > This seviceWorkerID is corresponding to the ServiceWorkerVersionId in
Chromium
> > and it's zero-based number (-1 is used as an invalid number[1]).
> 
> Why is it 0-based? If there's a good reason, brilliant! If not, I'd prefer to
> change that.

Hrm. All of those constants, really. It seems cleaner to be able to do `!id`
checks than to check against a constant. I don't understand the rationale behind
the current system, though I'm sure there's a good reason.

That said, I'm happy to deal with that in a separate CL. Given the current
infrastructure, this patch LGTM.

[nhiroki, 2014-10-28 09:32:03]

On 2014/10/28 08:36:58, Mike West wrote:
> On 2014/10/28 08:35:22, Mike West wrote:
> > On 2014/10/28 08:34:01, nhiroki wrote:
> > > This seviceWorkerID is corresponding to the ServiceWorkerVersionId in
> Chromium
> > > and it's zero-based number (-1 is used as an invalid number[1]).
> > 
> > Why is it 0-based? If there's a good reason, brilliant! If not, I'd prefer
to
> > change that.
> 
> Hrm. All of those constants, really. It seems cleaner to be able to do `!id`
> checks than to check against a constant. I don't understand the rationale
behind
> the current system, though I'm sure there's a good reason.
>
> That said, I'm happy to deal with that in a separate CL. Given the current
> infrastructure, this patch LGTM.

I don't know its reason, but I guess that the primal identifier (ProviderId) is
zero-based because it's generated by base::StaticAtomicSequenceNumber and other
identifiers follow it. Anyway, I'll consider it later. Thank you for pointing it
out.

[horo, 2014-10-29 03:16:53]

lgtm

[nhiroki, 2014-10-29 04:11:08]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-10-29 04:11:24]

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

[commit-bot: I haz the power, 2014-10-29 05:15:06]

Committed patchset #3 (id:140001) as 184561