Get chrome:// dev tool urls hooked up in content_shell.

Get chrome:// dev tool urls hooked up in content_shell.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=133107

[michaeln, 2012-03-31 01:15:52]

noticed these didn't quite work yet in content_shell

[michaeln, 2012-03-31 01:30:06]

http://codereview.chromium.org/9950040/diff/1/content/browser/resource_contex...
File content/browser/resource_context_impl.cc (right):

http://codereview.chromium.org/9950040/diff/1/content/browser/resource_contex...
content/browser/resource_context_impl.cc:127: return protocol ==
chrome::kChromeUIScheme;
This is needed otherwise 'chrome' urls will be assumed to have external
handling, and the content layer will punt and leave it to the 'delegate' to
handle (prior to invoking interceptors and such).

http://codereview.chromium.org/9950040/diff/1/content/shell/shell_main_delega...
File content/shell/shell_main_delegate.cc (right):

http://codereview.chromium.org/9950040/diff/1/content/shell/shell_main_delega...
content/shell/shell_main_delegate.cc:46: content::RegisterContentSchemes(NULL);
This is needed so the 'chrome' scheme is parsed in the standard way that
understands <scheme>://<host>/path, otherwise GURL treats it as <scheme>:<path>
semantics.

[jam, 2012-04-02 16:19:25]

http://codereview.chromium.org/9950040/diff/1/content/app/content_main_runner.cc
File content/app/content_main_runner.cc (right):

http://codereview.chromium.org/9950040/diff/1/content/app/content_main_runner...
content/app/content_main_runner.cc:470: else
you're assuming that the presence of a delegate means they'll call
content::RegisterContentSchemes which might not always be true

the model we've been gradually switching to, after adding the Content API, is
that content is the one that owns and is responsible for startup. it calls the
embedder for them to optionally customize things. this allows more code sharing
with other embedders of content

so can you make content's RegisterContentSchemes call the embedder through
ContentClient::RegisterSchemes?

[michaeln, 2012-04-02 19:37:22]

https://chromiumcodereview.appspot.com/9950040/diff/1/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://chromiumcodereview.appspot.com/9950040/diff/1/content/app/content_mai...
content/app/content_main_runner.cc:470: else
> so can you make content's RegisterContentSchemes call the embedder through
> ContentClient::RegisterSchemes?

Sure, is this what you intend...

* add an unconditional call to content::RegisterContentSchemes() here
* have that function call the 'clients' method
* remove the calls to content::RegisterSchemes() from the existing
PreSandboxStartup impls

[jam, 2012-04-02 20:23:13]

lgtm

https://chromiumcodereview.appspot.com/9950040/diff/1/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://chromiumcodereview.appspot.com/9950040/diff/1/content/app/content_mai...
content/app/content_main_runner.cc:470: else
On 2012/04/02 19:37:22, michaeln wrote:
> > so can you make content's RegisterContentSchemes call the embedder through
> > ContentClient::RegisterSchemes?
> 
> Sure, is this what you intend...
> 
> * add an unconditional call to content::RegisterContentSchemes() here
> * have that function call the 'clients' method
> * remove the calls to content::RegisterSchemes() from the existing
> PreSandboxStartup impls

yep

[jam, 2012-04-02 20:23:23]

clicked the lgtm button by accident :)

[michaeln, 2012-04-02 21:37:48]

k... i'll ping you after poking at the CL some more

[michaeln, 2012-04-16 21:24:39]

I still have to see what the trybots think about this, but here's a stab at
it... wdyt?

[michaeln, 2012-04-17 01:14:24]

ping

[michaeln, 2012-04-17 01:59:05]

There are more content_unittests out there that call url_util::AddStandardScheme
after the TestSuite initialization has run and locked it down.

I'll look into an initialization sequence for unittests that doesn't result in a
call to LockStandardSchemes() instead of poking at these tests individually.

[michaeln, 2012-04-17 02:45:07]

Done

> I'll look into an initialization sequence for unittests that doesn't result in
a
> call to LockStandardSchemes() instead of poking at these tests individually.

[jam, 2012-04-17 03:30:10]

http://codereview.chromium.org/9950040/diff/8026/chrome/app/chrome_main_deleg...
File chrome/app/chrome_main_delegate.cc (right):

http://codereview.chromium.org/9950040/diff/8026/chrome/app/chrome_main_deleg...
chrome/app/chrome_main_delegate.cc:458: // processes that work with these URLs
(i.e. including renderers).
nit: this comment is describing what the code does and not something, which
someone reading the code can figure out. so it shouldn't be needed, per the
style guide about comments

http://codereview.chromium.org/9950040/diff/8026/chrome/test/base/chrome_test...
File chrome/test/base/chrome_test_suite.cc (right):

http://codereview.chromium.org/9950040/diff/8026/chrome/test/base/chrome_test...
chrome/test/base/chrome_test_suite.cc:172:
content::RegisterContentSchemes(kDontLockStandardSchemes);
nit: i'm personally not a fan of adding variables to describe a bool; i know
this is common/expected in webkit but this isn't a convention in chrome. i think
if someone really cares about an argument, they can look at the header

http://codereview.chromium.org/9950040/diff/8026/content/public/common/url_co...
File content/public/common/url_constants.cc (right):

http://codereview.chromium.org/9950040/diff/8026/content/public/common/url_co...
content/public/common/url_constants.cc:90: if (GetContentClient()) {
we don't usually null check GetContentClient by design. is this for tests?

[michaeln, 2012-04-17 04:08:26]

thnx for looking

http://codereview.chromium.org/9950040/diff/8026/chrome/app/chrome_main_deleg...
File chrome/app/chrome_main_delegate.cc (right):

http://codereview.chromium.org/9950040/diff/8026/chrome/app/chrome_main_deleg...
chrome/app/chrome_main_delegate.cc:458: // processes that work with these URLs
(i.e. including renderers).
On 2012/04/17 03:30:10, John Abd-El-Malek wrote:
> nit: this comment is describing what the code does and not something, which
> someone reading the code can figure out. so it shouldn't be needed, per the
> style guide about comments

How about i leave it at...
// Initialize the content client very early to allow chrome's custom url
// schemes to be properly registered.

http://codereview.chromium.org/9950040/diff/8026/chrome/test/base/chrome_test...
File chrome/test/base/chrome_test_suite.cc (right):

http://codereview.chromium.org/9950040/diff/8026/chrome/test/base/chrome_test...
chrome/test/base/chrome_test_suite.cc:172:
content::RegisterContentSchemes(kDontLockStandardSchemes);
On 2012/04/17 03:30:10, John Abd-El-Malek wrote:
> nit: i'm personally not a fan of adding variables to describe a bool; i know
> this is common/expected in webkit but this isn't a convention in chrome. i
think
> if someone really cares about an argument, they can look at the header

I've warmed up to this self-documenting style, but i can remove if you prefer.

http://codereview.chromium.org/9950040/diff/8026/content/public/common/url_co...
File content/public/common/url_constants.cc (right):

http://codereview.chromium.org/9950040/diff/8026/content/public/common/url_co...
content/public/common/url_constants.cc:90: if (GetContentClient()) {
On 2012/04/17 03:30:10, John Abd-El-Malek wrote:
> we don't usually null check GetContentClient by design. is this for tests?

I noticed that. Yes, this is for content_unittest initialization see comments
below.

http://codereview.chromium.org/9950040/diff/8026/content/test/content_test_su...
File content/test/content_test_suite.cc (right):

http://codereview.chromium.org/9950040/diff/8026/content/test/content_test_su...
content/test/content_test_suite.cc:76:
content::RegisterContentSchemes(kDontLockStandardSchemes);
This test suite gets initialized prior SetContentClient having been called. I
could modify it to set a client prior to the register call as
ChromeTestSuite::Initialize() does. That's necessary in the chrome case because
it actually adds custom schemes. In this initializer, no custom schemes get
added.

[michaeln, 2012-04-17 04:11:45]

http://codereview.chromium.org/9950040/diff/8026/chrome/test/base/chrome_test...
File chrome/test/base/chrome_test_suite.cc (right):

http://codereview.chromium.org/9950040/diff/8026/chrome/test/base/chrome_test...
chrome/test/base/chrome_test_suite.cc:172:
content::RegisterContentSchemes(kDontLockStandardSchemes);
On 2012/04/17 04:08:26, michaeln wrote:
> On 2012/04/17 03:30:10, John Abd-El-Malek wrote:
> > nit: i'm personally not a fan of adding variables to describe a bool; i know
> > this is common/expected in webkit but this isn't a convention in chrome. i
> think
> > if someone really cares about an argument, they can look at the header
> 
> I've warmed up to this self-documenting style, but i can remove if you prefer.

Actually, the convention in webkit would be to make this an enum with two
possible values, i'm not so much a fan of that :)

[jam, 2012-04-17 04:49:51]

http://codereview.chromium.org/9950040/diff/8026/chrome/app/chrome_main_deleg...
File chrome/app/chrome_main_delegate.cc (right):

http://codereview.chromium.org/9950040/diff/8026/chrome/app/chrome_main_deleg...
chrome/app/chrome_main_delegate.cc:458: // processes that work with these URLs
(i.e. including renderers).
On 2012/04/17 04:08:26, michaeln wrote:
> On 2012/04/17 03:30:10, John Abd-El-Malek wrote:
> > nit: this comment is describing what the code does and not something, which
> > someone reading the code can figure out. so it shouldn't be needed, per the
> > style guide about comments
> 
> How about i leave it at...
> // Initialize the content client very early to allow chrome's custom url
> // schemes to be properly registered.
> 

I think that's still documenting why something is done. this can change in the
future (i.e. someone adds another ContentClient::Get() call earlier). if someone
really wants to try to move it, they'll figure out why it's in that place..

http://codereview.chromium.org/9950040/diff/8026/chrome/test/base/chrome_test...
File chrome/test/base/chrome_test_suite.cc (right):

http://codereview.chromium.org/9950040/diff/8026/chrome/test/base/chrome_test...
chrome/test/base/chrome_test_suite.cc:172:
content::RegisterContentSchemes(kDontLockStandardSchemes);
On 2012/04/17 04:08:26, michaeln wrote:
> On 2012/04/17 03:30:10, John Abd-El-Malek wrote:
> > nit: i'm personally not a fan of adding variables to describe a bool; i know
> > this is common/expected in webkit but this isn't a convention in chrome. i
> think
> > if someone really cares about an argument, they can look at the header
> 
> I've warmed up to this self-documenting style, but i can remove if you prefer.

my preference would  be to remove it :)

http://codereview.chromium.org/9950040/diff/8026/content/test/content_test_su...
File content/test/content_test_suite.cc (right):

http://codereview.chromium.org/9950040/diff/8026/content/test/content_test_su...
content/test/content_test_suite.cc:76:
content::RegisterContentSchemes(kDontLockStandardSchemes);
On 2012/04/17 04:08:26, michaeln wrote:
> This test suite gets initialized prior SetContentClient having been called. I
> could modify it to set a client prior to the register call as
> ChromeTestSuite::Initialize() does. That's necessary in the chrome case
because
> it actually adds custom schemes. In this initializer, no custom schemes get
> added.

yeah i think it's better to add a few more lines to the test suite code, instead
of starting to null check in the content code which we've managed to avoid so
far. i'm just worried about people starting to null check it and writing code to
handle a case that won't happen

[michaeln, 2012-04-17 19:23:32]

http://codereview.chromium.org/9950040/diff/8026/chrome/app/chrome_main_deleg...
File chrome/app/chrome_main_delegate.cc (right):

http://codereview.chromium.org/9950040/diff/8026/chrome/app/chrome_main_deleg...
chrome/app/chrome_main_delegate.cc:458: // processes that work with these URLs
(i.e. including renderers).
On 2012/04/17 04:49:51, John Abd-El-Malek wrote:
> On 2012/04/17 04:08:26, michaeln wrote:
> > On 2012/04/17 03:30:10, John Abd-El-Malek wrote:
> > > nit: this comment is describing what the code does and not something,
which
> > > someone reading the code can figure out. so it shouldn't be needed, per
the
> > > style guide about comments
> > 
> > How about i leave it at...
> > // Initialize the content client very early to allow chrome's custom url
> > // schemes to be properly registered.
> > 
> 
> I think that's still documenting why something is done. this can change in the
> future (i.e. someone adds another ContentClient::Get() call earlier). if
someone
> really wants to try to move it, they'll figure out why it's in that place..

Ok, removed these retained comments. Fyi, there are others of this form
throughout this file.

http://codereview.chromium.org/9950040/diff/8026/chrome/test/base/chrome_test...
File chrome/test/base/chrome_test_suite.cc (right):

http://codereview.chromium.org/9950040/diff/8026/chrome/test/base/chrome_test...
chrome/test/base/chrome_test_suite.cc:172:
content::RegisterContentSchemes(kDontLockStandardSchemes);
On 2012/04/17 04:49:51, John Abd-El-Malek wrote:
> On 2012/04/17 04:08:26, michaeln wrote:
> > On 2012/04/17 03:30:10, John Abd-El-Malek wrote:
> > > nit: i'm personally not a fan of adding variables to describe a bool; i
know
> > > this is common/expected in webkit but this isn't a convention in chrome. i
> > think
> > > if someone really cares about an argument, they can look at the header
> > 
> > I've warmed up to this self-documenting style, but i can remove if you
prefer.
> 
> my preference would  be to remove it :)

Done.

http://codereview.chromium.org/9950040/diff/8026/content/test/content_test_su...
File content/test/content_test_suite.cc (right):

http://codereview.chromium.org/9950040/diff/8026/content/test/content_test_su...
content/test/content_test_suite.cc:76:
content::RegisterContentSchemes(kDontLockStandardSchemes);
On 2012/04/17 04:49:51, John Abd-El-Malek wrote:
> On 2012/04/17 04:08:26, michaeln wrote:
> > This test suite gets initialized prior SetContentClient having been called.
I
> > could modify it to set a client prior to the register call as
> > ChromeTestSuite::Initialize() does. That's necessary in the chrome case
> because
> > it actually adds custom schemes. In this initializer, no custom schemes get
> > added.
> 
> yeah i think it's better to add a few more lines to the test suite code,
instead
> of starting to null check in the content code which we've managed to avoid so
> far. i'm just worried about people starting to null check it and writing code
to
> handle a case that won't happen

Done.

http://codereview.chromium.org/9950040/diff/14001/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/9950040/diff/14001/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:351: void
ChromeContentClient::AddAdditionalSchemes(
Also hoisted this method body up from now deleted helper function in
url_constants.

[jam, 2012-04-17 23:18:57]

lgtm

[commit-bot: I haz the power, 2012-04-19 19:50:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/9950040/22001

[commit-bot: I haz the power, 2012-04-19 21:08:04]

Try job failure for 9950040-22001 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...