Update chrome://serviceworker-internals when ServiceWorker is unregistered.

Update chrome://serviceworker-internals when ServiceWorker is unregistered.

BUG=358657
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270995

[horo, 2014-05-14 11:06:31]

kinuko@
Could you please review this?

[kinuko, 2014-05-15 15:34:59]

https://codereview.chromium.org/288683002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/288683002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_context_core.cc:293:
&ServiceWorkerContextObserver::OnRegistrationStored,
Would it be enough if we notify these in callbacks of
ServiceWorkerContextCore::{Register,Unregister}ServiceWorker in ContextCore? 
(I.e. RegistrationComplete and... Unregister doesn't have callback stub in
ContextCore, but we can have it)

(RegisterServiceWorker calls the given callback before it stores registration,
but GetAllRegistrations also returns the on-memory ones that are being
installed)

[horo, 2014-05-16 05:31:25]

https://codereview.chromium.org/288683002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/288683002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_context_core.cc:293:
&ServiceWorkerContextObserver::OnRegistrationStored,
On 2014/05/15 15:35:00, kinuko wrote:
> Would it be enough if we notify these in callbacks of
> ServiceWorkerContextCore::{Register,Unregister}ServiceWorker in ContextCore? 
> (I.e. RegistrationComplete and... Unregister doesn't have callback stub in
> ContextCore, but we can have it)
> 
> (RegisterServiceWorker calls the given callback before it stores registration,
> but GetAllRegistrations also returns the on-memory ones that are being
> installed)

I introduced UnregistrationComplete() and changed to call
ServiceWorkerContextObserver::OnRegistration{Stored/Deleted} from
ServiceWorkerContextCore::{Registration/Unregistration}Complete.

I think we shouldn't call
ServiceWorkerContextObserver::OnRegistration{Stored/Deleted} from
ServiceWorkerContextCore::{Register/Unregister}ServiceWorker, because at that
time the registration/unregistration may not be completed.

(If GetAllRegistrations is called when in
ServiceWorkerContextObserver::OnRegistration{Stored/Deleted}, it doesn't call
the callback until the registration/unregistration will be completed becase
GetAllRegistrations is executed in database_task_runner_)

[kinuko, 2014-05-16 05:44:47]

On 2014/05/16 05:31:25, horo wrote:
>
https://codereview.chromium.org/288683002/diff/1/content/browser/service_work...
> File content/browser/service_worker/service_worker_context_core.cc (right):
> 
>
https://codereview.chromium.org/288683002/diff/1/content/browser/service_work...
> content/browser/service_worker/service_worker_context_core.cc:293:
> &ServiceWorkerContextObserver::OnRegistrationStored,
> On 2014/05/15 15:35:00, kinuko wrote:
> > Would it be enough if we notify these in callbacks of
> > ServiceWorkerContextCore::{Register,Unregister}ServiceWorker in ContextCore?

> > (I.e. RegistrationComplete and... Unregister doesn't have callback stub in
> > ContextCore, but we can have it)
> > 
> > (RegisterServiceWorker calls the given callback before it stores
registration,
> > but GetAllRegistrations also returns the on-memory ones that are being
> > installed)
> 
> I introduced UnregistrationComplete() and changed to call
> ServiceWorkerContextObserver::OnRegistration{Stored/Deleted} from
> ServiceWorkerContextCore::{Registration/Unregistration}Complete.
> 
> I think we shouldn't call
> ServiceWorkerContextObserver::OnRegistration{Stored/Deleted} from
> ServiceWorkerContextCore::{Register/Unregister}ServiceWorker, because at that
> time the registration/unregistration may not be completed.

Yes I think I suggested what you did (hook OnRegistration{Stored,Deleted} in the
completion callbacks).

[kinuko, 2014-05-16 05:45:39]

lgtm

https://codereview.chromium.org/288683002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/288683002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.cc:197:
observer_list_->Notify(&ServiceWorkerContextObserver::OnRegistrationStored,
Could these (Stored, Deleted) be just 'OnRegistrationUpdated' ?

[horo, 2014-05-16 05:58:38]

The CQ bit was checked by horo@chromium.org

[horo, 2014-05-16 05:58:52]

On 2014/05/16 05:45:39, kinuko wrote:
> lgtm
> 
>
https://codereview.chromium.org/288683002/diff/40001/content/browser/service_...
> File content/browser/service_worker/service_worker_context_core.cc (right):
> 
>
https://codereview.chromium.org/288683002/diff/40001/content/browser/service_...
> content/browser/service_worker/service_worker_context_core.cc:197:
> observer_list_->Notify(&ServiceWorkerContextObserver::OnRegistrationStored,
> Could these (Stored, Deleted) be just 'OnRegistrationUpdated' ?

I like (Stored, Deleted) because it can provide the serviceworker-internals page
with richer information.

[commit-bot: I haz the power, 2014-05-16 05:59:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/horo@chromium.org/288683002/40001

[commit-bot: I haz the power, 2014-05-16 08:14:56]

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

[commit-bot: I haz the power, 2014-05-16 08:14:57]

Failed to apply patch for
@master/content/browser/resources/service_worker/serviceworker_internals.js:
While running svn add @master --force --config-dir
/b/infra_internal/commit_queue/subversion_config --non-interactive;
  svn: '@master' is just a peg revision. Maybe try '@master@' instead?

Patch:      
@master/content/browser/resources/service_worker/serviceworker_internals.js
Index: content/browser/resources/service_worker/serviceworker_internals.js
diff --git
@master/content/browser/resources/service_worker/serviceworker_internals.js
@master/content/browser/resources/service_worker/serviceworker_internals.js
index
7ffa11fb8846eafa8c4008855bee0c301107a2ad..ec2132c985ec66df5c642018ecf2cd4f3f4dc43c
100644
---
a/@master/content/browser/resources/service_worker/serviceworker_internals.js
+++
b/@master/content/browser/resources/service_worker/serviceworker_internals.js
@@ -133,6 +133,14 @@ cr.define('serviceworker', function() {
         update();
     }
 
+    function onRegistrationStored(scope) {
+        update();
+    }
+
+    function onRegistrationDeleted(scope) {
+        update();
+    }
+
     function outputLogMessage(partition_id, version_id, message) {
         if (!(partition_id in allLogMessages)) {
             allLogMessages[partition_id] = {};
@@ -164,6 +172,8 @@ cr.define('serviceworker', function() {
         onErrorReported: onErrorReported,
         onConsoleMessageReported: onConsoleMessageReported,
         onVersionStateChanged: onVersionStateChanged,
+        onRegistrationStored: onRegistrationStored,
+        onRegistrationDeleted: onRegistrationDeleted,
     };
 });

[horo, 2014-05-16 08:38:56]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-05-16 08:39:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/horo@chromium.org/288683002/40001

[horo, 2014-05-16 08:40:49]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-05-16 08:41:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/horo@chromium.org/288683002/60001

[commit-bot: I haz the power, 2014-05-16 11:35:03]

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

[commit-bot: I haz the power, 2014-05-16 11:35:04]

Failed to apply patch for
@master/content/browser/resources/service_worker/serviceworker_internals.js:
While running svn add @master --force --config-dir
/b/infra_internal/commit_queue/subversion_config --non-interactive;
  svn: '@master' is just a peg revision. Maybe try '@master@' instead?

Patch:      
@master/content/browser/resources/service_worker/serviceworker_internals.js
Index: content/browser/resources/service_worker/serviceworker_internals.js
diff --git
@master/content/browser/resources/service_worker/serviceworker_internals.js
@master/content/browser/resources/service_worker/serviceworker_internals.js
index
7ffa11fb8846eafa8c4008855bee0c301107a2ad..ec2132c985ec66df5c642018ecf2cd4f3f4dc43c
100644
---
a/@master/content/browser/resources/service_worker/serviceworker_internals.js
+++
b/@master/content/browser/resources/service_worker/serviceworker_internals.js
@@ -133,6 +133,14 @@ cr.define('serviceworker', function() {
         update();
     }
 
+    function onRegistrationStored(scope) {
+        update();
+    }
+
+    function onRegistrationDeleted(scope) {
+        update();
+    }
+
     function outputLogMessage(partition_id, version_id, message) {
         if (!(partition_id in allLogMessages)) {
             allLogMessages[partition_id] = {};
@@ -164,6 +172,8 @@ cr.define('serviceworker', function() {
         onErrorReported: onErrorReported,
         onConsoleMessageReported: onConsoleMessageReported,
         onVersionStateChanged: onVersionStateChanged,
+        onRegistrationStored: onRegistrationStored,
+        onRegistrationDeleted: onRegistrationDeleted,
     };
 });

[Paweł Hajdan Jr., 2014-05-16 11:49:32]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-05-16 11:49:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/horo@chromium.org/288683002/60001

[commit-bot: I haz the power, 2014-05-16 11:50:39]

Change committed as 270995