Added support for passing WindowOpenDisposition into BrowserInstantController::OpenURL() from the o…

Added support for passing WindowOpenDisposition into BrowserInstantController::OpenURL() from the omnibox in GWS.

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

[samarth, 2013-01-10 21:42:04]

On second thought, how about we just define an "enum" in the javascript for the
allowed values for disposition in the API? You can't share it in C++ easily so
you still need to check manually there, but at least the offline implementation
and any server implementation can easily just use the constants.

https://codereview.chromium.org/11644009/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_tab.cc (left):

https://codereview.chromium.org/11644009/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_tab.cc:6: 
nit: leave this line here.

https://codereview.chromium.org/11644009/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://codereview.chromium.org/11644009/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:13: var
WindowOpenDisposition_ = {
How come this doesn't match the other enum?

https://codereview.chromium.org/11644009/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:197:
WindowOpenDisposition_.CURRENT_TAB);
Middle clicking should work correctly here as well.

https://codereview.chromium.org/11644009/diff/1/chrome/renderer/searchbox/sea...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/11644009/diff/1/chrome/renderer/searchbox/sea...
chrome/renderer/searchbox/searchbox_extension.cc:704: if (args[1]->Uint32Value()
== NEW_BACKGROUND_TAB) {
Is disposition supposed to be optional?  Do you need to also check for
IsUint32()?

https://codereview.chromium.org/11644009/diff/1/chrome/renderer/searchbox/sea...
chrome/renderer/searchbox/searchbox_extension.cc:704: if (args[1]->Uint32Value()
== NEW_BACKGROUND_TAB) {
Don't use NEW_BACKGROUND_TAB here. We don't want the api changing just in case
the enum changes from underneath us.

How about something like:
disposition = CURRENT_TAB;
if (int32value() == 1)  // or whatever it is
  disposition = NEW_BACKGROUND_TAB;
NavigateToURL(..., disposition);

[dougw, 2013-01-18 23:54:30]

https://codereview.chromium.org/11644009/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_tab.cc (left):

https://codereview.chromium.org/11644009/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_tab.cc:6: 
On 2013/01/10 21:42:04, samarth wrote:
> nit: leave this line here.

Done.

https://codereview.chromium.org/11644009/diff/1/chrome/browser/resources/loca...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://codereview.chromium.org/11644009/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:13: var
WindowOpenDisposition_ = {
On 2013/01/10 21:42:04, samarth wrote:
> How come this doesn't match the other enum?

Done.

https://codereview.chromium.org/11644009/diff/1/chrome/browser/resources/loca...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:197:
WindowOpenDisposition_.CURRENT_TAB);
On 2013/01/10 21:42:04, samarth wrote:
> Middle clicking should work correctly here as well.

I'd like to do this in a separate cl :)

https://codereview.chromium.org/11644009/diff/1/chrome/renderer/searchbox/sea...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/11644009/diff/1/chrome/renderer/searchbox/sea...
chrome/renderer/searchbox/searchbox_extension.cc:704: if (args[1]->Uint32Value()
== NEW_BACKGROUND_TAB) {
On 2013/01/10 21:42:04, samarth wrote:
> Is disposition supposed to be optional?  Do you need to also check for
> IsUint32()?

Done.

https://codereview.chromium.org/11644009/diff/1/chrome/renderer/searchbox/sea...
chrome/renderer/searchbox/searchbox_extension.cc:704: if (args[1]->Uint32Value()
== NEW_BACKGROUND_TAB) {
On 2013/01/10 21:42:04, samarth wrote:
> Don't use NEW_BACKGROUND_TAB here. We don't want the api changing just in case
> the enum changes from underneath us.
> 
> How about something like:
> disposition = CURRENT_TAB;
> if (int32value() == 1)  // or whatever it is
>   disposition = NEW_BACKGROUND_TAB;
> NavigateToURL(..., disposition);

Done.

[sreeram, 2013-01-22 06:01:19]

Just a couple of nits. Fix them, and this is ready to go.

https://chromiumcodereview.appspot.com/11644009/diff/21004/chrome/browser/ins...
File chrome/browser/instant/instant_loader.cc (right):

https://chromiumcodereview.appspot.com/11644009/diff/21004/chrome/browser/ins...
chrome/browser/instant/instant_loader.cc:23: #include
"webkit/glue/window_open_disposition.h"
You don't need this include (it's already being included by way of
instant_client.h).

https://chromiumcodereview.appspot.com/11644009/diff/21004/chrome/browser/ins...
File chrome/browser/instant/instant_tab.h (right):

https://chromiumcodereview.appspot.com/11644009/diff/21004/chrome/browser/ins...
chrome/browser/instant/instant_tab.h:14: #include
"webkit/glue/window_open_disposition.h"
Same here. No need for this include.

https://chromiumcodereview.appspot.com/11644009/diff/21004/chrome/browser/res...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://chromiumcodereview.appspot.com/11644009/diff/21004/chrome/browser/res...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:19: // 
Component dependencies.
Huh? That's a strange name. Also, why define this section if it's not being
used? Surely you can do so when you actually add support for the middle mouse
button?

https://chromiumcodereview.appspot.com/11644009/diff/21004/chrome/renderer/se...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://chromiumcodereview.appspot.com/11644009/diff/21004/chrome/renderer/se...
chrome/renderer/searchbox/searchbox_extension.cc:704: if ((args[1]->IsUint32())
&& (args[1]->Uint32Value() == 2))
You don't need the first clause. Just this:
    if (args[1]->Uint32Value() == 2)

Reason is, args[1] is always present, given that searchbox_api.js line 232
unconditionally passes 2 params. It could be null, of course. In which case,
Uint32Value() will be zero, and not pass the 'if' check.

[samarth, 2013-01-22 16:13:32]

lgtm

How about a simple browsertest?

Thanks,
Samarth

https://codereview.chromium.org/11644009/diff/21004/chrome/browser/resources/...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://codereview.chromium.org/11644009/diff/21004/chrome/browser/resources/...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:19: // 
Component dependencies.
On 2013/01/22 06:01:19, sreeram wrote:
> Huh? That's a strange name. Also, why define this section if it's not being
> used? Surely you can do so when you actually add support for the middle mouse
> button?

Agreed. As you have it right now, you could just leave off the disposition
argument completely and it'll continue opening in the current tab. If you want
to add support for the middle mouse button here, that's fine as well.

[sreeram, 2013-01-22 22:34:14]

On Tue, Jan 22, 2013 at 2:22 PM, Douglas Wightman <dougw@google.com> wrote:
> Ok, for the browser test, I'm thinking of middle clicking on the top result
> and confirming that it opens in a new tab.
>
> I found code to test opening in a new tab (the ALT-ENTER case,
> instant_browsertest.cc), and I found some code in interactive_test_utils.h
> for moving and clicking with the mouse.
>
> Not certain how to determine the location of the top omnibox suggestion -
> any ideas?

You can fake it. In the test, inject some script (ExecuteScript) that adds an
onclick handler to the whole page (<body> element), that pretends the first
suggestion was clicked (thus, calls navigatecontentwindow(rid=0) or whatever).
Then have the test just click anywhere on the page.

[dougw, 2013-01-24 03:05:25]

https://codereview.chromium.org/11644009/diff/21004/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11644009/diff/21004/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:23: #include
"webkit/glue/window_open_disposition.h"
On 2013/01/22 06:01:19, sreeram wrote:
> You don't need this include (it's already being included by way of
> instant_client.h).

Done.

https://codereview.chromium.org/11644009/diff/21004/chrome/browser/instant/in...
File chrome/browser/instant/instant_tab.h (right):

https://codereview.chromium.org/11644009/diff/21004/chrome/browser/instant/in...
chrome/browser/instant/instant_tab.h:14: #include
"webkit/glue/window_open_disposition.h"
On 2013/01/22 06:01:19, sreeram wrote:
> Same here. No need for this include.

Done.

https://codereview.chromium.org/11644009/diff/21004/chrome/browser/resources/...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://codereview.chromium.org/11644009/diff/21004/chrome/browser/resources/...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:19: // 
Component dependencies.
On 2013/01/22 06:01:19, sreeram wrote:
> Huh? That's a strange name. Also, why define this section if it's not being
> used? Surely you can do so when you actually add support for the middle mouse
> button?

Done.

https://codereview.chromium.org/11644009/diff/21004/chrome/browser/resources/...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:19: // 
Component dependencies.
On 2013/01/22 16:13:32, samarth wrote:
> On 2013/01/22 06:01:19, sreeram wrote:
> > Huh? That's a strange name. Also, why define this section if it's not being
> > used? Surely you can do so when you actually add support for the middle
mouse
> > button?
> 
> Agreed. As you have it right now, you could just leave off the disposition
> argument completely and it'll continue opening in the current tab. If you want
> to add support for the middle mouse button here, that's fine as well.

Done.

https://codereview.chromium.org/11644009/diff/21004/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/11644009/diff/21004/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:704: if ((args[1]->IsUint32())
&& (args[1]->Uint32Value() == 2))
On 2013/01/22 06:01:19, sreeram wrote:
> You don't need the first clause. Just this:
>     if (args[1]->Uint32Value() == 2)
> 
> Reason is, args[1] is always present, given that searchbox_api.js line 232
> unconditionally passes 2 params. It could be null, of course. In which case,
> Uint32Value() will be zero, and not pass the 'if' check.

Done.

https://codereview.chromium.org/11644009/diff/43002/chrome/browser/instant/in...
File chrome/browser/instant/instant_browsertest.cc (right):

https://codereview.chromium.org/11644009/diff/43002/chrome/browser/instant/in...
chrome/browser/instant/instant_browsertest.cc:990: EXPECT_EQ(2,
browser()->tab_strip_model()->count());
Any idea why this is failing? 

I've started debugging this test in gdb. I'm hoping there's a better approach
(or that I'm doing something obviously wrong :).

[sreeram, 2013-01-24 03:50:08]

https://codereview.chromium.org/11644009/diff/43002/chrome/browser/instant/in...
File chrome/browser/instant/instant_browsertest.cc (right):

https://codereview.chromium.org/11644009/diff/43002/chrome/browser/instant/in...
chrome/browser/instant/instant_browsertest.cc:990: EXPECT_EQ(2,
browser()->tab_strip_model()->count());
On 2013/01/24 03:05:25, dougw wrote:
> Any idea why this is failing? 

Sure :) It's because these tests (InstantTest.*) are the old-style Instant. To
use NavigateToURL (and test the middle click and such), you need to write an
InstantExtendedTest, in instant_extended_browsertest.cc.

[dougw, 2013-01-24 18:58:46]

All inline comments addressed and browsertest added.

[sreeram, 2013-01-24 19:13:48]

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:86:
IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
MiddleClickOnSuggestionOpensInNewTab) {
80 column limit

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:90: GURL
url(std::string("http://google.com"));
You should assert here, before the navigation, that there's just 1 tab.

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:91:
instant()->NavigateToURL(url, content::PAGE_TRANSITION_TYPED,
NEW_BACKGROUND_TAB);
80 column limit

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:91:
instant()->NavigateToURL(url, content::PAGE_TRANSITION_TYPED,
NEW_BACKGROUND_TAB);
I think you should actually click on the page so that we can test that the API
and IPC are properly exercised.

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:96: EXPECT_EQ(2,
browser()->tab_strip_model()->count());
You're not EXPECT()ing all the things the comment is talking about.

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:24: 
Delete this extra blank line.

https://codereview.chromium.org/11644009/diff/31019/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/11644009/diff/31019/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:705: disposition =
NEW_BACKGROUND_TAB;
Indent only by 2 spaces from the previous line.

[dougw, 2013-02-06 21:42:11]

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:86:
IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
MiddleClickOnSuggestionOpensInNewTab) {
On 2013/01/24 19:13:48, sreeram wrote:
> 80 column limit

Done.

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:90: GURL
url(std::string("http://google.com"));
On 2013/01/24 19:13:48, sreeram wrote:
> You should assert here, before the navigation, that there's just 1 tab.

Done.

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:91:
instant()->NavigateToURL(url, content::PAGE_TRANSITION_TYPED,
NEW_BACKGROUND_TAB);
On 2013/01/24 19:13:48, sreeram wrote:
> 80 column limit

Done.

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:96: EXPECT_EQ(2,
browser()->tab_strip_model()->count());
On 2013/01/24 19:13:48, sreeram wrote:
> You're not EXPECT()ing all the things the comment is talking about.

Done.

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:24: 
On 2013/01/24 19:13:48, sreeram wrote:
> Delete this extra blank line.

Done.

https://codereview.chromium.org/11644009/diff/31019/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/11644009/diff/31019/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:705: disposition =
NEW_BACKGROUND_TAB;
On 2013/01/24 19:13:48, sreeram wrote:
> Indent only by 2 spaces from the previous line.

Done.

[sreeram, 2013-02-06 22:00:50]

https://codereview.chromium.org/11644009/diff/61001/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/11644009/diff/61001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:91:
ASSERT_TRUE(browser()->tab_strip_model()->count() == 1);
I'd just make this an EXPECT. Generally we use ASSERT when continuing with the
rest of the test will produce cascading crashes, say due to a null pointer or
such. If it's just a matter of initial assumptions/expectations being wrong, no
harm proceeding with the test. So:
EXPECT_EQ(1, ...);

https://codereview.chromium.org/11644009/diff/61001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:105:
"chrome.searchBox.navigateContentWindow(nativeSuggestions[0].rid, 2);"
I recommend that you navigate to a known URL, such as "chrome://version". So you
can then check that the newly added tab's URL is indeed that (and that we didn't
just open a tab and navigate to a random URL).

https://codereview.chromium.org/11644009/diff/61001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:108: "Click();"));
Why not actually click on the page using ui_test_utils::ClickOnView(browser(),
VIEW_ID_TAB_CONTAINER)? The reason why I insist on actually clicking on the page
is that it has a bunch of ripple effects - omnibox losing focus,
InstantController trying to commit the preview, etc. I'd like to ensure that the
navigation reliably succeeds amidst all that.

[dougw, 2013-02-20 21:46:56]

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:91:
instant()->NavigateToURL(url, content::PAGE_TRANSITION_TYPED,
NEW_BACKGROUND_TAB);
On 2013/01/24 19:13:48, sreeram wrote:
> I think you should actually click on the page so that we can test that the API
> and IPC are properly exercised.

Done.

https://codereview.chromium.org/11644009/diff/31019/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:91:
instant()->NavigateToURL(url, content::PAGE_TRANSITION_TYPED,
NEW_BACKGROUND_TAB);
On 2013/01/24 19:13:48, sreeram wrote:
> 80 column limit

Done.

https://codereview.chromium.org/11644009/diff/61001/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/11644009/diff/61001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:91:
ASSERT_TRUE(browser()->tab_strip_model()->count() == 1);
On 2013/02/06 22:00:50, sreeram wrote:
> I'd just make this an EXPECT. Generally we use ASSERT when continuing with the
> rest of the test will produce cascading crashes, say due to a null pointer or
> such. If it's just a matter of initial assumptions/expectations being wrong,
no
> harm proceeding with the test. So:
> EXPECT_EQ(1, ...);

Done.

https://codereview.chromium.org/11644009/diff/61001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:105:
"chrome.searchBox.navigateContentWindow(nativeSuggestions[0].rid, 2);"
On 2013/02/06 22:00:50, sreeram wrote:
> I recommend that you navigate to a known URL, such as "chrome://version". So
you
> can then check that the newly added tab's URL is indeed that (and that we
didn't
> just open a tab and navigate to a random URL).

Done.

https://codereview.chromium.org/11644009/diff/61001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:108: "Click();"));
On 2013/02/06 22:00:50, sreeram wrote:
> Why not actually click on the page using ui_test_utils::ClickOnView(browser(),
> VIEW_ID_TAB_CONTAINER)? The reason why I insist on actually clicking on the
page
> is that it has a bunch of ripple effects - omnibox losing focus,
> InstantController trying to commit the preview, etc. I'd like to ensure that
the
> navigation reliably succeeds amidst all that.

Done.

[sreeram, 2013-02-20 22:30:36]

Bad merge? instant_client.{h,cc} shouldn't exist anymore.

[sreeram, 2013-02-20 22:53:32]

Just a bunch of nits. This is very close to completion! Thanks for all the hard
work!

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:193:
FRIEND_TEST_ALL_PREFIXES(InstantExtendedTest,
MiddleClickOnSuggestionOpensInNewTab);
80-column limit.

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:22: #include
"chrome/browser/ui/tabs/tab_strip_model.h"
Delete lines 20 and 21 (unnecessary blank line and repeated include). Move line
22 into its correct place in the sorted order.

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:80:
VIEW_ID_TAB_CONTAINER);
Revert this change.

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:135: InstantExtendedTest,
MiddleClickOnSuggestionOpensInNewTab) {
Reformat like so:
...(InstantExtendedTest,
    MiddleClick...) {

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:151: "if
(nativeSuggestions.length > 0) {"
Why are you testing for nativeSuggestions here? You shouldn't touch
nativeSuggestions at all, as we discussed.

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:153: "rid, 2);}});"
The indentation of this code block (within the string) is wrong. Please fix.

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:158:
EXPECT_TRUE(instant()->IsPreviewingSearchResults());
No need for these expectations. Perhaps just delete line 156-157 and replace
line 144 with line 158.

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:170: // Check that the
new tab url is as expected.
url -> URL

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:171:
content::WebContents* newTabContents =
newTabContents -> new_tab_contents

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:174:
instant_url_.possibly_invalid_spec()+"q=santa");
I think you can just use ".spec()" here, since we expect this to succeed. If the
.spec() fails, our expectation would also have failed, so it's fine.

[dougw, 2013-02-20 23:35:11]

:)

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:193:
FRIEND_TEST_ALL_PREFIXES(InstantExtendedTest,
MiddleClickOnSuggestionOpensInNewTab);
On 2013/02/20 22:53:32, sreeram wrote:
> 80-column limit.

Done.

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:22: #include
"chrome/browser/ui/tabs/tab_strip_model.h"
On 2013/02/20 22:53:32, sreeram wrote:
> Delete lines 20 and 21 (unnecessary blank line and repeated include). Move
line
> 22 into its correct place in the sorted order.

Done.

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:80:
VIEW_ID_TAB_CONTAINER);
On 2013/02/20 22:53:32, sreeram wrote:
> Revert this change.

Done.

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:135: InstantExtendedTest,
MiddleClickOnSuggestionOpensInNewTab) {
On 2013/02/20 22:53:32, sreeram wrote:
> Reformat like so:
> ...(InstantExtendedTest,
>     MiddleClick...) {

Done.

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:151: "if
(nativeSuggestions.length > 0) {"
Done. That's a cached copy of nativeSuggestions - we need it to access the rid,
if nothing else. I've removed the if statement since it isn't necessary.

On 2013/02/20 22:53:32, sreeram wrote:
> Why are you testing for nativeSuggestions here? You shouldn't touch
> nativeSuggestions at all, as we discussed.

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:153: "rid, 2);}});"
Done.

On 2013/02/20 22:53:32, sreeram wrote:
> The indentation of this code block (within the string) is wrong. Please fix.

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:158:
EXPECT_TRUE(instant()->IsPreviewingSearchResults());
On 2013/02/20 22:53:32, sreeram wrote:
> No need for these expectations. Perhaps just delete line 156-157 and replace
> line 144 with line 158.

Done.

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:170: // Check that the
new tab url is as expected.
On 2013/02/20 22:53:32, sreeram wrote:
> url -> URL

Done.

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:171:
content::WebContents* newTabContents =
On 2013/02/20 22:53:32, sreeram wrote:
> newTabContents -> new_tab_contents

Done.

https://codereview.chromium.org/11644009/diff/94003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:174:
instant_url_.possibly_invalid_spec()+"q=santa");
On 2013/02/20 22:53:32, sreeram wrote:
> I think you can just use ".spec()" here, since we expect this to succeed. If
the
> .spec() fails, our expectation would also have failed, so it's fine.

Done.

[samarth, 2013-02-20 23:55:16]

(just a couple of drive-by nits)

https://codereview.chromium.org/11644009/diff/81003/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.h (right):

https://codereview.chromium.org/11644009/diff/81003/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:80: // Called when the page wants to
navigate to the specified URL. Usually
nit: s/the specified URL/|url|/

https://codereview.chromium.org/11644009/diff/81003/chrome/browser/resources/...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://codereview.chromium.org/11644009/diff/81003/chrome/browser/resources/...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:227:
getApiObjectHandle().navigateContentWindow(
nit: fits on one line?

https://codereview.chromium.org/11644009/diff/81003/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/11644009/diff/81003/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:21: #include
"ui/base/window_open_disposition.h"
nit: sort please

[dougw, 2013-02-21 00:49:00]

https://codereview.chromium.org/11644009/diff/81003/chrome/browser/resources/...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://codereview.chromium.org/11644009/diff/81003/chrome/browser/resources/...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:227:
getApiObjectHandle().navigateContentWindow(
On 2013/02/20 23:55:16, samarth wrote:
> nit: fits on one line?

Done.

https://codereview.chromium.org/11644009/diff/81003/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/11644009/diff/81003/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:21: #include
"ui/base/window_open_disposition.h"
On 2013/02/20 23:55:16, samarth wrote:
> nit: sort please

Done.

[dougw, 2013-02-21 00:54:29]

https://codereview.chromium.org/11644009/diff/81003/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.h (right):

https://codereview.chromium.org/11644009/diff/81003/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:80: // Called when the page wants to
navigate to the specified URL. Usually
On 2013/02/20 23:55:16, samarth wrote:
> nit: s/the specified URL/|url|/

Done.

[sreeram, 2013-02-21 20:36:51]

lgtm

https://codereview.chromium.org/11644009/diff/83022/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/11644009/diff/83022/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:163:
EXPECT_EQ(new_tab_contents->GetURL().spec(), instant_url_.spec()+"q=santa");
Nit: Spaces around "+".

https://codereview.chromium.org/11644009/diff/83022/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:166: EXPECT_EQ(2,
browser()->tab_strip_model()->count());
You should do this before line 160. I.e., check that the count is 2 before
trying GetWebContentsAt(1).

[commit-bot: I haz the power, 2013-02-21 23:44:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dougw@chromium.org/11644009/83022

[commit-bot: I haz the power, 2013-02-21 23:45:01]

Presubmit check for 11644009-83022 failed and returned exit status 1.

INFO:root:Found 13 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/resources/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/common/render_messages.h
    chrome/browser/resources
    chrome/browser/ui

Presubmit checks took 3.2s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[dougw, 2013-02-22 00:22:21]

+palmer for render_messages.cc
(added a new parameter to an IPC message for passing the desired navigation
behavior)


+sky for browser_instant_controller.cc+.h 
(renamed and modified OpenURLInCurrentTab)


+estade for local_omnibox_popup.js 
(added support to middle click to open in a background tab)


Thanks!

[Evan Stade, 2013-02-22 02:43:49]

can you use webui::GetDispositionFromClick()?

https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:7: //
==========================================================
\n

https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:13: var
WindowOpenDisposition_ = {
why are there trailing underscores here and below?

[sky, 2013-02-22 16:50:40]

LGTM

[dougw, 2013-02-22 19:02:52]

We'd rather not modify navigateContentWindow (that would require a modification
to the public API).

Is webui::GetDispositionFromClick() already exposed to this JavaScript code?


On 2013/02/22 02:43:49, Evan Stade wrote:
> can you use webui::GetDispositionFromClick()?
> 
>
https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
> File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
> (right):
> 
>
https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
> chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:7: //
> ==========================================================
> \n
> 
>
https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
> chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:13: var
> WindowOpenDisposition_ = {
> why are there trailing underscores here and below?

[Evan Stade, 2013-02-22 21:48:51]

On 2013/02/22 19:02:52, dougw wrote:
> We'd rather not modify navigateContentWindow (that would require a
modification
> to the public API).
> 
> Is webui::GetDispositionFromClick() already exposed to this JavaScript code?

honestly, I don't even know what local_omnibox_popup.js is as there is no
file-level comment in the html or js. My main point is that you should not be
re-implementing the logic of middle click => background tab. For one thing, you
didn't implement control-click or any of the other disposition modifiers. So at
the very least this logic should be in some shared javascript file that you
include, but if possible it should use the C++ implementation that we already
have.

> 
> 
> On 2013/02/22 02:43:49, Evan Stade wrote:
> > can you use webui::GetDispositionFromClick()?
> > 
> >
>
https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
> > File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
> > chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:7: //
> > ==========================================================
> > \n
> > 
> >
>
https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
> > chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:13: var
> > WindowOpenDisposition_ = {
> > why are there trailing underscores here and below?

[dougw, 2013-02-22 22:23:43]

I added a file overview for local_omnibox_popup.js. This code supports omnibox
behavior in offline mode or when API features are not supported on the server.

Looking into it further, we can't use webui::GetDispositionFromClick() because
the behavior is different than as desired for the omnibox. For example, pressing
control has a different, conflicting, behavior.

If you would prefer, I can move the disposition code (the if statement) to a new
function in the same file.


On 2013/02/22 21:48:51, Evan Stade wrote:
> On 2013/02/22 19:02:52, dougw wrote:
> > We'd rather not modify navigateContentWindow (that would require a
> modification
> > to the public API).
> > 
> > Is webui::GetDispositionFromClick() already exposed to this JavaScript code?
> 
> honestly, I don't even know what local_omnibox_popup.js is as there is no
> file-level comment in the html or js. My main point is that you should not be
> re-implementing the logic of middle click => background tab. For one thing,
you
> didn't implement control-click or any of the other disposition modifiers. So
at
> the very least this logic should be in some shared javascript file that you
> include, but if possible it should use the C++ implementation that we already
> have.
> 
> > 
> > 
> > On 2013/02/22 02:43:49, Evan Stade wrote:
> > > can you use webui::GetDispositionFromClick()?
> > > 
> > >
> >
>
https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
> > > File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
> > > chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:7: //
> > > ==========================================================
> > > \n
> > > 
> > >
> >
>
https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
> > > chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:13:
var
> > > WindowOpenDisposition_ = {
> > > why are there trailing underscores here and below?

[Evan Stade, 2013-02-22 22:28:29]

thanks

https://codereview.chromium.org/11644009/diff/96005/chrome/browser/resources/...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://codereview.chromium.org/11644009/diff/96005/chrome/browser/resources/...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:19: var
WindowOpenDisposition_ = {
why are there trailing underscores here and below?

https://codereview.chromium.org/11644009/diff/96005/chrome/browser/resources/...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:226: *
@param {Event} event The native click event.
not sure what "native" means in this context. Suggest removing this word.

[dougw, 2013-02-22 23:32:18]

https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:7: //
==========================================================
On 2013/02/22 02:43:49, Evan Stade wrote:
> \n

Done.

https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:13: var
WindowOpenDisposition_ = {
Unless I'm mistaken (and that's very possible), the JavaScript style guide
recommends underscores for private variables.


On 2013/02/22 02:43:49, Evan Stade wrote:
> why are there trailing underscores here and below?

https://codereview.chromium.org/11644009/diff/96005/chrome/browser/resources/...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://codereview.chromium.org/11644009/diff/96005/chrome/browser/resources/...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:19: var
WindowOpenDisposition_ = {
Unless I'm mistaken (and that's very possible), the JavaScript style guide
recommends underscores for private variables.


On 2013/02/22 22:28:29, Evan Stade wrote:
> why are there trailing underscores here and below?

https://codereview.chromium.org/11644009/diff/96005/chrome/browser/resources/...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:226: *
@param {Event} event The native click event.
On 2013/02/22 22:28:29, Evan Stade wrote:
> not sure what "native" means in this context. Suggest removing this word.

Done.

[Evan Stade, 2013-02-23 00:16:26]

https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
(right):

https://codereview.chromium.org/11644009/diff/83022/chrome/browser/resources/...
chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:13: var
WindowOpenDisposition_ = {
On 2013/02/22 23:32:18, dougw wrote:
> Unless I'm mistaken (and that's very possible), the JavaScript style guide
> recommends underscores for private variables.
> 
> 
> On 2013/02/22 02:43:49, Evan Stade wrote:
> > why are there trailing underscores here and below?
> 

This is not a private variable.

http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...

"Use NAMES_LIKE_THIS for constant values."

[Evan Stade, 2013-02-25 22:51:11]

lgtm

[palmer, 2013-02-25 23:12:49]

lgtm

[commit-bot: I haz the power, 2013-02-25 23:30:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dougw@chromium.org/11644009/96007

[commit-bot: I haz the power, 2013-02-25 23:30:26]

Failed to apply patch for chrome/browser/instant/instant_page.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/instant/instant_page.h
  Hunk #1 succeeded at 77 with fuzz 2.
  Hunk #2 FAILED at 205.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/instant/instant_page.h.rej

Patch:       chrome/browser/instant/instant_page.h
Index: chrome/browser/instant/instant_page.h
diff --git a/chrome/browser/instant/instant_page.h
b/chrome/browser/instant/instant_page.h
index
22565249edd20591b8a6336da9a73c0de8997d7a..2aa4d1883f6f8ec9cccde5fafe9bb61d6ae567d0
100644
--- a/chrome/browser/instant/instant_page.h
+++ b/chrome/browser/instant/instant_page.h
@@ -77,13 +77,14 @@ class InstantPage : public content::WebContentsObserver {
     // strokes.
     virtual void StopCapturingKeyStrokes(content::WebContents* contents) = 0;
 
-    // Called when the page wants to navigate to the specified URL. Usually
-    // used by the page to navigate to privileged destinations (e.g. chrome://
-    // URLs) or to navigate to URLs that are hidden from the page using
-    // Restricted IDs (rid in the API).
+    // Called when the page wants to navigate to |url|. Usually used by the
+    // page to navigate to privileged destinations (e.g. chrome:// URLs) or to
+    // navigate to URLs that are hidden from the page using Restricted IDs (rid
+    // in the API).
     virtual void NavigateToURL(const content::WebContents* contents,
                                const GURL& url,
-                               content::PageTransition transition) = 0;
+                               content::PageTransition transition,
+                               WindowOpenDisposition disposition) = 0;
 
    protected:
     virtual ~Delegate();
@@ -204,8 +205,10 @@ class InstantPage : public content::WebContentsObserver {
                             InstantSizeUnits units);
   void OnStartCapturingKeyStrokes(int page_id);
   void OnStopCapturingKeyStrokes(int page_id);
-  void OnSearchBoxNavigate(int page_id, const GURL& url,
-                           content::PageTransition transition);
+  void OnSearchBoxNavigate(int page_id,
+                           const GURL& url,
+                           content::PageTransition transition,
+                           WindowOpenDisposition disposition);
 
   Delegate* const delegate_;
   bool supports_instant_;

[commit-bot: I haz the power, 2013-02-26 00:02:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dougw@chromium.org/11644009/97005

[commit-bot: I haz the power, 2013-02-26 03:07:09]

Change committed as 184570