Finish removing url_constants from content/.

Finish removing url_constants from content/.

BUG=76697
TEST=compiles

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86469

[Avi (use Gerrit), 2011-05-19 15:02:49]

Moved two files, just dropped constants in two others (felt right).

[Avi (use Gerrit), 2011-05-19 15:43:09]

Not yet. I need to start waiting for clean trybot runs.

[jam, 2011-05-19 17:21:16]

http://codereview.chromium.org/7049010/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_process_host_browsertest.cc (right):

http://codereview.chromium.org/7049010/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_process_host_browsertest.cc:1: // Copyright
(c) 2011 The Chromium Authors. All rights reserved.
i don't think we want to move the whole test file, since only one of the tests
is related to dev tools.  the others belong in the content layer, since we'll
want to test that stuff with the multi-proc test shell when it exists.

can you create render_process_host_browsertest.h and reuse the base test class
in a new file in chrome that has the devtools test?

http://codereview.chromium.org/7049010/diff/1/chrome/browser/tab_contents/ren...
File chrome/browser/tab_contents/render_view_host_manager_unittest.cc (right):

http://codereview.chromium.org/7049010/diff/1/chrome/browser/tab_contents/ren...
chrome/browser/tab_contents/render_view_host_manager_unittest.cc:1: // Copyright
(c) 2011 The Chromium Authors. All rights reserved.
it seems that almost all of this file belongs in content.  where we use chrome
urls, can we use made up urls instead and keep this in content?

[Avi (use Gerrit), 2011-05-19 22:28:41]

Try it now.

http://codereview.chromium.org/7049010/diff/1/chrome/browser/tab_contents/ren...
File chrome/browser/tab_contents/render_view_host_manager_unittest.cc (right):

http://codereview.chromium.org/7049010/diff/1/chrome/browser/tab_contents/ren...
chrome/browser/tab_contents/render_view_host_manager_unittest.cc:1: // Copyright
(c) 2011 The Chromium Authors. All rights reserved.
On 2011/05/19 17:21:16, John Abd-El-Malek wrote:
> it seems that almost all of this file belongs in content.  where we use chrome
> urls, can we use made up urls instead and keep this in content?

Done.

[jam, 2011-05-19 22:45:04]

sorry to drag this on.  but if i'm reading this correctly, these tests now
depend on the ChromeBrowserContentClient implementation of WebUIFactory.  i
worry that using string constants in the code will make it harder to switch them
in the future.  can we at least have string constants for these urls?  i.e.
kTempTestWebUI1, kTempTestWebUI2 etc

[Avi (use Gerrit), 2011-05-20 13:46:01]

On 2011/05/19 22:45:04, John Abd-El-Malek wrote:
> these tests now
> depend on the ChromeBrowserContentClient implementation of WebUIFactory.

Not sure what you mean. There's no change in dependency; one test was split in
two.

[jam, 2011-05-20 16:10:07]

I meant in content/browser/renderer_host/render_process_host_browsertest.cc,
these tests implicitly depend that navigating to "chrome://history/"
and "chrome://bookmarks/" will navigate to WebUI pages.  This only happens
with Chrome's ContentBrowserClient implementation, since its WebUIFactory
looks for these URLs.  If we replace the constants from url_constants.h with
hardcoded strings in the code, it makes it harder in the future when we want
to make this test work outside of ChromeContentBrowserClient (i.e. one will
have to search the test code for all the webui urls from chrome).

On Fri, May 20, 2011 at 6:46 AM, <avi@chromium.org> wrote:

> On 2011/05/19 22:45:04, John Abd-El-Malek wrote:
>
>> these tests now
>> depend on the ChromeBrowserContentClient implementation of WebUIFactory.
>>
>
> Not sure what you mean. There's no change in dependency; one test was split
> in
> two.
>
>
>
>
> http://codereview.chromium.org/7049010/
>

[Avi (use Gerrit), 2011-05-23 23:54:46]

Like this? (ptal)

[jam, 2011-05-24 01:25:47]

can you move all these chrome:// urls used in test to constants in one header
and a comment about what we need to do?  lgtm with that

http://codereview.chromium.org/7049010/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host_uitest.cc (right):

http://codereview.chromium.org/7049010/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host_uitest.cc:386: GURL
url("chrome://newtab");
nit: here too

http://codereview.chromium.org/7049010/diff/8001/content/browser/tab_contents...
File content/browser/tab_contents/render_view_host_manager_unittest.cc (right):

http://codereview.chromium.org/7049010/diff/8001/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager_unittest.cc:62: const GURL
kNtpUrl("chrome://newtab");
ditto and below

[Avi (use Gerrit), 2011-05-24 18:08:44]

Like this? PTAL