[BackgroundSync] Remove BackgroundSyncRegistrationHandle

[BackgroundSync] Remove BackgroundSyncRegistrationHandle

Since "registration.done" is no longer part of the spec, the BackgroundSyncManager no longer needs to refcount its registrations. This CL removes the internal reference counting and the handles that were provided to clients.

BUG=583328
Committed: https://crrev.com/6b0615b5f2dd5744a9e606ec88a9062bca934d56
Cr-Commit-Position: refs/heads/master@{#381303}

[jkarlin, 2016-03-07 15:35:15]

Description was changed from

==========
[BackgroundSync] Remove BackgroundSyncRegistrationHandle

Previously, the Background Sync spec included
"registration.done". This required the BackgroundSyncManager to hold
onto registrations that had already completed in case the renderer
inquired about their completion status afterwards. This CL removes the
reference counting and handles that were added to support
registration.done.

BUG=583328
==========

to

==========
[BackgroundSync] Remove BackgroundSyncRegistrationHandle

Since "registration.done" is no longer part of the spec, the
BackgroundSyncManager no longer needs to refcount its registrations. This CL
removes the internal reference counting and the handles that were provided to
clients.

BUG=583328
==========

[jkarlin, 2016-03-07 19:03:13]

jkarlin@chromium.org changed reviewers:
+ iclelland@chromium.org

[jkarlin, 2016-03-07 19:03:14]

iclelland@: PTAL, thanks!

[iclelland, 2016-03-07 20:43:25]

On 2016/03/07 19:03:14, jkarlin wrote:
> iclelland@: PTAL, thanks!

I'll take a close look at this pretty soon -- the windows issue is apparently
VS2013 not supporting default move constructors (or move assignment operators)
-- backed up by a quick search for
https://code.google.com/p/chromium/codesearch#search/&q=%5C&%5C&.*%5C...

[iclelland, 2016-03-08 16:48:14]

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:771: new
BackgroundSyncRegistration(registration);
These are allocated here, but never released in the ServiceImpl as they're being
transferred to the the mojo array.

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1013: if
(!registration) {
Is this a failure case?

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1019:
DCHECK(registration);
No longer necessary

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1024:
num_firing_registrations_ -= 1;
Since we're not guarding this with the DCHECK any more, this looks like it might
need to be decremented in the "if (!registration)" block as well, to keep the
accounting accurate.

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration.h (right):

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_registration.h:31:
BackgroundSyncRegistration(const BackgroundSyncRegistration& other) = default;
Style question -- these are defined default inline here, while the constructor
and destructor are defined in the .cc file -- is there any reason behind that?
(I ask because I had to go look at the constructor to make sure it was trivial)

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.cc (right):

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:153:
result_registrations->weak_clear();
I think this leaks the registrations now (they were new'ed in
BackgroundSyncManager::GetRegistrationsImpl)

[jkarlin, 2016-03-08 19:37:36]

Thanks! PTAL.

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:771: new
BackgroundSyncRegistration(registration);
On 2016/03/08 16:48:14, iclelland wrote:
> These are allocated here, but never released in the ServiceImpl as they're
being
> transferred to the the mojo array.

That weak_clear had to go, thanks!

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1013: if
(!registration) {
On 2016/03/08 16:48:14, iclelland wrote:
> Is this a failure case?

This is what happens if the ServiceWorkerRegistration is removed mid-sync.

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1019:
DCHECK(registration);
On 2016/03/08 16:48:13, iclelland wrote:
> No longer necessary

Done.

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1024:
num_firing_registrations_ -= 1;
On 2016/03/08 16:48:14, iclelland wrote:
> Since we're not guarding this with the DCHECK any more, this looks like it
might
> need to be decremented in the "if (!registration)" block as well, to keep the
> accounting accurate.

Good catch, thanks!

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_registration.h (right):

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_registration.h:31:
BackgroundSyncRegistration(const BackgroundSyncRegistration& other) = default;
On 2016/03/08 16:48:14, iclelland wrote:
> Style question -- these are defined default inline here, while the constructor
> and destructor are defined in the .cc file -- is there any reason behind that?
> (I ask because I had to go look at the constructor to make sure it was
trivial)

Thanks. They're supposed to be in the header.

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_service_impl.cc (right):

https://codereview.chromium.org/1763123002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_service_impl.cc:153:
result_registrations->weak_clear();
On 2016/03/08 16:48:14, iclelland wrote:
> I think this leaks the registrations now (they were new'ed in
> BackgroundSyncManager::GetRegistrationsImpl)

Removed this line. Thanks for the catch!

[jkarlin, 2016-03-11 12:32:54]

iclelland@ PTAL, thanks!

[iclelland, 2016-03-11 14:50:12]

Sorry for the lag -- this lgtm, with just a couple of nits

https://codereview.chromium.org/1763123002/diff/120001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1763123002/diff/120001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:477:
base::Passed(std::move(out_registration))));
nit: More succinct as
  base::Passed(make_scoped_ptr(...
?

https://codereview.chromium.org/1763123002/diff/120001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:653:
base::Passed(std::move(out_registration))));
Same as above (do we need the name out_registration for anything?)

https://codereview.chromium.org/1763123002/diff/120001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1166:
scoped_ptr<ScopedVector<BackgroundSyncRegistration>> registration) {
nit: I think this should be plural 'registrations'

https://codereview.chromium.org/1763123002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/1763123002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:63: #include
"public/platform/modules/background_sync/WebSyncRegistration.h"
I think we can probably get rid of this include as well.

https://codereview.chromium.org/1763123002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/1763123002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h:50: struct
WebSyncRegistration;
Not needed anymore

https://codereview.chromium.org/1763123002/diff/120001/third_party/WebKit/pub...
File
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextProxy.h
(right):

https://codereview.chromium.org/1763123002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextProxy.h:47:
struct WebSyncRegistration;
ditto.

[jkarlin, 2016-03-15 12:50:14]

jkarlin@chromium.org changed reviewers:
+ jochen@chromium.org, michaeln@chromium.org, tsepez@chromium.org

[jkarlin, 2016-03-15 12:50:16]

Thanks iclelland!

tsepez@chromium.org: Please review changes in *.mojom

michaeln@chromium.org: Please review changes in ServiceWorker files

jochen@chromium.org: Please review changes in content/public/test and
third_party/WebKit/Source/web/

Thanks all!

https://codereview.chromium.org/1763123002/diff/120001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1763123002/diff/120001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:477:
base::Passed(std::move(out_registration))));
On 2016/03/11 14:50:12, iclelland wrote:
> nit: More succinct as
>   base::Passed(make_scoped_ptr(...
> ?

Done.

https://codereview.chromium.org/1763123002/diff/120001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:653:
base::Passed(std::move(out_registration))));
On 2016/03/11 14:50:12, iclelland wrote:
> Same as above (do we need the name out_registration for anything?)

Done.

https://codereview.chromium.org/1763123002/diff/120001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:1166:
scoped_ptr<ScopedVector<BackgroundSyncRegistration>> registration) {
On 2016/03/11 14:50:12, iclelland wrote:
> nit: I think this should be plural 'registrations'

Done.

https://codereview.chromium.org/1763123002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/1763123002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:63: #include
"public/platform/modules/background_sync/WebSyncRegistration.h"
On 2016/03/11 14:50:12, iclelland wrote:
> I think we can probably get rid of this include as well.

Done.

https://codereview.chromium.org/1763123002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/1763123002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h:50: struct
WebSyncRegistration;
On 2016/03/11 14:50:12, iclelland wrote:
> Not needed anymore

Done.

https://codereview.chromium.org/1763123002/diff/120001/third_party/WebKit/pub...
File
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextProxy.h
(right):

https://codereview.chromium.org/1763123002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextProxy.h:47:
struct WebSyncRegistration;
On 2016/03/11 14:50:12, iclelland wrote:
> ditto.

Done.

[Tom Sepez, 2016-03-15 15:33:55]

mojom LGTM

[michaeln, 2016-03-15 20:12:03]

ServiceWorker lgtm 2

(wait-a-minute, your making something simpler rather than more whiz bang
complicated :)

[jochen (gone - plz use gerrit), 2016-03-15 20:25:20]

lgtm

[jkarlin, 2016-03-15 20:36:13]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2016-03-15 20:36:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
iclelland@chromium.org
Link to the patchset: https://codereview.chromium.org/1763123002/#ps160001
(title: "Address comments from PS7")

[commit-bot: I haz the power, 2016-03-15 20:36:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1763123002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1763123002/160001

[commit-bot: I haz the power, 2016-03-15 20:41:59]

Description was changed from

==========
[BackgroundSync] Remove BackgroundSyncRegistrationHandle

Since "registration.done" is no longer part of the spec, the
BackgroundSyncManager no longer needs to refcount its registrations. This CL
removes the internal reference counting and the handles that were provided to
clients.

BUG=583328
==========

to

==========
[BackgroundSync] Remove BackgroundSyncRegistrationHandle

Since "registration.done" is no longer part of the spec, the
BackgroundSyncManager no longer needs to refcount its registrations. This CL
removes the internal reference counting and the handles that were provided to
clients.

BUG=583328
==========

[commit-bot: I haz the power, 2016-03-15 20:42:01]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-15 20:43:54]

Description was changed from

==========
[BackgroundSync] Remove BackgroundSyncRegistrationHandle

Since "registration.done" is no longer part of the spec, the
BackgroundSyncManager no longer needs to refcount its registrations. This CL
removes the internal reference counting and the handles that were provided to
clients.

BUG=583328
==========

to

==========
[BackgroundSync] Remove BackgroundSyncRegistrationHandle

Since "registration.done" is no longer part of the spec, the
BackgroundSyncManager no longer needs to refcount its registrations. This CL
removes the internal reference counting and the handles that were provided to
clients.

BUG=583328

Committed: https://crrev.com/6b0615b5f2dd5744a9e606ec88a9062bca934d56
Cr-Commit-Position: refs/heads/master@{#381303}
==========

[commit-bot: I haz the power, 2016-03-15 20:43:56]

Patchset 9 (id:??) landed as
https://crrev.com/6b0615b5f2dd5744a9e606ec88a9062bca934d56
Cr-Commit-Position: refs/heads/master@{#381303}