Add new IPC for searchbox to pipe SearchBox NavigateContentWindow calls to the browser.

Add new IPC for searchbox to pipe SearchBox NavigateContentWindow calls to the browser.


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

[Shishir, 2012-12-06 21:31:06]

This first patch uses the browser's OpenURL call which I am not too happy about
- eventhough it works correctly. I am working on trying to make the same code
path as when enter is pressed after pressing some arrow keys.

[Shishir, 2012-12-06 22:20:21]

On 2012/12/06 21:31:06, Shishir wrote:
> This first patch uses the browser's OpenURL call which I am not too happy
about
> - eventhough it works correctly. I am working on trying to make the same code
> path as when enter is pressed after pressing some arrow keys.

After talking to David and Samarth it seems that we want to allow for the
flexibility of an Instant page to be able to navigate to any URL. So this
approach seems fine. A few smaller issues would be that since this does not
follow the code path of "SetSuggestion" followed by "AcceptInput" any metrics
might get messed up, but I guess that is already broken if the user clicks on a
search query.

PTAL.

[Shishir, 2012-12-06 22:20:44]

PTAL.

[samarth, 2012-12-07 02:01:35]

https://codereview.chromium.org/11466007/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_client.h (right):

https://codereview.chromium.org/11466007/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_client.h:140: void SearchBoxNavigate(int page_id,
const GURL& url);
nit: keep the same name as in the Delegate, i.e. NavigateToURL

https://codereview.chromium.org/11466007/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_commit_type.h (right):

https://codereview.chromium.org/11466007/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_commit_type.h:23: INSTANT_COMMIT_CLICKED_RESULT,
This name is more specific than what the api allows.  Maybe something like
INSTANT_COMMIT_REQUESTED_NAVIGATION?

https://codereview.chromium.org/11466007/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11466007/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_loader.cc:295: controller_->NavigateToURL(url);
Also call InstantSupportDetermined

https://codereview.chromium.org/11466007/diff/1/chrome/browser/ui/browser_ins...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11466007/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.cc:160: content::Referrer(),
This is fine for now, but there might be cases where the referrer should be set
to the instant URL (it could be an option to the navigateContentWindow API).

https://codereview.chromium.org/11466007/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.cc:163:
//content::PAGE_TRANSITION_FROM_ADDRESS_BAR,
remove?

https://codereview.chromium.org/11466007/diff/1/chrome/renderer/searchbox/sea...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/11466007/diff/1/chrome/renderer/searchbox/sea...
chrome/renderer/searchbox/searchbox.h:42: // Send
ChromeViewHostMsg_SearchBoxNavigate to the browser.
nit: Sends ViewHostMsg (or fix the other ViewHostMsg => ChromeViewHostMsg)

[Shishir, 2012-12-10 20:40:38]

PTAL.

https://codereview.chromium.org/11466007/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_client.h (right):

https://codereview.chromium.org/11466007/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_client.h:140: void SearchBoxNavigate(int page_id,
const GURL& url);
On 2012/12/07 02:01:35, samarth wrote:
> nit: keep the same name as in the Delegate, i.e. NavigateToURL

This name is consistent with the IPC name which is how functions called as a
result of IPCs are usually named.

https://codereview.chromium.org/11466007/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_commit_type.h (right):

https://codereview.chromium.org/11466007/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_commit_type.h:23: INSTANT_COMMIT_CLICKED_RESULT,
On 2012/12/07 02:01:35, samarth wrote:
> This name is more specific than what the api allows.  Maybe something like
> INSTANT_COMMIT_REQUESTED_NAVIGATION?

Done.

https://codereview.chromium.org/11466007/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11466007/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_loader.cc:295: controller_->NavigateToURL(url);
On 2012/12/07 02:01:35, samarth wrote:
> Also call InstantSupportDetermined

Added.

Not sure why that is needed. The SearchBoxNavigate IPC should not be called
before the instant support is determined. Right?

https://codereview.chromium.org/11466007/diff/1/chrome/browser/ui/browser_ins...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11466007/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.cc:160: content::Referrer(),
On 2012/12/07 02:01:35, samarth wrote:
> This is fine for now, but there might be cases where the referrer should be
set
> to the instant URL (it could be an option to the navigateContentWindow API).

Sure. It's easy to add params and maybe we could even take the OpenURLParams as
the input in the IPC if required.

https://codereview.chromium.org/11466007/diff/1/chrome/browser/ui/browser_ins...
chrome/browser/ui/browser_instant_controller.cc:163:
//content::PAGE_TRANSITION_FROM_ADDRESS_BAR,
On 2012/12/07 02:01:35, samarth wrote:
> remove?

Not 100% sure what transition we want to label this. Perhaps scott can chime in.

https://codereview.chromium.org/11466007/diff/1/chrome/renderer/searchbox/sea...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/11466007/diff/1/chrome/renderer/searchbox/sea...
chrome/renderer/searchbox/searchbox.h:42: // Send
ChromeViewHostMsg_SearchBoxNavigate to the browser.
On 2012/12/07 02:01:35, samarth wrote:
> nit: Sends ViewHostMsg (or fix the other ViewHostMsg => ChromeViewHostMsg)

Done.

[samarth, 2012-12-11 04:42:09]

https://codereview.chromium.org/11466007/diff/8001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11466007/diff/8001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:796:
CommitIfPossible(INSTANT_COMMIT_REQUESTED_NAVIGATION);
Do we want to commit here?  If a URL suggestion was selected on the instant
overlay, we'd only be showing the "Press [enter] to navigate" message which
doesn't seem to be particularly useful to have in your navigation stack.

The other alternative is to just navigate the current tab, and in the cases
where we were using a loader, just hide it.  What do you think?

https://codereview.chromium.org/11466007/diff/8001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_tab.cc (right):

https://codereview.chromium.org/11466007/diff/8001/chrome/browser/instant/ins...
chrome/browser/instant/instant_tab.cc:88: // We don't honor this call from
committed tabs.
Sorry, I missed this before but we do need this for committed tabs too, no?  For
example, you search for [abc], hit enter.  Then if you type [chrome://settings]
in the omnibox, we'll reuse the committed search page to render the suggestions
and you still need to be able to click on the chrome:// nav suggestion.

[Shishir, 2012-12-11 20:22:39]

The instant page no longer commits on NavigateToURL. We just hide the loader.
The commit will happen if the full preview is showing due to the click on the
page.

PTAL.

https://codereview.chromium.org/11466007/diff/8001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11466007/diff/8001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:796:
CommitIfPossible(INSTANT_COMMIT_REQUESTED_NAVIGATION);
On 2012/12/11 04:42:10, samarth wrote:
> Do we want to commit here?  If a URL suggestion was selected on the instant
> overlay, we'd only be showing the "Press [enter] to navigate" message which
> doesn't seem to be particularly useful to have in your navigation stack.
> 
> The other alternative is to just navigate the current tab, and in the cases
> where we were using a loader, just hide it.  What do you think?

Like we discussed, we will keep the committing logic separate which should be
addressed by the user possibly clicking on the page.

https://codereview.chromium.org/11466007/diff/8001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_tab.cc (right):

https://codereview.chromium.org/11466007/diff/8001/chrome/browser/instant/ins...
chrome/browser/instant/instant_tab.cc:88: // We don't honor this call from
committed tabs.
On 2012/12/11 04:42:10, samarth wrote:
> Sorry, I missed this before but we do need this for committed tabs too, no? 
For
> example, you search for [abc], hit enter.  Then if you type
[chrome://settings]
> in the omnibox, we'll reuse the committed search page to render the
suggestions
> and you still need to be able to click on the chrome:// nav suggestion.

Thanks for pointing this out. Fixed.

[Shishir, 2012-12-11 21:57:35]

Adding palmer for the new IPC. PTAL.

[palmer, 2012-12-11 22:54:44]

lgtm

[sky, 2012-12-12 00:09:21]

Is there a reason we need IPC calls like this instead of having the page
navigation and then committing the change?

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:169:
content::PAGE_TRANSITION_TYPED,
Why is this typed, and should the referrer be set?

[Shishir, 2012-12-12 00:48:28]

> Is there a reason we need IPC calls like this instead of having the page
> navigation and then committing the change?

The issue arises with cross process navigations. For eg when the user clicks on
the link "chrome://settings" in the instant dropdown, the page cannot navigate
to it for security reasons. This seemed like a good approach rather than to
relax security restrictions for instant.

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:169:
content::PAGE_TRANSITION_TYPED,
On 2012/12/12 00:09:21, sky wrote:
> Why is this typed, and should the referrer be set?

We talked about this and said we dont want to set referers here since the
navigation happened due to a click on the area that is equivalent of the omnibox
popup .

As for the typed page transition type - because it was the default for
AutcompleteMatch. Do you think any other one fits better here?

[palmer, 2012-12-12 01:15:06]

> > Is there a reason we need IPC calls like this instead of having the page
> > navigation and then committing the change?
> 
> The issue arises with cross process navigations. For eg when the user clicks
on
> the link "chrome://settings" in the instant dropdown, the page cannot navigate
> to it for security reasons. This seemed like a good approach rather than to
> relax security restrictions for instant.

Yes. Thank you for thinking of us. :)

[sky, 2012-12-12 04:11:19]

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:169:
content::PAGE_TRANSITION_TYPED,
On 2012/12/12 00:48:28, Shishir wrote:
> On 2012/12/12 00:09:21, sky wrote:
> > Why is this typed, and should the referrer be set?
> 
> We talked about this and said we dont want to set referers here since the
> navigation happened due to a click on the area that is equivalent of the
omnibox
> popup .
> 
> As for the typed page transition type - because it was the default for
> AutcompleteMatch. Do you think any other one fits better here?

Omnibox also uses PAGE_TRANSITION_GENERATED. Which one it uses depends upon the
entry. Maybe the page needs to pass up some context when invoking this.

[samarth, 2012-12-12 16:32:26]

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:813: void
InstantController::NavigateToURL(const GURL& url) {
Let's check for extended_enabled_ here to be safe.

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:814: HideLoader();
If (loader_)

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:169:
content::PAGE_TRANSITION_TYPED,
On 2012/12/12 04:11:19, sky wrote:
> On 2012/12/12 00:48:28, Shishir wrote:
> > On 2012/12/12 00:09:21, sky wrote:
> > > Why is this typed, and should the referrer be set?
> > 
> > We talked about this and said we dont want to set referers here since the
> > navigation happened due to a click on the area that is equivalent of the
> omnibox
> > popup .
> > 
> > As for the typed page transition type - because it was the default for
> > AutcompleteMatch. Do you think any other one fits better here?
> 
> Omnibox also uses PAGE_TRANSITION_GENERATED. Which one it uses depends upon
the
> entry. Maybe the page needs to pass up some context when invoking this.

The searchbox code has access to InstantAutocompleteResult, so it could
determine the appropriate transition type and pass it through.

[Shishir, 2012-12-12 20:04:47]

PTAL.

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:813: void
InstantController::NavigateToURL(const GURL& url) {
On 2012/12/12 16:32:27, samarth wrote:
> Let's check for extended_enabled_ here to be safe.

Done.

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:814: HideLoader();
On 2012/12/12 16:32:27, samarth wrote:
> If (loader_)

This can come from the instant_tab also. Also to remember is that the loader or
the instant_tab will have to be on the stack so we should be ok.

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:169:
content::PAGE_TRANSITION_TYPED,
On 2012/12/12 16:32:27, samarth wrote:
> On 2012/12/12 04:11:19, sky wrote:
> > On 2012/12/12 00:48:28, Shishir wrote:
> > > On 2012/12/12 00:09:21, sky wrote:
> > > > Why is this typed, and should the referrer be set?
> > > 
> > > We talked about this and said we dont want to set referers here since the
> > > navigation happened due to a click on the area that is equivalent of the
> > omnibox
> > > popup .
> > > 
> > > As for the typed page transition type - because it was the default for
> > > AutcompleteMatch. Do you think any other one fits better here?
> > 
> > Omnibox also uses PAGE_TRANSITION_GENERATED. Which one it uses depends upon
> the
> > entry. Maybe the page needs to pass up some context when invoking this.
> 
> The searchbox code has access to InstantAutocompleteResult, so it could
> determine the appropriate transition type and pass it through.

Done.

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:169:
content::PAGE_TRANSITION_TYPED,
On 2012/12/12 04:11:19, sky wrote:
> On 2012/12/12 00:48:28, Shishir wrote:
> > On 2012/12/12 00:09:21, sky wrote:
> > > Why is this typed, and should the referrer be set?
> > 
> > We talked about this and said we dont want to set referers here since the
> > navigation happened due to a click on the area that is equivalent of the
> omnibox
> > popup .
> > 
> > As for the typed page transition type - because it was the default for
> > AutcompleteMatch. Do you think any other one fits better here?
> 
> Omnibox also uses PAGE_TRANSITION_GENERATED. Which one it uses depends upon
the
> entry. Maybe the page needs to pass up some context when invoking this.

Done.

[sky, 2012-12-12 21:25:38]

LGTM

[commit-bot: I haz the power, 2012-12-12 21:45:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/11466007/23003

[samarth, 2012-12-12 22:05:35]

lgtm

We can address my comments separately.

Thanks,
Samarth

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:814: HideLoader();
On 2012/12/12 20:04:47, Shishir wrote:
> On 2012/12/12 16:32:27, samarth wrote:
> > If (loader_)
> 
> This can come from the instant_tab also. Also to remember is that the loader
or
> the instant_tab will have to be on the stack so we should be ok.
> 

When it's coming from InstantTab, there's nothing to hide.  It will be a no-op,
but semantically, I think it would be clearer if this only hid when necessary.

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

https://codereview.chromium.org/11466007/diff/23003/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:636: content::PageTransition
transition = content::PAGE_TRANSITION_TYPED;
Maybe add a note/TODO here to reconsider what the transition should be in the
non-native-suggestion case.

[commit-bot: I haz the power, 2012-12-12 22:10:34]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Shishir, 2012-12-12 22:21:08]

Adding jam for the content/.. changes.

PTAL.

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11466007/diff/15001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:814: HideLoader();
On 2012/12/12 22:05:36, samarth wrote:
> On 2012/12/12 20:04:47, Shishir wrote:
> > On 2012/12/12 16:32:27, samarth wrote:
> > > If (loader_)
> > 
> > This can come from the instant_tab also. Also to remember is that the loader
> or
> > the instant_tab will have to be on the stack so we should be ok.
> > 
> 
> When it's coming from InstantTab, there's nothing to hide.  It will be a
no-op,
> but semantically, I think it would be clearer if this only hid when necessary.

Done.

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

https://codereview.chromium.org/11466007/diff/23003/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:636: content::PageTransition
transition = content::PAGE_TRANSITION_TYPED;
On 2012/12/12 22:05:36, samarth wrote:
> Maybe add a note/TODO here to reconsider what the transition should be in the
> non-native-suggestion case.

Since this code only seems to support NavigateContentWindow for browser
suggestions, the todo doesn't seem necessary. The change that will enable NCW
for instant suggestions will have to deal with it.

[jam, 2012-12-12 22:23:24]

content lgtm

[commit-bot: I haz the power, 2012-12-12 22:30:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/11466007/29008

[commit-bot: I haz the power, 2012-12-12 23:01:17]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[gavinp, 2012-12-13 18:56:37]

https://codereview.chromium.org/11466007/diff/37001/chrome/common/render_mess...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/11466007/diff/37001/chrome/common/render_mess...
chrome/common/render_messages.h:661: content::PageTransition /* transition */);
I think you don't want that semicolon.

[Shishir, 2012-12-13 18:58:44]

https://codereview.chromium.org/11466007/diff/37001/chrome/common/render_mess...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/11466007/diff/37001/chrome/common/render_mess...
chrome/common/render_messages.h:661: content::PageTransition /* transition */);
On 2012/12/13 18:56:38, gavinp wrote:
> I think you don't want that semicolon.

Thanks!

[commit-bot: I haz the power, 2012-12-13 19:00:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/11466007/47001

[commit-bot: I haz the power, 2012-12-13 23:46:10]

Change committed as 173004