Add ServiceWorker support in EmbeddedWorkerDevToolsManager.

Add ServiceWorker support in EmbeddedWorkerDevToolsManager.

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

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

[horo, 2014-05-01 11:08:46]

yurys@
Could you please review this?

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

[yurys, 2014-05-02 04:35:35]

lgtm w/ comments

https://codereview.chromium.org/262773004/diff/40001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_manager.cc (right):

https://codereview.chromium.org/262773004/diff/40001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_manager.cc:195:
DCHECK_EQ(WORKER_TERMINATED, it->second->state());
Consider extracting common tail into a separate method.

https://codereview.chromium.org/262773004/diff/40001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_manager.cc:293: const
base::FilePath& service_worker_path,
Would it make sense to have ServiceWorkerInstance (similar to
SharedWorkerInstance) and pass it everywhere instead of the path+scope pair?

https://codereview.chromium.org/262773004/diff/40001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_manager.h (right):

https://codereview.chromium.org/262773004/diff/40001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_manager.h:79: bool
Matches(const base::FilePath& other_service_worker_path,
nit: move implementation of Matches methods to the .cc file

[horo, 2014-05-02 06:31:55]

https://codereview.chromium.org/262773004/diff/40001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_manager.cc (right):

https://codereview.chromium.org/262773004/diff/40001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_manager.cc:195:
DCHECK_EQ(WORKER_TERMINATED, it->second->state());
On 2014/05/02 04:35:35, yurys wrote:
> Consider extracting common tail into a separate method.

Done.

https://codereview.chromium.org/262773004/diff/40001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_manager.cc:293: const
base::FilePath& service_worker_path,
On 2014/05/02 04:35:35, yurys wrote:
> Would it make sense to have ServiceWorkerInstance (similar to
> SharedWorkerInstance) and pass it everywhere instead of the path+scope pair?

Added comment in .h.

https://codereview.chromium.org/262773004/diff/40001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_manager.h (right):

https://codereview.chromium.org/262773004/diff/40001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_manager.h:79: bool
Matches(const base::FilePath& other_service_worker_path,
On 2014/05/02 04:35:35, yurys wrote:
> nit: move implementation of Matches methods to the .cc file

Done.

[kinuko, 2014-05-02 08:51:05]

https://codereview.chromium.org/262773004/diff/60001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_manager.h (right):

https://codereview.chromium.org/262773004/diff/60001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_manager.h:43: //
ServiceWorkerStorage and the scope of ServiceWorker. Consider having
'FilePath of ServiceWorkerStorage' is the information that is too internal to SW
impl, could we use 'storage partition path' or something more generic
term/identifier here?

Also.. I need to double-check but I don't think we can have same scope in
different storage partitions.

[horo, 2014-05-02 09:58:16]

https://codereview.chromium.org/262773004/diff/60001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_manager.h (right):

https://codereview.chromium.org/262773004/diff/60001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_manager.h:43: //
ServiceWorkerStorage and the scope of ServiceWorker. Consider having
On 2014/05/02 08:51:05, kinuko wrote:
> 'FilePath of ServiceWorkerStorage' is the information that is too internal to
SW
> impl, could we use 'storage partition path' or something more generic
> term/identifier here?
> 
> Also.. I need to double-check but I don't think we can have same scope in
> different storage partitions.

Changed the comment.
I think it would occur when the user is using multi accounts.

[kinuko, 2014-05-02 10:06:45]

On 2014/05/02 09:58:16, horo wrote:
>
https://codereview.chromium.org/262773004/diff/60001/content/browser/devtools...
> File content/browser/devtools/embedded_worker_devtools_manager.h (right):
> 
>
https://codereview.chromium.org/262773004/diff/60001/content/browser/devtools...
> content/browser/devtools/embedded_worker_devtools_manager.h:43: //
> ServiceWorkerStorage and the scope of ServiceWorker. Consider having
> On 2014/05/02 08:51:05, kinuko wrote:
> > 'FilePath of ServiceWorkerStorage' is the information that is too internal
to
> SW
> > impl, could we use 'storage partition path' or something more generic
> > term/identifier here?
> > 
> > Also.. I need to double-check but I don't think we can have same scope in
> > different storage partitions.
> 
> Changed the comment.
> I think it would occur when the user is using multi accounts.

Ah ok... it's singleton and shared by multiple profiles.

> const base::FilePath& service_worker_path,

nit: could we also change the parameter name? ('service worker path' could be
read in multiple meanings)

[horo, 2014-05-02 10:19:59]

On 2014/05/02 10:06:45, kinuko wrote:
> On 2014/05/02 09:58:16, horo wrote:
> >
>
https://codereview.chromium.org/262773004/diff/60001/content/browser/devtools...
> > File content/browser/devtools/embedded_worker_devtools_manager.h (right):
> > 
> >
>
https://codereview.chromium.org/262773004/diff/60001/content/browser/devtools...
> > content/browser/devtools/embedded_worker_devtools_manager.h:43: //
> > ServiceWorkerStorage and the scope of ServiceWorker. Consider having
> > On 2014/05/02 08:51:05, kinuko wrote:
> > > 'FilePath of ServiceWorkerStorage' is the information that is too internal
> to
> > SW
> > > impl, could we use 'storage partition path' or something more generic
> > > term/identifier here?
> > > 
> > > Also.. I need to double-check but I don't think we can have same scope in
> > > different storage partitions.
> > 
> > Changed the comment.
> > I think it would occur when the user is using multi accounts.
> 
> Ah ok... it's singleton and shared by multiple profiles.
> 
> > const base::FilePath& service_worker_path,
> 
> nit: could we also change the parameter name? ('service worker path' could be
> read in multiple meanings)

Done.

[horo, 2014-05-03 10:44:30]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-05-03 10:44:32]

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

[horo, 2014-05-03 10:44:51]

The CQ bit was unchecked by horo@chromium.org

[horo, 2014-05-06 00:07:20]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-05-06 00:07:55]

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

[commit-bot: I haz the power, 2014-05-06 00:13:48]

Change committed as 268332