ServiceWorker: Route unregister() through WebServiceWorkerRegistration for refactoring (2)

ServiceWorker: Route unregister() through WebServiceWorkerRegistration for refactoring (2)

This is a part of refactoring CLs for unregister(). See the CL[1] for details.

This CL renames existing Unregister* for "pattern" to DeprecatedUnregister*,
and adds new Unregister* for "registration_id".

[1] Blink: https://codereview.chromium.org/1185483003
[2] Chromium: THIS PATCH
[3] Blink: https://codereview.chromium.org/1190503002
[4] Chromium: https://codereview.chromium.org/1186803002

BUG=500404
TEST=compile

Committed: https://crrev.com/710d9db8a0bf8c5d087e4223b121b47093ae37de
Cr-Commit-Position: refs/heads/master@{#335009}

[nhiroki, 2015-06-15 06:35:14]

Patchset #1 (id:1) has been deleted

[nhiroki, 2015-06-15 06:56:04]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2015-06-16 02:05:22]

Patchset #2 (id:60001) has been deleted

[nhiroki, 2015-06-16 02:06:49]

Patchset #2 (id:80001) has been deleted

[nhiroki, 2015-06-16 02:07:09]

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

[nhiroki, 2015-06-16 02:07:10]

PTAL, thanks

[kinuko, 2015-06-17 14:23:12]

Looks like this patch still has some ifdef's, which look unnecessary now?

Otherwise lgtm

[nhiroki, 2015-06-17 16:27:02]

Patchset #3 (id:120001) has been deleted

[nhiroki, 2015-06-17 16:31:25]

Thank you for reviewing.

On 2015/06/17 14:23:12, kinuko wrote:
> Looks like this patch still has some ifdef's, which look unnecessary now?
> 
> Otherwise lgtm

Removed some of them used for WebServiceWorkerUnregistrationCallbacks switch.
Others are still necessary for tests to be broken.

[nhiroki, 2015-06-17 16:38:10]

nhiroki@chromium.org changed reviewers:
+ asvitkine@chromium.org, tsepez@chromium.org

[nhiroki, 2015-06-17 16:38:10]

+tsepez@ for service_worker_messages.h
+asvitline@ for histograms.xml

PTAL, thanks!

[Alexei Svitkine (slow), 2015-06-17 16:49:34]

lgtm

[Tom Sepez, 2015-06-17 16:55:20]

Messages LGTM.

[kinuko, 2015-06-17 17:44:57]

https://codereview.chromium.org/1181973004/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/1181973004/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:210:
#ifdef CRBUG_500404
I don't really understand why we still need these ifdefs.  When, where and how
are you going to add and remove the define?

Can't the old test code be just deleted if it's not going to pass, and can new
test code be added in the patch 4?

[nhiroki, 2015-06-17 23:14:02]

Patchset #4 (id:160001) has been deleted

[nhiroki, 2015-06-17 23:23:49]

OK, removed all ifdefs

https://codereview.chromium.org/1181973004/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/1181973004/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:210:
#ifdef CRBUG_500404
On 2015/06/17 17:44:56, kinuko wrote:
> I don't really understand why we still need these ifdefs.  When, where and how
> are you going to add and remove the define?

#define was already landed in the 1st patch and will be removed in the 3rd
patch. This #define is not used for enabling new codes, but for keeping existing
codes until the 3rd patch comes.

> Can't the old test code be just deleted if it's not going to pass, and can new
> test code be added in the patch 4?

I meant to keep tests until the very last minute. If we delete these tests,
tests for both old and new codes are absent until the 4th patch comes, it may
not be a big deal for this small cleanup though.

[kinuko, 2015-06-18 01:49:16]

On 2015/06/17 23:23:49, nhiroki wrote:
> OK, removed all ifdefs
> 
>
https://codereview.chromium.org/1181973004/diff/140001/content/browser/servic...
> File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/1181973004/diff/140001/content/browser/servic...
> content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:210:
> #ifdef CRBUG_500404
> On 2015/06/17 17:44:56, kinuko wrote:
> > I don't really understand why we still need these ifdefs.  When, where and
how
> > are you going to add and remove the define?
> 
> #define was already landed in the 1st patch and will be removed in the 3rd
> patch. This #define is not used for enabling new codes, but for keeping
existing
> codes until the 3rd patch comes.

Ah ok I didn't realize we kept the define in the patch 1.

> > Can't the old test code be just deleted if it's not going to pass, and can
new
> > test code be added in the patch 4?
> 
> I meant to keep tests until the very last minute. If we delete these tests,
> tests for both old and new codes are absent until the 4th patch comes, it may
> not be a big deal for this small cleanup though.

Yup.  Sorry for the rant, I was actually able to guess what you wanted. 

It's probably ok to have them temporarily, but in general using ifdef's is
strongly discouraged while it's common not to have tests while landing
multi-sided patches-- so I needed to ask if we really need to do so as a
reviewer. lgtm in either way, but I prefer non-ifdef ones if we can land these
without breaking build.

[nhiroki, 2015-06-18 04:15:05]

On 2015/06/18 01:49:16, kinuko wrote:
> On 2015/06/17 23:23:49, nhiroki wrote:
> > OK, removed all ifdefs
> > 
> >
>
https://codereview.chromium.org/1181973004/diff/140001/content/browser/servic...
> > File
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/1181973004/diff/140001/content/browser/servic...
> >
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:210:
> > #ifdef CRBUG_500404
> > On 2015/06/17 17:44:56, kinuko wrote:
> > > I don't really understand why we still need these ifdefs.  When, where and
> how
> > > are you going to add and remove the define?
> > 
> > #define was already landed in the 1st patch and will be removed in the 3rd
> > patch. This #define is not used for enabling new codes, but for keeping
> existing
> > codes until the 3rd patch comes.
> 
> Ah ok I didn't realize we kept the define in the patch 1.
> 
> > > Can't the old test code be just deleted if it's not going to pass, and can
> new
> > > test code be added in the patch 4?
> > 
> > I meant to keep tests until the very last minute. If we delete these tests,
> > tests for both old and new codes are absent until the 4th patch comes, it
may
> > not be a big deal for this small cleanup though.
> 
> Yup.  Sorry for the rant, I was actually able to guess what you wanted. 
> 
> It's probably ok to have them temporarily, but in general using ifdef's is
> strongly discouraged while it's common not to have tests while landing
> multi-sided patches-- so I needed to ask if we really need to do so as a
> reviewer. lgtm in either way, but I prefer non-ifdef ones if we can land these
> without breaking build.

Sounds reasonable. Actually, keeping ifdefs annoyed me and the newest patchset
without them looks much simpler :)

[nhiroki, 2015-06-18 04:16:01]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2015-06-18 04:16:01]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1181973004/#ps180001
(title: "remove ifdef")

[commit-bot: I haz the power, 2015-06-18 04:17:00]

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

[commit-bot: I haz the power, 2015-06-18 06:29:38]

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

[commit-bot: I haz the power, 2015-06-18 06:29:39]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[nhiroki, 2015-06-18 06:42:34]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-06-18 06:44:08]

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

[nhiroki, 2015-06-18 06:45:33]

The CQ bit was unchecked by nhiroki@chromium.org

[nhiroki, 2015-06-18 06:46:34]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-06-18 06:47:38]

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

[commit-bot: I haz the power, 2015-06-18 08:22:00]

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

[commit-bot: I haz the power, 2015-06-18 08:22:01]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[nhiroki, 2015-06-18 09:31:55]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-06-18 09:34:07]

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

[commit-bot: I haz the power, 2015-06-18 11:00:34]

Committed patchset #4 (id:180001)

[commit-bot: I haz the power, 2015-06-18 11:01:29]

Patchset 4 (id:??) landed as
https://crrev.com/710d9db8a0bf8c5d087e4223b121b47093ae37de
Cr-Commit-Position: refs/heads/master@{#335009}