[App Shell] Add dev tools for debugging

[App Shell] Add dev tools for debugging

BUG=None
TEST=launch app_shell with flag "--remote-debugging-port=9222", then open
"localhost:9222" in browser and see the console.

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

[Haojian Wu, 2014-02-09 13:06:56]

James@, I add dev tools in app_shell, so that it's easier for debugging. Does
app_shell need this feature?

[James Cook, 2014-02-10 20:31:23]

I think it would be nice to have devtools support like this.

https://codereview.chromium.org/147993009/diff/1/apps/shell/app/shell_main_de...
File apps/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/147993009/diff/1/apps/shell/app/shell_main_de...
apps/shell/app/shell_main_delegate.cc:100: pak_file =
pak_dir.Append(FILE_PATH_LITERAL("content_shell.pak"));
Why do we need content_shell's PAK file?  Isn't Chrome's sufficient?

If we need both pak files please comment in the source why we need both.

nit: I would put these lines below the AddDataPackFromPath line below.

https://codereview.chromium.org/147993009/diff/1/apps/shell/browser/shell_bro...
File apps/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/147993009/diff/1/apps/shell/browser/shell_bro...
apps/shell/browser/shell_browser_main_parts.cc:145: if (devtools_delegate_)
Why do you need the if()?  It looks like it should always exist.

[Haojian Wu, 2014-02-11 03:24:36]

https://codereview.chromium.org/147993009/diff/1/apps/shell/app/shell_main_de...
File apps/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/147993009/diff/1/apps/shell/app/shell_main_de...
apps/shell/app/shell_main_delegate.cc:100: pak_file =
pak_dir.Append(FILE_PATH_LITERAL("content_shell.pak"));
On 2014/02/10 20:31:23, James Cook wrote:
> Why do we need content_shell's PAK file?  Isn't Chrome's sufficient?
> 
> If we need both pak files please comment in the source why we need both.
> 
> nit: I would put these lines below the AddDataPackFromPath line below.

Done.

Yes, we need content_shell.pak here.
We use content's DevTools rather than chrome's, so we need
shell_devtools_discovery_page.html in content.pak instead of
devtools_discovery_page.html in chrome resource pak file.

https://codereview.chromium.org/147993009/diff/1/apps/shell/browser/shell_bro...
File apps/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/147993009/diff/1/apps/shell/browser/shell_bro...
apps/shell/browser/shell_browser_main_parts.cc:145: if (devtools_delegate_)
On 2014/02/10 20:31:23, James Cook wrote:
> Why do you need the if()?  It looks like it should always exist.

Yes, in this code logic, it's always valid (it gets reset in
PreMainMessageLoopRun).
But I think it will be better to do this check although it's valid here  since
the pointer initialized NULL when constructing ShellBrowserMainParts object.

[James Cook, 2014-02-11 17:11:16]

LGTM with one optional suggestion

https://codereview.chromium.org/147993009/diff/1/apps/shell/browser/shell_bro...
File apps/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/147993009/diff/1/apps/shell/browser/shell_bro...
apps/shell/browser/shell_browser_main_parts.cc:145: if (devtools_delegate_)
On 2014/02/11 03:24:36, Haojian Wu wrote:
> On 2014/02/10 20:31:23, James Cook wrote:
> > Why do you need the if()?  It looks like it should always exist.
> 
> Yes, in this code logic, it's always valid (it gets reset in
> PreMainMessageLoopRun).
> But I think it will be better to do this check although it's valid here  since
> the pointer initialized NULL when constructing ShellBrowserMainParts object.
> 

I wouldn't do the if() because it confuses the reader -- it makes the reader
think there is a code path where the pointer will be NULL. For example, in
DestroyRootWindow() we don't check if wm_test_helper_ is NULL, even though it
starts as NULL in the constructor. Likewise in CreateExtensionSystem() we don't
check if browser_context_ is NULL.

[Haojian Wu, 2014-02-12 13:42:22]

The CQ bit was checked by hokein.wu@gmail.com

[commit-bot: I haz the power, 2014-02-12 13:42:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/147993009/140001

[Haojian Wu, 2014-02-12 13:42:32]

On 2014/02/11 17:11:16, James Cook wrote:
> LGTM with one optional suggestion
> 
>
https://codereview.chromium.org/147993009/diff/1/apps/shell/browser/shell_bro...
> File apps/shell/browser/shell_browser_main_parts.cc (right):
> 
>
https://codereview.chromium.org/147993009/diff/1/apps/shell/browser/shell_bro...
> apps/shell/browser/shell_browser_main_parts.cc:145: if (devtools_delegate_)
> On 2014/02/11 03:24:36, Haojian Wu wrote:
> > On 2014/02/10 20:31:23, James Cook wrote:
> > > Why do you need the if()?  It looks like it should always exist.
> > 
> > Yes, in this code logic, it's always valid (it gets reset in
> > PreMainMessageLoopRun).
> > But I think it will be better to do this check although it's valid here 
since
> > the pointer initialized NULL when constructing ShellBrowserMainParts object.
> > 
> 
> I wouldn't do the if() because it confuses the reader -- it makes the reader
> think there is a code path where the pointer will be NULL. For example, in
> DestroyRootWindow() we don't check if wm_test_helper_ is NULL, even though it
> starts as NULL in the constructor. Likewise in CreateExtensionSystem() we
don't
> check if browser_context_ is NULL.

Done.

[commit-bot: I haz the power, 2014-02-12 14:13:09]

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

[commit-bot: I haz the power, 2014-02-12 14:13:10]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Haojian Wu, 2014-02-12 14:21:50]

+brettw@ for DEPS's dependency.

[brettw, 2014-02-13 19:00:24]

lgtm

[Haojian Wu, 2014-02-14 02:23:02]

The CQ bit was checked by hokein.wu@gmail.com

[commit-bot: I haz the power, 2014-02-14 02:24:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/147993009/140001

[commit-bot: I haz the power, 2014-02-14 05:27:03]

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

[commit-bot: I haz the power, 2014-02-14 05:27:03]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Haojian Wu, 2014-02-14 15:54:03]

The CQ bit was checked by hokein.wu@gmail.com

[commit-bot: I haz the power, 2014-02-14 15:54:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/147993009/140001

[commit-bot: I haz the power, 2014-02-14 16:49:16]

Change committed as 251351

[James Cook, 2014-02-19 20:39:44]

Haojian, I was just looking at this again. Does the content_shell.pak file need
to be loaded in all processes, or just the renderer, or just the browser?

We would get a faster startup time if we only loaded it in the processes that
need it.