Move content/renderer/background_sync to Blink

Move content/renderer/background_sync to Blink

- Add Blink interface registry to ServiceWorkerContextClient to allow
  the BackgroundSyncClientService to be registered in Blink.
- Map mojo Time struct to double in Blink
- Remove background_sync_type_converters
- Delete background_sync dirs in content/child and content/renderer
  ServiceWorkerGlobalScopeProxy and associated Web headers
- Remove dispatchSync function from ServiceWorkerContextClient,
- Remove didHandleSyncEvent from a whole bunch of files
- Change WaitUntilObserver to take a callback that's called only for
  SyncEvents
- Move callback clean up code from ServiceWorkerContextClient into
  WaitUntilObserver (using the contextDestroyed method)

BUG=662134

[adithyas, 2016-11-21 19:31:43]

The CQ bit was checked by adithyas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 19:33:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[adithyas, 2016-11-21 20:51:08]

adithyas@chromium.org changed reviewers:
+ dcheng@chromium.org, haraken@chromium.org, iclelland@chromium.org,
jbroman@chromium.org, nhiroki@chromium.org

[commit-bot: I haz the power, 2016-11-21 20:55:09]

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

[commit-bot: I haz the power, 2016-11-21 20:55:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbroman, 2016-11-21 21:36:02]

I'm happy to see content/child/background_sync and
content/renderer/background_sync go away entirely (and have Blink talking
directly to content/browser via Mojo), and this approach looks good (and much
more direct) to me in general.

The test failures on the bots look real, though; please fix those.

https://codereview.chromium.org/2517223002/diff/20001/mojo/common/common_cust...
File mojo/common/common_custom_types.mojom (right):

https://codereview.chromium.org/2517223002/diff/20001/mojo/common/common_cust...
mojo/common/common_custom_types.mojom:17: double seconds;
You'll want the opinion of a mojo/common/ owner, but I think I'd rather store an
int64 number of microseconds in the Mojo struct, and leave the conversion to
double for blink to do (in its traits). Basically, outside of
third_party/WebKit/ I'd be inclined to prefer Chromium's representation of
things.

https://codereview.chromium.org/2517223002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_sync/BackgroundSyncClientImpl.cpp
(right):

https://codereview.chromium.org/2517223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncClientImpl.cpp:26:
mojo::MakeStrongBinding(wrapUnique(new BackgroundSyncClientImpl(globalScope)),
nit: since makeUnique has started to make its way into Blink now, this could be:

mojo::MakeStrongBinding(makeUnique<BackgroundSyncClientImpl>(globalScope),
                        std::move(request));

You use makeUnique elsewhere, so you might as well do so here.

https://codereview.chromium.org/2517223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_sync/BackgroundSyncClientImpl.cpp:56:
makeUnique<WTF::Function<void(blink::mojom::ServiceWorkerEventStatus,
nit: probably easier to read if you use the type alias you created:

makeUnique<WaitUntilObserver::Callback>(...)

Unless, of course, you end up having to bind.

https://codereview.chromium.org/2517223002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h
(right):

https://codereview.chromium.org/2517223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:30: const
double&)>;
A few nits:
1. "blink::" is unnecessary while in namespace blink
2. Please name the variable here (e.g. "eventDispatchTime") so it's clear what
it gets called with.
3. This last one is more open to debate, but while it's very convenient that
making this const double& makes it match perfectly with what
mojom::blink::BackgroundSyncClient wants, from the perspective of this class
alone a const& to a primitive type seems odd. (And, if this approach is used to
deal with other events in the future, const double& may not make sense to those
clients.)

i.e. I think I'd prefer:
using Callback = WTF::Function<void(mojom::ServiceWorkerEventStatus, double
eventDispatchTime)>;

Regrettably, doing 3 does mean that you'll need an intermediate callback that
just adapts the call signature:

WTF::bind([](SyncCallback callback, mojom::ServiceWorkerEventStatus status,
double eventDispatchTime) {
  callback.Run(status, eventDispatchTime);
}, WTF::passed(std::move(callback)));

So you might want to wait and see what modules/serviceworkers/OWNERS think.

https://codereview.chromium.org/2517223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:65: //
Overriden from LifecycleObserver
spelling nit: "Overridden"

https://codereview.chromium.org/2517223002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2517223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:44: #include
"modules/background_sync/SyncEvent.h"
The include of SyncEvent here can probably go away now.

https://codereview.chromium.org/2517223002/diff/20001/third_party/WebKit/publ...
File
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h
(right):

https://codereview.chromium.org/2517223002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h:267:
virtual InterfaceRegistry* interfaceRegistry() { return nullptr; }
nit: I know this file is already inconsistent, but since this really is
required, pure virtual would be nice here.

[Marijn Kruisselbrink, 2016-11-21 22:31:59]

(FYI this will conflict with https://codereview.chromium.org/2490623005/, which
moves away from using an interface registry for these kinds of things)

[adithyas, 2016-11-21 23:00:02]

On 2016/11/21 at 22:31:59, mek wrote:
> (FYI this will conflict with https://codereview.chromium.org/2490623005/,
which moves away from using an interface registry for these kinds of things)

Looks like that CL removes content/renderer/background_sync altogether, moving
it to content/renderer/service_worker instead. That makes this CL obsolete, so
I'm closing it.