Introduce ServiceWorkerRegistrationOptions struct for service worker options

Introduce ServiceWorkerRegistrationOptions struct for service worker options

This patch introduces ServiceWorkerRegistrationOptions struct to
the ServiceWorkerRegistration IPC message. The service worker spec has
a couple of options that need to be passed between the browser and blink.
It allows future implementation of other options, as currently
ServiceWorkerRegistration has the maximum number of arguments.

BUG=675540
Review-Url: https://codereview.chromium.org/2968123002
Cr-Commit-Position: refs/heads/master@{#485227}
Committed: https://chromium.googlesource.com/chromium/src/+/85a04df639d3e3c05220e7bdb311b05a9470e0f1

[yuryu, 2017-07-06 08:49:34]

The CQ bit was checked by yuryu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-06 08:49:42]

Dry run: CQ is trying da patch.

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

[yuryu, 2017-07-06 08:57:43]

yuryu@chromium.org changed reviewers:
+ nhiroki@chromium.org

[yuryu, 2017-07-06 08:57:44]

Ptal, I've split the patch into two, and this will first change the IPC to allow
introduction of the updateViaCache option. thanks.

[commit-bot: I haz the power, 2017-07-06 11:11:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-06 11:11:13]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2017-07-07 02:02:09]

LGTM with minor comments.

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
File content/common/service_worker/service_worker_types.cc (right):

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.cc:135:
ServiceWorkerRegistrationOptions::ServiceWorkerRegistrationOptions() {}
= default; ?

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.h:223: 
Can you add a struct-level comment like this?

// Represents options for register():
// https://w3c.github.io/ServiceWorker/v1/#dictdef-registrationoptions

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.h:225:
ServiceWorkerRegistrationOptions();
Is this ctor necessary?

(I guess this is required by the Chrome IPC mechanism, but want to confirm it
just in case)

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.h:226: // Initialize |scope|
with the specified value and the rest with defaults
s/Initialize/Initializes/

See: https://google.github.io/styleguide/cppguide.html#Function_Comments

"These comments should be descriptive ("Opens the file") rather than imperative
("Open the file");"

[nhiroki, 2017-07-07 02:03:07]

nhiroki@chromium.org changed reviewers:
+ dominickn@chromium.org, kinuko@chromium.org

[nhiroki, 2017-07-07 02:03:08]

+kinuko@ for content/
+dominickn@ for
chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc

[nhiroki, 2017-07-07 02:05:01]

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

[nhiroki, 2017-07-07 02:05:02]

+mkwst@ for content/common/service_worker/service_worker_messages.h

[nhiroki, 2017-07-07 02:08:59]

Regarding the CL description:

"It allows future implementation of other options, as currently
ServiceWorkerRegistration has the maximum number of arguments."

What's the maximum number of arguments?

[kinuko, 2017-07-07 02:59:31]

lgtm

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.h:228: GURL scope;
nit: maybe add a lightweight TODO comment to add/support other spec'ed options
(to clarify why we need this struct)

[dominickn, 2017-07-07 03:02:55]

add_to_homescreen_data_fetcher_unittest.cc lgtm

[dominickn, 2017-07-07 03:04:32]

General nit: you should explicitly #include
"content/common/service_worker/service_worker_types.h" in the test files you've
changed to IWYU.

https://codereview.chromium.org/2968123002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc
(right):

https://codereview.chromium.org/2968123002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:25:
#include "content/browser/service_worker/service_worker_context_core.h"
Nit: #include "content/common/service_worker/service_worker_types.h" as well

[falken, 2017-07-07 03:25:31]

falken@chromium.org changed reviewers:
+ falken@chromium.org

[falken, 2017-07-07 03:25:32]

Looks good.

https://codereview.chromium.org/2968123002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/2968123002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_core.h:199: // worker for
initial installation.
not related to your CL but while here can you delete this obsolete comment?

https://codereview.chromium.org/2968123002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/2968123002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_job_unittest.cc:870:
ServiceWorkerRegistrationOptions(test_origin.Resolve(kScope)), NULL,
optional nit: if you like you can just s/NULL/nullptr in this entire file.

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.h:226: // Initialize |scope|
with the specified value and the rest with defaults
On 2017/07/07 02:02:09, nhiroki wrote:
> s/Initialize/Initializes/
> 
> See: https://google.github.io/styleguide/cppguide.html#Function_Comments
> 
> "These comments should be descriptive ("Opens the file") rather than
imperative
> ("Open the file");"

IMO this comment is unnecessary... it's known that the constructor will
initialize with the params. Also there are no members other than |scope| now, so
it's a bit confusing.

[Mike West, 2017-07-07 11:11:05]

IPC LGTM.

[yuryu, 2017-07-10 03:38:11]

https://codereview.chromium.org/2968123002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc
(right):

https://codereview.chromium.org/2968123002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc:25:
#include "content/browser/service_worker/service_worker_context_core.h"
On 2017/07/07 03:04:32, dominickn wrote:
> Nit: #include "content/common/service_worker/service_worker_types.h" as well

Done.

https://codereview.chromium.org/2968123002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/2968123002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_core.h:199: // worker for
initial installation.
On 2017/07/07 03:25:32, falken wrote:
> not related to your CL but while here can you delete this obsolete comment?

Done.

https://codereview.chromium.org/2968123002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/2968123002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_job_unittest.cc:870:
ServiceWorkerRegistrationOptions(test_origin.Resolve(kScope)), NULL,
On 2017/07/07 03:25:32, falken wrote:
> optional nit: if you like you can just s/NULL/nullptr in this entire file.

Done.

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
File content/common/service_worker/service_worker_types.cc (right):

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.cc:135:
ServiceWorkerRegistrationOptions::ServiceWorkerRegistrationOptions() {}
On 2017/07/07 02:02:09, nhiroki wrote:
> = default; ?

Done.

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.h:223: 
On 2017/07/07 02:02:09, nhiroki wrote:
> Can you add a struct-level comment like this?
> 
> // Represents options for register():
> // https://w3c.github.io/ServiceWorker/v1/#dictdef-registrationoptions

Done.

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.h:225:
ServiceWorkerRegistrationOptions();
On 2017/07/07 02:02:09, nhiroki wrote:
> Is this ctor necessary?
> 
> (I guess this is required by the Chrome IPC mechanism, but want to confirm it
> just in case)

Default ctor is necessary because RegistrationOptions can be initialized without
any explicit values. I'm changing it to = default to make it clearer.

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.h:226: // Initialize |scope|
with the specified value and the rest with defaults
On 2017/07/07 03:25:32, falken wrote:
> On 2017/07/07 02:02:09, nhiroki wrote:
> > s/Initialize/Initializes/
> > 
> > See: https://google.github.io/styleguide/cppguide.html#Function_Comments
> > 
> > "These comments should be descriptive ("Opens the file") rather than
> imperative
> > ("Open the file");"
> 
> IMO this comment is unnecessary... it's known that the constructor will
> initialize with the params. Also there are no members other than |scope| now,
so
> it's a bit confusing.

Removing the comment entirely.

https://codereview.chromium.org/2968123002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.h:228: GURL scope;
On 2017/07/07 02:59:31, kinuko wrote:
> nit: maybe add a lightweight TODO comment to add/support other spec'ed options
> (to clarify why we need this struct)

Done.

[yuryu, 2017-07-10 03:39:18]

The CQ bit was checked by yuryu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-10 03:39:24]

Dry run: CQ is trying da patch.

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

[falken, 2017-07-10 03:46:30]

lgtm

https://codereview.chromium.org/2968123002/diff/20001/content/common/service_...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/2968123002/diff/20001/content/common/service_...
content/common/service_worker/service_worker_types.h:225: //
https://w3c.github.io/ServiceWorker/v1/#dictdef-registrationoptions
Please link to Service Worker Nightly, not v1:
https://w3c.github.io/ServiceWorker/#dictdef-registrationoptions

Chromium almost always tracks the most recent spec, not a versioned spec.

https://codereview.chromium.org/2968123002/diff/20001/content/common/service_...
content/common/service_worker/service_worker_types.h:228: // Initialize |scope|
with the specified value and the rest with defaults
I recommend removing this comment, or else changing it to "Initializes" and
adding a period at the end.

[commit-bot: I haz the power, 2017-07-10 05:40:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-10 05:40:09]

Dry run: This issue passed the CQ dry run.

[yuryu, 2017-07-10 09:01:30]

https://codereview.chromium.org/2968123002/diff/20001/content/common/service_...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/2968123002/diff/20001/content/common/service_...
content/common/service_worker/service_worker_types.h:225: //
https://w3c.github.io/ServiceWorker/v1/#dictdef-registrationoptions
On 2017/07/10 03:46:30, falken wrote:
> Please link to Service Worker Nightly, not v1:
> https://w3c.github.io/ServiceWorker/#dictdef-registrationoptions
> 
> Chromium almost always tracks the most recent spec, not a versioned spec.
> 

Done.

https://codereview.chromium.org/2968123002/diff/20001/content/common/service_...
content/common/service_worker/service_worker_types.h:228: // Initialize |scope|
with the specified value and the rest with defaults
On 2017/07/10 03:46:30, falken wrote:
> I recommend removing this comment, or else changing it to "Initializes" and
> adding a period at the end.

I meant to remove it, but somehow it's left in the previous commit. Removing it
entirely.

[yuryu, 2017-07-10 09:02:14]

The CQ bit was checked by yuryu@chromium.org

[yuryu, 2017-07-10 09:02:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, mkwst@chromium.org, nhiroki@chromium.org,
kinuko@chromium.org, falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2968123002/#ps40001
(title: "address more comments")

[commit-bot: I haz the power, 2017-07-10 09:02:23]

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-07-10 10:43:10]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1499677334962000,
"parent_rev": "00824436b20d36ccd8c98f895364940aed1a17c1", "commit_rev":
"85a04df639d3e3c05220e7bdb311b05a9470e0f1"}

[commit-bot: I haz the power, 2017-07-10 10:43:34]

Description was changed from

==========
Introduce ServiceWorkerRegistrationOptions struct for service worker options

This patch introduces ServiceWorkerRegistrationOptions struct to
the ServiceWorkerRegistration IPC message. The service worker spec has
a couple of options that need to be passed between the browser and blink.
It allows future implementation of other options, as currently
ServiceWorkerRegistration has the maximum number of arguments.

BUG=675540
==========

to

==========
Introduce ServiceWorkerRegistrationOptions struct for service worker options

This patch introduces ServiceWorkerRegistrationOptions struct to
the ServiceWorkerRegistration IPC message. The service worker spec has
a couple of options that need to be passed between the browser and blink.
It allows future implementation of other options, as currently
ServiceWorkerRegistration has the maximum number of arguments.

BUG=675540

Review-Url: https://codereview.chromium.org/2968123002
Cr-Commit-Position: refs/heads/master@{#485227}
Committed:
https://chromium.googlesource.com/chromium/src/+/85a04df639d3e3c05220e7bdb311...
==========

[commit-bot: I haz the power, 2017-07-10 10:43:36]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/85a04df639d3e3c05220e7bdb311...

[yuryu, 2017-07-10 10:48:43]

On 2017/07/07 02:08:59, nhiroki wrote:
> Regarding the CL description:
> 
> "It allows future implementation of other options, as currently
> ServiceWorkerRegistration has the maximum number of arguments."
> 
> What's the maximum number of arguments?

The maximum is 5.

[nhiroki, 2017-07-11 00:05:27]

On 2017/07/10 10:48:43, yuryu wrote:
> On 2017/07/07 02:08:59, nhiroki wrote:
> > Regarding the CL description:
> > 
> > "It allows future implementation of other options, as currently
> > ServiceWorkerRegistration has the maximum number of arguments."
> > 
> > What's the maximum number of arguments?
> 
> The maximum is 5.

Ah, I understand it means *IPC's* maximum number of arguments :)