Open Options page on top of NTP.

Open Options page on top of NTP.


R=jhawkins@chromium.org
BUG=22951
TEST=


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

[Greg Billock, 2011-06-16 00:01:05]

On 2011/06/15 23:40:16, gbillock1 wrote:

I'm seeing a

LEAK: 4 WebCoreNode
LEAK: 21 CachedResource

message when the options page replaces the NTP. Is there page destruction logic
needed that browser::Navigate isn't doing? I see browser.cc:686 is doing
something other than Navigate() for CURRENT_TAB. Are there expectations of that
function such that it doesn't handle contents-replacement with CURRENT_TAB
correctly? Should lines 686-700 be refactored there?

[James Hawkins, 2011-06-16 04:04:05]

On 2011/06/16 00:01:05, gbillock1 wrote:
> On 2011/06/15 23:40:16, gbillock1 wrote:
> 
> I'm seeing a
> 
> LEAK: 4 WebCoreNode
> LEAK: 21 CachedResource
> 
> message when the options page replaces the NTP. Is there page destruction
logic
> needed that browser::Navigate isn't doing? I see browser.cc:686 is doing
> something other than Navigate() for CURRENT_TAB. Are there expectations of
that
> function such that it doesn't handle contents-replacement with CURRENT_TAB
> correctly? Should lines 686-700 be refactored there?

Those are common/harmless, afaik.

[James Hawkins, 2011-06-16 04:06:36]

This needs tests.

[Greg Billock, 2011-06-16 23:52:48]

On 2011/06/16 04:06:36, James Hawkins wrote:
> This needs tests.

Yes. Now added, except the last three are timing out. They look basically the
same as the others to me. Can you see what's wrong?

I see this warning in the first failure:
[32727:32727:1402879834645:WARNING:render_view.cc(1402)] didStartLoading called
while loading

so I took out a WaitForNavigationInCurrentTab call on them to see if that was
it, but they still don't work. I'm suspicious of the AddSelectedTabWithURL, but
a ton of other tests in this file use it in what looks like the same way, and
are fine.

[James Hawkins, 2011-06-17 17:44:57]

I unfortunately don't have any ideas about the tests. Pawel might.

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1993: GURL ntp(chrome::kChromeUINewTabURL);
You only use |ntp| once, so just use the const var.

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1994: GURL blank(chrome::kAboutBlankURL);
Same for blank.

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:2009: void Browser::OpenOptionsDialog() {
Wha?? Why do we have two methods for opening Options? This needs a refactoring.

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser_na...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser_na...
chrome/browser/ui/browser_navigator.cc:86: }  // anonymous namespace
s/anonymous namespace/namespace/

Technically it's an unnamed namespace, but as far as the style guide is
concerned, the closing comment should match the opening declaration, i.e.,
namespace.

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser_na...
chrome/browser/ui/browser_navigator.cc:136: namespace {
Why do we have two unnamed namespaces?

[Greg Billock, 2011-06-17 21:16:07]

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1993: GURL ntp(chrome::kChromeUINewTabURL);
On 2011/06/17 17:44:57, James Hawkins wrote:
> You only use |ntp| once, so just use the const var.

Done.

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1994: GURL blank(chrome::kAboutBlankURL);
On 2011/06/17 17:44:57, James Hawkins wrote:
> Same for blank.

Done.

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:2009: void Browser::OpenOptionsDialog() {
On 2011/06/17 17:44:57, James Hawkins wrote:
> Wha?? Why do we have two methods for opening Options? This needs a
refactoring.

I can do that. I wasn't sure what all went in to the two command environments.
It sure seems like they could be refactored together.

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser_na...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser_na...
chrome/browser/ui/browser_navigator.cc:86: }  // anonymous namespace
On 2011/06/17 17:44:57, James Hawkins wrote:
> s/anonymous namespace/namespace/
> 
> Technically it's an unnamed namespace, but as far as the style guide is
> concerned, the closing comment should match the opening declaration, i.e.,
> namespace.

Done.

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser_na...
chrome/browser/ui/browser_navigator.cc:136: namespace {
On 2011/06/17 17:44:57, James Hawkins wrote:
> Why do we have two unnamed namespaces?
Just so the diffs are easier to follow. I plan to move GetIndexOfSingletonTab
down next to Navigate.

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser_na...
File chrome/browser/ui/browser_navigator_browsertest.cc (right):

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser_na...
chrome/browser/ui/browser_navigator_browsertest.cc:945:
IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest,
I moved this to the end, and now they all pass with no timeouts! :-/

[James Hawkins, 2011-06-17 21:20:32]

LGTM, thanks!

[commit-bot: I haz the power, 2011-06-17 22:46:39]

Change committed as 89566

[Paweł Hajdan Jr., 2011-06-18 10:38:51]

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser_na...
File chrome/browser/ui/browser_navigator_browsertest.cc (right):

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser_na...
chrome/browser/ui/browser_navigator_browsertest.cc:894:
EXPECT_EQ(GURL("chrome://settings/browser"),
nit: Please use chrome/common/url_constants for all of those chrome:// URLs.

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser_na...
chrome/browser/ui/browser_navigator_browsertest.cc:902: browser::Navigate(&p);
Why not just NavigateToURL?

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser_na...
chrome/browser/ui/browser_navigator_browsertest.cc:933:
ui_test_utils::WaitForNavigationInCurrentTab(browser());
Those WaitForNavigation... calls are all racy. I think this should all use a
WindowedNotificationObserver, to register for notifications _before_ triggering
the navigation. I think that to fix it you'd need to change
NavigationNotificationObserver in ui_test_utils a bit. Please let me know if
you'd need more details.

http://codereview.chromium.org/7178002/diff/8001/chrome/browser/ui/browser_na...
chrome/browser/ui/browser_navigator_browsertest.cc:948:
ui_test_utils::WaitForNavigationInCurrentTab(browser());
Please check return value of this.