Adds paste function to searchbox api and handles paste event on fakebox

Adds paste function to searchbox api and handles paste event on fakebox

Pasting into fakebox via context-menu didn't work before.
This change adds a paste function to the searchbox api, that gets the text
from the clipboard into the omnibox using SetUserText ( opening the
suggestions panel.)

BUG=164784
TEST=Right click on fakebox and paste. The pasted value should appear on the omnibox and the suggestions panel should be open.

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

[jfweitz, 2013-07-25 21:25:52]

Hi,
Please take a look at the following files:

jschuh: render_messages
jeremycho: local_ntp.js
samarth: all files

Thank you!

[samarth, 2013-07-26 22:51:41]

Awesome, this works great! Just a few comments below.

Thanks,
Samarth

https://codereview.chromium.org/20501002/diff/3001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/20501002/diff/3001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:199:
omnibox_view->SetUserText(clipboardText);
I think you also want to call omnibox_view->model()->on_paste() to get all the
paste-specific behavior in OmniboxEditModel.

https://codereview.chromium.org/20501002/diff/3001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/20501002/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:339:
contents->GetView()->Focus();
As we discussed before, this is troublesome because this lets the page close the
popup right after pasting. Instead of handling this call here, let's call
BrowserInstantController::FocusOmnibox() in this branch as well. (Well, at that
point, you can get rid of the switch and just call it directly.)

Then in BrowserInstantController, you should only handle this case if the popup
is not open. You can check using OmniboxEditModel::popup_model::IsOpen I think.

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

https://codereview.chromium.org/20501002/diff/3001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:862: 
nit: omit this line

[jeremycho, 2013-07-28 00:03:08]

lgtm

Sorry for the delay.  LGTM for local_ntp.js.

[jfweitz, 2013-07-29 18:48:25]

Comments addressed.

Checking if the popup is open in BrowserInstantController::FocusOmnibox did
work. It's preventing from removing focus if the popup is open.

https://codereview.chromium.org/20501002/diff/3001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/20501002/diff/3001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:199:
omnibox_view->SetUserText(clipboardText);
On 2013/07/26 22:51:41, samarth wrote:
> I think you also want to call omnibox_view->model()->on_paste() to get all the
> paste-specific behavior in OmniboxEditModel.

Done.

https://codereview.chromium.org/20501002/diff/3001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/20501002/diff/3001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:339:
contents->GetView()->Focus();
On 2013/07/26 22:51:41, samarth wrote:
> As we discussed before, this is troublesome because this lets the page close
the
> popup right after pasting. Instead of handling this call here, let's call
> BrowserInstantController::FocusOmnibox() in this branch as well. (Well, at
that
> point, you can get rid of the switch and just call it directly.)
> 
> Then in BrowserInstantController, you should only handle this case if the
popup
> is not open. You can check using OmniboxEditModel::popup_model::IsOpen I
think.

Done.

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

https://codereview.chromium.org/20501002/diff/3001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:862: 
On 2013/07/26 22:51:41, samarth wrote:
> nit: omit this line

Done.

[samarth, 2013-07-31 01:44:37]

Thinking about drag-and-drop, I think we should do what adds the least
complexity.  So how about we change the searchBox.paste() api to take an
optional text param? The semantics will be:
- If no param is given, Chrome pastes what's in the clipboard
- If the param is present, Chrome pastes the param after stripping out
javascript schemes, collapsing whitespace, etc.
It won't navigate in either version.

I like this because it's simpler (only 1 api function) and also in both cases,
we force the popup to open and the user to take action.  So the page can't
initiate a browser-level navigation/search without user input.  It's also
consistent with the google.com searchbox which is what the fakebox ultimately
looks like.

It probably makes sense to make that change in this CL as well.  That way we can
get a comprehensive review from Justin for this from a security perspective.

Thanks,
Samarth

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

https://codereview.chromium.org/20501002/diff/11001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:189:
contents->GetView()->Focus();
Instead of passing in |contents| here you can get use GetActiveWebContents()

https://codereview.chromium.org/20501002/diff/11001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:227:
omnibox_view->model()->on_paste();
This needs to be called before you SetUserText.

[jfweitz, 2013-08-03 00:01:59]

+pkasting for omnibox_view*

Samarth's comments addressed:
I added an optional param to the paste function, and used for the drag&drop to
the fakebox.

Jeremy PTAL at local_ntp.js/.css
 
Thank you all!

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

https://codereview.chromium.org/20501002/diff/11001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:189:
contents->GetView()->Focus();
On 2013/07/31 01:44:37, samarth wrote:
> Instead of passing in |contents| here you can get use GetActiveWebContents()

Done.

https://codereview.chromium.org/20501002/diff/11001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:227:
omnibox_view->model()->on_paste();
On 2013/07/31 01:44:37, samarth wrote:
> This needs to be called before you SetUserText.

Done.

[samarth, 2013-08-06 18:39:44]

A bunch of nits below but lgtm.

Awesome work!

Thanks,
Samarth

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1042: var text =
event.dataTransfer.getData('text/plain');
super nit: please swap the two lines

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

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:187: content::WebContents*
content = GetActiveWebContents();
nit: s/content/contents

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:188: if (content) {
nit: no braces for single-line condition

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:222: string16 text_to_paste;
nit: simply:
string16 text_to_paste = text.empty() ?
    omnibox_view->GetClipboardText() :
    omnibox_view->SanitizeTextForPaste(text);

Please also a comment above this line describing the two cases (e.g.
GetClipboardText already sanitizes and the second form is needed to handle
drag-and-drop).

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.h (right):

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.h:159: const string16& text)
OVERRIDE;
nit: align with const above.

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp_prerenderer.cc (right):

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.cc:219: const
content::WebContents* contents, const string16& text) {
nit: comment out the unused variables like so
const content::WebContents* /* contents */,

While you're here, feel free to do the same for the other such functions in this
file.

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp_prerenderer.h (right):

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.h:134: const string16& text)
OVERRIDE;
nit: should be aligned with the const above.

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

https://codereview.chromium.org/20501002/diff/22001/chrome/common/render_mess...
chrome/common/render_messages.h:675: // Tells InstantExtended to paste the text
or the clipboard content if text is
nit: Tells InstantExtended to paste text into the omnibox.  If text is empty,
the clipboard contents will be pasted. This causes the omnibox dropdown to open.

https://codereview.chromium.org/20501002/diff/22001/chrome/common/render_mess...
chrome/common/render_messages.h:677:
IPC_MESSAGE_ROUTED2(ChromeViewHostMsg_SearchBoxPaste,
Perhaps PasteAndOpenDropdown to make it clearer and less scary sounding.

https://codereview.chromium.org/20501002/diff/22001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/20501002/diff/22001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.h:74: // Pastes text or clipboard content if
text is empty into the omnibox.
Just
// Sends ChromeViewHostMsg_SearchBoxPaste to the browser.
since we'll document the |text| behavior elsewhere.

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

https://codereview.chromium.org/20501002/diff/22001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:865: DVLOG(1) << render_view <<
" Paste";
Might as well also output the text here.

https://codereview.chromium.org/20501002/diff/22001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:867: 
nit: remove empty line

[Peter Kasting, 2013-08-06 21:55:12]

LGTM

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.h:234: // Returns a string with any
leading javascript schemas stripped from the
Nit: While you're here, improve this comment:

Returns |text| with any leading javascript schemas stripped

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.h:239: // collapsing whitespace from the
input text.
This needs to say more about what's happening.  Basically, you should hoist the
comments from the implementation, integrating them along the lines of:

First, calls StripJavascriptSchemas().  Then automatically collapses internal
whitespace as follows:
* If the only whitespace in |text| is newlines, users are most likely pasting in
URLs...
* Otherwise, users may be pasting in search data, e.g. street addresses, ...

(At this point the comments in the .cc file can basically go away)

[samarth, 2013-08-08 16:19:22]

ping Justin? We'd love to get this in this week if possible.

Thanks!
Samarth

[jschuh, 2013-08-08 16:24:47]

I was just waiting for the last round of feedback to get actioned before
reviewing.

[samarth, 2013-08-08 16:25:51]

Ok, awesome, thanks!

[jfweitz, 2013-08-09 02:37:41]

Peter and Samarth's comments addressed. 
PTAL. Thanks!

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1042: var text =
event.dataTransfer.getData('text/plain');
On 2013/08/06 18:39:44, samarth wrote:
> super nit: please swap the two lines

Done.

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

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:187: content::WebContents*
content = GetActiveWebContents();
On 2013/08/06 18:39:44, samarth wrote:
> nit: s/content/contents

Done.

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:188: if (content) {
On 2013/08/06 18:39:44, samarth wrote:
> nit: no braces for single-line condition

Done.

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:222: string16 text_to_paste;
On 2013/08/06 18:39:44, samarth wrote:
> nit: simply:
> string16 text_to_paste = text.empty() ?
>     omnibox_view->GetClipboardText() :
>     omnibox_view->SanitizeTextForPaste(text);
> 
> Please also a comment above this line describing the two cases (e.g.
> GetClipboardText already sanitizes and the second form is needed to handle
> drag-and-drop).

Done.

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.h:234: // Returns a string with any
leading javascript schemas stripped from the
On 2013/08/06 21:55:13, Peter Kasting wrote:
> Nit: While you're here, improve this comment:
> 
> Returns |text| with any leading javascript schemas stripped

Done.

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.h:239: // collapsing whitespace from the
input text.
On 2013/08/06 21:55:13, Peter Kasting wrote:
> This needs to say more about what's happening.  Basically, you should hoist
the
> comments from the implementation, integrating them along the lines of:
> 
> First, calls StripJavascriptSchemas().  Then automatically collapses internal
> whitespace as follows:
> * If the only whitespace in |text| is newlines, users are most likely pasting
in
> URLs...
> * Otherwise, users may be pasting in search data, e.g. street addresses, ...
> 
> (At this point the comments in the .cc file can basically go away)

Done.

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.h (right):

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.h:159: const string16& text)
OVERRIDE;
On 2013/08/06 18:39:44, samarth wrote:
> nit: align with const above.

Done.

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp_prerenderer.cc (right):

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.cc:219: const
content::WebContents* contents, const string16& text) {
On 2013/08/06 18:39:44, samarth wrote:
> nit: comment out the unused variables like so
> const content::WebContents* /* contents */,
> 
> While you're here, feel free to do the same for the other such functions in
this
> file.

Done.

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp_prerenderer.h (right):

https://codereview.chromium.org/20501002/diff/22001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp_prerenderer.h:134: const string16& text)
OVERRIDE;
On 2013/08/06 18:39:44, samarth wrote:
> nit: should be aligned with the const above.

Done.

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

https://codereview.chromium.org/20501002/diff/22001/chrome/common/render_mess...
chrome/common/render_messages.h:675: // Tells InstantExtended to paste the text
or the clipboard content if text is
On 2013/08/06 18:39:44, samarth wrote:
> nit: Tells InstantExtended to paste text into the omnibox.  If text is empty,
> the clipboard contents will be pasted. This causes the omnibox dropdown to
open.

Done.

https://codereview.chromium.org/20501002/diff/22001/chrome/common/render_mess...
chrome/common/render_messages.h:677:
IPC_MESSAGE_ROUTED2(ChromeViewHostMsg_SearchBoxPaste,
On 2013/08/06 18:39:44, samarth wrote:
> Perhaps PasteAndOpenDropdown to make it clearer and less scary sounding.

Done.

https://codereview.chromium.org/20501002/diff/22001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/20501002/diff/22001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.h:74: // Pastes text or clipboard content if
text is empty into the omnibox.
On 2013/08/06 18:39:44, samarth wrote:
> Just
> // Sends ChromeViewHostMsg_SearchBoxPaste to the browser.
> since we'll document the |text| behavior elsewhere.

Done.

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

https://codereview.chromium.org/20501002/diff/22001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:865: DVLOG(1) << render_view <<
" Paste";
On 2013/08/06 18:39:44, samarth wrote:
> Might as well also output the text here.

Done.

https://codereview.chromium.org/20501002/diff/22001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:867: 
On 2013/08/06 18:39:44, samarth wrote:
> nit: remove empty line

Done.

[Peter Kasting, 2013-08-09 18:11:58]

LGTM with a few more comment tweaks

https://codereview.chromium.org/20501002/diff/41001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

https://codereview.chromium.org/20501002/diff/41001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.cc:28: // Convert newlines to spaces.
These two implementation comments aren't quite correct.  Perhaps this for here:

// Check for non-newline whitespace; if found, collapse whitespace runs down to
single spaces.

And this for below:

// Otherwise, all whitespace is newlines; remove it entirely.

https://codereview.chromium.org/20501002/diff/41001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

https://codereview.chromium.org/20501002/diff/41001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.h:243: // this case newlines are
converted to spaces.
Nit: "...this case, runs of whitespace are collapsed down to single spaces."

[jfweitz, 2013-08-09 18:49:55]

Peter's comments fixed in patch set 9.

Thanks!

https://codereview.chromium.org/20501002/diff/41001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

https://codereview.chromium.org/20501002/diff/41001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.cc:28: // Convert newlines to spaces.
On 2013/08/09 18:11:59, Peter Kasting wrote:
> These two implementation comments aren't quite correct.  Perhaps this for
here:
> 
> // Check for non-newline whitespace; if found, collapse whitespace runs down
to
> single spaces.
> 
> And this for below:
> 
> // Otherwise, all whitespace is newlines; remove it entirely.

Done.

https://codereview.chromium.org/20501002/diff/41001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

https://codereview.chromium.org/20501002/diff/41001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.h:243: // this case newlines are
converted to spaces.
On 2013/08/09 18:11:59, Peter Kasting wrote:
> Nit: "...this case, runs of whitespace are collapsed down to single spaces."

Done.

[jschuh, 2013-08-09 21:37:23]

ipc security lgtm. renderer -> browser search string text.

[commit-bot: I haz the power, 2013-08-09 21:52:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jfweitz@chromium.org/20501002/50001

[commit-bot: I haz the power, 2013-08-09 22:35:38]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-08-10 01:35:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jfweitz@chromium.org/20501002/50001

[commit-bot: I haz the power, 2013-08-10 13:49:24]

Change committed as 216842