Content Settings bubble does not display RTL text. This is the case because we

Content Settings bubble does not display RTL text. This is the case because we
manually draw the string in the bubble without taking RTL into account. We fix
this by using a views::Label instead that takes care of RTL when drawing text.

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

[Peter Kasting, 2012-09-19 21:32:30]

LGTM

https://chromiumcodereview.appspot.com/10949024/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc
(right):

https://chromiumcodereview.appspot.com/10949024/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc:128:
RemoveChildView(text_label_);  // will also delete the view.
Why is this necessary?

https://chromiumcodereview.appspot.com/10949024/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc:188:
(slide_animator_->is_animating() || pause_animation_))) {
Nit: {} not necessary

https://chromiumcodereview.appspot.com/10949024/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_decoration_view.h
(right):

https://chromiumcodereview.appspot.com/10949024/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.h:103:
views::Label* text_label_;
Tiny nit: Putting these child views just after the parent view seems a little
more logical

[varunjain, 2012-09-19 21:45:34]

https://chromiumcodereview.appspot.com/10949024/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc
(right):

https://chromiumcodereview.appspot.com/10949024/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc:128:
RemoveChildView(text_label_);  // will also delete the view.
On 2012/09/19 21:32:30, Peter Kasting wrote:
> Why is this necessary?

Changed it to a bit nicer.. it is necessary, so that the label is not added
twice when StartLabelAnimation() is called again (for example, when the user
refreshes the page while the label is still added).

https://chromiumcodereview.appspot.com/10949024/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc:188:
(slide_animator_->is_animating() || pause_animation_))) {
On 2012/09/19 21:32:30, Peter Kasting wrote:
> Nit: {} not necessary

Done.

https://chromiumcodereview.appspot.com/10949024/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_decoration_view.h
(right):

https://chromiumcodereview.appspot.com/10949024/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.h:103:
views::Label* text_label_;
On 2012/09/19 21:32:30, Peter Kasting wrote:
> Tiny nit: Putting these child views just after the parent view seems a little
> more logical

Done.

[commit-bot: I haz the power, 2012-09-19 21:47:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/10949024/7001

[commit-bot: I haz the power, 2012-09-19 21:47:07]

Failed to apply patch for
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/ui/views/location_bar/content_setting_image_view.cc
Hunk #1 FAILED at 46.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/views/location_bar/content_setting_image_view.cc.rej

[commit-bot: I haz the power, 2012-09-19 22:11:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/10949024/8001

[commit-bot: I haz the power, 2012-09-20 00:53:34]

Change committed as 157675