Strip leading "javascript:" schemas from text pasted or dropped into the omnibox.

Strip leading "javascript:" schemas from text pasted or dropped into the omnibox.
Do not show the Paste and Go option for URIs with a javascript schema.

BUG=82181
TEST=N/A
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86525

[Peter Kasting, 2011-05-10 23:27:42]

http://codereview.chromium.org/6982011/diff/4002/base/string_util.h
File base/string_util.h (right):

http://codereview.chromium.org/6982011/diff/4002/base/string_util.h#newcode532
base/string_util.h:532: // Strips any leading javascript schemas from a string.
This does not belong in base/; it is not generic enough.

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:416: return
paste_and_go_match_.destination_url.is_valid() &&
You shouldn't need this; callers should fix this up themselves.

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/cocoa/omni...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/cocoa/omni...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:939:
StripJavascriptSchema(text, &text);
Do this in GetClipboardText(), not here.

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/gtk/omnibo...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right):

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/gtk/omnibo...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:687: if
(paste_clipboard_requested_) {
I don't think this is the right place for this.  We should be doing this as part
of the process that actually gets the text off the clipboard.  I can't seem to
find where that is, though.  There's not currently a GetClipboardText() function
like on Mac and Windows.

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/gtk/omnibo...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1595:
SetUserText(stripped_url);
Does GTK navigate on drop?  If so, where does that happen?  We're not preventing
any navigation here like we do on Mac/Win.

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1727:
StripJavascriptSchema(text, &text);
You should be doing this fixup in GetClipboardText(), not here.

[Avi (use Gerrit), 2011-05-10 23:41:06]

http://codereview.chromium.org/6982011/diff/4002/base/string_util.cc
File base/string_util.cc (right):

http://codereview.chromium.org/6982011/diff/4002/base/string_util.cc#newcode914
base/string_util.cc:914: const string16 kJsSchema(ASCIIToUTF16("javascript:"));
Plus, when you move this out of base, like pkasting suggests, you can use the
kJavaScriptScheme constant from content/common/url_constants.h.

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:417:
!paste_and_go_match_.destination_url.SchemeIs("javascript");
constant plz

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/cocoa/tool...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:761: if
(url.SchemeIs("javascript")) {
ditto

[Cris Neckar, 2011-05-11 21:42:36]

http://codereview.chromium.org/6982011/diff/4002/base/string_util.cc
File base/string_util.cc (right):

http://codereview.chromium.org/6982011/diff/4002/base/string_util.cc#newcode914
base/string_util.cc:914: const string16 kJsSchema(ASCIIToUTF16("javascript:"));
On 2011/05/10 23:41:06, Avi wrote:
> Plus, when you move this out of base, like pkasting suggests, you can use the
> kJavaScriptScheme constant from content/common/url_constants.h.

Done.

http://codereview.chromium.org/6982011/diff/4002/base/string_util.h
File base/string_util.h (right):

http://codereview.chromium.org/6982011/diff/4002/base/string_util.h#newcode532
base/string_util.h:532: // Strips any leading javascript schemas from a string.
On 2011/05/10 23:27:42, Peter Kasting wrote:
> This does not belong in base/; it is not generic enough.

Done.

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:416: return
paste_and_go_match_.destination_url.is_valid() &&
On 2011/05/10 23:27:42, Peter Kasting wrote:
> You shouldn't need this; callers should fix this up themselves.

We also want to prevent the Paste And Go menu option from showing (right click
menu) when a javascript url is in the clipboard so this is needed.

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:417:
!paste_and_go_match_.destination_url.SchemeIs("javascript");
On 2011/05/10 23:41:06, Avi wrote:
> constant plz

Done.

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/cocoa/omni...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/cocoa/omni...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:939:
StripJavascriptSchema(text, &text);
On 2011/05/10 23:27:42, Peter Kasting wrote:
> Do this in GetClipboardText(), not here.

Done.

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/cocoa/tool...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:761: if
(url.SchemeIs("javascript")) {
On 2011/05/10 23:41:06, Avi wrote:
> ditto

Done.

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/gtk/omnibo...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right):

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/gtk/omnibo...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:687: if
(paste_clipboard_requested_) {
On 2011/05/10 23:27:42, Peter Kasting wrote:
> I don't think this is the right place for this.  We should be doing this as
part
> of the process that actually gets the text off the clipboard.  I can't seem to
> find where that is, though.  There's not currently a GetClipboardText()
function
> like on Mac and Windows.

The problem is that this is handled within the gtk control so there is no way to
check the content before the control text is changed. The callbacks for this are

1. A paste is about to occur
2. The control state will change when you return (OnBeforePossibleChange)
3. The text changes out of band
4. The control state just changed (OnAfterPossibleChange)

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/gtk/omnibo...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1595:
SetUserText(stripped_url);
On 2011/05/10 23:27:42, Peter Kasting wrote:
> Does GTK navigate on drop?  If so, where does that happen?  We're not
preventing
> any navigation here like we do on Mac/Win.


If the dragged text is a url it occurs in OnPerformDropImpl() by calling into
PasteAndGo()

When it is not a URL I am not sure. Looks like it would have to be in some
callback fired later but I don't see an AcceptInput() anywhere for it :/

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/6982011/diff/4002/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1727:
StripJavascriptSchema(text, &text);
On 2011/05/10 23:27:42, Peter Kasting wrote:
> You should be doing this fixup in GetClipboardText(), not here.

Done.

[Cris Neckar, 2011-05-12 00:10:18]

As discussed this leaves gtk functionality as before.

[Peter Kasting, 2011-05-20 22:04:14]

http://codereview.chromium.org/6982011/diff/15001/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

http://codereview.chromium.org/6982011/diff/15001/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.h:195: static inline bool
StripJavascriptSchema(const string16& text, string16* out) {
Nit: Don't do this inline.  It's not speed-critical.  Do it out-of-line in a .cc
file.

Also, I think if you make the signature "string16 StripJavascriptSchemas(const
string16&)", callers that don't care about checking for changes get simpler,
while callers that do care can check the new string length against the old in
about the same number of lines of code as they're using now.

Also, Schema -> Schemas.

http://codereview.chromium.org/6982011/diff/15001/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.h:197: UTF8ToUTF16(":"));
Nit: I think both of these can be ASCIIToUTF16().

http://codereview.chromium.org/6982011/diff/15001/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.h:199: *out = text;
Nit: Shorter:

  for (*out = text; StartsWith(*out, kJsPrefix, false);
       TrimWhitespace(out->substr(kJsPrefix.length()), TRIM_LEADING, out))
    changed = true;

[Cris Neckar, 2011-05-24 00:10:06]

http://codereview.chromium.org/6982011/diff/15001/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

http://codereview.chromium.org/6982011/diff/15001/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.h:195: static inline bool
StripJavascriptSchema(const string16& text, string16* out) {
On 2011/05/20 22:04:14, Peter Kasting wrote:
> Nit: Don't do this inline.  It's not speed-critical.  Do it out-of-line in a
.cc
> file.
> 
> Also, I think if you make the signature "string16 StripJavascriptSchemas(const
> string16&)", callers that don't care about checking for changes get simpler,
> while callers that do care can check the new string length against the old in
> about the same number of lines of code as they're using now.
> 
> Also, Schema -> Schemas.

Done.

http://codereview.chromium.org/6982011/diff/15001/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.h:197: UTF8ToUTF16(":"));
On 2011/05/20 22:04:14, Peter Kasting wrote:
> Nit: I think both of these can be ASCIIToUTF16().

Done.

http://codereview.chromium.org/6982011/diff/15001/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.h:199: *out = text;
On 2011/05/20 22:04:14, Peter Kasting wrote:
> Nit: Shorter:
> 
>   for (*out = text; StartsWith(*out, kJsPrefix, false);
>        TrimWhitespace(out->substr(kJsPrefix.length()), TRIM_LEADING, out))
>     changed = true;

Done.

[Peter Kasting, 2011-05-24 00:15:18]

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/cocoa/too...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/cocoa/too...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:763: string16
text(views::StripJavascriptSchemas(UTF8ToUTF16(url.spec())));
Nit: Just inline this into the next statement

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.cc:8: #include
"chrome\browser\ui\omnibox\omnibox_view.h"
Slashes are wrong direction

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.h:37: string16
StripJavascriptSchemas(const string16&);
?? Why did you move this here?  It belonged in OmniboxView where you had it
before.

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2361: string16
stripped_text(views::StripJavascriptSchemas(text));
Nit: Just inline this into the next statement

[Cris Neckar, 2011-05-24 05:23:33]

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.cc:8: #include
"chrome\browser\ui\omnibox\omnibox_view.h"
On 2011/05/24 00:15:18, Peter Kasting wrote:
> Slashes are wrong direction

Done.

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.h:37: string16
StripJavascriptSchemas(const string16&);
> ?? Why did you move this here?  It belonged in OmniboxView where you had it
> before.

?? It was never in OmniboxView. It can't be as I need to call it from outside of
that context (specifically toolbar_controller.mm). Check the previous patches.

[Cris Neckar, 2011-05-24 17:09:40]

check the comment before this for my response on the other two issues. 

Most importantly this function was never in OmniboxView. I need to be able to
call it from toolbar_controller.mm etc.

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/cocoa/too...
File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right):

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/cocoa/too...
chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:763: string16
text(views::StripJavascriptSchemas(UTF8ToUTF16(url.spec())));
On 2011/05/24 00:15:18, Peter Kasting wrote:
> Nit: Just inline this into the next statement

Done.

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:2361: string16
stripped_text(views::StripJavascriptSchemas(text));
On 2011/05/24 00:15:18, Peter Kasting wrote:
> Nit: Just inline this into the next statement

Done.

[Peter Kasting, 2011-05-24 17:40:47]

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.h:37: string16
StripJavascriptSchemas(const string16&);
On 2011/05/24 05:23:33, Cris Neckar wrote:
> ?? It was never in OmniboxView. It can't be as I need to call it from outside
of
> that context (specifically toolbar_controller.mm).

I don't understand why that matters.  I think this should be a public static
function in OmniboxView.  That's callable everywhere.

It definitely shouldn't be in the views namespace, as that's not used on Mac.

[Cris Neckar, 2011-05-24 18:48:11]

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

http://codereview.chromium.org/6982011/diff/21001/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.h:37: string16
StripJavascriptSchemas(const string16&);
On 2011/05/24 17:40:47, Peter Kasting wrote:
> On 2011/05/24 05:23:33, Cris Neckar wrote:
> > ?? It was never in OmniboxView. It can't be as I need to call it from
outside
> of
> > that context (specifically toolbar_controller.mm).
> 
> I don't understand why that matters.  I think this should be a public static
> function in OmniboxView.  That's callable everywhere.
> 
> It definitely shouldn't be in the views namespace, as that's not used on Mac.

Done.

[Peter Kasting, 2011-05-24 19:34:58]

LGTM

http://codereview.chromium.org/6982011/diff/23004/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

http://codereview.chromium.org/6982011/diff/23004/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.cc:18: for (; StartsWith(out, kJsPrefix,
false);
Nit: Now that we no longer have |changed|, using a for loop doesn't buy us as
much.  You could either move the declaration of kJsPrefix into here, or turn
this back into a while loop.

http://codereview.chromium.org/6982011/diff/23004/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.cc:19:
TrimWhitespace(out.substr(kJsPrefix.length()), TRIM_LEADING, &out)) { }
Nit: If you keep this as a for loop, indent to just after '(' above (exception
to the 4-space rule).  Also, I'd replace "{ }" with a single ";" on the
subsequent line.

http://codereview.chromium.org/6982011/diff/23004/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

http://codereview.chromium.org/6982011/diff/23004/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.h:190: static string16
StripJavascriptSchemas(const string16&);
Nit: Go ahead and name the arg here -- dropping the name is more WebKit style. 
Also add a comment about what the function does.

[Cris Neckar, 2011-05-24 22:22:35]

http://codereview.chromium.org/6982011/diff/23004/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

http://codereview.chromium.org/6982011/diff/23004/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.cc:18: for (; StartsWith(out, kJsPrefix,
false);
On 2011/05/24 19:34:59, Peter Kasting wrote:
> Nit: Now that we no longer have |changed|, using a for loop doesn't buy us as
> much.  You could either move the declaration of kJsPrefix into here, or turn
> this back into a while loop.

Done.

http://codereview.chromium.org/6982011/diff/23004/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.cc:19:
TrimWhitespace(out.substr(kJsPrefix.length()), TRIM_LEADING, &out)) { }
On 2011/05/24 19:34:59, Peter Kasting wrote:
> Nit: If you keep this as a for loop, indent to just after '(' above (exception
> to the 4-space rule).  Also, I'd replace "{ }" with a single ";" on the
> subsequent line.

I changed this back to a while loop. I actually tried to do just a semi-colon
originally and xcode actually warns on that so I switched it. irrelevant now
that it's a while loop but still interesting.

http://codereview.chromium.org/6982011/diff/23004/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

http://codereview.chromium.org/6982011/diff/23004/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_view.h:190: static string16
StripJavascriptSchemas(const string16&);
On 2011/05/24 19:34:59, Peter Kasting wrote:
> Nit: Go ahead and name the arg here -- dropping the name is more WebKit style.

> Also add a comment about what the function does.

Done.