The blink half of the new ServiceWorker 'sync' event.

The blink half of the new ServiceWorker 'sync' event.

The browser side is https://codereview.chromium.org/205033002/

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

[michaeln, 2014-03-20 23:07:28]

Code lgtm, but you'll have to find a blink owner to look at this too. Also
there's a question of how ready we are to commit to the onsync event. Idk if its
too soon to put this into the idl or not? We might want to wake up that
discussion on github.

https://codereview.chromium.org/205013002/diff/1/Source/web/ServiceWorkerGlob...
File Source/web/ServiceWorkerGlobalScopeClientImpl.cpp (right):

https://codereview.chromium.org/205013002/diff/1/Source/web/ServiceWorkerGlob...
Source/web/ServiceWorkerGlobalScopeClientImpl.cpp:73: if (m_client)
Not related to your CL, but i don't see how/why m_client would ever be null.

https://codereview.chromium.org/205013002/diff/1/public/web/WebServiceWorkerC...
File public/web/WebServiceWorkerContextProxy.h (right):

https://codereview.chromium.org/205013002/diff/1/public/web/WebServiceWorkerC...
public/web/WebServiceWorkerContextProxy.h:48: // FIXME: This needs to pass the
active service worker info.
The fixme comment seems out of place for this event. If it's receiving a sync
event, its the active worker.

Would be good to document that a call to WebServiceWorkerContextClient
didHandleSyncEvent is expected in response when the sw has completed its sync
logic.

[jkarlin, 2014-03-21 16:50:55]

As to whether we should add 'onsync' to the idl I'm not clear on what the right
timing for that is.  Is it not easy to modify during this rapid development
process?

https://codereview.chromium.org/205013002/diff/1/Source/web/ServiceWorkerGlob...
File Source/web/ServiceWorkerGlobalScopeClientImpl.cpp (right):

https://codereview.chromium.org/205013002/diff/1/Source/web/ServiceWorkerGlob...
Source/web/ServiceWorkerGlobalScopeClientImpl.cpp:73: if (m_client)
On 2014/03/20 23:07:28, michaeln wrote:
> Not related to your CL, but i don't see how/why m_client would ever be null.

Agreed.  Removed the conditionals.

https://codereview.chromium.org/205013002/diff/1/public/web/WebServiceWorkerC...
File public/web/WebServiceWorkerContextProxy.h (right):

https://codereview.chromium.org/205013002/diff/1/public/web/WebServiceWorkerC...
public/web/WebServiceWorkerContextProxy.h:48: // FIXME: This needs to pass the
active service worker info.
On 2014/03/20 23:07:28, michaeln wrote:
> The fixme comment seems out of place for this event. If it's receiving a sync
> event, its the active worker.

Thanks for the catch.

> 
> Would be good to document that a call to WebServiceWorkerContextClient
> didHandleSyncEvent is expected in response when the sw has completed its sync
> logic.

Done.

[jsbell, 2014-03-21 16:55:32]

On 2014/03/21 16:50:55, jkarlin wrote:
> As to whether we should add 'onsync' to the idl I'm not clear on what the
right
> timing for that is.  Is it not easy to modify during this rapid development
> process?

I think it's fine to add it. It doesn't impose a new compat risk since the
property isn't exposed in a context visible to code unless the
--enable-service-workers flag is set, and we're not promising stability of any
kind with that flag. (i.e. we may totally redo algorithms and break your demo,
we may crash, ...)

[jkarlin, 2014-03-21 17:08:39]

eseidel@chromium.org: Please review for Blink ownership for the files outside of
modules/

[falken, 2014-03-24 07:29:16]

nitty drive-by comment

https://codereview.chromium.org/205013002/diff/20001/Source/web/ServiceWorker...
File Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/205013002/diff/20001/Source/web/ServiceWorker...
Source/web/ServiceWorkerGlobalScopeProxy.cpp:84: 
extra newline

[jkarlin, 2014-03-25 15:57:46]

https://codereview.chromium.org/205013002/diff/20001/Source/web/ServiceWorker...
File Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/205013002/diff/20001/Source/web/ServiceWorker...
Source/web/ServiceWorkerGlobalScopeProxy.cpp:84: 
On 2014/03/24 07:29:16, falken wrote:
> extra newline

Thanks! Done.

[jkarlin, 2014-03-25 16:00:12]

jochen: Please review all files for owners.  Thanks!

[jkarlin, 2014-03-25 16:00:53]

On 2014/03/25 16:00:12, jkarlin wrote:
> jochen: Please review all files for owners.  Thanks!

jochen: You don't need to review the modules/serviceworkers/ files.

[jochen (gone - plz use gerrit), 2014-03-26 08:31:22]

lgtm

[jkarlin, 2014-03-26 13:49:51]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-03-26 13:49:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jkarlin@chromium.org/205013002/110001

[commit-bot: I haz the power, 2014-03-26 14:58:12]

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

[commit-bot: I haz the power, 2014-03-26 14:58:12]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[jkarlin, 2014-03-26 15:00:10]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-03-26 15:00:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jkarlin@chromium.org/205013002/110001

[commit-bot: I haz the power, 2014-03-26 16:12:49]

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

[commit-bot: I haz the power, 2014-03-26 16:12:50]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[jkarlin, 2014-03-26 17:03:41]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-03-26 17:03:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jkarlin@chromium.org/205013002/110001

[commit-bot: I haz the power, 2014-03-26 18:11:43]

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

[commit-bot: I haz the power, 2014-03-26 18:11:43]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[jkarlin, 2014-03-26 18:43:51]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-03-26 18:44:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jkarlin@chromium.org/205013002/110001

[commit-bot: I haz the power, 2014-03-26 18:44:07]

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

[commit-bot: I haz the power, 2014-03-26 18:44:08]

Failed to apply patch for Source/web/ServiceWorkerGlobalScopeClientImpl.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/web/ServiceWorkerGlobalScopeClientImpl.cpp
  Hunk #1 FAILED at 50.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/web/ServiceWorkerGlobalScopeClientImpl.cpp.rej

Patch:       Source/web/ServiceWorkerGlobalScopeClientImpl.cpp
Index: Source/web/ServiceWorkerGlobalScopeClientImpl.cpp
diff --git a/Source/web/ServiceWorkerGlobalScopeClientImpl.cpp
b/Source/web/ServiceWorkerGlobalScopeClientImpl.cpp
index
10b5cff59cf7575a133eb5fb2d86729c59e4e250..e5e43aed26d97c63662a4896a094c96a04e56bf0
100644
--- a/Source/web/ServiceWorkerGlobalScopeClientImpl.cpp
+++ b/Source/web/ServiceWorkerGlobalScopeClientImpl.cpp
@@ -50,14 +50,11 @@
ServiceWorkerGlobalScopeClientImpl::~ServiceWorkerGlobalScopeClientImpl()
 
 void ServiceWorkerGlobalScopeClientImpl::didHandleInstallEvent(int
installEventID)
 {
-    if (m_client)
-        m_client->didHandleInstallEvent(installEventID);
+    m_client->didHandleInstallEvent(installEventID);
 }
 
 void ServiceWorkerGlobalScopeClientImpl::didHandleFetchEvent(int fetchEventID,
PassRefPtr<WebCore::Response> response)
 {
-    if (!m_client)
-        return;
     if (!response) {
         m_client->didHandleFetchEvent(fetchEventID);
         return;
@@ -68,6 +65,11 @@ void
ServiceWorkerGlobalScopeClientImpl::didHandleFetchEvent(int fetchEventID, P
     m_client->didHandleFetchEvent(fetchEventID, webResponse);
 }
 
+void ServiceWorkerGlobalScopeClientImpl::didHandleSyncEvent(int syncEventID)
+{
+    m_client->didHandleSyncEvent(syncEventID);
+}
+

ServiceWorkerGlobalScopeClientImpl::ServiceWorkerGlobalScopeClientImpl(PassOwnPtr<WebServiceWorkerContextClient>
client)
     : m_client(client)
 {

[jkarlin, 2014-03-26 18:54:05]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-03-26 18:54:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jkarlin@chromium.org/205013002/110001

[commit-bot: I haz the power, 2014-03-26 18:54:34]

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

[commit-bot: I haz the power, 2014-03-26 18:54:35]

Failed to apply patch for Source/web/ServiceWorkerGlobalScopeClientImpl.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/web/ServiceWorkerGlobalScopeClientImpl.cpp
  Hunk #1 FAILED at 50.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/web/ServiceWorkerGlobalScopeClientImpl.cpp.rej

Patch:       Source/web/ServiceWorkerGlobalScopeClientImpl.cpp
Index: Source/web/ServiceWorkerGlobalScopeClientImpl.cpp
diff --git a/Source/web/ServiceWorkerGlobalScopeClientImpl.cpp
b/Source/web/ServiceWorkerGlobalScopeClientImpl.cpp
index
10b5cff59cf7575a133eb5fb2d86729c59e4e250..e5e43aed26d97c63662a4896a094c96a04e56bf0
100644
--- a/Source/web/ServiceWorkerGlobalScopeClientImpl.cpp
+++ b/Source/web/ServiceWorkerGlobalScopeClientImpl.cpp
@@ -50,14 +50,11 @@
ServiceWorkerGlobalScopeClientImpl::~ServiceWorkerGlobalScopeClientImpl()
 
 void ServiceWorkerGlobalScopeClientImpl::didHandleInstallEvent(int
installEventID)
 {
-    if (m_client)
-        m_client->didHandleInstallEvent(installEventID);
+    m_client->didHandleInstallEvent(installEventID);
 }
 
 void ServiceWorkerGlobalScopeClientImpl::didHandleFetchEvent(int fetchEventID,
PassRefPtr<WebCore::Response> response)
 {
-    if (!m_client)
-        return;
     if (!response) {
         m_client->didHandleFetchEvent(fetchEventID);
         return;
@@ -68,6 +65,11 @@ void
ServiceWorkerGlobalScopeClientImpl::didHandleFetchEvent(int fetchEventID, P
     m_client->didHandleFetchEvent(fetchEventID, webResponse);
 }
 
+void ServiceWorkerGlobalScopeClientImpl::didHandleSyncEvent(int syncEventID)
+{
+    m_client->didHandleSyncEvent(syncEventID);
+}
+

ServiceWorkerGlobalScopeClientImpl::ServiceWorkerGlobalScopeClientImpl(PassOwnPtr<WebServiceWorkerContextClient>
client)
     : m_client(client)
 {

[jkarlin, 2014-03-26 19:07:34]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-03-26 19:07:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jkarlin@chromium.org/205013002/300001

[commit-bot: I haz the power, 2014-03-26 20:11:30]

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

[commit-bot: I haz the power, 2014-03-26 20:11:31]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[jkarlin, 2014-03-26 21:07:08]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-03-26 21:07:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jkarlin@chromium.org/205013002/300001

[commit-bot: I haz the power, 2014-03-26 21:37:05]

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

[commit-bot: I haz the power, 2014-03-26 21:37:06]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[jkarlin, 2014-03-27 00:55:18]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-03-27 00:55:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jkarlin@chromium.org/205013002/300001

[commit-bot: I haz the power, 2014-03-27 01:31:38]

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

[commit-bot: I haz the power, 2014-03-27 01:31:38]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[jkarlin, 2014-03-27 10:40:24]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-03-27 10:40:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jkarlin@chromium.org/205013002/300001

[commit-bot: I haz the power, 2014-03-27 10:51:41]

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

[commit-bot: I haz the power, 2014-03-27 10:51:41]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[jkarlin, 2014-03-27 11:11:57]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-03-27 11:12:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jkarlin@chromium.org/205013002/300001

[commit-bot: I haz the power, 2014-03-27 11:47:39]

Change committed as 170165