Trim whitespace and check for unsafe scheme before adding a url in context me...

Trim whitespace and check for unsafe scheme before adding a url in context menu "Go To" item. 

BUG=41778
TEST=None


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

[inferno, 2010-04-17 05:40:22]

Peter, can you please review this fix.

[Peter Kasting, 2010-04-17 17:05:09]

http://codereview.chromium.org/1594039/diff/1/2
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/1594039/diff/1/2#newcode447
chrome/browser/tab_contents/render_view_context_menu.cc:447:
TrimWhitespace(params_.selection_text, TRIM_ALL, &params_.selection_text);
Do this above the check for empty.

http://codereview.chromium.org/1594039/diff/1/2#newcode463
chrome/browser/tab_contents/render_view_context_menu.cc:463: if
(ChildProcessSecurityPolicy::GetInstance()->IsWebSafeScheme(
Please remove this per my comments on the bug.

[inferno, 2010-04-17 17:30:40]

http://codereview.chromium.org/1594039/diff/1/2
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/1594039/diff/1/2#newcode447
chrome/browser/tab_contents/render_view_context_menu.cc:447:
TrimWhitespace(params_.selection_text, TRIM_ALL, &params_.selection_text);
On 2010/04/17 17:05:09, Peter Kasting wrote:
> Do this above the check for empty.

Yes TrimWhitespace does check for empty. plus, it is not needed as we can
.empty() check is done 1 line above.

http://codereview.chromium.org/1594039/diff/1/2#newcode463
chrome/browser/tab_contents/render_view_context_menu.cc:463: if
(ChildProcessSecurityPolicy::GetInstance()->IsWebSafeScheme(
On 2010/04/17 17:05:09, Peter Kasting wrote:
> Please remove this per my comments on the bug.

lcamtuf and I believe the this check is important to prevent people from opening
unsafe schemes from websites. it will let a user open still open normal http,
https, feed, ftp, data, chrome-extension url links.

[Peter Kasting, 2010-04-17 23:25:24]

http://codereview.chromium.org/1594039/diff/1/2
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/1594039/diff/1/2#newcode447
chrome/browser/tab_contents/render_view_context_menu.cc:447:
TrimWhitespace(params_.selection_text, TRIM_ALL, &params_.selection_text);
On 2010/04/17 17:30:40, inferno wrote:
> On 2010/04/17 17:05:09, Peter Kasting wrote:
> > Do this above the check for empty.
> 
> Yes TrimWhitespace does check for empty. plus, it is not needed as we can
> .empty() check is done 1 line above.

I wasn't asking a question, so I don't know why you're saying yes.

Do it above the check for empty because a non-empty string can become empty
after whitespace is trimmed and there is no point in going through the rest of
the function at that point.

[inferno, 2010-04-18 02:54:46]

Peter, i misread your previous comment. sorry about that. 

i fixed the code.

[Peter Kasting, 2010-04-20 18:25:31]

LGTM.  Sorry for the delay, your reply to me got spam-trapped (bad Gmail!), so I
didn't notice it.

http://codereview.chromium.org/1594039/diff/7001/2002
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/1594039/diff/7001/2002#newcode445
chrome/browser/tab_contents/render_view_context_menu.cc:445: 
Nit: You could probably remove this blank line.

[inferno, 2010-04-20 19:23:48]

Thanks Peter. I removed the that blank line before committing.