Permission bubble should display URL with appropriate language pref

Permission bubble should display URL with appropriate language pref

BUG=444586
TEST=
1. Change language to RTL language
2. Set chrome://flags/#enable-permissions-bubbles to 'Enabled'
3. Go to https://adrifelt.github.io/demos/bubble-demo.html
4. Verify that URL is displayed correctly

Committed: https://crrev.com/d5abaad2ef59ebfd6300317ee4a74512dcb1891b
Cr-Commit-Position: refs/heads/master@{#313795}

[felt, 2015-01-28 16:45:07]

felt@chromium.org changed reviewers:
+ markusheintz@chromium.org

[felt, 2015-01-28 16:46:22]

felt@chromium.org changed reviewers:
+ sky@chromium.org

[felt, 2015-01-28 16:47:14]

PTAL
sky: chrome/browser/ui/views/frame/browser_view.cc
markusheintz_:
chrome/browser/ui/views/website_settings/permissions_bubble_view.{cc|h}

[markusheintz_, 2015-01-28 20:19:55]

website_settings parts LGTM

[sky, 2015-01-28 21:28:47]

https://codereview.chromium.org/882113004/diff/1/chrome/browser/ui/views/webs...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
(right):

https://codereview.chromium.org/882113004/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:241: if
(profile)
Can profile really be null?

https://codereview.chromium.org/882113004/diff/1/chrome/browser/ui/views/webs...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right):

https://codereview.chromium.org/882113004/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/permissions_bubble_view.h:21: explicit
PermissionBubbleViewViews(views::View* anchor_view,
remove explicit.

[felt, 2015-01-28 22:31:03]

https://codereview.chromium.org/882113004/diff/1/chrome/browser/ui/views/webs...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
(right):

https://codereview.chromium.org/882113004/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:241: if
(profile)
On 2015/01/28 21:28:46, sky wrote:
> Can profile really be null?

I think likely not in a normal case; however it seems every place where this
languages check is done, they add this check, so it seems to be convention;
perhaps there is some situation in which it might happen?

https://codereview.chromium.org/882113004/diff/1/chrome/browser/ui/views/webs...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right):

https://codereview.chromium.org/882113004/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/permissions_bubble_view.h:21: explicit
PermissionBubbleViewViews(views::View* anchor_view,
On 2015/01/28 21:28:47, sky wrote:
> remove explicit.

Done.

[sky, 2015-01-29 02:48:16]

https://codereview.chromium.org/882113004/diff/1/chrome/browser/ui/views/webs...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
(right):

https://codereview.chromium.org/882113004/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:241: if
(profile)
On 2015/01/28 22:31:02, felt wrote:
> On 2015/01/28 21:28:46, sky wrote:
> > Can profile really be null?
> 
> I think likely not in a normal case; however it seems every place where this
> languages check is done, they add this check, so it seems to be convention;
> perhaps there is some situation in which it might happen?

That doesn't make it right. Either can be null, in which case the check is
valid, or it shouldn't be null. I think it's the later here, but you'll have to
investigate. I suspect a DCHECK(profile_) is what you want.

[felt, 2015-01-29 06:56:48]

https://codereview.chromium.org/882113004/diff/1/chrome/browser/ui/views/webs...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
(right):

https://codereview.chromium.org/882113004/diff/1/chrome/browser/ui/views/webs...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:241: if
(profile)
On 2015/01/29 02:48:16, sky wrote:
> On 2015/01/28 22:31:02, felt wrote:
> > On 2015/01/28 21:28:46, sky wrote:
> > > Can profile really be null?
> > 
> > I think likely not in a normal case; however it seems every place where this
> > languages check is done, they add this check, so it seems to be convention;
> > perhaps there is some situation in which it might happen?
> 
> That doesn't make it right. Either can be null, in which case the check is
> valid, or it shouldn't be null. I think it's the later here, but you'll have
to
> investigate. I suspect a DCHECK(profile_) is what you want.

I can't find any way this could be null in normal browser operation. Replaced
with a DCHECK.

[sky, 2015-01-29 17:40:44]

https://codereview.chromium.org/882113004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
(right):

https://codereview.chromium.org/882113004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:242:
profile->GetPrefs()->GetString(prefs::kAcceptLanguages);
One question though. Can PermissionBubbleViewViews take the value of
kAcceptLanguages in its constructor rather than the profile? Or might the value
change?

[felt, 2015-01-29 19:51:13]

https://codereview.chromium.org/882113004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc
(right):

https://codereview.chromium.org/882113004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:242:
profile->GetPrefs()->GetString(prefs::kAcceptLanguages);
On 2015/01/29 17:40:44, sky wrote:
> One question though. Can PermissionBubbleViewViews take the value of
> kAcceptLanguages in its constructor rather than the profile? Or might the
value
> change?

Looked into this. Changing the language will prompt a restart, so the value
should not change. Updated accordingly.

[sky, 2015-01-29 20:13:37]

LGTM

https://codereview.chromium.org/882113004/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right):

https://codereview.chromium.org/882113004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/website_settings/permissions_bubble_view.h:45:
std::string languages_;
const

[felt, 2015-01-29 20:16:58]

https://codereview.chromium.org/882113004/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right):

https://codereview.chromium.org/882113004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/website_settings/permissions_bubble_view.h:45:
std::string languages_;
On 2015/01/29 20:13:37, sky wrote:
> const

Done.

[felt, 2015-01-29 20:17:05]

The CQ bit was checked by felt@chromium.org

[commit-bot: I haz the power, 2015-01-29 20:17:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/882113004/100001

[commit-bot: I haz the power, 2015-01-29 21:23:58]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-29 21:24:55]

Patchset 6 (id:??) landed as
https://crrev.com/d5abaad2ef59ebfd6300317ee4a74512dcb1891b
Cr-Commit-Position: refs/heads/master@{#313795}