Rename WebServiceWorkerRegistry to WebServiceWorkerProvider

Rename WebServiceWorkerRegistry to WebServiceWorkerProvider

After working on the chromium side, we changed the model a bit.
- Now the WebFrameClient method is more of a factory, and the WebServiceWorkerProvider (formerly the Registry) is owned per-page
- There's now a per-page client so that errors that originate from the browser side can bubble back out to the renderer.

The client interface hasn't been flushed out, but there's a stub didFailToStart() call, that
is the only known error that can propagate from the browser

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

[alecflett, 2013-10-04 23:57:00]

Michael/Kinuko - here's a revision to the Blink side of things, now that the
ownership is a little better factored, we also have a place to put an error
handler.

[michaeln, 2013-10-05 02:35:58]

https://codereview.chromium.org/26078002/diff/4001/Source/modules/servicework...
File Source/modules/serviceworkers/NavigatorServiceWorker.h (right):

https://codereview.chromium.org/26078002/diff/4001/Source/modules/servicework...
Source/modules/serviceworkers/NavigatorServiceWorker.h:48: class
NavigatorServiceWorker : public Supplement<Navigator>, public
WebKit::WebServiceWorkerProviderClient {
At what point in the Document creation process / lifecycle does this object get
instanced and later disposed? I'm gonna assume its lazily created on first
access to 'navigator' (or something there abouts).

I'm thinking a better 'owner' for the Provider is ultimately somewhere in the
loader classes, and this object that provides scriptable API could access it
thru that owner which establishes ownership early on.

This looks like an ok way to hook things up so that it work for just script
method calls for now, but at some point soon'ish i'll probably need to give it a
new owner, probably DocumentLoader, and ensure it's instanced prior to
DocumentLoader::startLoadingMainResource() time.

Does that make sense?

One thing that doesn't work out for that is having the
WebServiceWorkerProviderClient interface be implemented by this class since it
won't be created till some time long after startLoadingMainResource time.

[alecflett, 2013-10-07 18:00:30]

On 2013/10/05 02:35:58, michaeln wrote:
>
https://codereview.chromium.org/26078002/diff/4001/Source/modules/servicework...
> File Source/modules/serviceworkers/NavigatorServiceWorker.h (right):
> 
>
https://codereview.chromium.org/26078002/diff/4001/Source/modules/servicework...
> Source/modules/serviceworkers/NavigatorServiceWorker.h:48: class
> NavigatorServiceWorker : public Supplement<Navigator>, public
> WebKit::WebServiceWorkerProviderClient {
> At what point in the Document creation process / lifecycle does this object
get
> instanced and later disposed? I'm gonna assume its lazily created on first
> access to 'navigator' (or something there abouts).

At the moment, yes.

> 
> I'm thinking a better 'owner' for the Provider is ultimately somewhere in the
> loader classes, and this object that provides scriptable API could access it
> thru that owner which establishes ownership early on.

Agreed.

> 
> This looks like an ok way to hook things up so that it work for just script
> method calls for now, but at some point soon'ish i'll probably need to give it
a
> new owner, probably DocumentLoader, and ensure it's instanced prior to
> DocumentLoader::startLoadingMainResource() time.

Yep.

> 
> Does that make sense?
> 
> One thing that doesn't work out for that is having the
> WebServiceWorkerProviderClient interface be implemented by this class since it
> won't be created till some time long after startLoadingMainResource time.

Yeah exactly - it's temporary so I had *something* to hook up. I'd like to
eventually hook this up to the console, and even for that I need to be connected
to an object that has a ScriptExecutionContext, so I'll need a new object for
that.

I'll add a log message just so we know where this would fire, and which method
to move when the time comes.

[alecflett, 2013-10-07 18:31:08]

ok, new version is up with FIXMEs and logging error handler. michaeln@ PTAL?

[michaeln, 2013-10-07 20:09:53]

https://codereview.chromium.org/26078002/diff/9001/Source/modules/servicework...
File Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right):

https://codereview.chromium.org/26078002/diff/9001/Source/modules/servicework...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:72: m_provider =
adoptPtr(client->createServiceWorkerProvider(this));
I'm glad my comments made sense and you bought that view of how to stitch this
together. With that in mind, maybe instead of deriving NavigatorServiceWorker
from WebServiceWorkerProviderClient and passing 'this' here, declare a
datamember and pass &m_emptyProviderClient to the create call.

WebServiceWorkerProviderClient m_emptyProviderClient;

And, we'll take it as a FIXME to hook those up properly deeper in the loader.

The reason is, that deriviation is not going to stick and there is no burning
need for the didFailToStart() method doing anything right now, whereas there is
a burning need to get registerServiceWorker() to actually do something.

Does that make sense?

[alecflett, 2013-10-07 20:42:00]

ok, I made it a local anonymous-namespaced class - I think being able to catch
errors on the blink/renderer side is going to be useful even before this gets
totally hooked up.

[michaeln, 2013-10-07 20:48:59]

On 2013/10/07 20:42:00, alecflett wrote:
> ok, I made it a local anonymous-namespaced class - I think being able to catch
> errors on the blink/renderer side is going to be useful even before this gets
> totally hooked up.

bah... r[i|e]tveld is screwing up the diff somehow, just sayz "error: old chunk
mismatch". Can you reupload or do something to remedy that?

[michaeln, 2013-10-07 21:08:06]

https://codereview.chromium.org/26078002/diff/46001/Source/modules/servicewor...
File Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right):

https://codereview.chromium.org/26078002/diff/46001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:86: m_provider =
adoptPtr(client->createServiceWorkerProvider(new
ServiceWorkerProviderClientLogger()));
The ownership doesn't look right. Who is responsible for deleting the 'client'
object? Let's get that nailed down and spelled out clearly up in the webkit API.

I'd suggest that 'blink' is responsible for deleting the 'client' and the
embedder should NOT call delete on it. Those are the semantics your first
earlier snapshots suggested, so i was surprised by this new snapshot which
inverted things.

To express these semantics in the webkit API, let's add a protected virtual
destructor to WebKit::WebServiceWorkerProviderClient... so the embedder can't
delete it and if somebody tries to write code that does so, the compiler will
complain at them.

We'll have to new/delete the stub 'client' object in this class.

[michaeln, 2013-10-07 21:35:43]

> We'll have to new/delete the stub 'client' object in this class.

lgtm... passing null for now works too :)

[jamesr, 2013-10-07 21:41:01]

Who provides the Client in this interface?  The usual pattern when having a
WebFoo and a WebFooClient is that the embedder creates+owns the WebFooClient and
blink creates the WebFoo assuming that the WebFooClient will outlive it.  See
http://dev.chromium.org/blink/public-c-api.  It looks like this is the other way
'round, which makes the naming confusing.

[commit-bot: I haz the power, 2013-10-07 21:42:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/26078002/51001

[alecflett, 2013-10-07 21:54:42]

On 2013/10/07 21:41:01, jamesr wrote:
> Who provides the Client in this interface?  The usual pattern when having a
> WebFoo and a WebFooClient is that the embedder creates+owns the WebFooClient
and
> blink creates the WebFoo assuming that the WebFooClient will outlive it.  See
> http://dev.chromium.org/blink/public-c-api.  It looks like this is the other
way
> 'round, which makes the naming confusing.

jamesr@ - yes, it is the other way around, but it's more or less following
existing patterns for things that need it - appcache, media player, etc. 

In this case the shared worker is something the browser owns, and the renderer
wants to observe its behavior.

[alecflett, 2013-10-07 21:56:44]

abarth@ - after doing some work on the chrome side, we needed to change the Web*
side of ServiceWorkers a bit to make ownership clearer, add a 'Client' (Which we
thought we could avoid, but now have a use case for) and improve the naming.
(Registry -> Provider)

[kinuko, 2013-10-08 00:51:58]

On 2013/10/07 21:54:42, alecflett wrote:
> On 2013/10/07 21:41:01, jamesr wrote:
> > Who provides the Client in this interface?  The usual pattern when having a
> > WebFoo and a WebFooClient is that the embedder creates+owns the WebFooClient
> and
> > blink creates the WebFoo assuming that the WebFooClient will outlive it. 
See
> > http://dev.chromium.org/blink/public-c-api.  It looks like this is the other
> way
> > 'round, which makes the naming confusing.
> 
> jamesr@ - yes, it is the other way around, but it's more or less following
> existing patterns for things that need it - appcache, media player, etc. 
> 
> In this case the shared worker is something the browser owns, and the renderer
> wants to observe its behavior.

Does it make sense to rename the Client interface 'Observer' or something to
avoid confusion?  SWProvider can provide {add,remove}Observer() method so that
we can attach NavigatorServiceWorker or some scriptable objects later than the
SWProvider creation.

[michaeln, 2013-10-08 01:33:28]

> Does it make sense to rename the Client interface 'Observer' or something to
> avoid confusion?  SWProvider can provide {add,remove}Observer() method so that
> we can attach NavigatorServiceWorker or some scriptable objects later than the
> SWProvider creation.

Personally, i don't think it helps much. Jame's confusion is due to the fact
that chromium is providing the implementation instead of blink. No amount of
naming voodoo is going to change that fact. This just isn't like WebFrame
(implemented by blink) + WebFrameClient (by chromium), it is like other
WebThings (implemented by chromium) + WebThingClients (by blink). Both of those
are in use in the project and I think the WebFoo and WebFooClient naming pattern
work fine in both cases.

As for late binding, i'd like to just say it won't be late bound and will be
setup at creation time and make that so eventually, so we won't need a setter
for it (or a list of obervers like you might expect given the name observer).

[kinuko, 2013-10-08 01:56:47]

On 2013/10/08 01:33:28, michaeln wrote:
> > Does it make sense to rename the Client interface 'Observer' or something to
> > avoid confusion?  SWProvider can provide {add,remove}Observer() method so
that
> > we can attach NavigatorServiceWorker or some scriptable objects later than
the
> > SWProvider creation.
> 
> Personally, i don't think it helps much. Jame's confusion is due to the fact
> that chromium is providing the implementation instead of blink. No amount of
> naming voodoo is going to change that fact. This just isn't like WebFrame
> (implemented by blink) + WebFrameClient (by chromium), it is like other
> WebThings (implemented by chromium) + WebThingClients (by blink).
> Both of those are in use in the project and I think the WebFoo and
WebFooClient naming pattern
> work fine in both cases.

The fact that chromium providing implementation itself should be fine as it's
also a prevalent pattern especially in platform API.  I just worried that the
term 'client' is becoming to indicate more specific pattern in Blink in these
days (while it's true that there're some existing WebThing+WebThingClient
patterns), and also separately wondered if 'observer' might be a better
abstraction here.  (I'd like to defer these decisions to API owners though)

> As for late binding, i'd like to just say it won't be late bound and will be
> setup at creation time and make that so eventually, so we won't need a setter
> for it (or a list of obervers like you might expect given the name observer).

I'm slightly worried about module/serviceworker and core/loader layering issues,
though looks like you have some design ideas on it.

[alecflett, 2013-10-08 21:48:37]

On 2013/10/08 01:56:47, kinuko wrote:
> On 2013/10/08 01:33:28, michaeln wrote:
> > > Does it make sense to rename the Client interface 'Observer' or something
to
> > > avoid confusion?  SWProvider can provide {add,remove}Observer() method so
> that
> > > we can attach NavigatorServiceWorker or some scriptable objects later than
> the
> > > SWProvider creation.
> > 
> > Personally, i don't think it helps much. Jame's confusion is due to the fact
> > that chromium is providing the implementation instead of blink. No amount of
> > naming voodoo is going to change that fact. This just isn't like WebFrame
> > (implemented by blink) + WebFrameClient (by chromium), it is like other
> > WebThings (implemented by chromium) + WebThingClients (by blink).
> > Both of those are in use in the project and I think the WebFoo and
> WebFooClient naming pattern
> > work fine in both cases.
> 
> The fact that chromium providing implementation itself should be fine as it's
> also a prevalent pattern especially in platform API.  I just worried that the
> term 'client' is becoming to indicate more specific pattern in Blink in these
> days (while it's true that there're some existing WebThing+WebThingClient
> patterns), and also separately wondered if 'observer' might be a better
> abstraction here.  (I'd like to defer these decisions to API owners though)

There's enough of a a precedent for this in blink that I think we're ok. I'd
like to avoid an observer/client pattern that lets you add/remove - it makes
reasoning about those systems much harder.
> 
> > As for late binding, i'd like to just say it won't be late bound and will be
> > setup at creation time and make that so eventually, so we won't need a
setter
> > for it (or a list of obervers like you might expect given the name
observer).
> 
> I'm slightly worried about module/serviceworker and core/loader layering
issues,
> though looks like you have some design ideas on it.

Yeah, michael and I have talked through it, I think we have a good idea where to
hook this all up.

The basic simplifications that serviceworkers have that appcache and shared
workers do not is that the setup (register/unregister) is totally separate from
the function of the service worker itself. So all those objects can have
somewhat disconnected (and even ephemeral) lifetimes. The only place where this
isn't quite as clean is the error handler for for the script not loading in the
first place. I'm stubbing that out here right now, but I'll be moving it to the
correct place later.

[alecflett, 2013-10-08 21:49:06]

abarth@ - sorry we just need your input / lgtm here.

[abarth-chromium, 2013-10-10 03:35:37]

On 2013/10/08 21:48:37, alecflett wrote:
> There's enough of a a precedent for this in blink that I think we're ok. I'd
> like to avoid an observer/client pattern that lets you add/remove - it makes
> reasoning about those systems much harder.

Makes sense.

[abarth-chromium, 2013-10-10 03:44:01]

https://codereview.chromium.org/26078002/diff/62001/Source/modules/servicewor...
File Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right):

https://codereview.chromium.org/26078002/diff/62001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:71: FrameLoaderClient*
client = m_navigator->frame()->loader()->client();
m_navigator->frame() can be null.  What should we do in that case?

https://codereview.chromium.org/26078002/diff/62001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:73: m_provider =
adoptPtr(client->createServiceWorkerProvider(0));
We shouldn't be calling adoptPtr here.  Isntead, we should call adoptPtr
immediately after receiving the value over the API.  The rest of the functions
should work in terms of PassOwnPtr and OwnPtr.

https://codereview.chromium.org/26078002/diff/62001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:102:
provider->registerServiceWorker(scriptExecutionContext->url(), pattern,
scriptUrl, new CallbackPromiseAdapter(resolver, scriptExecutionContext));
Why are you passing in the scriptExecutionContext->url() here instead of the
m_navigator->frame()->document()->url() ?  What do you do if
|scriptExecutionContext| and m_navigator->frame()->document() aren't the same
object?  Also, you're not handling the case where m_navigator->frame() is null.

https://codereview.chromium.org/26078002/diff/62001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:121:
provider->unregisterServiceWorker(scriptExecutionContext->url(), pattern, new
CallbackPromiseAdapter(resolver, scriptExecutionContext));
Again, why are you using the |scriptExecutionContext| for the context instead of
m_navigator->frame()->document() ?

https://codereview.chromium.org/26078002/diff/62001/Source/web/FrameLoaderCli...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/26078002/diff/62001/Source/web/FrameLoaderCli...
Source/web/FrameLoaderClientImpl.cpp:759: return
m_webFrame->client()->createServiceWorkerProvider(m_webFrame, client);
This is where you should call adoptPtr.

Who controls the lifetime of |client|?  Is ownership of the object passed to the
embedder?  Is the object controlled by the whims of the V8 garbage collector? 
It's hard to tell in this CL because it looks like you're always passing 0.

https://codereview.chromium.org/26078002/diff/62001/public/platform/WebServic...
File public/platform/WebServiceWorkerProvider.h (right):

https://codereview.chromium.org/26078002/diff/62001/public/platform/WebServic...
public/platform/WebServiceWorkerProvider.h:45: virtual void
registerServiceWorker(const WebURL& origin, const WebString& pattern, const
WebURL& scriptUrl, WebServiceWorkerCallbacks*) { }
Is this a URL or an origin?  The caller is passing in a URL rather than an
origin.  The two aren't the same thing.

https://codereview.chromium.org/26078002/diff/62001/public/platform/WebServic...
File public/platform/WebServiceWorkerProviderClient.h (right):

https://codereview.chromium.org/26078002/diff/62001/public/platform/WebServic...
public/platform/WebServiceWorkerProviderClient.h:47: virtual
~WebServiceWorkerProviderClient() { }
I guess this means the client is owned by Blink.  How long is the embedder
allowed to retain the client?

[alecflett, 2013-10-10 23:03:35]

Cleaned up url resolution, simplified ownership model.

https://codereview.chromium.org/26078002/diff/62001/Source/modules/servicewor...
File Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right):

https://codereview.chromium.org/26078002/diff/62001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:71: FrameLoaderClient*
client = m_navigator->frame()->loader()->client();
On 2013/10/10 03:44:02, abarth wrote:
> m_navigator->frame() can be null.  What should we do in that case?

Done.

https://codereview.chromium.org/26078002/diff/62001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:73: m_provider =
adoptPtr(client->createServiceWorkerProvider(0));
On 2013/10/10 03:44:02, abarth wrote:
> We shouldn't be calling adoptPtr here.  Isntead, we should call adoptPtr
> immediately after receiving the value over the API.  The rest of the functions
> should work in terms of PassOwnPtr and OwnPtr.

Done.

https://codereview.chromium.org/26078002/diff/62001/Source/web/FrameLoaderCli...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/26078002/diff/62001/Source/web/FrameLoaderCli...
Source/web/FrameLoaderClientImpl.cpp:759: return
m_webFrame->client()->createServiceWorkerProvider(m_webFrame, client);
On 2013/10/10 03:44:02, abarth wrote:
> This is where you should call adoptPtr.
> 
> Who controls the lifetime of |client|?  Is ownership of the object passed to
the
> embedder?  Is the object controlled by the whims of the V8 garbage collector? 
> It's hard to tell in this CL because it looks like you're always passing 0.

Done.

https://codereview.chromium.org/26078002/diff/62001/public/platform/WebServic...
File public/platform/WebServiceWorkerProvider.h (right):

https://codereview.chromium.org/26078002/diff/62001/public/platform/WebServic...
public/platform/WebServiceWorkerProvider.h:45: virtual void
registerServiceWorker(const WebURL& origin, const WebString& pattern, const
WebURL& scriptUrl, WebServiceWorkerCallbacks*) { }
On 2013/10/10 03:44:02, abarth wrote:
> Is this a URL or an origin?  The caller is passing in a URL rather than an
> origin.  The two aren't the same thing.

Ditching the passing of the origin, and just letting the embedder evaluate that.

https://codereview.chromium.org/26078002/diff/62001/public/platform/WebServic...
File public/platform/WebServiceWorkerProviderClient.h (right):

https://codereview.chromium.org/26078002/diff/62001/public/platform/WebServic...
public/platform/WebServiceWorkerProviderClient.h:47: virtual
~WebServiceWorkerProviderClient() { }
On 2013/10/10 03:44:02, abarth wrote:
> I guess this means the client is owned by Blink.  How long is the embedder
> allowed to retain the client?

So I tweaked ownership a bit, letting the embedder own this. There's no good
reason for the embedder to delete it early, but the ownership model is simpler
this way and there are enough OwnPtr's around so that at least from blink's
perspective, ownership has been handed off.

In reality the Provider is owned by Blink, and will pass ownership of the client
back to the provider, so when the provider goes away, it will delete the client.

[abarth-chromium, 2013-10-11 18:08:13]

https://codereview.chromium.org/26078002/diff/76001/Source/core/loader/EmptyC...
File Source/core/loader/EmptyClients.cpp (right):

https://codereview.chromium.org/26078002/diff/76001/Source/core/loader/EmptyC...
Source/core/loader/EmptyClients.cpp:144:
PassOwnPtr<WebKit::WebServiceWorkerProvider>
EmptyFrameLoaderClient::createServiceWorkerProvider(PassOwnPtr<WebKit::WebServiceWorkerProviderClient>)
We probably avoided a leak in this case too...  Does the caller expect that the
WebKit::WebServiceWorkerProviderClient might be deleted during the call to
createServiceWorkerProvider, as happens here?

https://codereview.chromium.org/26078002/diff/76001/Source/core/loader/EmptyC...
File Source/core/loader/EmptyClients.h (right):

https://codereview.chromium.org/26078002/diff/76001/Source/core/loader/EmptyC...
Source/core/loader/EmptyClients.h:251: virtual
PassOwnPtr<WebKit::WebServiceWorkerProvider>
createServiceWorkerProvider(PassOwnPtr<WebKit::WebServiceWorkerProviderClient>)
OVERRIDE;
Thanks!  This is so much clearer!

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
File Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right):

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:109:
RefPtr<SecurityOrigin> documentOrigin = SecurityOrigin::create(documentUrl);
Instead of creating a new security origin from the document's URL, please just
use frame->document()->securityOrigin() instead.  You can't determine the
SecurityOrigin of a Document from it's URL.  The only way to determine its
SecurityOrigin is to read document()->securityOrigin().

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:112: if
(SecurityOrigin::create(patternUrl)->equal(documentOrigin.get())) {
It's extremely rare that you want to call SecurityOrigin::equal.  What are you
trying to test here?  It's very likely you want to call
document()->securityOrigin()->canRequest(patternURL).

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:118: if
(SecurityOrigin::create(scriptUrl)->equal(documentOrigin.get())) {
Ditto.

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:125: if (provider)
How can this function return |0|?  The only case where it can return |0| is when
|m_frame| is |0|, but you've checked that case already.

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:128:
resolver->reject(ScriptValue());
I don't think this case can ever occur.

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:148:
RefPtr<SecurityOrigin> documentOrigin = SecurityOrigin::create(documentUrl);
Same problem.

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:151: if
(SecurityOrigin::create(patternUrl)->equal(documentOrigin.get())) {
ditto

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:158: if (provider)
ditto

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
File Source/modules/serviceworkers/NavigatorServiceWorker.h (right):

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.h:63:
WebKit::WebServiceWorkerProvider* serviceWorkerProvider();
Sometimes we call functions like this ensureProvider() to indicate that it will
create m_provider if it's not there already.

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.h:68:
OwnPtr<WebKit::WebServiceWorkerProvider> m_provider;
I don't understand how m_provider is cleaned up.  Don't we need to drop
m_provider when we get detached from the frame?  Otherwise we'll try to talk to
the m_provide after the WebFrame has been torn down.  Presumably you need to
subclass DOMWindowProperty and watch for willDetachGlobalObjectFromFrame and
willDestroyGlobalObjectInFrame like DOMWindowIndexedDatabase does:

void DOMWindowIndexedDatabase::willDetachGlobalObjectFromFrame()
{
    m_idbFactory = nullptr;
    DOMWindowProperty::willDetachGlobalObjectFromFrame();
}

https://codereview.chromium.org/26078002/diff/76001/public/platform/WebServic...
File public/platform/WebServiceWorkerProvider.h (right):

https://codereview.chromium.org/26078002/diff/76001/public/platform/WebServic...
public/platform/WebServiceWorkerProvider.h:45: virtual void
registerServiceWorker(const WebURL& pattern, const WebURL& scriptUrl,
WebServiceWorkerCallbacks*) { }
I don't understand.  A pattern is something different from a URL.  A pattern
stands for a set of URLs, not a single URL.

[alecflett, 2013-10-11 18:52:22]

Thanks for the feedback. I've addressed everything except the pattern / WebURL
stuff. LMK what you think.

https://codereview.chromium.org/26078002/diff/76001/Source/core/loader/EmptyC...
File Source/core/loader/EmptyClients.cpp (right):

https://codereview.chromium.org/26078002/diff/76001/Source/core/loader/EmptyC...
Source/core/loader/EmptyClients.cpp:144:
PassOwnPtr<WebKit::WebServiceWorkerProvider>
EmptyFrameLoaderClient::createServiceWorkerProvider(PassOwnPtr<WebKit::WebServiceWorkerProviderClient>)
On 2013/10/11 18:08:14, abarth wrote:
> We probably avoided a leak in this case too...  Does the caller expect that
the
> WebKit::WebServiceWorkerProviderClient might be deleted during the call to
> createServiceWorkerProvider, as happens here?

Yes, I think that's reasonable.

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
File Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right):

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:109:
RefPtr<SecurityOrigin> documentOrigin = SecurityOrigin::create(documentUrl);
On 2013/10/11 18:08:14, abarth wrote:
> Instead of creating a new security origin from the document's URL, please just
> use frame->document()->securityOrigin() instead.  You can't determine the
> SecurityOrigin of a Document from it's URL.  The only way to determine its
> SecurityOrigin is to read document()->securityOrigin().

Done.

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:112: if
(SecurityOrigin::create(patternUrl)->equal(documentOrigin.get())) {
On 2013/10/11 18:08:14, abarth wrote:
> It's extremely rare that you want to call SecurityOrigin::equal.  What are you
> trying to test here?  It's very likely you want to call
> document()->securityOrigin()->canRequest(patternURL).

Thanks I really wasn't sure what was appropriate there.. I haven't completely
figured out if the origin restrictions that we've described here are the same as
canRequest() or something else:

https://github.com/slightlyoff/ServiceWorker/blob/master/explainer.md#cross-o...


I mean there we mostly say "same origin" which is pretty specific.. canRequest
looks sufficiently restrictive for now.

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
File Source/modules/serviceworkers/NavigatorServiceWorker.h (right):

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.h:63:
WebKit::WebServiceWorkerProvider* serviceWorkerProvider();
On 2013/10/11 18:08:14, abarth wrote:
> Sometimes we call functions like this ensureProvider() to indicate that it
will
> create m_provider if it's not there already.

Done.

https://codereview.chromium.org/26078002/diff/76001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.h:68:
OwnPtr<WebKit::WebServiceWorkerProvider> m_provider;
On 2013/10/11 18:08:14, abarth wrote:
> I don't understand how m_provider is cleaned up.  Don't we need to drop
> m_provider when we get detached from the frame?  Otherwise we'll try to talk
to
> the m_provide after the WebFrame has been torn down.  Presumably you need to
> subclass DOMWindowProperty and watch for willDetachGlobalObjectFromFrame and
> willDestroyGlobalObjectInFrame like DOMWindowIndexedDatabase does:
> 
> void DOMWindowIndexedDatabase::willDetachGlobalObjectFromFrame()
> {
>     m_idbFactory = nullptr;
>     DOMWindowProperty::willDetachGlobalObjectFromFrame();
> }

Done. This hangs off navigator, but I assume it's ok to use navigator->frame()
in the constructor here.

https://codereview.chromium.org/26078002/diff/76001/public/platform/WebServic...
File public/platform/WebServiceWorkerProvider.h (right):

https://codereview.chromium.org/26078002/diff/76001/public/platform/WebServic...
public/platform/WebServiceWorkerProvider.h:45: virtual void
registerServiceWorker(const WebURL& pattern, const WebURL& scriptUrl,
WebServiceWorkerCallbacks*) { }
On 2013/10/11 18:08:14, abarth wrote:
> I don't understand.  A pattern is something different from a URL.  A pattern
> stands for a set of URLs, not a single URL.

Well for now WebURL is about the closest container we have, and we don't expect
to change it until we get some developer feedback. 

I could switch back to strings, but I feel like that drops even more semantics
than WebURL, and would just be sending a url string that would be
reconsistituted into a URL on the other side again:
https://github.com/slightlyoff/ServiceWorker/blob/master/explainer.md#urls-do...

We may possibly think about something like this:
http://developer.chrome.com/extensions/match_patterns.html but we don't really
know right now. Also, we know we're going to have to evaluate the pattern
browser-side, but we don't' know if we'll also want to evaluate it blink-side,
for instance if we need to synchronously decide if a pattern is valid or not.

Would it be alright if I use WebURL for now and change later once we know more?

[abarth-chromium, 2013-10-11 18:59:18]

On 2013/10/11 18:52:22, alecflett wrote:
> Would it be alright if I use WebURL for now and change later once we know
more?

Sure.  It sounds like that part of the spec in a bit of flux.  I bet you're
going to want to create a WebURLPattern class at some point, but you don't need
to do that now.

[abarth-chromium, 2013-10-11 19:02:40]

LGTM!  Thanks for iterating on this CL.

https://codereview.chromium.org/26078002/diff/88001/Source/modules/servicewor...
File Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right):

https://codereview.chromium.org/26078002/diff/88001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:56: :
DOMWindowProperty(navigator->frame())
Yeah, that's fine.

https://codereview.chromium.org/26078002/diff/88001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:74: ASSERT(frame);
I would move this ASSERT above the "if (!m_provider)" line so that you get more
coverage from it.  It's an invariant of this function that it must be called
while we're still connected with the frame, regardless of whether we're already
created the provider.

https://codereview.chromium.org/26078002/diff/88001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:110: KURL patternUrl =
scriptExecutionContext->completeURL(pattern);
patternUrl -> patternURL

https://codereview.chromium.org/26078002/diff/88001/Source/modules/servicewor...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:116: KURL scriptUrl =
scriptExecutionContext->completeURL(scriptSrc);
scriptUrl -> scriptURL

(Notice, for example, that completeURL has URL in all caps.)

[commit-bot: I haz the power, 2013-10-11 19:11:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/26078002/93001

[commit-bot: I haz the power, 2013-10-11 21:13:21]

Failed to trigger a try job on win_blink_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-10-11 21:13:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/26078002/103001

[commit-bot: I haz the power, 2013-10-12 00:25:11]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_blink_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-13 17:26:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/26078002/113001

[commit-bot: I haz the power, 2013-10-13 18:49:23]

Change committed as 159543