Ignore title parameter for navigator.registerProtocolHandler.

Ignore title parameter for navigator.registerProtocolHandler

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

[meacer, 2014-05-06 17:28:03]

jochen: Can you please review?

This is the current dialog:
https://drive.google.com/file/d/0B9q2eN9gDoUIZzQ0VHJfcTZIeDg/edit?usp=sharing

[jochen (gone - plz use gerrit), 2014-05-06 17:47:38]

deferring to scott

[sky, 2014-05-06 18:24:07]

Sorry to pass the buck, but I'm not sure of the effects of this. Maybe creis?

[jochen (gone - plz use gerrit), 2014-05-06 18:26:28]

the cl looks good from point of view of code, but i'm not sure whether the UI
change is desired (as opposed to properly sanitizing the title)

[meacer, 2014-05-06 18:36:38]

On 2014/05/06 18:26:28, jochen wrote:
> the cl looks good from point of view of code, but i'm not sure whether the UI
> change is desired (as opposed to properly sanitizing the title)

The effect is only a UI change, the title param is only used as part of the UI
in the screenshot.

[jochen (gone - plz use gerrit), 2014-05-06 18:38:30]

https://codereview.chromium.org/267103002/diff/20001/chrome/browser/custom_ha...
File
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc
(left):

https://codereview.chromium.org/267103002/diff/20001/chrome/browser/custom_ha...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc:76:
handler_.title(), base::UTF8ToUTF16(handler_.url().host()),
is title() used anywhere else? Otherwise, you should rip it out entirely

[meacer, 2014-05-06 18:46:49]

https://codereview.chromium.org/267103002/diff/20001/chrome/browser/custom_ha...
File
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc
(left):

https://codereview.chromium.org/267103002/diff/20001/chrome/browser/custom_ha...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc:76:
handler_.title(), base::UTF8ToUTF16(handler_.url().host()),
On 2014/05/06 18:38:30, jochen wrote:
> is title() used anywhere else? Otherwise, you should rip it out entirely

I was actually looking at it now: The content settings page uses the title
instead of the url under the "Site" column. mailto and webcal are initiated as
"Google.com Mail" and "Google Calendar" on Chrome OS. Of course nothing prevents
another site from using the same names.

[meacer, 2014-05-06 21:29:30]

Here are the screenshots after removing all references to |title| in the browser
side:
https://docs.google.com/document/d/1CA7lmXBjMixWGTPAA7dwOXhxFpgBqiIKjUxNuoFy5...

Gmail and Google Calendar are set as default protocol handlers on Chrome OS, so
the content setting dialog will now show the host names rather than titles.

[Charlie Reis, 2014-05-07 00:13:58]

On 2014/05/06 21:29:30, Mustafa Emre Acer wrote:
> Here are the screenshots after removing all references to |title| in the
browser
> side:
>
https://docs.google.com/document/d/1CA7lmXBjMixWGTPAA7dwOXhxFpgBqiIKjUxNuoFy5...
> 
> Gmail and Google Calendar are set as default protocol handlers on Chrome OS,
so
> the content setting dialog will now show the host names rather than titles.

It's certainly less spoofable to omit the title parameter entirely, and I like
that.  However, it also seems like it breaks the intended behavior of the
feature (as opposed to sanitizing the string and showing both the
attacker-controlled title and the origin).  That means you should probably get
UI approval for the change.

[meacer, 2014-05-08 18:23:19]

On 2014/05/07 00:13:58, Charlie Reis wrote:
> On 2014/05/06 21:29:30, Mustafa Emre Acer wrote:
> > Here are the screenshots after removing all references to |title| in the
> browser
> > side:
> >
>
https://docs.google.com/document/d/1CA7lmXBjMixWGTPAA7dwOXhxFpgBqiIKjUxNuoFy5...
> > 
> > Gmail and Google Calendar are set as default protocol handlers on Chrome OS,
> so
> > the content setting dialog will now show the host names rather than titles.
> 
> It's certainly less spoofable to omit the title parameter entirely, and I like
> that.  However, it also seems like it breaks the intended behavior of the
> feature (as opposed to sanitizing the string and showing both the
> attacker-controlled title and the origin).  That means you should probably get
> UI approval for the change.

OK I got approval from Glen for this.

[Charlie Reis, 2014-05-08 22:39:46]

Thanks.  The two files in content/ LGTM with one nit.

https://codereview.chromium.org/267103002/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/267103002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2601: const base::string16&,
Style nit: Please leave the parameter name:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
"All parameters should be named, with identical names in the declaration and
implementation."

[sky, 2014-05-08 23:36:54]

LGTM

[meacer, 2014-05-09 01:16:40]

https://codereview.chromium.org/267103002/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/267103002/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2601: const base::string16&,
On 2014/05/08 22:39:46, Charlie Reis wrote:
> Style nit: Please leave the parameter name:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> "All parameters should be named, with identical names in the declaration and
> implementation."

Done.

[meacer, 2014-05-09 01:16:54]

The CQ bit was checked by meacer@chromium.org

[commit-bot: I haz the power, 2014-05-09 01:25:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/meacer@chromium.org/267103002/80001

[commit-bot: I haz the power, 2014-05-09 05:01:19]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-09 05:15:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-09 05:15:49]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[meacer, 2014-05-09 23:57:37]

FYI, I rebased and fixed the tests.

[meacer, 2014-05-10 18:00:20]

The CQ bit was checked by meacer@chromium.org

[commit-bot: I haz the power, 2014-05-10 18:05:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/meacer@chromium.org/267103002/100001

[commit-bot: I haz the power, 2014-05-10 20:39:09]

Change committed as 269630