[Background Sync] Align exposed API with spec

[Background Sync] Align exposed API with spec

This is the second of a series of commits to align the exposed
BackgroundSync API with the spec at https://wicg.github.io/BackgroundSync/spec/

Part 1: Removing Exposed Periodic Sync: https://crrev.com/1436723002/

BUG=552029
Committed: https://crrev.com/449769316764e3922e50920bb62b186221301e47
Cr-Commit-Position: refs/heads/master@{#360068}

[jkarlin, 2015-11-12 17:29:40]

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

[jkarlin, 2015-11-12 17:29:41]

I realize this isn't up for review yet, I just wanted to make sure you added
something..

https://codereview.chromium.org/1437883002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/background_sync/SyncManager.idl (right):

https://codereview.chromium.org/1437883002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/background_sync/SyncManager.idl:17:
[CallWith=(ScriptState,ExecutionContext),ImplementedAs=registerFunction]
Promise<void> register(DOMString tag);
Can you add MeasureAs=BackgroundSyncRegister? So it looks like..

[MeasureAs=BackgroundSyncRegister,CallWith=(ScriptState,ExecutionContext),ImplementedAs=registerFunction]
Pro
    mise<void> register(DOMString tag);

And also add BackgroundSyncRegister to UseCounter.h

That will give us an idea of sync usage.

[iclelland, 2015-11-12 17:54:59]

Thanks, Josh -- It's not up yet, but it's really close. I just need to rewrite
the browser tests to not depend on .unregister() and it should be good to go.

[iclelland, 2015-11-12 21:21:21]

jkarlin, PTAL, thanks!

[jkarlin, 2015-11-13 01:32:18]

Also note the failing bots.

https://codereview.chromium.org/1437883002/diff/60001/content/test/data/backg...
File content/test/data/background_sync/background_sync_test_helpers.js (left):

https://codereview.chromium.org/1437883002/diff/60001/content/test/data/backg...
content/test/data/background_sync/background_sync_test_helpers.js:193:
registrationReference = syncRegistration;
Delete var registrationReference at the top of the file

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/background_sync/oneshot.html
(right):

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/background_sync/oneshot.html:17: //
One-shot syncs can only be registered from a controlled document. This
This comment needs to be updated as they can also be registered from
uncontrolled documents (e.g., the test below).

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
(right):

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt:596:
interface SyncRegistration
Forgot to get rid of SyncRegistration

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/background_sync/SyncCallbacks.cpp (left):

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_sync/SyncCallbacks.cpp:126: }
We now have a memory leak, webSyncRegistration's elements aren't being deleted.

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/background_sync/SyncCallbacks.cpp
(right):

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_sync/SyncCallbacks.cpp:127: for
(WebSyncRegistration* r : webSyncRegistrations) {
const WebSyncRegistration*

[iclelland, 2015-11-13 15:26:19]

https://codereview.chromium.org/1437883002/diff/60001/content/test/data/backg...
File content/test/data/background_sync/background_sync_test_helpers.js (left):

https://codereview.chromium.org/1437883002/diff/60001/content/test/data/backg...
content/test/data/background_sync/background_sync_test_helpers.js:193:
registrationReference = syncRegistration;
On 2015/11/13 01:32:18, jkarlin wrote:
> Delete var registrationReference at the top of the file

Good catch, thanks. Done.

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/background_sync/oneshot.html
(right):

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/background_sync/oneshot.html:17: //
One-shot syncs can only be registered from a controlled document. This
On 2015/11/13 01:32:18, jkarlin wrote:
> This comment needs to be updated as they can also be registered from
> uncontrolled documents (e.g., the test below).

Yeah, that's an old comment. Fixed.

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
(right):

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt:596:
interface SyncRegistration
On 2015/11/13 01:32:18, jkarlin wrote:
> Forgot to get rid of SyncRegistration

Thanks, done.

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/background_sync/SyncCallbacks.cpp (left):

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_sync/SyncCallbacks.cpp:126: }
On 2015/11/13 01:32:18, jkarlin wrote:
> We now have a memory leak, webSyncRegistration's elements aren't being
deleted.

Maybe it's early and I haven't had enough coffee yet, but I'm not seeing it --
the elements are deleted when the vector is destroyed, which happens when it
goes out of scope at the end of BackgroundSyncProvider::GetRegistrationCallback
(after this method returns) We don't use the elements for anything except
copying their tags, so there's no need to adopt them here, as far as I can see.

(But I haven't had the coffee, so I'll come back and look again afterwards.
Maybe I'll run the LSan tests in the meantime)

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/background_sync/SyncCallbacks.cpp
(right):

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_sync/SyncCallbacks.cpp:127: for
(WebSyncRegistration* r : webSyncRegistrations) {
On 2015/11/13 01:32:18, jkarlin wrote:
> const WebSyncRegistration*

Done.

[jkarlin, 2015-11-13 17:00:42]

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/background_sync/SyncCallbacks.cpp (left):

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_sync/SyncCallbacks.cpp:126: }
On 2015/11/13 15:26:19, iclelland wrote:
> On 2015/11/13 01:32:18, jkarlin wrote:
> > We now have a memory leak, webSyncRegistration's elements aren't being
> deleted.
> 
> Maybe it's early and I haven't had enough coffee yet, but I'm not seeing it --
> the elements are deleted when the vector is destroyed, which happens when it
> goes out of scope at the end of
BackgroundSyncProvider::GetRegistrationCallback
> (after this method returns) We don't use the elements for anything except
> copying their tags, so there's no need to adopt them here, as far as I can
see.
> 
> (But I haven't had the coffee, so I'll come back and look again afterwards.
> Maybe I'll run the LSan tests in the meantime)

My bad. I didn't realize that WebVector deleted its elements.

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/background_sync/SyncCallbacks.cpp
(right):

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_sync/SyncCallbacks.cpp:127: for
(WebSyncRegistration* r : webSyncRegistrations) {
On 2015/11/13 15:26:19, iclelland wrote:
> On 2015/11/13 01:32:18, jkarlin wrote:
> > const WebSyncRegistration*
> 
> Done.

I don't see the change.

[jkarlin, 2015-11-13 18:04:27]

lgtm after you upload the const change

[iclelland, 2015-11-13 20:48:27]

Description was changed from

==========
[Background Sync] Align exposed API with spec

This is the second of a series of commits to align the exposed
BackgroundSync API with the spec at https://wicg.github.io/BackgroundSync/spec/

Part 1: Removing Exposed Periodic Sync: https://crrev.com/1436723002/

BUG=552029
==========

to

==========
[Background Sync] Align exposed API with spec

This is the second of a series of commits to align the exposed
BackgroundSync API with the spec at https://wicg.github.io/BackgroundSync/spec/

Part 1: Removing Exposed Periodic Sync: https://crrev.com/1436723002/

BUG=552029
==========

[iclelland, 2015-11-13 20:48:27]

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

[iclelland, 2015-11-13 20:49:24]

+r jochen -- can you PTAL at UseCounter.h and ServiceWorkerGlobalScopeProxy.cpp?
Thanks!

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/background_sync/SyncCallbacks.cpp
(right):

https://codereview.chromium.org/1437883002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_sync/SyncCallbacks.cpp:127: for
(WebSyncRegistration* r : webSyncRegistrations) {
On 2015/11/13 17:00:42, jkarlin wrote:
> On 2015/11/13 15:26:19, iclelland wrote:
> > On 2015/11/13 01:32:18, jkarlin wrote:
> > > const WebSyncRegistration*
> > 
> > Done.
> 
> I don't see the change.

Okay, *really* done now.

[jochen (gone - plz use gerrit), 2015-11-16 11:56:55]

lgtm

[iclelland, 2015-11-16 22:00:18]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2015-11-16 22:00:18]

The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/1437883002/#ps110022
(title: "Address last comment from PS#4")

[commit-bot: I haz the power, 2015-11-16 22:02:39]

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

[commit-bot: I haz the power, 2015-11-16 22:05:39]

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

[commit-bot: I haz the power, 2015-11-16 22:05:40]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[iclelland, 2015-11-16 22:36:22]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2015-11-16 22:36:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkarlin@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1437883002/#ps130001
(title: "Rebase")

[commit-bot: I haz the power, 2015-11-16 22:37:17]

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

[commit-bot: I haz the power, 2015-11-16 23:07:03]

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

[commit-bot: I haz the power, 2015-11-16 23:07:04]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[iclelland, 2015-11-17 12:44:57]

The CQ bit was checked by iclelland@chromium.org

[commit-bot: I haz the power, 2015-11-17 12:45:06]

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

[commit-bot: I haz the power, 2015-11-17 12:49:27]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2015-11-17 12:50:16]

Patchset 8 (id:??) landed as
https://crrev.com/449769316764e3922e50920bb62b186221301e47
Cr-Commit-Position: refs/heads/master@{#360068}