Unify shared and service workers in chrome://inspect, remove service workers iframe.

Unify shared and service workers in chrome://inspect, remove service workers iframe.

BUG=none

Committed: https://crrev.com/ffe70eca0f81753c11052b9229128f72d49c45ea
Cr-Commit-Position: refs/heads/master@{#295466}

[dgozman, 2014-09-15 08:19:30]

dgozman@chromium.org changed reviewers:
+ horo@chromium.org, pfeldman@chromium.org, vkuzkokov@chromium.org

[dgozman, 2014-09-15 08:19:30]

Could you please take a look?

[horo, 2014-09-16 07:41:38]

The list of ServiceWorkers in chrome://inspect will not be updated when the
running ServiceWorkers stops.
We need to reload the chrome://inspect page.

Is this acceptable?

https://codereview.chromium.org/568403002/diff/1/chrome/browser/devtools/devt...
File chrome/browser/devtools/devtools_target_impl.cc (right):

https://codereview.chromium.org/568403002/diff/1/chrome/browser/devtools/devt...
chrome/browser/devtools/devtools_target_impl.cc:272:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Move to the top of the method.

https://codereview.chromium.org/568403002/diff/1/chrome/browser/devtools/devt...
chrome/browser/devtools/devtools_target_impl.cc:281: bool is_tab =
const

https://codereview.chromium.org/568403002/diff/1/chrome/browser/ui/webui/insp...
File chrome/browser/ui/webui/inspect_ui.cc (right):

https://codereview.chromium.org/568403002/diff/1/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.cc:281: ForceUpdateIfNeeded(source_id,
target_type);
ForceUpdateIfNeeded(source_id, target->GetType());

https://codereview.chromium.org/568403002/diff/1/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.cc:291: ForceUpdateIfNeeded(source_id,
target_type);
ditto

https://codereview.chromium.org/568403002/diff/1/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.cc:301: ForceUpdateIfNeeded(source_id,
target_type);
ditto

https://codereview.chromium.org/568403002/diff/1/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.cc:311: ForceUpdateIfNeeded(source_id,
target_type);
ditto

[dgozman, 2014-09-16 08:42:58]

On 2014/09/16 07:41:38, horo wrote:
> The list of ServiceWorkers in chrome://inspect will not be updated when the
> running ServiceWorkers stops.
> We need to reload the chrome://inspect page.
> 
> Is this acceptable?

Unfortunately, chrome://inspect does not update in many circumstances (e.g. tab
navigated and url/title changed).
So, not updating due to non-user-triggered stop doesn't sound really bad in the
short-term (esp. considering service workers are still an experimental feature).

I'm working on a better approach for targets discovery/presentation right now,
so this problem would be solved.

[dgozman, 2014-09-16 08:55:34]

PTAL

https://codereview.chromium.org/568403002/diff/1/chrome/browser/devtools/devt...
File chrome/browser/devtools/devtools_target_impl.cc (right):

https://codereview.chromium.org/568403002/diff/1/chrome/browser/devtools/devt...
chrome/browser/devtools/devtools_target_impl.cc:272:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/09/16 07:41:37, horo wrote:
> Move to the top of the method.

Done.

https://codereview.chromium.org/568403002/diff/1/chrome/browser/devtools/devt...
chrome/browser/devtools/devtools_target_impl.cc:281: bool is_tab =
On 2014/09/16 07:41:37, horo wrote:
> const

Done.

https://codereview.chromium.org/568403002/diff/1/chrome/browser/ui/webui/insp...
File chrome/browser/ui/webui/inspect_ui.cc (right):

https://codereview.chromium.org/568403002/diff/1/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.cc:281: ForceUpdateIfNeeded(source_id,
target_type);
On 2014/09/16 07:41:37, horo wrote:
> ForceUpdateIfNeeded(source_id, target->GetType());

What if executing action will lead to target being synchronously removed? We
have no protection here. This is not likely with Inspect, but Close or Reload
have no guarantees.

[horo, 2014-09-16 10:31:20]

> Unfortunately, chrome://inspect does not update in many circumstances (e.g.
tab
> navigated and url/title changed).

Ah, I didn't know that.

> So, not updating due to non-user-triggered stop doesn't sound really bad in
the
> short-term (esp. considering service workers are still an experimental
feature).
>
> I'm working on a better approach for targets discovery/presentation right now,
> so this problem would be solved.

It sounds reasonable.
Do you have an issue ID in crbug.com? Please let me know.

https://codereview.chromium.org/568403002/diff/1/chrome/browser/ui/webui/insp...
File chrome/browser/ui/webui/inspect_ui.cc (right):

https://codereview.chromium.org/568403002/diff/1/chrome/browser/ui/webui/insp...
chrome/browser/ui/webui/inspect_ui.cc:281: ForceUpdateIfNeeded(source_id,
target_type);
On 2014/09/16 08:55:34, dgozman wrote:
> On 2014/09/16 07:41:37, horo wrote:
> > ForceUpdateIfNeeded(source_id, target->GetType());
> 
> What if executing action will lead to target being synchronously removed? We
> have no protection here. This is not likely with Inspect, but Close or Reload
> have no guarantees.

Ah, I got it.

https://codereview.chromium.org/568403002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/inspect_ui.cc (right):

https://codereview.chromium.org/568403002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/inspect_ui.cc:279: std::string target_type =
target->GetType();
const

https://codereview.chromium.org/568403002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/inspect_ui.cc:289: std::string target_type =
target->GetType();
ditto

https://codereview.chromium.org/568403002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/inspect_ui.cc:299: std::string target_type =
target->GetType();
ditto

[dgozman, 2014-09-16 12:42:38]

Please refer to crbug.com/398049 for target discovery plan.

https://codereview.chromium.org/568403002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/inspect_ui.cc (right):

https://codereview.chromium.org/568403002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/inspect_ui.cc:279: std::string target_type =
target->GetType();
On 2014/09/16 10:31:20, horo wrote:
> const

All done.

[horo, 2014-09-16 12:46:17]

lgtm

[vkuzkokov, 2014-09-16 15:52:05]

On 2014/09/15 08:19:30, dgozman wrote:
> Could you please take a look?

lgtm

[pfeldman, 2014-09-18 11:46:36]

lgtm

[dgozman, 2014-09-18 13:28:06]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-09-18 13:29:34]

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

[commit-bot: I haz the power, 2014-09-18 14:28:19]

Committed patchset #3 (id:40001) as 36580c02add297e290d90368fffa68860dcea0cc

[commit-bot: I haz the power, 2014-09-18 14:28:55]

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