Add and use a base::i18n::StringWithDirection for carrying titles.

Add and use a base::i18n::StringWithDirection for carrying titles.

This is a refactoring of r82400.  We're going to need the title
direction in a bunch of different places; it's better to package
it up as one object.

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

[Evan Martin, 2011-04-21 00:01:48]

Darin,

I realized I'm gonna need to store the title+direction all over the place, like
in navigation entries, the tab structures, history... better to create an object
so it's easier to get/set.

[Evan Martin, 2011-04-21 00:02:11]

+avi FYI

[darin (slow to review), 2011-04-21 02:55:59]

I assume you'll now go ahead and add WebStringWithDirection with implicit
conversion to base::i18n::String16WithDirection to reduce friction?? :)

http://codereview.chromium.org/6878089/diff/1/base/i18n/rtl.h
File base/i18n/rtl.h (right):

http://codereview.chromium.org/6878089/diff/1/base/i18n/rtl.h#newcode37
base/i18n/rtl.h:37: class StringWithDirection {
String16WithDirection maybe?  see NullableString16 and StringPiece

http://codereview.chromium.org/6878089/diff/1/base/i18n/rtl.h#newcode43
base/i18n/rtl.h:43: string16 string;
do you need to mutate these later?  how about just providing const accessors
like NullableString16 to enforce RAII?

http://codereview.chromium.org/6878089/diff/1/chrome/browser/notifications/ba...
File chrome/browser/notifications/balloon_host.h (right):

http://codereview.chromium.org/6878089/diff/1/chrome/browser/notifications/ba...
chrome/browser/notifications/balloon_host.h:62: const
base::i18n::StringWithDirection& title) OVERRIDE {}
so much namespacing :(

[Evan Martin, 2011-04-21 17:56:34]

I made your suggested changes.

[darin (slow to review), 2011-04-21 18:03:30]

http://codereview.chromium.org/6878089/diff/4002/base/i18n/rtl.h
File base/i18n/rtl.h (right):

http://codereview.chromium.org/6878089/diff/4002/base/i18n/rtl.h#newcode46
base/i18n/rtl.h:46: bool empty() const { return string_.empty(); }
in chromium, we generally add a verb to methods like this:  "is_empty" or
"IsEmpty"... but maybe you are trying to make this look like std::string?

http://codereview.chromium.org/6878089/diff/4002/base/i18n/rtl.h#newcode48
base/i18n/rtl.h:48: string16 string_;
nit: these should be marked private.

http://codereview.chromium.org/6878089/diff/4002/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/6878089/diff/4002/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.h:54: class
String16WithDirection;
this forward declaration seems unnecessary since String16WithDirection does not
get mentioned in this header file.

[Evan Martin, 2011-04-21 18:11:52]

PTAL

http://codereview.chromium.org/6878089/diff/4002/base/i18n/rtl.h
File base/i18n/rtl.h (right):

http://codereview.chromium.org/6878089/diff/4002/base/i18n/rtl.h#newcode46
base/i18n/rtl.h:46: bool empty() const { return string_.empty(); }
On 2011/04/21 18:03:30, darin wrote:
> in chromium, we generally add a verb to methods like this:  "is_empty" or
> "IsEmpty"... but maybe you are trying to make this look like std::string?

In the equivalent WebKit review, I tried to add no methods and my reviewer
wanted me to make the methods match string, so I was attempting to head that off
here.  I will just remove it if you don't mind.

[darin (slow to review), 2011-04-21 20:16:03]

On Thu, Apr 21, 2011 at 11:11 AM, <evan@chromium.org> wrote:

> PTAL
>
>
>
> http://codereview.chromium.org/6878089/diff/4002/base/i18n/rtl.h
> File base/i18n/rtl.h (right):
>
> http://codereview.chromium.org/6878089/diff/4002/base/i18n/rtl.h#newcode46
> base/i18n/rtl.h:46: bool empty() const { return string_.empty(); }
> On 2011/04/21 18:03:30, darin wrote:
>
>> in chromium, we generally add a verb to methods like this:  "is_empty"
>>
> or
>
>> "IsEmpty"... but maybe you are trying to make this look like
>>
> std::string?
>
> In the equivalent WebKit review, I tried to add no methods and my
> reviewer wanted me to make the methods match string, so I was attempting
> to head that off here.  I will just remove it if you don't mind.


OK


>
>
> http://codereview.chromium.org/6878089/
>

[darin (slow to review), 2011-04-21 20:16:22]

LGTM

http://codereview.chromium.org/6878089/diff/9002/base/i18n/rtl.h
File base/i18n/rtl.h (right):

http://codereview.chromium.org/6878089/diff/9002/base/i18n/rtl.h#newcode38
base/i18n/rtl.h:38: public:
nit: public and private labels should be indented by 1 space right?