ServiceWorker: Consolidate version change messages

ServiceWorker: Consolidate version change messages

This consolidates IPC messages for setting up version attributes
(ie. installing, waiting and active), and cleans up related functions.

The new message is intended to be used for ServiceWorkerRegistration
object that will be introduced by following CL[1] and it doesn't have
the controller field, so the controller still use a separate message.

[1] https://codereview.chromium.org/463013002/


BUG=384119, 396400
TEST=content_unittests --gtest_filter=ServiceWorker*
TEST=run_webkit_tests.py --debug http/tests/serviceworker

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

[nhiroki, 2014-08-11 05:03:17]

Ptal, thanks!

[falken, 2014-08-11 11:58:22]

This is nice and clean.

One thing I'm not sure I grasp is why controller is different than the others.
Why is that?

https://codereview.chromium.org/445883003/diff/320001/content/common/service_...
File content/common/service_worker/service_worker_types.cc (right):

https://codereview.chromium.org/445883003/diff/320001/content/common/service_...
content/common/service_worker/service_worker_types.cc:50:
ServiceWorkerVersionAttributes::~ServiceWorkerVersionAttributes() {}
What's the purpose of the empty dtor?

[nhiroki, 2014-08-11 12:46:29]

Thanks! Updated.

https://codereview.chromium.org/445883003/diff/320001/content/common/service_...
File content/common/service_worker/service_worker_types.cc (right):

https://codereview.chromium.org/445883003/diff/320001/content/common/service_...
content/common/service_worker/service_worker_types.cc:50:
ServiceWorkerVersionAttributes::~ServiceWorkerVersionAttributes() {}
On 2014/08/11 11:58:22, falken wrote:
> What's the purpose of the empty dtor?

This struct was more complex and a compiler gave warning "Complex class/struct
needs an explicit out-of-line constructor/destructor"[1]. Now this struct is
simplified and doesn't have to have such ctor/dtor. Removed them.

[1]
http://www.chromium.org/developers/coding-style/chromium-style-checker-errors...

[nhiroki, 2014-08-11 12:47:15]

On 2014/08/11 11:58:22, falken wrote:
> This is nice and clean.
> 
> One thing I'm not sure I grasp is why controller is different than the others.
> Why is that?

The new message is intended to use for ServiceWorkerRegistration object and it
doesn't have the controller field, so I think this separation could fit for the
situation. Wdyt?

(I should have explained it in the CL description, sorry!)

[michaeln, 2014-08-11 22:15:41]

this is look pretty good

i see some possible further simplications and i think it'd be worth whacking at
the providerhost tests to use the new public api and to test the new listening
behaviors... wdyt?

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:51: const
ServiceWorkerRegistrationInfo& info) {
maybe DCHECK(registration == accociated_registration), which is essentially the
gist of the CanSetVersion helper

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:77: if
(CanSetVersionAttribute(type, installing_version)) {
could this be replace with if (installing_version != installing_version_)?

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:113:
ServiceWorkerVersion* version) {
if this method took a ptr to the version and a ptr to the scoped_refptr<>
datamember to set, this method might be simplicified some (no switch statement
or type param required).

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:143: if
(!CanSetVersionAttribute(ChangedVersionAttributesMask::CONTROLLING_VERSION,
can this be simplified to if (version == controlling_version_) return;

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:226: bool
ServiceWorkerProviderHost::CanSetVersionAttribute(
I think this method could maybe be simplified or even removed? The version
setters are all private and only called by other methods if this class. Given
the callsites we know the versions are from our associated registrations so i
think we don't need all the runtime checks for that stuff.

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host_unittest.cc
(right):

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:64: void
SetActiveVersion(
would it make sense to use the public
ServiceWorkerRegistration::SetActiveVersion method for thee existing test now?
Also please add tests to specifically check the host listens for regisrtation
version changes.

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:124:
SetActiveVersion(provider_host1_, NULL);
This can be accomplished with the public host->UnassociateRegistration() method.

[nhiroki, 2014-08-12 01:45:56]

Thank you for reviewing! I think this became really simpler than the previous
patchset :)

Please take another look.

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:51: const
ServiceWorkerRegistrationInfo& info) {
On 2014/08/11 22:15:41, michaeln wrote:
> maybe DCHECK(registration == accociated_registration), which is essentially
the
> gist of the CanSetVersion helper

Done.

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:77: if
(CanSetVersionAttribute(type, installing_version)) {
On 2014/08/11 22:15:41, michaeln wrote:
> could this be replace with if (installing_version != installing_version_)?

Done.

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:113:
ServiceWorkerVersion* version) {
On 2014/08/11 22:15:41, michaeln wrote:
> if this method took a ptr to the version and a ptr to the scoped_refptr<>
> datamember to set, this method might be simplicified some (no switch statement
> or type param required).

Done.

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:143: if
(!CanSetVersionAttribute(ChangedVersionAttributesMask::CONTROLLING_VERSION,
On 2014/08/11 22:15:41, michaeln wrote:
> can this be simplified to if (version == controlling_version_) return;

Done.

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:226: bool
ServiceWorkerProviderHost::CanSetVersionAttribute(
On 2014/08/11 22:15:41, michaeln wrote:
> I think this method could maybe be simplified or even removed? The version
> setters are all private and only called by other methods if this class. Given
> the callsites we know the versions are from our associated registrations so i
> think we don't need all the runtime checks for that stuff.

Ah, good point! I think we can completely remove this function. Done.

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host_unittest.cc
(right):

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:64: void
SetActiveVersion(
On 2014/08/11 22:15:41, michaeln wrote:
> would it make sense to use the public
> ServiceWorkerRegistration::SetActiveVersion method for thee existing test now?

Done.

> Also please add tests to specifically check the host listens for regisrtation
> version changes.

Added tests.

https://codereview.chromium.org/445883003/diff/360001/content/browser/service...
content/browser/service_worker/service_worker_provider_host_unittest.cc:124:
SetActiveVersion(provider_host1_, NULL);
On 2014/08/11 22:15:41, michaeln wrote:
> This can be accomplished with the public host->UnassociateRegistration()
method.

Done.

[nhiroki, 2014-08-12 01:50:04]

jschuh@: Could you review service_worker_messages.h? Thanks.

[michaeln, 2014-08-12 03:02:38]

lgtm!

[falken, 2014-08-12 10:29:36]

lgtm2

[nasko, 2014-08-13 00:12:27]

IPC LGTM

[nhiroki, 2014-08-13 00:25:00]

On 2014/08/13 00:12:27, nasko wrote:
> IPC LGTM

Thank you!

[nhiroki, 2014-08-13 00:25:12]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2014-08-13 00:25:18]

The CQ bit was unchecked by nhiroki@chromium.org

[nhiroki, 2014-08-13 00:26:36]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-08-13 00:47:17]

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

[commit-bot: I haz the power, 2014-08-13 01:28:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/40863)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/46090)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[nhiroki, 2014-08-13 01:36:08]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-08-13 01:42:34]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/40874)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/46101)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-13 01:43:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nhiroki@chromium.org/445883003/440001

[commit-bot: I haz the power, 2014-08-13 09:55:28]

Change committed as 289240