Add RenderText DirectionalityMode enum and support; etc.

Add RenderText DirectionalityMode enum and support; etc.

Add gfx::DirectionalityMode enum.
Add RenderText::SetDirectionalityMode and member.
Revise/consolidate RenderText::GetTextDirection.
Expand on existing unit tests; minor cleanup.

Consume GetTextDirection in layout initialization:
-Windows: RenderTextWin::ItemizeLogicalText()
-Linux: SetupPangoLayoutWithoutFont()
-Mac: Add TODO in RenderTextMac::EnsureLayout()

BUG=134746
TEST=Existing/updated unit tests, no behavioral changes!

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

[msw, 2012-07-26 20:43:21]

Hey Alexei, please take a look; thanks!

[Alexei Svitkine (slow), 2012-07-26 23:23:10]

Can you delete Label::SetURL() in a separate CL? This one is already pretty
crowded.

[msw, 2012-07-26 23:45:38]

Done, I'll make a separate CL for that.

I also fixed tests by using GetDisplayText instead of text() for determining
directionality, as this is on par with existing behavior (though conceptually,
perhaps password textfields should match UI directionality?).

Please take another look; thanks!

[Alexei Svitkine (slow), 2012-07-27 11:40:35]

I'm travelling today, so I don't have time to do a thorough review yet, but here
are some initial comments.

http://codereview.chromium.org/10807082/diff/9004/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/10807082/diff/9004/ui/gfx/render_text.h#newcode98
ui/gfx/render_text.h:98: CHARACTER_BREAK = 0,
Does our style guide require this?

http://codereview.chromium.org/10807082/diff/9004/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:112: DERIVE_FROM_TEXT = 0, // Use the first strong
character's direction.
This is not a good name for an enum entry in the gfx namespace.

gfx::DERIVE_FROM_TEXT doesn't tell me what it's talking about (what is it
deriving from the text?).

I suggest prefixing these names with DIRECTIONALITY_*.

I also dislike that these enums are in render_text.h. Code that uses e.g. Label
shouldn't know about RenderText and include its header.

How about making a gfx_constants.h header file and moving these three enums
there?

[msw, 2012-07-28 01:17:10]

Done, and I expanded on unit tests, please take a look as you have time, thanks!

https://chromiumcodereview.appspot.com/10807082/diff/9004/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://chromiumcodereview.appspot.com/10807082/diff/9004/ui/gfx/render_text....
ui/gfx/render_text.h:98: CHARACTER_BREAK = 0,
On 2012/07/27 11:40:36, Alexei Svitkine wrote:
> Does our style guide require this?

Not explicitly, but the Google C++ style guide shows both enum examples
specifying at least the first value. The Chromium style guide only shows one
enum example (wrt namespaces) and doesn't supply a value. I think it's optional,
but beneficial; I'll remove it if you disagree.

https://chromiumcodereview.appspot.com/10807082/diff/9004/ui/gfx/render_text....
ui/gfx/render_text.h:112: DERIVE_FROM_TEXT = 0, // Use the first strong
character's direction.
On 2012/07/27 11:40:36, Alexei Svitkine wrote:
> This is not a good name for an enum entry in the gfx namespace.
> 
> gfx::DERIVE_FROM_TEXT doesn't tell me what it's talking about (what is it
> deriving from the text?).
> 
> I suggest prefixing these names with DIRECTIONALITY_*.
> 
> I also dislike that these enums are in render_text.h. Code that uses e.g.
Label
> shouldn't know about RenderText and include its header.
> 
> How about making a gfx_constants.h header file and moving these three enums
> there?

Done (ui/gfx/text_constants.h and "DIRECTIONALITY_*" w/o "DERIVE_").

[msw, 2012-07-28 01:18:45]

Xiaomei, please take a look too; thanks!

[xji, 2012-07-30 18:32:16]

Very nice!

http://codereview.chromium.org/10807082/diff/8016/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

http://codereview.chromium.org/10807082/diff/8016/ui/gfx/canvas_skia.cc#newco...
ui/gfx/canvas_skia.cc:35:
render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_RTL);
this is DIRECTIONALITY_FROM_UI.

we probably should not do anything for this case and use render_text's default
value (either DIRECTIONALITY_FROM_UI, or DIRECTIONALTY_FROM_TEXT).

http://codereview.chromium.org/10807082/diff/8016/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

http://codereview.chromium.org/10807082/diff/8016/ui/views/controls/label.cc#...
ui/views/controls/label.cc:109: (directionality_mode_ ==
gfx::DIRECTIONALITY_FROM_UI))
I can see that you are trying to re-use gfx::DirectionalityMode, but I feel the
name is pretty confusing here (at least I was confused at first).

The gfx::DIRECTIONALITY_FROM_UI/TEXT here is used to control whether the
alignment is going to be flipped or not (or whether the label's
directionality_mode is a forced one or not).
But in RenderText, in the current implementation, DIRECTIONALITY_FROM_TEXT is
the default, DIRECTIONALITY_FROM_UI means text directionality is based on UI.

the combinations are as following:
Label's DIRECTIONALITY_FROM_UI --> (falls to) RenderText's
DIRECTIONALTY_FROM_TEXT.
Label's DIRECTIONALITY_FROM_TEXT --> (falls to) RenderText's
DIRECTIONALITY_FORCE_xxx.

[xji, 2012-07-30 19:00:04]

http://codereview.chromium.org/10807082/diff/8016/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

http://codereview.chromium.org/10807082/diff/8016/ui/views/controls/label.cc#...
ui/views/controls/label.cc:438: const base::i18n::TextDirection direction =

if we introduce gfx::Canvas::DIRECTIONALITY_FROM_TEXT, then, we do not need the
computation here. we can just do: 
flags |= gfx::Canvas::DIRECTIONALITY_FROM_TEXT

And in "bool AdjustStringDirection()" in canvas_skia.cc,
we need to add:

else if (flags & gfx::Canvas::DIRECTIONALITY_FROM_TEXT)
  render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FROM_TEXT);


So far, there is no real usage of FORCE directionality (they are actually the
current DIRECTIONALITY_FROM_TEXT. They were introduced before is because we
previously use DIRECTIONALITY_FROM_UI as default).
But I think the forced directionality might be useful later for the title page
case (http://code.google.com/p/chromium/issues/detail?id=27094).

[Alexei Svitkine (slow), 2012-07-30 22:29:16]

Sorry, I still need to find time to review this in full. But here's some nits
for now.

http://codereview.chromium.org/10807082/diff/8016/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

http://codereview.chromium.org/10807082/diff/8016/ui/gfx/render_text_linux.cc...
ui/gfx/render_text_linux.cc:292: ((GetTextDirection() ==
base::i18n::RIGHT_TO_LEFT) ?
Nit: Too many parentheses. Suggest omitting the inner ones around the cond.

http://codereview.chromium.org/10807082/diff/8016/ui/gfx/render_text_unittest.cc
File ui/gfx/render_text_unittest.cc (right):

http://codereview.chromium.org/10807082/diff/8016/ui/gfx/render_text_unittest...
ui/gfx/render_text_unittest.cc:413: // Ensure that directionality modes yeild
the correct text directions.
NIt: yield misspelled.

http://codereview.chromium.org/10807082/diff/8016/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/10807082/diff/8016/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:597: // Set the base text direction yeilded by the
directionality mode.
Nit: yielded. Although, the comment isn't really correct.

In this case, you're not looking at the directionality mode (that's an
implementation detail of GetTextDirection()).

Perhaps just: "Set Uniscribe text direction." or even omit entirely.

http://codereview.chromium.org/10807082/diff/8016/ui/gfx/text_constants.h
File ui/gfx/text_constants.h (right):

http://codereview.chromium.org/10807082/diff/8016/ui/gfx/text_constants.h#new...
ui/gfx/text_constants.h:14: CHARACTER_BREAK = 0, // Stop cursor movement on
neighboring characters.
Nit: Please put comments on the preceding line. I think there's a preference for
that style in gfx/ - I know sky has given that comment to me before.

http://codereview.chromium.org/10807082/diff/8016/ui/views/controls/label.h
File ui/views/controls/label.h (right):

http://codereview.chromium.org/10807082/diff/8016/ui/views/controls/label.h#n...
ui/views/controls/label.h:265: // first strong directionality character or from
the UI's locale.
Could also be forced right? Perhaps the comment doesn't need to go into so much
detail.

[msw, 2012-07-31 03:03:06]

I addressed the comments and reverted the Label and MessageBoxView changes (I'll
follow up in a separate CL).
Please take another look as you have time; thanks!

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/gfx/canvas_skia....
ui/gfx/canvas_skia.cc:35:
render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_RTL);
On 2012/07/30 18:32:16, xji wrote:
> this is DIRECTIONALITY_FROM_UI.
> 
> we probably should not do anything for this case and use render_text's default
> value (either DIRECTIONALITY_FROM_UI, or DIRECTIONALTY_FROM_TEXT).

Changed to DIRECTIONALITY_FROM_UI for now (to avoid any behavioral change),
since this is actually checking if the string contains any strong RTL
characters, not if the string *starts* with strong RTL characters.

We can follow up later if we want a behavior change to use the default,
DIRECTIONALITY_FROM_TEXT (which would make these strings that start with LTR but
contain RTL actually show as LTR).

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/gfx/render_text_...
File ui/gfx/render_text_linux.cc (right):

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/gfx/render_text_...
ui/gfx/render_text_linux.cc:292: ((GetTextDirection() ==
base::i18n::RIGHT_TO_LEFT) ?
On 2012/07/30 22:29:16, Alexei Svitkine wrote:
> Nit: Too many parentheses. Suggest omitting the inner ones around the cond.

Done.

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/gfx/render_text_...
File ui/gfx/render_text_unittest.cc (right):

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/gfx/render_text_...
ui/gfx/render_text_unittest.cc:413: // Ensure that directionality modes yeild
the correct text directions.
On 2012/07/30 22:29:16, Alexei Svitkine wrote:
> NIt: yield misspelled.

Done.

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/gfx/render_text_...
File ui/gfx/render_text_win.cc (right):

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/gfx/render_text_...
ui/gfx/render_text_win.cc:597: // Set the base text direction yeilded by the
directionality mode.
On 2012/07/30 22:29:16, Alexei Svitkine wrote:
> Nit: yielded. Although, the comment isn't really correct.
> 
> In this case, you're not looking at the directionality mode (that's an
> implementation detail of GetTextDirection()).
> 
> Perhaps just: "Set Uniscribe text direction." or even omit entirely.

Done ("Set Uniscribe's base text direction."), similar for Pango.

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/gfx/text_constan...
File ui/gfx/text_constants.h (right):

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/gfx/text_constan...
ui/gfx/text_constants.h:14: CHARACTER_BREAK = 0, // Stop cursor movement on
neighboring characters.
On 2012/07/30 22:29:16, Alexei Svitkine wrote:
> Nit: Please put comments on the preceding line. I think there's a preference
for
> that style in gfx/ - I know sky has given that comment to me before.

Done.

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/views/controls/l...
File ui/views/controls/label.cc (right):

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/views/controls/l...
ui/views/controls/label.cc:109: (directionality_mode_ ==
gfx::DIRECTIONALITY_FROM_UI))
On 2012/07/30 18:32:16, xji wrote:
> I can see that you are trying to re-use gfx::DirectionalityMode, but I feel
the
> name is pretty confusing here (at least I was confused at first).
> 
> The gfx::DIRECTIONALITY_FROM_UI/TEXT here is used to control whether the
> alignment is going to be flipped or not (or whether the label's
> directionality_mode is a forced one or not).
> But in RenderText, in the current implementation, DIRECTIONALITY_FROM_TEXT is
> the default, DIRECTIONALITY_FROM_UI means text directionality is based on UI.
> 
> the combinations are as following:
> Label's DIRECTIONALITY_FROM_UI --> (falls to) RenderText's
> DIRECTIONALTY_FROM_TEXT.
> Label's DIRECTIONALITY_FROM_TEXT --> (falls to) RenderText's
> DIRECTIONALITY_FORCE_xxx.

You're right; Label doesn't currently support gfx::DIRECTIONALITY_FROM_UI.
I reverted this portion of the CL and will followup in a separate CL.

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/views/controls/l...
ui/views/controls/label.cc:438: const base::i18n::TextDirection direction =
On 2012/07/30 19:00:04, xji wrote:
> 
> if we introduce gfx::Canvas::DIRECTIONALITY_FROM_TEXT, then, we do not need
the
> computation here. we can just do: 
> flags |= gfx::Canvas::DIRECTIONALITY_FROM_TEXT
> 
> And in "bool AdjustStringDirection()" in canvas_skia.cc,
> we need to add:
> 
> else if (flags & gfx::Canvas::DIRECTIONALITY_FROM_TEXT)
>   render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FROM_TEXT);
> 
> 
> So far, there is no real usage of FORCE directionality (they are actually the
> current DIRECTIONALITY_FROM_TEXT. They were introduced before is because we
> previously use DIRECTIONALITY_FROM_UI as default).
> But I think the forced directionality might be useful later for the title page
> case (http://code.google.com/p/chromium/issues/detail?id=27094). 

I've reverted these changes in this CL and I'll follup in a separate CL.
(DIRECTIONALITY_FROM_TEXT is the default underlying behavior and this class
currently doesn't actually propagate DIRECTIONALITY_FROM_UI, so this block can
be removed altogether).

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/views/controls/l...
File ui/views/controls/label.h (right):

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/views/controls/l...
ui/views/controls/label.h:265: // first strong directionality character or from
the UI's locale.
On 2012/07/30 22:29:16, Alexei Svitkine wrote:
> Could also be forced right? Perhaps the comment doesn't need to go into so
much
> detail.

Reverted this portion of the CL; will address in a followup CL.

[xji, 2012-07-31 23:03:25]

http://codereview.chromium.org/10807082/diff/8016/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

http://codereview.chromium.org/10807082/diff/8016/ui/gfx/canvas_skia.cc#newco...
ui/gfx/canvas_skia.cc:35:
render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_RTL);
On 2012/07/31 03:03:06, msw wrote:
> On 2012/07/30 18:32:16, xji wrote:
> > this is DIRECTIONALITY_FROM_UI.
> > 
> > we probably should not do anything for this case and use render_text's
default
> > value (either DIRECTIONALITY_FROM_UI, or DIRECTIONALTY_FROM_TEXT).
> 
> Changed to DIRECTIONALITY_FROM_UI for now (to avoid any behavioral change),
> since this is actually checking if the string contains any strong RTL
> characters, not if the string *starts* with strong RTL characters.
> 
> We can follow up later if we want a behavior change to use the default,
> DIRECTIONALITY_FROM_TEXT (which would make these strings that start with LTR
but
> contain RTL actually show as LTR).

are we already introducing behavior changes in windows?
previously, in LTR UI, strings were always in LTR direction.
Now, their direction is based on first character's.

then, why we want to keep the behavior in RTL UI unchanged?
to be consistent, should the string's direction also based on its first
character's?

And from Aharon's comment in
http://code.google.com/p/chromium/issues/detail?id=134746#c13, looks like this
behavior change is ok.

[Alexei Svitkine (slow), 2012-07-31 23:33:29]

Here some more comments.

Please be sure to test RTL and BiDi UI text with this change in both LTR and RTL
UIs.

http://codereview.chromium.org/10807082/diff/15036/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

http://codereview.chromium.org/10807082/diff/15036/ui/gfx/canvas_skia.cc#newc...
ui/gfx/canvas_skia.cc:23: // Returns true if the text will be rendered with an
RTL base direction.
Remove the return value here and use GetTextDirection() in callers.

That will also do the right thing in the default case (notice that at this
point, we haven't yet set the text on the render_text).

http://codereview.chromium.org/10807082/diff/15036/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

http://codereview.chromium.org/10807082/diff/15036/ui/gfx/render_text.cc#newc...
ui/gfx/render_text.cc:400: text_direction_ = base::i18n::UNKNOWN_DIRECTION;
Do the unit tests catch if this is omitted?

http://codereview.chromium.org/10807082/diff/15036/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

http://codereview.chromium.org/10807082/diff/15036/ui/gfx/render_text.h#newco...
ui/gfx/render_text.h:430: // The cached text direction, potentially computed
from the text or UI locale.
Mention that it shouldn't be used directly but through :GetTextDirection().

http://codereview.chromium.org/10807082/diff/15036/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

http://codereview.chromium.org/10807082/diff/15036/ui/gfx/render_text_linux.c...
ui/gfx/render_text_linux.cc:291:
pango_context_set_base_dir(pango_layout_get_context(layout_),
Should this be done in SetupPangoLayoutWithFontDescription()?

[msw, 2012-08-01 17:30:10]

Comments addressed, pics attached at http://crbug.com/134746
Please take another look; thanks!

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://chromiumcodereview.appspot.com/10807082/diff/8016/ui/gfx/canvas_skia....
ui/gfx/canvas_skia.cc:35:
render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_FORCE_RTL);
On 2012/07/31 23:03:25, xji wrote:
> On 2012/07/31 03:03:06, msw wrote:
> > On 2012/07/30 18:32:16, xji wrote:
> > > this is DIRECTIONALITY_FROM_UI.
> > > 
> > > we probably should not do anything for this case and use render_text's
> default
> > > value (either DIRECTIONALITY_FROM_UI, or DIRECTIONALTY_FROM_TEXT).
> > 
> > Changed to DIRECTIONALITY_FROM_UI for now (to avoid any behavioral change),
> > since this is actually checking if the string contains any strong RTL
> > characters, not if the string *starts* with strong RTL characters.
> > 
> > We can follow up later if we want a behavior change to use the default,
> > DIRECTIONALITY_FROM_TEXT (which would make these strings that start with LTR
> but
> > contain RTL actually show as LTR).
> 
> are we already introducing behavior changes in windows?
> previously, in LTR UI, strings were always in LTR direction.
> Now, their direction is based on first character's.
> 
> then, why we want to keep the behavior in RTL UI unchanged?
> to be consistent, should the string's direction also based on its first
> character's?
> 
> And from Aharon's comment in
> http://code.google.com/p/chromium/issues/detail?id=134746#c13, looks like this
> behavior change is ok.

Done. I made the recommended change (which amounts to removing this block and
defaulting to FROM_TEXT). One case might be negatively effected (but seems ok):
An RTL string in RTL UI with gfx::Canvas::FORCE_LTR_DIRECTIONALITY will now be
LTR (this code previously ignored the forced override and used RTL anyway).

https://chromiumcodereview.appspot.com/10807082/diff/15036/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://chromiumcodereview.appspot.com/10807082/diff/15036/ui/gfx/canvas_skia...
ui/gfx/canvas_skia.cc:23: // Returns true if the text will be rendered with an
RTL base direction.
On 2012/07/31 23:33:29, Alexei Svitkine wrote:
> Remove the return value here and use GetTextDirection() in callers.
> 
> That will also do the right thing in the default case (notice that at this
> point, we haven't yet set the text on the render_text).

Good point! Done.

https://chromiumcodereview.appspot.com/10807082/diff/15036/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://chromiumcodereview.appspot.com/10807082/diff/15036/ui/gfx/render_text...
ui/gfx/render_text.cc:400: text_direction_ = base::i18n::UNKNOWN_DIRECTION;
On 2012/07/31 23:33:29, Alexei Svitkine wrote:
> Do the unit tests catch if this is omitted?

I don't think tests need to ensure that implementation detail, but I added a
case at the end of RenderTextTest.GetTextDirection ensuring that SetText updates
the direction in this mode.

https://chromiumcodereview.appspot.com/10807082/diff/15036/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://chromiumcodereview.appspot.com/10807082/diff/15036/ui/gfx/render_text...
ui/gfx/render_text.h:430: // The cached text direction, potentially computed
from the text or UI locale.
On 2012/07/31 23:33:29, Alexei Svitkine wrote:
> Mention that it shouldn't be used directly but through :GetTextDirection().

Done.

https://chromiumcodereview.appspot.com/10807082/diff/15036/ui/gfx/render_text...
File ui/gfx/render_text_linux.cc (right):

https://chromiumcodereview.appspot.com/10807082/diff/15036/ui/gfx/render_text...
ui/gfx/render_text_linux.cc:291:
pango_context_set_base_dir(pango_layout_get_context(layout_),
On 2012/07/31 23:33:29, Alexei Svitkine wrote:
> Should this be done in SetupPangoLayoutWithFontDescription()?

Good idea! Moved lower to SetupPangoLayoutWithoutFont.

[msw, 2012-08-01 18:28:56]

I removed the behavioral changes to simplify this CL. Please take a look.

[Alexei Svitkine (slow), 2012-08-01 18:29:13]

LGTM with a possible nit.

https://chromiumcodereview.appspot.com/10807082/diff/14006/ui/gfx/text_consta...
File ui/gfx/text_constants.h (right):

https://chromiumcodereview.appspot.com/10807082/diff/14006/ui/gfx/text_consta...
ui/gfx/text_constants.h:1: 
Nit: Unnecessary blank line?

[msw, 2012-08-01 18:48:31]

Done. +Ben for OWNERS review; please take a look, thanks!

https://chromiumcodereview.appspot.com/10807082/diff/14006/ui/gfx/text_consta...
File ui/gfx/text_constants.h (right):

https://chromiumcodereview.appspot.com/10807082/diff/14006/ui/gfx/text_consta...
ui/gfx/text_constants.h:1: 
On 2012/08/01 18:29:13, Alexei Svitkine wrote:
> Nit: Unnecessary blank line?

Done.

[Ben Goodger (Google), 2012-08-01 19:08:34]

owner lgtm

[xji, 2012-08-01 19:54:57]

lgtm

[commit-bot: I haz the power, 2012-08-01 20:26:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/10807082/22009

[commit-bot: I haz the power, 2012-08-01 21:57:32]

Change committed as 149503