Don't show "Inspect Element" in the context menu if we can't inspect.

Don't show "Inspect Element" in the context menu if we can't inspect.

Ubuntu want to ship with the inspector files in a separate package and having menu items which are broken isn't nice.

[Evan Martin, 2009-08-20 21:07:28]

http://codereview.chromium.org/174162/diff/1001/1002
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/174162/diff/1001/1002#newcode787
Line 787: browser_process->CheckForInspectorFiles();
Will this regress startup?  (More disk contention.)

http://codereview.chromium.org/174162/diff/1001/1003
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/174162/diff/1001/1003#newcode146
Line 146: // call this function before we have a definate answer from the disk.
In that
spelling: definite

http://codereview.chromium.org/174162/diff/1001/1004
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/174162/diff/1001/1004#newcode448
Line 448: if (PathService::Get(chrome::DIR_INSPECTOR, &inspector_dir)) {
Is PathService thread-safe?  never really thought about it.

http://codereview.chromium.org/174162/diff/1001/1005
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/174162/diff/1001/1005#newcode285
Line 285: void DoInspectorFilesCheck();
comment on this and variable below

http://codereview.chromium.org/174162/diff/1001/1006
File chrome/browser/gtk/standard_menus.cc (right):

http://codereview.chromium.org/174162/diff/1001/1006#newcode64
Line 64: // The encoding menu submenu is filled in by code below.
s/encoding/developer/

[agl, 2009-08-20 21:15:38]

http://codereview.chromium.org/174162/diff/1001/1002
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/174162/diff/1001/1002#newcode787
Line 787: browser_process->CheckForInspectorFiles();
On 2009/08/20 21:07:28, Evan Martin wrote:
> Will this regress startup?  (More disk contention.)

Hopefully a single stat will be too minor to bother with. I could disable it for
non-Windows or put a delay in there. The perf bots will know if such added
complexity is needed.

http://codereview.chromium.org/174162/diff/1001/1003
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/174162/diff/1001/1003#newcode146
Line 146: // call this function before we have a definate answer from the disk.
In that
On 2009/08/20 21:07:28, Evan Martin wrote:
> spelling: definite

Done.

http://codereview.chromium.org/174162/diff/1001/1004
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/174162/diff/1001/1004#newcode448
Line 448: if (PathService::Get(chrome::DIR_INSPECTOR, &inspector_dir)) {
On 2009/08/20 21:07:28, Evan Martin wrote:
> Is PathService thread-safe?  never really thought about it.

All the code looks ok.

http://codereview.chromium.org/174162/diff/1001/1005
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/174162/diff/1001/1005#newcode285
Line 285: void DoInspectorFilesCheck();
On 2009/08/20 21:07:28, Evan Martin wrote:
> comment on this and variable below

Done.

[Evan Martin, 2009-08-20 21:27:32]

ship it

[Evan Martin, 2009-08-20 21:28:14]

(Note that I wonder when the menu creation happens -- is it when the browser
starts or is it when you first click the button?  But if you think this code
works, then LGTM)

[tony, 2009-08-20 21:41:17]

On 2009/08/20 21:28:14, Evan Martin wrote:
> (Note that I wonder when the menu creation happens -- is it when the browser
> starts or is it when you first click the button?  But if you think this code
> works, then LGTM)

Right, if it's when you first bring up the page menu/context menu, maybe we
should do the check before we show the menu?  It would avoid the extra disk
activity during startup on the file thread, but maybe there's not as much
contention on that thread during startup as I think there is.

[tony, 2009-08-20 21:42:04]

Which is to say, if the startup tests don't regress, seems fine to do it this
way too.

[pfeldman, 2009-11-12 10:32:21]

Do we know why Ubuntu wants it this way? Here is why I think this should be
reverted:

- Extensions will still invoke devtools using DevTools API and things 
would break. Disabling this and not asking for a review (adding a CC) from
devtools team was not a smart move.
- We want to make Chromium a development platform and that is why we ship
devtools with all platforms by default. We actually expect to see most of our
clients using GTK.
- This thing forks behavior platforms (otherwise please fix windows and mac
menus to make it consistent?)
- Introduces a startup lag

So if I was reviewing it, it won't be not LGTM at all.

[Evan Martin, 2009-11-12 16:55:22]

fta: see comment by pfeldman