[Background Sync] Converting Blink code to the MVP API

[Background Sync] Converting Blink code to the MVP API

This commit implements the new API in https://github.com/slightlyoff/BackgroundSync/blob/26e17d82882f4b57641b5e2984c8680b64e7804a/idl.md

Key points:
  The API has been split at the IDL level into [One Shot] Sync and PeriodicSync.
  Only items identified as MVP are enabled with the BackgroundSync runtime flag. Additional API items are controlled by a flag named BackgroundSyncV2
  String "id"s have been changed to "tag"s everywhere. Other options are also changed (See the IDL changes for the complete list)
  A new internal-only "id" member is now used to track registration identity
  A new internal-only "periodicity" member is now used to distinguish between one-shot and periodic syncs.

BUG=477705
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194551

[iclelland, 2015-04-16 18:14:10]

iclelland@chromium.org changed reviewers:
+ jkarlin@chromium.org, jochen@chromium.org, mvanouwerkerk@chromium.org

[iclelland, 2015-04-16 18:21:06]

I'm sorry about the huge patch; there's a lot of duplicated code in here, and
one of the things I'm hoping to get out of review is a good pattern for
eliminating that.

(I've started to refactor it three times now, and realized that I should just
ask for guidance here instead :) )

PeriodicSyncManager and SyncManager, for instance, share almost all of their
code. Similarly with PeriodicSyncEvent and SyncEvent, PeriodicSyncRegistration
and SyncRegistration, etc. They're all separate classes an account of the IDL,
but I'd like to factor out a common superclass, or templatize them, or something
to eliminate the duplication.

Similarly, I'm unhappy with the SyncGetRegistrationsCallbacks::onSuccess code in
SyncCallbacks.cpp, but I don't know if my instinct to start writing abstract
factories is correct here.

Experienced design guidance is much appreciated here, thanks!

[jkarlin, 2015-04-17 19:04:49]

I think this design is fine.

https://codereview.chromium.org/1096503002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/background_sync/namespaces.html (right):

https://codereview.chromium.org/1096503002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/background_sync/namespaces.html:5: <script
src="../resources/testharness.js"></script>
I see a pretty large style difference between this file (patterened after the
push_messaging/ tests) and the service_worker/ tests. jsbell@, is one preferred
over the other?

https://codereview.chromium.org/1096503002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/background_sync/namespaces.html:86: /*
Remove commented section

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
File Source/modules/background_sync/PeriodicSyncEvent.h (right):

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncEvent.h:9: #include
"modules/background_sync/PeriodicSyncEventInit.h"
forward declare instead?

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncEvent.h:10: #include
"modules/background_sync/PeriodicSyncRegistration.h"
ditto

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncEvent.h:12: #include
"platform/heap/Handle.h"
unused?

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncEvent.h:14: #include
"wtf/text/WTFString.h"
unused?

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncEvent.h:47:
PersistentWillBeMember<PeriodicSyncRegistration> m_syncRegistration;
perhaps m_periodicRegistration?

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
File Source/modules/background_sync/PeriodicSyncManager.cpp (right):

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncManager.cpp:52: return
kMinPossiblePeriod;
Please create a bug to push this value up to the browser process eventually

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncManager.cpp:118:
resolver->resolve(String::fromUTF8("granted"));
This needs to call up to the browser process. Please create a bug for that.

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
File Source/modules/background_sync/PeriodicSyncRegistration.h (right):

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncRegistration.h:11: #include
"platform/heap/Handle.h"
Remove?

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncRegistration.h:52: Nullable<int64_t>
m_id;
Why nullable? This isn't exposed to javascript.

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
File Source/modules/background_sync/SyncRegistration.h (right):

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/SyncRegistration.h:41: Nullable<int64_t> m_id;
Doesn't need to be nullable.

[jkarlin, 2015-04-17 19:07:12]

+cc'd jsbell for the first comment in namespaces.html

[jsbell, 2015-04-17 19:22:03]

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

[jsbell, 2015-04-17 19:22:04]

https://codereview.chromium.org/1096503002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/background_sync/namespaces.html (right):

https://codereview.chromium.org/1096503002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/background_sync/namespaces.html:5: <script
src="../resources/testharness.js"></script>
On 2015/04/17 19:04:49, jkarlin wrote:
> I see a pretty large style difference between this file (patterened after the
> push_messaging/ tests) and the service_worker/ tests. jsbell@, is one
preferred
> over the other?

While we don't have a unified standard for blink layout tests, for Service
Workers and related things like Fetch we wrote/followed the layout test part of
style guide:

https://www.chromium.org/blink/serviceworker/testing

Glancing at this file, the key differences would be:

* Remove HTML tags that are implied (html, head, body, etc) - they're just
clutter
* Don't indent the body of <script> - saves horizontal space
* Wrap at 80 columns, splitting strings when necessary
* All asserts should have a message, ideally phrased as '.. should ...' and
referencing the spec.
* Use snake_case rather than camelCase
* Indenting standards for promise chains

I would advocate for following that style guide for testharness.js-based tests
if possible (although we're not rigorous about it outside service_workers and
I'm guilty of violations myself!), especially as we are wanting to upstream
these tests to the W3C web-platform-tests repo eventually and consistency will
ease the reviewers' jobs.

Also... I think we were able to get clang-format styling the JS correctly? Not
finding the docs, I'll see what I can dig up.

https://codereview.chromium.org/1096503002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/background_sync/namespaces.html:50: }).then(function() {
Personally, I'd eliminate this line i.e. merge two functions on either side,
here and everywhere else where an async step isn't actually necessary. I admit
there are pros and cons: each function does one thing vs. making the sync/async
flow more obvious and reducing visual noise.

[iclelland, 2015-04-22 14:05:11]

https://codereview.chromium.org/1096503002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/background_sync/namespaces.html (right):

https://codereview.chromium.org/1096503002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/background_sync/namespaces.html:5: <script
src="../resources/testharness.js"></script>
On 2015/04/17 19:22:04, jsbell wrote:
> On 2015/04/17 19:04:49, jkarlin wrote:
> > I see a pretty large style difference between this file (patterened after
the
> > push_messaging/ tests) and the service_worker/ tests. jsbell@, is one
> preferred
> > over the other?
> 
> While we don't have a unified standard for blink layout tests, for Service
> Workers and related things like Fetch we wrote/followed the layout test part
of
> style guide:
> 
> https://www.chromium.org/blink/serviceworker/testing
> 
> Glancing at this file, the key differences would be:
> 
> * Remove HTML tags that are implied (html, head, body, etc) - they're just
> clutter
> * Don't indent the body of <script> - saves horizontal space
> * Wrap at 80 columns, splitting strings when necessary
> * All asserts should have a message, ideally phrased as '.. should ...' and
> referencing the spec.
> * Use snake_case rather than camelCase
> * Indenting standards for promise chains
> 
> I would advocate for following that style guide for testharness.js-based tests
> if possible (although we're not rigorous about it outside service_workers and
> I'm guilty of violations myself!), especially as we are wanting to upstream
> these tests to the W3C web-platform-tests repo eventually and consistency will
> ease the reviewers' jobs.
> 
> Also... I think we were able to get clang-format styling the JS correctly? Not
> finding the docs, I'll see what I can dig up.

Done. -- I've removed all of the boilerplate, moved helpers into a separate
script file, added assert messages (no formal spec to reference yet), and
restyled all of the promise chains to match the service worker tests.

https://codereview.chromium.org/1096503002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/background_sync/namespaces.html:50: }).then(function() {
On 2015/04/17 19:22:04, jsbell wrote:
> Personally, I'd eliminate this line i.e. merge two functions on either side,
> here and everywhere else where an async step isn't actually necessary. I admit
> there are pros and cons: each function does one thing vs. making the
sync/async
> flow more obvious and reducing visual noise.

Done.

https://codereview.chromium.org/1096503002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/background_sync/namespaces.html:86: /*
On 2015/04/17 19:04:49, jkarlin wrote:
> Remove commented section

I've removed the comments -- the test itself is actually good, I think.

https://codereview.chromium.org/1096503002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/background_sync/namespaces.html:86: /*
On 2015/04/17 19:04:49, jkarlin wrote:
> Remove commented section

I've removed the comment markers and reinstated the test instead.

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
File Source/modules/background_sync/PeriodicSyncEvent.h (right):

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncEvent.h:9: #include
"modules/background_sync/PeriodicSyncEventInit.h"
On 2015/04/17 19:04:49, jkarlin wrote:
> forward declare instead?

Done.

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncEvent.h:10: #include
"modules/background_sync/PeriodicSyncRegistration.h"
On 2015/04/17 19:04:49, jkarlin wrote:
> ditto

Done.

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncEvent.h:12: #include
"platform/heap/Handle.h"
On 2015/04/17 19:04:49, jkarlin wrote:
> unused?

Done.

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncEvent.h:14: #include
"wtf/text/WTFString.h"
On 2015/04/17 19:04:49, jkarlin wrote:
> unused?

Done.

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncEvent.h:47:
PersistentWillBeMember<PeriodicSyncRegistration> m_syncRegistration;
On 2015/04/17 19:04:49, jkarlin wrote:
> perhaps m_periodicRegistration?

Sure. Done.

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
File Source/modules/background_sync/PeriodicSyncManager.cpp (right):

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncManager.cpp:52: return
kMinPossiblePeriod;
On 2015/04/17 19:04:49, jkarlin wrote:
> Please create a bug to push this value up to the browser process eventually

Done. crbug.com/479707

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncManager.cpp:118:
resolver->resolve(String::fromUTF8("granted"));
On 2015/04/17 19:04:49, jkarlin wrote:
> This needs to call up to the browser process. Please create a bug for that.

Done. crbug.com/479708 (Also for SyncManager.cpp)

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
File Source/modules/background_sync/PeriodicSyncRegistration.h (right):

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncRegistration.h:11: #include
"platform/heap/Handle.h"
On 2015/04/17 19:04:49, jkarlin wrote:
> Remove?

Done.

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/PeriodicSyncRegistration.h:52: Nullable<int64_t>
m_id;
On 2015/04/17 19:04:49, jkarlin wrote:
> Why nullable? This isn't exposed to javascript.

It was to allow Null (distinct from 0) values to represent "no registration id",
for example, for calls to .register()

In other layers, I switched to using a constant value (set to -1) for the same
idea. I'll switch this here as well.

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
File Source/modules/background_sync/SyncRegistration.h (right):

https://codereview.chromium.org/1096503002/diff/20001/Source/modules/backgrou...
Source/modules/background_sync/SyncRegistration.h:41: Nullable<int64_t> m_id;
On 2015/04/17 19:04:49, jkarlin wrote:
> Doesn't need to be nullable.

Done. Fixed (also fixed in PeriodicSyncRegistration.cpp)

[jkarlin, 2015-04-22 16:46:03]

lgtm

[iclelland, 2015-04-24 13:32:37]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2015-04-24 13:32:37]

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/1096503002/#ps100001
(title: "Fix threading issue (stack-allocated pointers) and interface tests.")

[commit-bot: I haz the power, 2015-04-24 13:33:00]

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

[commit-bot: I haz the power, 2015-04-24 13:40:27]

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

[commit-bot: I haz the power, 2015-04-24 13:40:29]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[iclelland, 2015-04-25 11:32:36]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2015-04-25 11:32:36]

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/1096503002/#ps140001
(title: "Unregister method requires the sync registration tag")

[commit-bot: I haz the power, 2015-04-25 11:32:54]

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

[commit-bot: I haz the power, 2015-04-25 11:40:51]

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

[commit-bot: I haz the power, 2015-04-25 11:40:52]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[iclelland, 2015-04-26 02:14:20]

On 2015/04/25 11:40:52, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   blink_presubmit on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

Now I see the issue :)

jochen, can I get a review for RuntimeEnabledFeatures.in and EventTypeNames.in?

Thanks

[jochen (gone - plz use gerrit), 2015-04-27 19:27:42]

*.in files lgtm

[iclelland, 2015-04-27 20:24:29]

The CQ bit was checked by iclelland@chromium.org

[commit-bot: I haz the power, 2015-04-27 20:24:51]

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

[commit-bot: I haz the power, 2015-04-27 21:44:34]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194551