Display origin url on the chooser permission dialog title

Display origin url on the chooser permission dialog title

Instead of showing "This site wants to connect to:", the chooser
permission dialog title needs to replace "This site" with the actual
URL string of the site which requests the permission from the user.

BUG=604031, 529950
Committed: https://crrev.com/2245f138687425230a9d8275b3e67dac11da4944
Cr-Commit-Position: refs/heads/master@{#388045}

[juncai, 2016-04-15 22:11:14]

juncai@chromium.org changed reviewers:
+ palmer@chromium.org

[juncai, 2016-04-15 22:11:14]

Please take a look.

[palmer, 2016-04-18 18:28:10]

Thanks for doing this! Some UX and implementation thoughts.

https://codereview.chromium.org/1892293003/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1892293003/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:14598: +        <ph
name="URL">$1<ex>https://www.google.com</ex></ph> wants to connect to:
I would name this |Origin| instead of |URL|, since we don't want to show the
whole URL.

Also, this might be one of those places in which only secure origins will ever
show. Is that right? If so, you can (and, I think, should) leave off the
"https://". Thus the example string could/should be "www.google.com".

See
https://www.chromium.org/Home/chromium-security/enamel#TOC-Presenting-Origins
for more.

https://codereview.chromium.org/1892293003/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/chooser_bubble_controller.cc (right):

https://codereview.chromium.org/1892293003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/chooser_bubble_controller.cc:21: return
const_cast<content::RenderFrameHost*>(owning_frame_)
Is this const_cast necessary? If so, why? I think it's because
|GetLastCommittedURL| (and GetLastCommittedOrigin) are not declared const? I
wonder if that could be changed/refactored?

https://codereview.chromium.org/1892293003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/chooser_bubble_controller.cc:22:
->GetLastCommittedURL();
Try GetLastCommittedOrigin.

https://codereview.chromium.org/1892293003/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/chooser_bubble_controller.h (right):

https://codereview.chromium.org/1892293003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/chooser_bubble_controller.h:57: GURL
GetOriginUrl() const;
I think it might make more conceptual sense to return a url::Origin, or 
std::string (url::Origin::Serialize) than a GURL.

[juncai, 2016-04-18 19:43:24]

https://codereview.chromium.org/1892293003/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1892293003/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:14598: +        <ph
name="URL">$1<ex>https://www.google.com</ex></ph> wants to connect to:
On 2016/04/18 18:28:10, palmer wrote:
> I would name this |Origin| instead of |URL|, since we don't want to show the
> whole URL.
> 
> Also, this might be one of those places in which only secure origins will ever
> show. Is that right? If so, you can (and, I think, should) leave off the
> "https://". Thus the example string could/should be "www.google.com".
> 
> See
> https://www.chromium.org/Home/chromium-security/enamel#TOC-Presenting-Origins
> for more.

Yes, only secure origins are shown on the title.

Done.

https://codereview.chromium.org/1892293003/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/chooser_bubble_controller.cc (right):

https://codereview.chromium.org/1892293003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/chooser_bubble_controller.cc:21: return
const_cast<content::RenderFrameHost*>(owning_frame_)
On 2016/04/18 18:28:10, palmer wrote:
> Is this const_cast necessary? If so, why? I think it's because
> |GetLastCommittedURL| (and GetLastCommittedOrigin) are not declared const? I
> wonder if that could be changed/refactored?

Yes, it is necessary because GetLastCommittedOrigin() is not declared const. And
I asked this question on the chromium dev forum, they pointed out the
documentation:
http://www.chromium.org/developers/content-module/content-api
"don't add the const identifier to interfaces. For interfaces implemented by the
embedder, we can't make assumptions about what the embedder needs to implement
it. For interfaces implemented by content, the implementation details doesn't
have to be exposed."

https://codereview.chromium.org/1892293003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/chooser_bubble_controller.cc:22:
->GetLastCommittedURL();
On 2016/04/18 18:28:10, palmer wrote:
> Try GetLastCommittedOrigin.

Done.

https://codereview.chromium.org/1892293003/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/chooser_bubble_controller.h (right):

https://codereview.chromium.org/1892293003/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/chooser_bubble_controller.h:57: GURL
GetOriginUrl() const;
On 2016/04/18 18:28:10, palmer wrote:
> I think it might make more conceptual sense to return a url::Origin, or 
> std::string (url::Origin::Serialize) than a GURL.

Done.

[palmer, 2016-04-18 21:14:00]

LGTM % nit.

https://codereview.chromium.org/1892293003/diff/60001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/chooser_bubble_controller.h (right):

https://codereview.chromium.org/1892293003/diff/60001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/chooser_bubble_controller.h:61: url::Origin
GetOriginUrl() const;
Nit: I'd probably name this simply |GetOrigin|.

[juncai, 2016-04-18 21:20:36]

https://codereview.chromium.org/1892293003/diff/60001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/chooser_bubble_controller.h (right):

https://codereview.chromium.org/1892293003/diff/60001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/chooser_bubble_controller.h:61: url::Origin
GetOriginUrl() const;
On 2016/04/18 21:13:59, palmer wrote:
> Nit: I'd probably name this simply |GetOrigin|.

Done.

[juncai, 2016-04-18 21:20:48]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-04-18 21:20:48]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1892293003/#ps80001
(title: "address palmer@'s comment")

[commit-bot: I haz the power, 2016-04-18 21:21:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1892293003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1892293003/80001

[commit-bot: I haz the power, 2016-04-18 22:19:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-18 22:22:07]

Description was changed from

==========
Display origin url on the chooser permission dialog title

Instead of showing "This site wants to connect to:", the chooser
permission dialog title needs to replace "This site" with the actual
URL string of the site which requests the permission from the user.

BUG=604031, 529950
==========

to

==========
Display origin url on the chooser permission dialog title

Instead of showing "This site wants to connect to:", the chooser
permission dialog title needs to replace "This site" with the actual
URL string of the site which requests the permission from the user.

BUG=604031, 529950

Committed: https://crrev.com/2245f138687425230a9d8275b3e67dac11da4944
Cr-Commit-Position: refs/heads/master@{#388045}
==========

[commit-bot: I haz the power, 2016-04-18 22:22:08]

Patchset 5 (id:??) landed as
https://crrev.com/2245f138687425230a9d8275b3e67dac11da4944
Cr-Commit-Position: refs/heads/master@{#388045}