Add the beginning of a public Service Worker API.

Add the beginning of a public Service Worker API.

This supports registration and unregistration from the UI thread.

BUG=355131
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260338

[Jeffrey Yasskin, 2014-03-22 01:47:19]

jam will need to look at this to approve the new public API, but I'd like the
ServiceWorker team to check it first.

[alecflett, 2014-03-25 20:50:38]

lgtm

This looks nice and clean!

https://codereview.chromium.org/208843004/diff/60001/content/public/common/se...
File content/public/common/service_worker_status_code.h (right):

https://codereview.chromium.org/208843004/diff/60001/content/public/common/se...
content/public/common/service_worker_status_code.h:5: #ifndef
CONTENT_COMMON_SERVICE_WORKER_SERVICE_WORKER_STATUS_CODE_H_
Make sure to fix the header defines..

[Jeffrey Yasskin, 2014-03-25 21:13:09]

John, your turn! Are you happy with this as a public content API?

Two questions to consider:
1. Should this API live on the UI thread as is currently proposed, or on the IO
thread where most of its work happens?
2. Is it right to share the StatusCode enum between the public API and the
internal implementation, or should there be a separately-curated public set of
error values?

https://codereview.chromium.org/208843004/diff/60001/content/public/common/se...
File content/public/common/service_worker_status_code.h (right):

https://codereview.chromium.org/208843004/diff/60001/content/public/common/se...
content/public/common/service_worker_status_code.h:5: #ifndef
CONTENT_COMMON_SERVICE_WORKER_SERVICE_WORKER_STATUS_CODE_H_
On 2014/03/25 20:50:39, alecflett wrote:
> Make sure to fix the header defines..

Oops, thanks, done. :)

[jam, 2014-03-25 22:59:29]

It's hard for me to review this (and answer the questions below) without knowing
how this will be used in src\chrome. The cl description and bug are too sparse.
Can you add more background?

On 2014/03/25 21:13:09, Jeffrey Yasskin wrote:
> John, your turn! Are you happy with this as a public content API?
> 
> Two questions to consider:
> 1. Should this API live on the UI thread as is currently proposed, or on the
IO
> thread where most of its work happens?
> 2. Is it right to share the StatusCode enum between the public API and the
> internal implementation, or should there be a separately-curated public set of
> error values?
> 
>
https://codereview.chromium.org/208843004/diff/60001/content/public/common/se...
> File content/public/common/service_worker_status_code.h (right):
> 
>
https://codereview.chromium.org/208843004/diff/60001/content/public/common/se...
> content/public/common/service_worker_status_code.h:5: #ifndef
> CONTENT_COMMON_SERVICE_WORKER_SERVICE_WORKER_STATUS_CODE_H_
> On 2014/03/25 20:50:39, alecflett wrote:
> > Make sure to fix the header defines..
> 
> Oops, thanks, done. :)

[Jeffrey Yasskin, 2014-03-25 23:04:24]

Ah, sorry. I'm working on this to support http://crbug.com/344048 (which this
CL's bug blocks indirectly), a Chrome App based on Service Workers, which has a
design doc at
https://docs.google.com/a/chromium.org/document/d/1szeOHrr_qEJGSNbDtEqeKcGDkL....

The initial use is in https://codereview.chromium.org/182253010/, which I'm
splitting up into submittable pieces.

I believe Alec also needs to build on top of this API surface, but I'll let him
explain what his plans are.

On 2014/03/25 22:59:29, jam wrote:
> It's hard for me to review this (and answer the questions below) without
knowing
> how this will be used in src\chrome. The cl description and bug are too
sparse.
> Can you add more background?
> 
> On 2014/03/25 21:13:09, Jeffrey Yasskin wrote:
> > John, your turn! Are you happy with this as a public content API?
> > 
> > Two questions to consider:
> > 1. Should this API live on the UI thread as is currently proposed, or on the
> IO
> > thread where most of its work happens?
> > 2. Is it right to share the StatusCode enum between the public API and the
> > internal implementation, or should there be a separately-curated public set
of
> > error values?
> > 
> >
>
https://codereview.chromium.org/208843004/diff/60001/content/public/common/se...
> > File content/public/common/service_worker_status_code.h (right):
> > 
> >
>
https://codereview.chromium.org/208843004/diff/60001/content/public/common/se...
> > content/public/common/service_worker_status_code.h:5: #ifndef
> > CONTENT_COMMON_SERVICE_WORKER_SERVICE_WORKER_STATUS_CODE_H_
> > On 2014/03/25 20:50:39, alecflett wrote:
> > > Make sure to fix the header defines..
> > 
> > Oops, thanks, done. :)

[alecflett, 2014-03-25 23:30:31]

On 2014/03/25 23:04:24, Jeffrey Yasskin wrote:
> Ah, sorry. I'm working on this to support http://crbug.com/344048 (which this
> CL's bug blocks indirectly), a Chrome App based on Service Workers, which has
a
> design doc at
>
https://docs.google.com/a/chromium.org/document/d/1szeOHrr_qEJGSNbDtEqeKcGDkL....
> 
> The initial use is in https://codereview.chromium.org/182253010/, which I'm
> splitting up into submittable pieces.
> 
> I believe Alec also needs to build on top of this API surface, but I'll let
him
> explain what his plans are.

In short:
1) I'd like to add a method to this to find all open registrations, (following
the same patterns here for Register/Unregister) and I'll be moving 
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...
and related code into public/browser/
2) I'm working on an EmbeddedWorkerService which handles the worker part of this
on the browser side, so that DevTools can get at it. Not specifically blocked by
this, but it will be less out of place when this lands.

> 
> On 2014/03/25 22:59:29, jam wrote:
> > It's hard for me to review this (and answer the questions below) without
> knowing
> > how this will be used in src\chrome. The cl description and bug are too
> sparse.
> > Can you add more background?
> > 
> > On 2014/03/25 21:13:09, Jeffrey Yasskin wrote:
> > > John, your turn! Are you happy with this as a public content API?
> > > 
> > > Two questions to consider:
> > > 1. Should this API live on the UI thread as is currently proposed, or on
the
> > IO
> > > thread where most of its work happens?
> > > 2. Is it right to share the StatusCode enum between the public API and the
> > > internal implementation, or should there be a separately-curated public
set
> of
> > > error values?
> > > 
> > >
> >
>
https://codereview.chromium.org/208843004/diff/60001/content/public/common/se...
> > > File content/public/common/service_worker_status_code.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/208843004/diff/60001/content/public/common/se...
> > > content/public/common/service_worker_status_code.h:5: #ifndef
> > > CONTENT_COMMON_SERVICE_WORKER_SERVICE_WORKER_STATUS_CODE_H_
> > > On 2014/03/25 20:50:39, alecflett wrote:
> > > > Make sure to fix the header defines..
> > > 
> > > Oops, thanks, done. :)

[jam, 2014-03-25 23:41:23]

On 2014/03/25 23:04:24, Jeffrey Yasskin wrote:
> Ah, sorry. I'm working on this to support http://crbug.com/344048 (which this
> CL's bug blocks indirectly), a Chrome App based on Service Workers, which has
a
> design doc at
>
https://docs.google.com/a/chromium.org/document/d/1szeOHrr_qEJGSNbDtEqeKcGDkL....
> 
> The initial use is in https://codereview.chromium.org/182253010/, which I'm
> splitting up into submittable pieces.
> 
> I believe Alec also needs to build on top of this API surface, but I'll let
him
> explain what his plans are.

From the design doc, I see that Chrome Apps would use SWs. Ok. But I don't get
why it needs a C++ interface to SWs? i.e. can you explain to me why the
extensions code can't interact with SWs purely through JS, i.e. what the web
does?


On 2014/03/25 23:30:31, alecflett wrote:
> In short:
> 1) I'd like to add a method to this to find all open registrations, (following
> the same patterns here for Register/Unregister) and I'll be moving 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...
> and related code into public/browser/
> 2) I'm working on an EmbeddedWorkerService which handles the worker part of
this
> on the browser side, so that DevTools can get at it. Not specifically blocked
by
> this, but it will be less out of place when this lands.

devtools is implemented in content, so it seems that's orthogonal from exposing
a public API to SWs?

[alecflett, 2014-03-25 23:53:14]

On 2014/03/25 23:41:23, jam wrote:
> 
> devtools is implemented in content, so it seems that's orthogonal from
exposing
> a public API to SWs?

The code which enumerates debuggable devtools targets is in
chrome/browser/devtools/devtools_target_impl.cc

[Jeffrey Yasskin, 2014-03-25 23:54:23]

On Tue, Mar 25, 2014 at 4:41 PM,  <jam@chromium.org> wrote:
> On 2014/03/25 23:04:24, Jeffrey Yasskin wrote:
>>
>> Ah, sorry. I'm working on this to support http://crbug.com/344048 (which
>> this
>> CL's bug blocks indirectly), a Chrome App based on Service Workers, which
>> has
>
> a
>>
>> design doc at
>
>
>
https://docs.google.com/a/chromium.org/document/d/1szeOHrr_qEJGSNbDtEqeKcGDkL....
>
>> The initial use is in https://codereview.chromium.org/182253010/, which
>> I'm
>> splitting up into submittable pieces.
>
>
>> I believe Alec also needs to build on top of this API surface, but I'll
>> let
>
> him
>>
>> explain what his plans are.
>
>
> From the design doc, I see that Chrome Apps would use SWs. Ok. But I don't
> get
> why it needs a C++ interface to SWs? i.e. can you explain to me why the
> extensions code can't interact with SWs purely through JS, i.e. what the web
> does?

Extensions are installed, uninstalled, and updated in the browser
process, which doesn't run JS. This API deals with registering and
unregistering the Service Worker in response.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kinuko, 2014-03-26 05:01:34]

Drive-by, mostly nits/minor comments only.

For having UI thread restriction on public ServiceWorkerContext interface I
think it's reasonable since it's going to be accessed via StoragePartition
(which is also a resident of UI thread) and the interface class itself isn't
made thread-safe.  (While I also feel the wrapper's impl itself could be
permissive for the restriction)

https://codereview.chromium.org/208843004/diff/170001/content/browser/service...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/208843004/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:435: class
ServiceWorkerBlackBoxBrowserTest : public ServiceWorkerBrowserTest {
(Ok to have this for now, but this file's going a bit wild, maybe we want to
have separate files for each)

https://codereview.chromium.org/208843004/diff/170001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/208843004/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:61:
base::Bind(&ServiceWorkerContextWrapper::RegisterServiceWorkerOnIO,
Instead of having yet another indirection method we can also do:

 if (!BrowserThread::CurrentlyOn(IO)) {
   BrowserThread::PostTask(IO, ..., Bind(&RegisterServiceWorker));
   return;
 }
 context()->RegisterServiceWorker(...);

We do this in ServiceWorkerContextWrapper::Shutdown().

Also FinishRegistrationOnIO and FinishUnregistrationOnIO could be a local static
method, then this class only needs 1 method impl for one public API instead of
3. I'm fine with the current rigid style but compactness has some win too (esp.
when we're in a wrapper class which is supposed to be thin)... wdyt?

https://codereview.chromium.org/208843004/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:69: void
ServiceWorkerContextWrapper::RegisterServiceWorkerOnIO(
I know this ordering has some benfits, but can we match method order in .cc and
in .h? (Per our style guide)

https://codereview.chromium.org/208843004/diff/170001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/208843004/diff/170001/content/public/browser/...
content/public/browser/service_worker_context.h:31: UnregistrationCallback;
nit: feels slightly verbose to typedef two callbacks for the same type, which
just returns status code. Are you going to typedef one callback type for each
async method regardless of actual callback type?  Another way to do is just
define a generic StatusCallback type for the ones that just return SWStatusCode
(so we'll have one callback typedef for one signature).

[jam, 2014-03-26 16:50:45]

On 2014/03/25 23:53:14, alecflett wrote:
> On 2014/03/25 23:41:23, jam wrote:
> > 
> > devtools is implemented in content, so it seems that's orthogonal from
> exposing
> > a public API to SWs?
> 
> The code which enumerates debuggable devtools targets is in
> chrome/browser/devtools/devtools_target_impl.cc

so this means that content_shell won't show service workers unless some other
code there is modified?

why can't the code which enumerates service workers be in src\content?

[jam, 2014-03-26 16:52:08]

On 2014/03/25 23:54:23, Jeffrey Yasskin wrote:
> On Tue, Mar 25, 2014 at 4:41 PM,  <mailto:jam@chromium.org> wrote:
> > On 2014/03/25 23:04:24, Jeffrey Yasskin wrote:
> >>
> >> Ah, sorry. I'm working on this to support http://crbug.com/344048 (which
> >> this
> >> CL's bug blocks indirectly), a Chrome App based on Service Workers, which
> >> has
> >
> > a
> >>
> >> design doc at
> >
> >
> >
>
https://docs.google.com/a/chromium.org/document/d/1szeOHrr_qEJGSNbDtEqeKcGDkL....
> >
> >> The initial use is in https://codereview.chromium.org/182253010/, which
> >> I'm
> >> splitting up into submittable pieces.
> >
> >
> >> I believe Alec also needs to build on top of this API surface, but I'll
> >> let
> >
> > him
> >>
> >> explain what his plans are.
> >
> >
> > From the design doc, I see that Chrome Apps would use SWs. Ok. But I don't
> > get
> > why it needs a C++ interface to SWs? i.e. can you explain to me why the
> > extensions code can't interact with SWs purely through JS, i.e. what the web
> > does?
> 
> Extensions are installed, uninstalled, and updated in the browser
> process, which doesn't run JS. This API deals with registering and
> unregistering the Service Worker in response.

I understand that we don't run JS in the browser; I meant why doesn't the
extension system run JS to interact with SW in the extension process?

[alecflett, 2014-03-26 17:35:25]

On 2014/03/26 16:50:45, jam wrote:
> On 2014/03/25 23:53:14, alecflett wrote:
> > On 2014/03/25 23:41:23, jam wrote:
> > > 
> > > devtools is implemented in content, so it seems that's orthogonal from
> > exposing
> > > a public API to SWs?
> > 
> > The code which enumerates debuggable devtools targets is in
> > chrome/browser/devtools/devtools_target_impl.cc
> 
> so this means that content_shell won't show service workers unless some other
> code there is modified?
> 
> why can't the code which enumerates service workers be in src\content?

I don't know - that's been beyond the scope of what I've been hooking up.

Yes, this does seem to mean that remote-debugging over http doesn't work in
content shell at the moment (but that's not serviceworker-specific)

Another use case for some of the stuff like enumeration is access from the
content settings UI in chrome - the thing that lets you clear your cache/idb/etc
will also want to expose the ability to find and clear/unregister service worker
registrations and caches

[Jeffrey Yasskin, 2014-03-26 18:03:37]

On Wed, Mar 26, 2014 at 9:52 AM,  <jam@chromium.org> wrote:
> On 2014/03/25 23:54:23, Jeffrey Yasskin wrote:
>
>> On Tue, Mar 25, 2014 at 4:41 PM,  <mailto:jam@chromium.org> wrote:
>> > On 2014/03/25 23:04:24, Jeffrey Yasskin wrote:
>> >>
>> >> Ah, sorry. I'm working on this to support http://crbug.com/344048
>> >> (which
>> >> this
>> >> CL's bug blocks indirectly), a Chrome App based on Service Workers,
>> >> which
>> >> has
>> >
>> > a
>> >>
>> >> design doc at
>> >
>> >
>> >
>
>
>
https://docs.google.com/a/chromium.org/document/d/1szeOHrr_qEJGSNbDtEqeKcGDkL....
>>
>> >
>> >> The initial use is in https://codereview.chromium.org/182253010/, which
>> >> I'm
>> >> splitting up into submittable pieces.
>> >
>> >
>> >> I believe Alec also needs to build on top of this API surface, but I'll
>> >> let
>> >
>> > him
>> >>
>> >> explain what his plans are.
>> >
>> >
>> > From the design doc, I see that Chrome Apps would use SWs. Ok. But I
>> > don't
>> > get
>> > why it needs a C++ interface to SWs? i.e. can you explain to me why the
>> > extensions code can't interact with SWs purely through JS, i.e. what the
>> > web
>> > does?
>
>
>> Extensions are installed, uninstalled, and updated in the browser
>> process, which doesn't run JS. This API deals with registering and
>> unregistering the Service Worker in response.
>
>
> I understand that we don't run JS in the browser; I meant why doesn't the
> extension system run JS to interact with SW in the extension process?

With the proposed Service Worker backend for the extension system,
there is no background page anymore, so there's actually no extension
process or JS context until the SW is registered. We could possibly
tell the extension to uninstall itself at the cost of some extra IPCs,
but doing that for installation would require a fake RenderView that
executes one line of JS and exits. That's not the sort of complexity I
want to make people maintain in the future.

We're also going to need to fire events in the SW based on
browser-known conditions. The SW team had the foresight to provide a
SendMessage function that can deliver an IPC to the SW, but we need to
be able to call it or something like it from places like
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex....

Jeffrey

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jeffrey Yasskin, 2014-03-26 19:58:51]

Thanks for looking over this, Kinuko.

https://codereview.chromium.org/208843004/diff/170001/content/browser/service...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/208843004/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:435: class
ServiceWorkerBlackBoxBrowserTest : public ServiceWorkerBrowserTest {
On 2014/03/26 05:01:34, kinuko wrote:
> (Ok to have this for now, but this file's going a bit wild, maybe we want to
> have separate files for each)

I agree with separate files. This test seems to belong in this file, even after
things are split up, but I can try to send you a CL moving some of the other
tests out.

https://codereview.chromium.org/208843004/diff/170001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/208843004/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:61:
base::Bind(&ServiceWorkerContextWrapper::RegisterServiceWorkerOnIO,
On 2014/03/26 05:01:34, kinuko wrote:
> Instead of having yet another indirection method we can also do:
> 
>  if (!BrowserThread::CurrentlyOn(IO)) {
>    BrowserThread::PostTask(IO, ..., Bind(&RegisterServiceWorker));
>    return;
>  }
>  context()->RegisterServiceWorker(...);
> 
> We do this in ServiceWorkerContextWrapper::Shutdown().

Sure, done.

> Also FinishRegistrationOnIO and FinishUnregistrationOnIO could be a local
static
> method, then this class only needs 1 method impl for one public API instead of
> 3. I'm fine with the current rigid style but compactness has some win too
(esp.
> when we're in a wrapper class which is supposed to be thin)... wdyt?

Sounds good. I think I liked making them member functions in order to keep the
order of definitions in the same order as the control flow, but with only one
helper function that's less compelling, and they could have been static in the
first place.

https://codereview.chromium.org/208843004/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:69: void
ServiceWorkerContextWrapper::RegisterServiceWorkerOnIO(
On 2014/03/26 05:01:34, kinuko wrote:
> I know this ordering has some benfits, but can we match method order in .cc
and
> in .h? (Per our style guide)

Obsolete now.

https://codereview.chromium.org/208843004/diff/170001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/208843004/diff/170001/content/public/browser/...
content/public/browser/service_worker_context.h:31: UnregistrationCallback;
On 2014/03/26 05:01:34, kinuko wrote:
> nit: feels slightly verbose to typedef two callbacks for the same type, which
> just returns status code. Are you going to typedef one callback type for each
> async method regardless of actual callback type?  Another way to do is just
> define a generic StatusCallback type for the ones that just return
SWStatusCode
> (so we'll have one callback typedef for one signature).

I'm suspicious that we'll want the RegistrationCallback to also take a
service_worker_id or ServiceWorkerHost or some such in order to tell the caller
where to send messages. But I'm not certain of that, so, sure, it makes sense to
just have a StatusCallback for now.

[jam, 2014-03-26 22:58:39]

On 2014/03/26 18:03:37, Jeffrey Yasskin wrote:
> On Wed, Mar 26, 2014 at 9:52 AM,  <mailto:jam@chromium.org> wrote:
> > On 2014/03/25 23:54:23, Jeffrey Yasskin wrote:
> >
> >> On Tue, Mar 25, 2014 at 4:41 PM,  <mailto:jam@chromium.org> wrote:
> >> > On 2014/03/25 23:04:24, Jeffrey Yasskin wrote:
> >> >>
> >> >> Ah, sorry. I'm working on this to support http://crbug.com/344048
> >> >> (which
> >> >> this
> >> >> CL's bug blocks indirectly), a Chrome App based on Service Workers,
> >> >> which
> >> >> has
> >> >
> >> > a
> >> >>
> >> >> design doc at
> >> >
> >> >
> >> >
> >
> >
> >
>
https://docs.google.com/a/chromium.org/document/d/1szeOHrr_qEJGSNbDtEqeKcGDkL....
> >>
> >> >
> >> >> The initial use is in https://codereview.chromium.org/182253010/, which
> >> >> I'm
> >> >> splitting up into submittable pieces.
> >> >
> >> >
> >> >> I believe Alec also needs to build on top of this API surface, but I'll
> >> >> let
> >> >
> >> > him
> >> >>
> >> >> explain what his plans are.
> >> >
> >> >
> >> > From the design doc, I see that Chrome Apps would use SWs. Ok. But I
> >> > don't
> >> > get
> >> > why it needs a C++ interface to SWs? i.e. can you explain to me why the
> >> > extensions code can't interact with SWs purely through JS, i.e. what the
> >> > web
> >> > does?
> >
> >
> >> Extensions are installed, uninstalled, and updated in the browser
> >> process, which doesn't run JS. This API deals with registering and
> >> unregistering the Service Worker in response.
> >
> >
> > I understand that we don't run JS in the browser; I meant why doesn't the
> > extension system run JS to interact with SW in the extension process?
> 
> With the proposed Service Worker backend for the extension system,
> there is no background page anymore, so there's actually no extension
> process or JS context until the SW is registered. We could possibly
> tell the extension to uninstall itself at the cost of some extra IPCs,
> but doing that for installation would require a fake RenderView that
> executes one line of JS and exits. That's not the sort of complexity I
> want to make people maintain in the future.
> 
> We're also going to need to fire events in the SW based on
> browser-known conditions. The SW team had the foresight to provide a
> SendMessage function that can deliver an IPC to the SW, but we need to
> be able to call it or something like it from places like
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex....

ah I see, thanks for the explanation. I wasn't familiar with these plans. It
would be great to put them in the bug and/or cl description

https://codereview.chromium.org/208843004/diff/320001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/208843004/diff/320001/content/public/browser/...
content/public/browser/service_worker_context.h:21: typedef GURL Scope;
nit: why this typdef? i.e. why not just have "const GURL& scope" below? having
typedefs is a bit confusing

https://codereview.chromium.org/208843004/diff/320001/content/public/browser/...
content/public/browser/service_worker_context.h:25: virtual void
RegisterServiceWorker(const Scope& pattern,
nit: document all the methods

https://codereview.chromium.org/208843004/diff/320001/content/public/browser/...
content/public/browser/service_worker_context.h:41:
DISALLOW_COPY_AND_ASSIGN(ServiceWorkerContext);
get rid of this private seciton; interfaces aren't copyable

https://codereview.chromium.org/208843004/diff/320001/content/public/common/s...
File content/public/common/service_worker_status_code.h (right):

https://codereview.chromium.org/208843004/diff/320001/content/public/common/s...
content/public/common/service_worker_status_code.h:13: enum
ServiceWorkerStatusCode {
where is this going to be used in chrome? i looked at the other cl
(https://codereview.chromium.org/182253010/) but didn't see them mentioned them.

will chrome layer need all this granularity of errors? or will a simple bool
suffice?

[Jeffrey Yasskin, 2014-03-27 02:51:59]

I haven't uploaded a new patch yet, but assume anything I haven't replied to
will be done tomorrow.

The bug blocks other bugs that eventually link to
https://docs.google.com/document/d/1szeOHrr_qEJGSNbDtEqeKcGDkLmwvftqTV731kQw2....
Is that the link you'd like to see in this CL's bug too?

https://codereview.chromium.org/208843004/diff/320001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/208843004/diff/320001/content/public/browser/...
content/public/browser/service_worker_context.h:21: typedef GURL Scope;
On 2014/03/26 22:58:39, jam wrote:
> nit: why this typdef? i.e. why not just have "const GURL& scope" below? having
> typedefs is a bit confusing

I don't feel terribly strongly about this, since it's not my API, but I've found
that when there's a parameter with extra semantics on top of its C++ type, it's
easier to figure out those semantics at call sites when the callee consistently
has a link to some documentation. If we provide a typedef name, that can act as
the link.

https://codereview.chromium.org/208843004/diff/320001/content/public/common/s...
File content/public/common/service_worker_status_code.h (right):

https://codereview.chromium.org/208843004/diff/320001/content/public/common/s...
content/public/common/service_worker_status_code.h:13: enum
ServiceWorkerStatusCode {
On 2014/03/26 22:58:39, jam wrote:
> where is this going to be used in chrome? i looked at the other cl
> (https://codereview.chromium.org/182253010/) but didn't see them mentioned
them.
> 
> will chrome layer need all this granularity of errors? or will a simple bool
> suffice?

This is used at
https://codereview.chromium.org/182253010/diff/120001/extensions/browser/serv....

I don't think the Chrome layer will need all of this granularity, but if the
registration fails we need to be able to give the user/developer some indication
of why. It's possible that we can do enough checking beforehand that we can
DCHECK that registration succeeds, in which case a 2-value result would be
enough. (I'm nervous about using a bool, since it's hard to tell if true or
false is success, so would encourage the API owners to pick a 2-value enum
instead.)

ServiceWorker folks: does that sound possible, for a script that should already
exist on the filesystem?

[jam, 2014-03-27 17:06:26]

https://codereview.chromium.org/208843004/diff/320001/content/public/common/s...
File content/public/common/service_worker_status_code.h (right):

https://codereview.chromium.org/208843004/diff/320001/content/public/common/s...
content/public/common/service_worker_status_code.h:13: enum
ServiceWorkerStatusCode {
On 2014/03/27 02:51:59, Jeffrey Yasskin wrote:
> On 2014/03/26 22:58:39, jam wrote:
> > where is this going to be used in chrome? i looked at the other cl
> > (https://codereview.chromium.org/182253010/) but didn't see them mentioned
> them.
> > 
> > will chrome layer need all this granularity of errors? or will a simple bool
> > suffice?
> 
> This is used at
>
https://codereview.chromium.org/182253010/diff/120001/extensions/browser/serv....
> 
> I don't think the Chrome layer will need all of this granularity, but if the
> registration fails we need to be able to give the user/developer some
indication
> of why. It's possible that we can do enough checking beforehand that we can
> DCHECK that registration succeeds, in which case a 2-value result would be
> enough. (I'm nervous about using a bool, since it's hard to tell if true or
> false is success, so would encourage the API owners to pick a 2-value enum
> instead.)
> 
> ServiceWorker folks: does that sound possible, for a script that should
already
> exist on the filesystem?

ok, yeah if content can expose less, that's better. perhaps the callback,
instead of currently receiving an enum value and having a method to convert that
to a string, can instead return a boolean and a string which contains the error
when the boolean is false? extension side can expose whatever it wants to apps

[michaeln, 2014-03-27 19:46:58]

> ServiceWorker folks: does that sound possible, for a script that should
already
> exist on the filesystem?

You mean can registration fail given the script is in the local filesystem? Yes,
one way is if the script is buggy and fails to load, but other things could go
wrong too (diskspace).

[Jeffrey Yasskin, 2014-03-27 20:33:05]

https://codereview.chromium.org/208843004/diff/320001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/208843004/diff/320001/content/public/browser/...
content/public/browser/service_worker_context.h:25: virtual void
RegisterServiceWorker(const Scope& pattern,
On 2014/03/26 22:58:39, jam wrote:
> nit: document all the methods

Done. I'll need the SW folks to double-check my comments.

https://codereview.chromium.org/208843004/diff/320001/content/public/browser/...
content/public/browser/service_worker_context.h:41:
DISALLOW_COPY_AND_ASSIGN(ServiceWorkerContext);
On 2014/03/26 22:58:39, jam wrote:
> get rid of this private seciton; interfaces aren't copyable

Done.

https://codereview.chromium.org/208843004/diff/320001/content/public/common/s...
File content/public/common/service_worker_status_code.h (right):

https://codereview.chromium.org/208843004/diff/320001/content/public/common/s...
content/public/common/service_worker_status_code.h:13: enum
ServiceWorkerStatusCode {
On 2014/03/27 17:06:27, jam wrote:
> On 2014/03/27 02:51:59, Jeffrey Yasskin wrote:
> > On 2014/03/26 22:58:39, jam wrote:
> > > where is this going to be used in chrome? i looked at the other cl
> > > (https://codereview.chromium.org/182253010/) but didn't see them mentioned
> > them.
> > > 
> > > will chrome layer need all this granularity of errors? or will a simple
bool
> > > suffice?
> > 
> > This is used at
> >
>
https://codereview.chromium.org/182253010/diff/120001/extensions/browser/serv....
> > 
> > I don't think the Chrome layer will need all of this granularity, but if the
> > registration fails we need to be able to give the user/developer some
> indication
> > of why. It's possible that we can do enough checking beforehand that we can
> > DCHECK that registration succeeds, in which case a 2-value result would be
> > enough. (I'm nervous about using a bool, since it's hard to tell if true or
> > false is success, so would encourage the API owners to pick a 2-value enum
> > instead.)
> > 
> > ServiceWorker folks: does that sound possible, for a script that should
> already
> > exist on the filesystem?
> 
> ok, yeah if content can expose less, that's better. perhaps the callback,
> instead of currently receiving an enum value and having a method to convert
that
> to a string, can instead return a boolean and a string which contains the
error
> when the boolean is false? extension side can expose whatever it wants to apps

Alec and Josh were skeptical of exposing a string, so I've tentatively switched
to a separate enum for public use. However, we might need to go back to a string
if things like parse errors and top-level exceptions can't be reported on the
content side.

[jam, 2014-03-27 22:48:25]

https://codereview.chromium.org/208843004/diff/320001/content/public/common/s...
File content/public/common/service_worker_status_code.h (right):

https://codereview.chromium.org/208843004/diff/320001/content/public/common/s...
content/public/common/service_worker_status_code.h:13: enum
ServiceWorkerStatusCode {
On 2014/03/27 20:33:06, Jeffrey Yasskin wrote:
> On 2014/03/27 17:06:27, jam wrote:
> > On 2014/03/27 02:51:59, Jeffrey Yasskin wrote:
> > > On 2014/03/26 22:58:39, jam wrote:
> > > > where is this going to be used in chrome? i looked at the other cl
> > > > (https://codereview.chromium.org/182253010/) but didn't see them
mentioned
> > > them.
> > > > 
> > > > will chrome layer need all this granularity of errors? or will a simple
> bool
> > > > suffice?
> > > 
> > > This is used at
> > >
> >
>
https://codereview.chromium.org/182253010/diff/120001/extensions/browser/serv....
> > > 
> > > I don't think the Chrome layer will need all of this granularity, but if
the
> > > registration fails we need to be able to give the user/developer some
> > indication
> > > of why. It's possible that we can do enough checking beforehand that we
can
> > > DCHECK that registration succeeds, in which case a 2-value result would be
> > > enough. (I'm nervous about using a bool, since it's hard to tell if true
or
> > > false is success, so would encourage the API owners to pick a 2-value enum
> > > instead.)
> > > 
> > > ServiceWorker folks: does that sound possible, for a script that should
> > already
> > > exist on the filesystem?
> > 
> > ok, yeah if content can expose less, that's better. perhaps the callback,
> > instead of currently receiving an enum value and having a method to convert
> that
> > to a string, can instead return a boolean and a string which contains the
> error
> > when the boolean is false? extension side can expose whatever it wants to
apps
> 
> Alec and Josh were skeptical of exposing a string, so I've tentatively
switched
> to a separate enum for public use. However, we might need to go back to a
string
> if things like parse errors and top-level exceptions can't be reported on the
> content side.

I had suggested bool+string in the public API because that's all what was used
in the link you sent me above. Is that changing? I prefer just having a boolean
until we know that something else is needed (instead of the current enum with
two values), since as a guideline we avoid enums with two values in the content
api

[Jeffrey Yasskin, 2014-03-28 00:33:19]

On 2014/03/27 22:48:25, jam wrote:
>
https://codereview.chromium.org/208843004/diff/320001/content/public/common/s...
> File content/public/common/service_worker_status_code.h (right):
> 
>
https://codereview.chromium.org/208843004/diff/320001/content/public/common/s...
> content/public/common/service_worker_status_code.h:13: enum
> ServiceWorkerStatusCode {
> On 2014/03/27 20:33:06, Jeffrey Yasskin wrote:
> > On 2014/03/27 17:06:27, jam wrote:
> > > On 2014/03/27 02:51:59, Jeffrey Yasskin wrote:
> > > > On 2014/03/26 22:58:39, jam wrote:
> > > > > where is this going to be used in chrome? i looked at the other cl
> > > > > (https://codereview.chromium.org/182253010/) but didn't see them
> mentioned
> > > > them.
> > > > > 
> > > > > will chrome layer need all this granularity of errors? or will a
simple
> > bool
> > > > > suffice?
> > > > 
> > > > This is used at
> > > >
> > >
> >
>
https://codereview.chromium.org/182253010/diff/120001/extensions/browser/serv....
> > > > 
> > > > I don't think the Chrome layer will need all of this granularity, but if
> the
> > > > registration fails we need to be able to give the user/developer some
> > > indication
> > > > of why. It's possible that we can do enough checking beforehand that we
> can
> > > > DCHECK that registration succeeds, in which case a 2-value result would
be
> > > > enough. (I'm nervous about using a bool, since it's hard to tell if true
> or
> > > > false is success, so would encourage the API owners to pick a 2-value
enum
> > > > instead.)
> > > > 
> > > > ServiceWorker folks: does that sound possible, for a script that should
> > > already
> > > > exist on the filesystem?
> > > 
> > > ok, yeah if content can expose less, that's better. perhaps the callback,
> > > instead of currently receiving an enum value and having a method to
convert
> > that
> > > to a string, can instead return a boolean and a string which contains the
> > error
> > > when the boolean is false? extension side can expose whatever it wants to
> apps
> > 
> > Alec and Josh were skeptical of exposing a string, so I've tentatively
> switched
> > to a separate enum for public use. However, we might need to go back to a
> string
> > if things like parse errors and top-level exceptions can't be reported on
the
> > content side.
> 
> I had suggested bool+string in the public API because that's all what was used
> in the link you sent me above. Is that changing? I prefer just having a
boolean
> until we know that something else is needed (instead of the current enum with
> two values), since as a guideline we avoid enums with two values in the
content
> api

We're going to need to report details of failures somehow, but the way isn't
totally clear yet, and maybe it can live inside Content. I've switched to a
simple bool result (success==true), without a string, for now.

[jam, 2014-03-28 18:08:06]

lgtm for files outside of service_worker directory

https://codereview.chromium.org/208843004/diff/320001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/208843004/diff/320001/content/public/browser/...
content/public/browser/service_worker_context.h:21: typedef GURL Scope;
On 2014/03/27 02:51:59, Jeffrey Yasskin wrote:
> On 2014/03/26 22:58:39, jam wrote:
> > nit: why this typdef? i.e. why not just have "const GURL& scope" below?
having
> > typedefs is a bit confusing
> 
> I don't feel terribly strongly about this, since it's not my API, but I've
found
> that when there's a parameter with extra semantics on top of its C++ type,
it's
> easier to figure out those semantics at call sites when the callee
consistently
> has a link to some documentation. If we provide a typedef name, that can act
as
> the link.

perhaps it should be a string instead of a GURL then (or a typdef of one)?

[Jeffrey Yasskin, 2014-03-28 20:33:31]

On Fri, Mar 28, 2014 at 11:08 AM,  <jam@chromium.org> wrote:
> lgtm for files outside of service_worker directory

Thanks!

>
https://codereview.chromium.org/208843004/diff/320001/content/public/browser/...
> File content/public/browser/service_worker_context.h (right):
>
>
https://codereview.chromium.org/208843004/diff/320001/content/public/browser/...
> content/public/browser/service_worker_context.h:21: typedef GURL Scope;
> On 2014/03/27 02:51:59, Jeffrey Yasskin wrote:
>>
>> On 2014/03/26 22:58:39, jam wrote:
>> > nit: why this typdef? i.e. why not just have "const GURL& scope"
>
> below? having
>>
>> > typedefs is a bit confusing
>
>
>> I don't feel terribly strongly about this, since it's not my API, but
>
> I've found
>>
>> that when there's a parameter with extra semantics on top of its C++
>
> type, it's
>>
>> easier to figure out those semantics at call sites when the callee
>
> consistently
>>
>> has a link to some documentation. If we provide a typedef name, that
>
> can act as
>>
>> the link.
>
>
> perhaps it should be a string instead of a GURL then (or a typdef of
> one)?

This suggestion doesn't make a whole lot of sense to me. `string` has
fewer constraints than GURL, while "scope" has more, so GURL is the
closest existing C++ type. If the content API doesn't like typedefs,
I'm not sure why it would prefer a typedef of a string over a typedef
of a GURL. I'm probably just missing something.

> https://codereview.chromium.org/208843004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2014-03-28 20:37:17]

On 2014/03/28 20:33:31, Jeffrey Yasskin wrote:
> On Fri, Mar 28, 2014 at 11:08 AM,  <mailto:jam@chromium.org> wrote:
> > lgtm for files outside of service_worker directory
> 
> Thanks!
> 
> >
>
https://codereview.chromium.org/208843004/diff/320001/content/public/browser/...
> > File content/public/browser/service_worker_context.h (right):
> >
> >
>
https://codereview.chromium.org/208843004/diff/320001/content/public/browser/...
> > content/public/browser/service_worker_context.h:21: typedef GURL Scope;
> > On 2014/03/27 02:51:59, Jeffrey Yasskin wrote:
> >>
> >> On 2014/03/26 22:58:39, jam wrote:
> >> > nit: why this typdef? i.e. why not just have "const GURL& scope"
> >
> > below? having
> >>
> >> > typedefs is a bit confusing
> >
> >
> >> I don't feel terribly strongly about this, since it's not my API, but
> >
> > I've found
> >>
> >> that when there's a parameter with extra semantics on top of its C++
> >
> > type, it's
> >>
> >> easier to figure out those semantics at call sites when the callee
> >
> > consistently
> >>
> >> has a link to some documentation. If we provide a typedef name, that
> >
> > can act as
> >>
> >> the link.
> >
> >
> > perhaps it should be a string instead of a GURL then (or a typdef of
> > one)?
> 
> This suggestion doesn't make a whole lot of sense to me. `string` has
> fewer constraints than GURL, while "scope" has more, so GURL is the
> closest existing C++ type. If the content API doesn't like typedefs,
> I'm not sure why it would prefer a typedef of a string over a typedef
> of a GURL. I'm probably just missing something.

oh nvm, I read the comment incorrectly (I thought it wasn't a url). personally I
think a typdef isn't needed and just complicates things for folks not very
familiar with the spec. but, i don't feel strongly about it so i defer to you

[Jeffrey Yasskin, 2014-03-28 23:40:23]

Alec suggests that we'll iterate on the API if it's not perfect today, and that
I should just commit. Hence, checking the commit box.

[Jeffrey Yasskin, 2014-03-28 23:40:29]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2014-03-28 23:42:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/208843004/420001

[commit-bot: I haz the power, 2014-03-29 01:42:07]

Change committed as 260338