Revise ServiceWorker DevTools protocols.

Revise ServiceWorker DevTools protocols.

After I landed https://codereview.chromium.org/985663002/ I noticed that there is some problems in ServiceWorker DevTools protocol.

1. ServiceWorkerHandler::ContextObserver can't get the state of ServiceWorkerVersion correctly.
 ServiceWorkerHandler::ContextObserver is using GetLiveVersion in OnVersionUpdated().
 But when OnVersionUpdated() is called the live versions may already be deleted.
 This is because OnVersionUpdated() is called via ObserverListThreadSafe<ServiceWorkerContextObserver> in ServiceWorkerContextCore.
 So I change ServiceWorkerContextObserver::OnRunningStateChanged() and ServiceWorkerContextObserver::OnVersionStateChanged() to pass the new state and introduce OnNewLiveRegistration() and OnNewLiveVersion().

2. We have to distinguish the ServiceWorkerRegistrations which are deleted from the storage.
 We shuold show the registrations which are already deleted from the storage but are still exist in the memory.
 This could happen when ServiceWorkerRegistration.unregister() has been called but there are still some controlled pages.
 So I will add isDeleted flag to ServiceWorkerRegistration DevTool's protocol. (https://codereview.chromium.org/996363002/)
 And we can stop using workerRegistrationDeleted event.

3. Stop using {active/waiting/installing}_version properties of ServiceWorkerRegistration in DevTools protocols.
 We send the all ServiceWorkerVersion infromation with workerVersionUpdated event and it has "state" properties.
 So we don't need {active/waiting/installing}_version properties of ServiceWorkerRegistration.

This change depends on https://codereview.chromium.org/996363002/.

BUG=466871
Committed: https://crrev.com/a6bb251916eec43cb0c7c6709c57aa6b4209be11
Cr-Commit-Position: refs/heads/master@{#321277}

[horo, 2015-03-12 05:10:12]

Patchset #1 (id:1) has been deleted

[horo, 2015-03-12 05:10:18]

Patchset #1 (id:20001) has been deleted

[horo, 2015-03-12 05:26:03]

Patchset #1 (id:40001) has been deleted

[horo, 2015-03-12 05:26:26]

horo@chromium.org changed reviewers:
+ ksakamoto@chromium.org

[horo, 2015-03-12 05:26:27]

ksakamoto@
Could you please review this?

[Kunihiko Sakamoto, 2015-03-12 08:08:46]

https://codereview.chromium.org/998173002/diff/60001/content/browser/devtools...
File content/browser/devtools/protocol/service_worker_handler.cc (right):

https://codereview.chromium.org/998173002/diff/60001/content/browser/devtools...
content/browser/devtools/protocol/service_worker_handler.cc:137:
base::ScopedPtrHashMap<int64, ServiceWorkerVersionInfo> version_info_map_;
It seems items entered in these maps are never removed. Do we need to keep them
all?

https://codereview.chromium.org/998173002/diff/60001/content/browser/devtools...
content/browser/devtools/protocol/service_worker_handler.cc:320:
registration->is_deleted = true;
A deleted registration can be revived if register() is called again before it
loses all the controllees.
I think you need to observe OnRegistrationStored() to reset is_deleted flag.

https://codereview.chromium.org/998173002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/998173002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:1136:
info.waiting_version.version_id = registration_data.version_id;
This line is duplicated.

[horo, 2015-03-12 09:12:59]

https://codereview.chromium.org/998173002/diff/60001/content/browser/devtools...
File content/browser/devtools/protocol/service_worker_handler.cc (right):

https://codereview.chromium.org/998173002/diff/60001/content/browser/devtools...
content/browser/devtools/protocol/service_worker_handler.cc:137:
base::ScopedPtrHashMap<int64, ServiceWorkerVersionInfo> version_info_map_;
On 2015/03/12 08:08:46, Kunihiko Sakamoto wrote:
> It seems items entered in these maps are never removed. Do we need to keep
them
> all?
> 

Changed to delete STOPPED & REDUNDANT versions.

https://codereview.chromium.org/998173002/diff/60001/content/browser/devtools...
content/browser/devtools/protocol/service_worker_handler.cc:320:
registration->is_deleted = true;
On 2015/03/12 08:08:46, Kunihiko Sakamoto wrote:
> A deleted registration can be revived if register() is called again before it
> loses all the controllees.
> I think you need to observe OnRegistrationStored() to reset is_deleted flag.

Done.

https://codereview.chromium.org/998173002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/998173002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:1136:
info.waiting_version.version_id = registration_data.version_id;
On 2015/03/12 08:08:46, Kunihiko Sakamoto wrote:
> This line is duplicated.

Done.

[Kunihiko Sakamoto, 2015-03-12 10:06:58]

lgtm

[horo, 2015-03-12 11:19:59]

horo@chromium.org changed reviewers:
+ pfeldman@chromium.org

[horo, 2015-03-12 11:19:59]

pfeldman@
Could you please review
content/browser/devtools/protocol/service_worker_handler.*?

[pfeldman, 2015-03-12 12:23:23]

https://codereview.chromium.org/998173002/diff/100001/content/browser/devtool...
File content/browser/devtools/protocol/service_worker_handler.cc (right):

https://codereview.chromium.org/998173002/diff/100001/content/browser/devtool...
content/browser/devtools/protocol/service_worker_handler.cc:103: class
ServiceWorkerHandler::ContextObserver
Is this time to kick it out of the class? Seems like it is growing fast.

[horo, 2015-03-12 13:47:16]

Patchset #5 (id:140001) has been deleted

[horo, 2015-03-12 13:54:15]

https://codereview.chromium.org/998173002/diff/100001/content/browser/devtool...
File content/browser/devtools/protocol/service_worker_handler.cc (right):

https://codereview.chromium.org/998173002/diff/100001/content/browser/devtool...
content/browser/devtools/protocol/service_worker_handler.cc:103: class
ServiceWorkerHandler::ContextObserver
On 2015/03/12 12:23:23, pfeldman wrote:
> Is this time to kick it out of the class? Seems like it is growing fast.

Done.

[pfeldman, 2015-03-12 14:00:46]

devtools lgtm, but you might want to run the new watcher by other sw owners now.

[horo, 2015-03-12 14:24:13]

horo@chromium.org changed reviewers:
+ kinuko@chromium.org

[horo, 2015-03-12 14:24:13]

pfeldman@
Thank you.

kinuko@
Could you please review this?

[kinuko, 2015-03-13 09:09:21]

looking good

https://codereview.chromium.org/998173002/diff/160001/content/browser/devtool...
File content/browser/devtools/protocol/service_worker_handler.cc (left):

https://codereview.chromium.org/998173002/diff/160001/content/browser/devtool...
content/browser/devtools/protocol/service_worker_handler.cc:99:
registration->set_installing_version(
Ok so we assume version.state info should be enough to show (identify) active-,
waiting- and installing versions. (I assume that's true?)

Should we remove set_xxx_version methods in devtools_protocol_dispatcher.h?

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
File content/browser/service_worker/service_worker_context_watcher.cc (right):

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_context_watcher.cc:55: this));
This is what we've been doing so it's ok, but will we continue to get all
registrations in a partition, or are we going to do so per origin eventually?

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_context_watcher.cc:152:
SendRegistrationInfo(registration_id, pattern, false);
nit: please annotate the boolean field with comment (e.g. false /* is_deleted
*/) or use enum

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_context_watcher.cc:161:
DCHECK(version->script_url == script_url);
nit: DCHECK_EQ

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
File content/browser/service_worker/service_worker_context_watcher.h (right):

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_context_watcher.h:19: class
ServiceWorkerContextWatcher
nit: now that it has its own header can we have a short class comment about
object granularity (e.g. per SWContext etc), lifetime (e.g. when it's created on
which thread) etc?

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
File content/browser/service_worker/service_worker_info.h (right):

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_info.h:40: public:
nit: could be a struct if all fields are public

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_info.h:44: bool is_deleted);
nit: prefer using enum over boolean in general

[horo, 2015-03-13 10:01:34]

Patchset #6 (id:180001) has been deleted

[horo, 2015-03-13 10:03:50]

https://codereview.chromium.org/998173002/diff/160001/content/browser/devtool...
File content/browser/devtools/protocol/service_worker_handler.cc (left):

https://codereview.chromium.org/998173002/diff/160001/content/browser/devtool...
content/browser/devtools/protocol/service_worker_handler.cc:99:
registration->set_installing_version(
On 2015/03/13 09:09:21, kinuko wrote:
> Ok so we assume version.state info should be enough to show (identify)
active-,
> waiting- and installing versions. (I assume that's true?)

Yes.

> Should we remove set_xxx_version methods in devtools_protocol_dispatcher.h?

I will remove them in https://codereview.chromium.org/1003653002/.

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
File content/browser/service_worker/service_worker_context_watcher.cc (right):

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_context_watcher.cc:55: this));
On 2015/03/13 09:09:21, kinuko wrote:
> This is what we've been doing so it's ok, but will we continue to get all
> registrations in a partition, or are we going to do so per origin eventually?

Yes. In future, it may better to filter the registration using origin.

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_context_watcher.cc:152:
SendRegistrationInfo(registration_id, pattern, false);
On 2015/03/13 09:09:21, kinuko wrote:
> nit: please annotate the boolean field with comment (e.g. false /* is_deleted
> */) or use enum

Done.

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_context_watcher.cc:161:
DCHECK(version->script_url == script_url);
On 2015/03/13 09:09:21, kinuko wrote:
> nit: DCHECK_EQ

Done.

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
File content/browser/service_worker/service_worker_context_watcher.h (right):

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_context_watcher.h:19: class
ServiceWorkerContextWatcher
On 2015/03/13 09:09:21, kinuko wrote:
> nit: now that it has its own header can we have a short class comment about
> object granularity (e.g. per SWContext etc), lifetime (e.g. when it's created
on
> which thread) etc?

Done.

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
File content/browser/service_worker/service_worker_info.h (right):

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_info.h:40: public:
On 2015/03/13 09:09:21, kinuko wrote:
> nit: could be a struct if all fields are public

Done.

https://codereview.chromium.org/998173002/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_info.h:44: bool is_deleted);
On 2015/03/13 09:09:21, kinuko wrote:
> nit: prefer using enum over boolean in general

Done.

[kinuko, 2015-03-13 12:17:24]

lgtm

https://codereview.chromium.org/998173002/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_info.h (right):

https://codereview.chromium.org/998173002/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_info.h:41: enum DeleteFlag {
IsNotDeleted, IsDeleted };
enum name should be CAPITAL_NAME (or kCamelName, less popular in chromium)

[horo, 2015-03-13 16:03:30]

https://codereview.chromium.org/998173002/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_info.h (right):

https://codereview.chromium.org/998173002/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_info.h:41: enum DeleteFlag {
IsNotDeleted, IsDeleted };
On 2015/03/13 12:17:24, kinuko wrote:
> enum name should be CAPITAL_NAME (or kCamelName, less popular in chromium)

Done.

[horo, 2015-03-13 23:53:18]

Patchset #8 (id:230001) has been deleted

[horo, 2015-03-14 03:22:05]

horo@chromium.org changed reviewers:
+ mek@chromium.org

[horo, 2015-03-14 03:22:06]

mek@chromium.org:
Please review changes in geofencing_manager_unittest.cc.

[horo, 2015-03-18 01:15:37]

mek@chromium.org:
Ping?
> Please review changes in geofencing_manager_unittest.cc.

[Marijn Kruisselbrink, 2015-03-18 16:10:16]

On 2015/03/18 01:15:37, horo wrote:
> mailto:mek@chromium.org:
> Ping?
> > Please review changes in geofencing_manager_unittest.cc.

I'm terribly sorry for the delay, LGTM

[horo, 2015-03-18 23:09:03]

The CQ bit was checked by horo@chromium.org

[horo, 2015-03-18 23:09:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
ksakamoto@chromium.org, pfeldman@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/998173002/#ps290001
(title: " ")

[commit-bot: I haz the power, 2015-03-18 23:09:23]

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

[commit-bot: I haz the power, 2015-03-18 23:17:49]

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

[commit-bot: I haz the power, 2015-03-18 23:17:51]

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...)

[horo, 2015-03-19 01:36:52]

The CQ bit was checked by horo@chromium.org

[horo, 2015-03-19 01:36:53]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
ksakamoto@chromium.org, mek@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/998173002/#ps330001
(title: "use DCHECK_CURRENTLY_ON")

[commit-bot: I haz the power, 2015-03-19 01:37:23]

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

[commit-bot: I haz the power, 2015-03-19 02:49:03]

Committed patchset #12 (id:330001)

[commit-bot: I haz the power, 2015-03-19 02:49:40]

Patchset 12 (id:??) landed as
https://crrev.com/a6bb251916eec43cb0c7c6709c57aa6b4209be11
Cr-Commit-Position: refs/heads/master@{#321277}