Supports gfx::FontList in gfx::Canvas and ui::ElideText family.

Supports gfx::FontList in gfx::Canvas and ui::ElideText family.

This is a part of the plan to support gfx::FontList in views::Label.
https://docs.google.com/a/chromium.org/document/d/1D_25fp9B8b9aZJORfAjDIFq61NWvUquZ5xmKH-VcC4k/edit

BUG=265485
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218926

[Yuki, 2013-08-12 14:16:32]

Could you review this CL?

[Alexei Svitkine (slow), 2013-08-12 18:16:20]

mostly looks good, here's some comments your way

Thanks!

https://codereview.chromium.org/22835002/diff/22001/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

https://codereview.chromium.org/22835002/diff/22001/ui/base/text/text_elider....
ui/base/text/text_elider.cc:209: available_pixel_width - GetStringWidth(domain,
font_list), ELIDE_AT_END);
Nit: How about:

  available_pixel_width -= GetStringWidth(domain, font_list);
  username = ElideText(username, font_list, available_pixel_width,
                       ELIDE_AT_END);

https://codereview.chromium.org/22835002/diff/22001/ui/base/text/text_elider....
ui/base/text/text_elider.cc:877: int GetStringWidth(const base::string16& text)
const;
Nit: Add a comment.

https://codereview.chromium.org/22835002/diff/22001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/22835002/diff/22001/ui/gfx/canvas.h#newcode132
ui/gfx/canvas.h:132: int* width, int* height,
Nit: can you fix this to be 1 param per line, per style guide. Here and below.

https://codereview.chromium.org/22835002/diff/22001/ui/gfx/canvas.h#newcode342
ui/gfx/canvas.h:342: void DrawStringWithAligned(const base::string16& text,
Flags can specify more than just alignment. How about DrawStringWithFlags?

https://codereview.chromium.org/22835002/diff/22001/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://codereview.chromium.org/22835002/diff/22001/ui/gfx/canvas_skia.cc#new...
ui/gfx/canvas_skia.cc:409: void Canvas::DrawStringWithHalo(const base::string16&
text,
Can all of these functions that take a |font| go into canvas.cc? Since they
should be the same for all the platforms (i.e. calling the font_list ones) and
have the platforms implement the font_list ones?

[Yuki, 2013-08-13 09:32:12]

https://codereview.chromium.org/22835002/diff/22001/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

https://codereview.chromium.org/22835002/diff/22001/ui/base/text/text_elider....
ui/base/text/text_elider.cc:209: available_pixel_width - GetStringWidth(domain,
font_list), ELIDE_AT_END);
On 2013/08/12 18:16:21, Alexei Svitkine wrote:
> Nit: How about:
> 
>   available_pixel_width -= GetStringWidth(domain, font_list);
>   username = ElideText(username, font_list, available_pixel_width,
>                        ELIDE_AT_END);

Done.

https://codereview.chromium.org/22835002/diff/22001/ui/base/text/text_elider....
ui/base/text/text_elider.cc:877: int GetStringWidth(const base::string16& text)
const;
On 2013/08/12 18:16:21, Alexei Svitkine wrote:
> Nit: Add a comment.

Done.

https://codereview.chromium.org/22835002/diff/22001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/22835002/diff/22001/ui/gfx/canvas.h#newcode132
ui/gfx/canvas.h:132: int* width, int* height,
On 2013/08/12 18:16:21, Alexei Svitkine wrote:
> Nit: can you fix this to be 1 param per line, per style guide. Here and below.

Done.

https://codereview.chromium.org/22835002/diff/22001/ui/gfx/canvas.h#newcode342
ui/gfx/canvas.h:342: void DrawStringWithAligned(const base::string16& text,
On 2013/08/12 18:16:21, Alexei Svitkine wrote:
> Flags can specify more than just alignment. How about DrawStringWithFlags?

Done.

https://codereview.chromium.org/22835002/diff/22001/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://codereview.chromium.org/22835002/diff/22001/ui/gfx/canvas_skia.cc#new...
ui/gfx/canvas_skia.cc:409: void Canvas::DrawStringWithHalo(const base::string16&
text,
On 2013/08/12 18:16:21, Alexei Svitkine wrote:
> Can all of these functions that take a |font| go into canvas.cc? Since they
> should be the same for all the platforms (i.e. calling the font_list ones) and
> have the platforms implement the font_list ones?

Done.

[Alexei Svitkine (slow), 2013-08-13 13:41:52]

LGTM, thanks!

[Alexei Svitkine (slow), 2013-08-13 13:43:49]

On 2013/08/13 13:41:52, Alexei Svitkine wrote:
> LGTM, thanks!

However, it looks like the iOS builder isn't happy. I think iOS doesn't actually
have an implementation of some of those code (i.e. RenderText, Canvas, etc), and
you might have introduced it as a dependency in this CL. :\

http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...

[Alexei Svitkine (slow), 2013-08-13 14:58:39]

+cc stuartmorgan for thoughts / FYI about the iOS stuff

I think currently iOS might not compile canvas.cc since there's no corresponding
canvas_ios.cc class for it (and since iOS doesn't use Skia), but this change is
making some other code in ui/ like text_elider.cc depend on Canvas, causing the
link failures.

I think we should add a canvas_ios.cc/mm class that's mostly stubs like
canvas_android.cc except for Canvas::SizeStringInt(), which can end up either
calling PlatformFontIOS::GetStringWidth() or just having that same
implementation in it.

This way, we can proceed with this CL which makes less things depend on
Font.GetStringWith()/Font.SizeStringInt() (which I want to get rid of
eventually) without iOS blocking the progress.

[msw, 2013-08-13 17:39:21]

Perhaps it's okay to overlook the discouraged overloading if you're committed to
resolving all overloads immediately after this CL.

https://codereview.chromium.org/22835002/diff/33001/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

https://codereview.chromium.org/22835002/diff/33001/ui/base/text/text_elider....
ui/base/text/text_elider.cc:1084: int RectangleText::GetStringWidth(const
base::string16& text) const {
nit: remove this, just inline the file-local helper with font_list_.

https://codereview.chromium.org/22835002/diff/33001/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

https://codereview.chromium.org/22835002/diff/33001/ui/base/text/text_elider....
ui/base/text/text_elider.h:49: UI_EXPORT string16 ElideEmail(const string16&
email,
Our style guide discourages overloading functions, can you rename one of these?
Perhaps it'd be easiest to rename the new function something like
ElideEmailWithFontList. Then, in a followup CL, rename the old function
ElideEmailDeprecated and rename the new function ElideEmail. Same suggestion for
the other overloaded Elide* functions below.

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas.cc#newcode96
ui/gfx/canvas.cc:96: Canvas::SizeStringInt(text, gfx::FontList(font), width,
height, line_height,
nit: you shouldn't need to explicitly qualify canvas here.
Ditto for at least 3 other places below.

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas.cc#newcode471
ui/gfx/canvas.cc:471: DrawStringWithShadows(text,
nit: wrap some args here.

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas.h#newcode137
ui/gfx/canvas.h:137: static void SizeStringInt(const base::string16& text,
Ditto to my overloading comment throughout this file.

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas_android.cc
File ui/gfx/canvas_android.cc (right):

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas_android.cc#...
ui/gfx/canvas_android.cc:12: void Canvas::SizeStringInt(const base::string16&
text,
Why are only a subset of Canvas functions stubbed out?

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas_skia.cc#new...
ui/gfx/canvas_skia.cc:206: const Size& string_size =
render_text->GetStringSize();
nit: I'd feel safer leaving this a copy, not a ref.

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas_skia.cc#new...
ui/gfx/canvas_skia.cc:225: const Size& string_size =
render_text->GetStringSize();
ditto ref nit.

[stuartmorgan, 2013-08-13 17:52:15]

iOS does use Skia for some things, just not for fonts. Making the text code only
work for canvas implementations of the text system is not a good situation for
iOS; stubbing will make it link, but still fundamentally break things. (Also,
these kinds of stubs are discouraged by Chromium leadership.)

I thought the whole idea of the Font abstraction was to avoid tying generic
utilities like this to a specific implementation. Why do you need to use
Canvas::GetStringWidth specifically, instead of FontList::GetStringWidth?

> This way, we can proceed with this CL which makes less things depend on
> Font.GetStringWith()/Font.SizeStringInt() (which I want to get rid of
> eventually) without iOS blocking the progress.

We've talked about this before; can you explain why the thinking has changed? I
don't understand why we want to remove something that abstracts away the
different platform implementations when we have different implementations on
different platforms. That seems like a very useful abstraction.

[Alexei Svitkine (slow), 2013-08-13 18:09:33]

On Tue, Aug 13, 2013 at 1:52 PM, <stuartmorgan@chromium.org> wrote:

> iOS does use Skia for some things, just not for fonts. Making the text
> code only
> work for canvas implementations of the text system is not a good situation
> for
> iOS; stubbing will make it link, but still fundamentally break things.
> (Also,
> these kinds of stubs are discouraged by Chromium leadership.)
>
> I thought the whole idea of the Font abstraction was to avoid tying generic
> utilities like this to a specific implementation. Why do you need to use
> Canvas::GetStringWidth specifically, instead of FontList::GetStringWidth?


The problem is asking the font for the string width avoids all kinds of
important questions - the same font may be used to render the text using
different mechanisms (for example, an OS native rendering API, Skia, or
something else entirely), each of those may have different widths, since
they depend on the implementation. So having a single abstraction on the
Font object is not appropriate given the above. Rather, you want to ask the
specific rendering mechanism (not the Font) what width the string will have
when rendered using it. The existing Font API doesn't capture that
information in any good way.

Regarding this specific change, the goal is switching most of (desktop)
Chrome UI code to use FontList (a list of multiple fonts, e.g. include
fallback fonts), which is currently supported by RenderText (the
implementation behind canvas_skia.cc's rendering mechanism) and this needs
to be properly plumbed though. It can't rely on just asking Font about it,
because that doesn't take into account the fallback fonts that will be
retrieved via FontList.

So fundamentally, we want to get rid of Font::GetStringWidth().


>
>
>  This way, we can proceed with this CL which makes less things depend on
>> Font.GetStringWith()/Font.**SizeStringInt() (which I want to get rid of
>> eventually) without iOS blocking the progress.
>>
>
> We've talked about this before; can you explain why the thinking has
> changed? I
> don't understand why we want to remove something that abstracts away the
> different platform implementations when we have different implementations
> on
> different platforms. That seems like a very useful abstraction.
>

See above for the reason. If you re-read my previous mail, you can see that
I'm not proposing that we remove the abstraction - just move it. Instead of
the abstraction being Font::GetStringWidth() (which can't support FontList
which is a layer above Font), the abstract will be
Canvas::GetStringWidth(). The iOS-specific implementation can still exist,
but be in canvas_ios.cc (and not use Skia).

Does that make more sense to you now?

>
>
https://codereview.chromium.**org/22835002/<https://codereview.chromium.org/2...
>

[stuartmorgan, 2013-08-13 18:26:13]

On 2013/08/13 18:09:33, Alexei Svitkine wrote:
> See above for the reason. If you re-read my previous mail, you can see that
> I'm not proposing that we remove the abstraction - just move it. Instead of
> the abstraction being Font::GetStringWidth() (which can't support FontList
> which is a layer above Font), the abstract will be
> Canvas::GetStringWidth(). The iOS-specific implementation can still exist,
> but be in canvas_ios.cc (and not use Skia).

The documentation for Canvas says that "Canvas is a SkCanvas wrapper". It seems
like a stretch to call shim implementations of canvas methods that are actually
based on something else that has nothing to do with canvas an 'abstraction'.

If Font is the wrong abstraction, it seems like there should be a new one—maybe
just some stand-alone utility methods if that's all that's needed—that captures
the concept you want. (I don't understand why if FontList is the abstraction you
want to build on, FontList::GetStringWidth shouldn't be the home of that
abstraction; your argument seems to be about Font::GetStringWidth, not
FontList::GetStringWidth.) Forcing platforms that don't actually use SkCanvas to
hide a totally unrelated implementation behind Canvas shims seems like the same
approach that we are explicitly not using with content/ on iOS, and the same
arguments against it apply here.

[Alexei Svitkine (slow), 2013-08-13 19:02:48]

On Tue, Aug 13, 2013 at 2:26 PM, <stuartmorgan@chromium.org> wrote:

> On 2013/08/13 18:09:33, Alexei Svitkine wrote:
>
>> See above for the reason. If you re-read my previous mail, you can see
>> that
>> I'm not proposing that we remove the abstraction - just move it. Instead
>> of
>> the abstraction being Font::GetStringWidth() (which can't support FontList
>> which is a layer above Font), the abstract will be
>> Canvas::GetStringWidth(). The iOS-specific implementation can still exist,
>> but be in canvas_ios.cc (and not use Skia).
>>
>
> The documentation for Canvas says that "Canvas is a SkCanvas wrapper". It
> seems
> like a stretch to call shim implementations of canvas methods that are
> actually
> based on something else that has nothing to do with canvas an
> 'abstraction'.
>

I agree, the abstraction leaves a lot to be desired. For example, it
couples the text rendering implementation (which is quite complicated) with
simple wrappers over Canvas primitives - which really shouldn't be
implemented together. And there are other issues, but that's getting off
topic here..


> If Font is the wrong abstraction, it seems like there should be a new
> one—maybe
> just some stand-alone utility methods if that's all that's needed—that
> captures
> the concept you want. (I don't understand why if FontList is the
> abstraction you
> want to build on, FontList::GetStringWidth shouldn't be the home of that
> abstraction; your argument seems to be about Font::GetStringWidth, not
> FontList::GetStringWidth.)


Right now, FontList has a single implementation without any
platform-specific code.

Yes, we could make FontList::GetStringWidth() be an abstraction that's
implemented differently between platforms, though this would make it much
uglier than it currently is. Since it's literally the only function that
needs different implementations - and really only for iOS. The reason I was
suggesting the canvas_ios.mm approach is that it would make the platforms
consistent and would avoid ugly hacks like the above.

I guess the crux of the problem is that iOS uses only selective parts of
ui/ and gfx/ with some classes not existing, so any change to ui/, gfx/ or
code that depends on those has to be careful not to break iOS since it
works differently from every other platform. This is bad - it puts extra
burden on developers that they shouldn't have to bear. Ideally, we should
split 'ui' into two - one subset that iOS depends on that's fully
implemented and another target that all the other platforms depend on that
has the extra stuff that iOS doesn't implement - such as Skia utilities.
Then, the distinction would be clearly enforced by the source code
structure - without this sort of thing arising all the time.

Anyway, back to this specific problem - perhaps you're right that we should
add a new top-level API that callers can use - e.g. gfx::GetStringSize()
that can be implemented in terms of Canvas::GetStringSize() on non-iOS
platforms and by Font::GetStringSize() on iOS. It adds an extra bit of
complexity to this CL (i.e. without iOS - the API would just be
Canvas::GetStringSize()), but perhaps it's unavoidable.


> Forcing platforms that don't actually use SkCanvas to
> hide a totally unrelated implementation behind Canvas shims seems like the
> same
> approach that we are explicitly not using with content/ on iOS, and the
> same
> arguments against it apply here.
>
>
https://codereview.chromium.**org/22835002/<https://codereview.chromium.org/2...
>

[Yuki, 2013-08-14 15:42:16]

To: msw

Thanks for comments.  I've addressed your comments except for canvas_android.cc.
 I'm now working on this point.  I think I can remove canvas_android.cc.


To all,

In general, text-width is determined depending on how the text is going to be
rendered, not only the text and font metrics, but also how the text is layouted,
etc.  So I'd vote that Canvas class has GetStringWidth method because Canvas
class has DrawString family.  I guess it's theoretically possible that
GetStringWidth would have other versions such as GetStringWidthWithFlags(text,
fonts, flags), but not this time.

Assuming that only the layout engine can tell how many pixels are needed to draw
the text, the class of the layout engine should have GetStringWidth.

I thought the above reading Pango document.  Pango, which is a layout engine,
tells us the number of pixels needed depending on a lot of parameters.

Also, In Cocoa, NSFont's (CGFloat)widthOfString:(NSString*)aString is deprecated
and replaced by AppKitAdditions's
(NSSize)sizeWithAttributes:(NSDictionary*)attributes.  This change seems to me
the same movement as Font(List) to Canvas.


To: Alexei

At top of canvas.h, I found a comment that says:
// All methods that take integer arguments (as is used throughout views)
// end with Int. If you need to use methods provided by SkCanvas, you'll
// need to do a conversion. In particular you'll need to use |SkIntToScalar()|,
// or if converting from a scalar to an integer |SkScalarRound()|.

Should I rename so methods end with Int/Rect?  If so, here is a plan.  I'll
rename only new methods and leave obsolete methods as is, which will be removed
soon.
  DrawString => DrawStringRect
  DrawStringWithHalo => DrawStrintWithHaloRect
  DrawStringWithFlags => DrawStringWithFlagsRect
  DrawStringWithShadows => DrawStringWithShadowsRect
  DrawFadeTruncatingString => DrawFadeTruncatingStringRect
(All new methods end with "Rect" because I'll prefer Rect for new methods.)

Cheers,
Yuki Shiino

https://codereview.chromium.org/22835002/diff/33001/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

https://codereview.chromium.org/22835002/diff/33001/ui/base/text/text_elider....
ui/base/text/text_elider.cc:1084: int RectangleText::GetStringWidth(const
base::string16& text) const {
On 2013/08/13 17:39:22, msw wrote:
> nit: remove this, just inline the file-local helper with font_list_.

Done.

https://codereview.chromium.org/22835002/diff/33001/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

https://codereview.chromium.org/22835002/diff/33001/ui/base/text/text_elider....
ui/base/text/text_elider.h:49: UI_EXPORT string16 ElideEmail(const string16&
email,
On 2013/08/13 17:39:22, msw wrote:
> Our style guide discourages overloading functions, can you rename one of
these?
> Perhaps it'd be easiest to rename the new function something like
> ElideEmailWithFontList. Then, in a followup CL, rename the old function
> ElideEmailDeprecated and rename the new function ElideEmail. Same suggestion
for
> the other overloaded Elide* functions below.

Renaming is really painful.  If we rename methods in a single CL, there are many
client code and we have to ask each owner to review the CL.  If something is
wrong in the CL and it's reverted, we have to ask the same owners to review
almost the same CL again.  I'd like to avoid such situation.

We can avoid such situation in the following way.
step 1.  Add ElideEmailWithFontList.
step 2.  Rewrite client code to call ElideEmailWithFontList.
    (This can be done with separating CL for each module/owner.)
step 3.  Remove old ElideEmail and add new FontList version of ElideEmail which
is the same as ElideEmailWithFontList.
step 4.  Rewrite client code to call new ElideEmailWithFontList.
    (This can be done with separating CL for each module/owner.)

It's possible.  However, I don't want to ask each owner to review renaming CLs
twice (in step 2 and 4).  I think it's painful for both of owners and me.

As you suggested, can I overload these methods until I rewrite all client of old
Font versions?  I won't leave these obsolete methods.

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas.cc#newcode96
ui/gfx/canvas.cc:96: Canvas::SizeStringInt(text, gfx::FontList(font), width,
height, line_height,
On 2013/08/13 17:39:22, msw wrote:
> nit: you shouldn't need to explicitly qualify canvas here.
> Ditto for at least 3 other places below.

Done.

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas.cc#newcode471
ui/gfx/canvas.cc:471: DrawStringWithShadows(text,
On 2013/08/13 17:39:22, msw wrote:
> nit: wrap some args here.

Done.

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas.h#newcode137
ui/gfx/canvas.h:137: static void SizeStringInt(const base::string16& text,
On 2013/08/13 17:39:22, msw wrote:
> Ditto to my overloading comment throughout this file.

Please allow me to overload for a while.
As I'll remove gfx::Font::GetStringWidth, I'll remove this Font version, too.

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas_android.cc
File ui/gfx/canvas_android.cc (right):

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas_android.cc#...
ui/gfx/canvas_android.cc:12: void Canvas::SizeStringInt(const base::string16&
text,
On 2013/08/13 17:39:22, msw wrote:
> Why are only a subset of Canvas functions stubbed out?

At a glance, Android version seems not using gfx::Canvas nor ElideText family. 
Let me investigate this point in this CL.

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas_skia.cc#new...
ui/gfx/canvas_skia.cc:206: const Size& string_size =
render_text->GetStringSize();
On 2013/08/13 17:39:22, msw wrote:
> nit: I'd feel safer leaving this a copy, not a ref.

This is not harmful.  This is safe and potentially more efficient.

With reference type, compilers have choices.
  a) Directly bind the variable to the returned object, or
  b) Copy the returned object to a temporary object, and bind the variable to
the temporary object.

In the case of b), it's guaranteed by the standard that the temporary object has
the same lifetime as the variable.

See
8.5.3  References (paragraph 9 to 11)
http://www.open-std.org/jtc1/sc22/open/n2356/decl.html#dcl.init.ref
12.2  Temporary objects (paragraph 5)
http://www.open-std.org/jtc1/sc22/open/n2356/special.html#class.temporary

https://codereview.chromium.org/22835002/diff/33001/ui/gfx/canvas_skia.cc#new...
ui/gfx/canvas_skia.cc:225: const Size& string_size =
render_text->GetStringSize();
On 2013/08/13 17:39:22, msw wrote:
> ditto ref nit.

Done.

[msw, 2013-08-14 17:26:40]

LGTM, thanks.

https://codereview.chromium.org/22835002/diff/33001/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

https://codereview.chromium.org/22835002/diff/33001/ui/base/text/text_elider....
ui/base/text/text_elider.h:49: UI_EXPORT string16 ElideEmail(const string16&
email,
On 2013/08/14 15:42:16, Yuki wrote:
> On 2013/08/13 17:39:22, msw wrote:
> > Our style guide discourages overloading functions, can you rename one of
> these?
> > Perhaps it'd be easiest to rename the new function something like
> > ElideEmailWithFontList. Then, in a followup CL, rename the old function
> > ElideEmailDeprecated and rename the new function ElideEmail. Same suggestion
> for
> > the other overloaded Elide* functions below.
> 
> Renaming is really painful.  If we rename methods in a single CL, there are
many
> client code and we have to ask each owner to review the CL.  If something is
> wrong in the CL and it's reverted, we have to ask the same owners to review
> almost the same CL again.  I'd like to avoid such situation.
> 
> We can avoid such situation in the following way.
> step 1.  Add ElideEmailWithFontList.
> step 2.  Rewrite client code to call ElideEmailWithFontList.
>     (This can be done with separating CL for each module/owner.)
> step 3.  Remove old ElideEmail and add new FontList version of ElideEmail
which
> is the same as ElideEmailWithFontList.
> step 4.  Rewrite client code to call new ElideEmailWithFontList.
>     (This can be done with separating CL for each module/owner.)
> 
> It's possible.  However, I don't want to ask each owner to review renaming CLs
> twice (in step 2 and 4).  I think it's painful for both of owners and me.
> 
> As you suggested, can I overload these methods until I rewrite all client of
old
> Font versions?  I won't leave these obsolete methods.

I'd allow a strictly temporary overload, but you may need to convince others.

https://codereview.chromium.org/22835002/diff/56001/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

https://codereview.chromium.org/22835002/diff/56001/ui/base/text/text_elider....
ui/base/text/text_elider.cc:36: int GetStringWidth(const base::string16& text,
const gfx::FontList& font_list) {
nit: keep this in the existing anon namespace nested in ui.

[Yuki, 2013-08-14 17:48:26]

https://codereview.chromium.org/22835002/diff/33001/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

https://codereview.chromium.org/22835002/diff/33001/ui/base/text/text_elider....
ui/base/text/text_elider.h:49: UI_EXPORT string16 ElideEmail(const string16&
email,
On 2013/08/14 17:26:40, msw wrote:
> On 2013/08/14 15:42:16, Yuki wrote:
> > On 2013/08/13 17:39:22, msw wrote:
> > > Our style guide discourages overloading functions, can you rename one of
> > these?
> > > Perhaps it'd be easiest to rename the new function something like
> > > ElideEmailWithFontList. Then, in a followup CL, rename the old function
> > > ElideEmailDeprecated and rename the new function ElideEmail. Same
suggestion
> > for
> > > the other overloaded Elide* functions below.
> > 
> > Renaming is really painful.  If we rename methods in a single CL, there are
> many
> > client code and we have to ask each owner to review the CL.  If something is
> > wrong in the CL and it's reverted, we have to ask the same owners to review
> > almost the same CL again.  I'd like to avoid such situation.
> > 
> > We can avoid such situation in the following way.
> > step 1.  Add ElideEmailWithFontList.
> > step 2.  Rewrite client code to call ElideEmailWithFontList.
> >     (This can be done with separating CL for each module/owner.)
> > step 3.  Remove old ElideEmail and add new FontList version of ElideEmail
> which
> > is the same as ElideEmailWithFontList.
> > step 4.  Rewrite client code to call new ElideEmailWithFontList.
> >     (This can be done with separating CL for each module/owner.)
> > 
> > It's possible.  However, I don't want to ask each owner to review renaming
CLs
> > twice (in step 2 and 4).  I think it's painful for both of owners and me.
> > 
> > As you suggested, can I overload these methods until I rewrite all client of
> old
> > Font versions?  I won't leave these obsolete methods.
> 
> I'd allow a strictly temporary overload, but you may need to convince others.

Thanks.

To: other reviewers,

Please raise your hand if you really don't like (temporary) overloading.

https://codereview.chromium.org/22835002/diff/56001/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

https://codereview.chromium.org/22835002/diff/56001/ui/base/text/text_elider....
ui/base/text/text_elider.cc:36: int GetStringWidth(const base::string16& text,
const gfx::FontList& font_list) {
On 2013/08/14 17:26:40, msw wrote:
> nit: keep this in the existing anon namespace nested in ui.

There is a problem.  This file's namespace structure is the following.

namespace ui {
namespace {  // [A]
helper functions
}  // namespace
implementation
}  // namespace ui

namespace {  // [B]
class RectangleString { ... }  // helper class
}  // namespace

namespace ui {  // [C]
rest of implementation using RectangleString
}  // namespace ui


The helper function GetStringWidth is used in RectangleString class, which is
defined in [B].  Anonymous namespace [A] is invisible from the other anonymous
namespace [B].  So if I defined the helper function in [A], then it's
inaccessible from [B].

We can solve this problem in many ways.
1. Define two same helper functions in both of [A] and [B]
  => I'm sure no one likes this solution.
2. Define a helper method in RectangleString.  (Patch set 2)
3. Define the helper function outside both [A] and [B].  (Patch set 3)
4. Move [B] and merge it into [A].
  => The original author probably wanted to place [B] close to [C].
Probably more ways.

I'd prefer 2 or 3 to 4.

[msw, 2013-08-14 17:50:50]

https://codereview.chromium.org/22835002/diff/56001/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

https://codereview.chromium.org/22835002/diff/56001/ui/base/text/text_elider....
ui/base/text/text_elider.cc:36: int GetStringWidth(const base::string16& text,
const gfx::FontList& font_list) {
On 2013/08/14 17:48:26, Yuki wrote:
> On 2013/08/14 17:26:40, msw wrote:
> > nit: keep this in the existing anon namespace nested in ui.
> 
> There is a problem.  This file's namespace structure is the following.
> 
> namespace ui {
> namespace {  // [A]
> helper functions
> }  // namespace
> implementation
> }  // namespace ui
> 
> namespace {  // [B]
> class RectangleString { ... }  // helper class
> }  // namespace
> 
> namespace ui {  // [C]
> rest of implementation using RectangleString
> }  // namespace ui
> 
> 
> The helper function GetStringWidth is used in RectangleString class, which is
> defined in [B].  Anonymous namespace [A] is invisible from the other anonymous
> namespace [B].  So if I defined the helper function in [A], then it's
> inaccessible from [B].
> 
> We can solve this problem in many ways.
> 1. Define two same helper functions in both of [A] and [B]
>   => I'm sure no one likes this solution.
> 2. Define a helper method in RectangleString.  (Patch set 2)
> 3. Define the helper function outside both [A] and [B].  (Patch set 3)
> 4. Move [B] and merge it into [A].
>   => The original author probably wanted to place [B] close to [C].
> Probably more ways.
> 
> I'd prefer 2 or 3 to 4.

Ah, I'd have a slight preference for 4, but 3 is okay.

[Yuki, 2013-08-16 16:46:57]

Hi reviewers,

I've removed canvas_android.cc (which was not needed) and added new
canvas_ios.mm (which comes from platform_font_ios.mm).
Also renamed some methods following canvas.h convention, so they end with
...Rect.

I think this CL is almost done, but somehow this CL doesn't compile on iOS.
I see the following error message.

Ld ../xcodebuild/Debug-iphonesimulator/content_unittests.app/content_unittests
normal i386
Undefined symbols for architecture i386:
  "skia::CreatePlatformCanvas(int, int, bool, unsigned char*,
skia::OnFailureType)", referenced from:
      skia::CreatePlatformCanvas(int, int, bool) in libui.a(canvas.o)

I'm wondering how it compiled before this CL.  Any clues are welcome.

[msw, 2013-08-16 16:59:24]

Note that it looks like you didn't actually "rm canvas_android.cc" but instead
just cleared out the file. It should be listed as "D", not "M" if you were
actually deleting it, which I believe is the wrong decision, as I mention below.

https://codereview.chromium.org/22835002/diff/94002/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/22835002/diff/94002/ui/gfx/canvas.cc#newcode96
ui/gfx/canvas.cc:96: #if defined(OS_ANDROID)
I believe avoiding these platform-specific preprocessor blocks is precisely why
we use canvas_android|ios|mac|skia, etc.; let's keep it that way.

[Yuki, 2013-08-16 17:24:31]

Thanks for the comments.  I thought I had done "git rm
ui/gfx/canvas_android.cc".  Let me check it again (if I really need to remove
it).

https://codereview.chromium.org/22835002/diff/94002/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/22835002/diff/94002/ui/gfx/canvas.cc#newcode96
ui/gfx/canvas.cc:96: #if defined(OS_ANDROID)
On 2013/08/16 16:59:25, msw wrote:
> I believe avoiding these platform-specific preprocessor blocks is precisely
why
> we use canvas_android|ios|mac|skia, etc.; let's keep it that way.

Note that these defined(OS_ANDROID) appear only inside obsolete methods of
gfx::Font version.  And sooner or later they're removed.

Alexei suggested putting all obsolete methods in canvas.cc because they're just
redirection to new methods (and at that point they seemed platform-independent).
 Now I've realized that these redirection cause forces us to have stub code in
canvas_{android,ios,mac}.cc, otherwise it causes unresolved linkage errors.

I'm okay to move these obsolete redirection methods to
canvas_{android,ios,mac,skia}.cc.  They will be removed after all, and the goal
is the same.  However, in that case, we have the same redirection code here and
there.

With above note, which do you really want?
I'm okay with either way.

[msw, 2013-08-16 17:31:39]

https://codereview.chromium.org/22835002/diff/94002/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/22835002/diff/94002/ui/gfx/canvas.cc#newcode96
ui/gfx/canvas.cc:96: #if defined(OS_ANDROID)
On 2013/08/16 17:24:31, Yuki wrote:
> On 2013/08/16 16:59:25, msw wrote:
> > I believe avoiding these platform-specific preprocessor blocks is precisely
> why
> > we use canvas_android|ios|mac|skia, etc.; let's keep it that way.
> 
> Note that these defined(OS_ANDROID) appear only inside obsolete methods of
> gfx::Font version.  And sooner or later they're removed.
> 
> Alexei suggested putting all obsolete methods in canvas.cc because they're
just
> redirection to new methods (and at that point they seemed
platform-independent).
>  Now I've realized that these redirection cause forces us to have stub code in
> canvas_{android,ios,mac}.cc, otherwise it causes unresolved linkage errors.
> 
> I'm okay to move these obsolete redirection methods to
> canvas_{android,ios,mac,skia}.cc.  They will be removed after all, and the
goal
> is the same.  However, in that case, we have the same redirection code here
and
> there.
> 
> With above note, which do you really want?
> I'm okay with either way.

If the obsolete redirection methods can be consolidated in canvas.cc and then
just stub out their underlying functions in platform-specific files where
canvas_skia isn't used, that sounds best. I don't quite follow why you'd need to
duplicate the redirection code. But this is just my opinion, if other reviewers
disagree, they should speak up.

[Yuki, 2013-08-16 18:29:27]

https://codereview.chromium.org/22835002/diff/94002/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/22835002/diff/94002/ui/gfx/canvas.cc#newcode96
ui/gfx/canvas.cc:96: #if defined(OS_ANDROID)
On 2013/08/16 17:31:39, msw wrote:
> On 2013/08/16 17:24:31, Yuki wrote:
> > On 2013/08/16 16:59:25, msw wrote:
> > > I believe avoiding these platform-specific preprocessor blocks is
precisely
> > why
> > > we use canvas_android|ios|mac|skia, etc.; let's keep it that way.
> > 
> > Note that these defined(OS_ANDROID) appear only inside obsolete methods of
> > gfx::Font version.  And sooner or later they're removed.
> > 
> > Alexei suggested putting all obsolete methods in canvas.cc because they're
> just
> > redirection to new methods (and at that point they seemed
> platform-independent).
> >  Now I've realized that these redirection cause forces us to have stub code
in
> > canvas_{android,ios,mac}.cc, otherwise it causes unresolved linkage errors.
> > 
> > I'm okay to move these obsolete redirection methods to
> > canvas_{android,ios,mac,skia}.cc.  They will be removed after all, and the
> goal
> > is the same.  However, in that case, we have the same redirection code here
> and
> > there.
> > 
> > With above note, which do you really want?
> > I'm okay with either way.
> 
> If the obsolete redirection methods can be consolidated in canvas.cc and then
> just stub out their underlying functions in platform-specific files where
> canvas_skia isn't used, that sounds best. I don't quite follow why you'd need
to
> duplicate the redirection code. But this is just my opinion, if other
reviewers
> disagree, they should speak up.

Okay, I'll leave the redirection code here in canvas.cc and add stub code in
canvas_{android,ios,mac,skia}.cc.

The drawback is that we need to put stub code (or real implementation) in every
platform's *.cc file, even if it's never called, otherwise it causes linkage
error.  Since we'll have stub code, it won't cause linkage error at compile-time
if we accidentally used such not-implemented stub methods.  Runtime error is
harder to handle than compile-time error, so I think this is drawback.

If we want to minimize the amount of such not-implemented stub code, an option
is to move the redirection code to each platofrm's *.cc file.  Then, we can omit
both redirection code and stub code for some methods on some platforms, and then
accidental use of not-implemented code causes linkage error.
This is what you asked why canvas_android.cc partially has stub code, not for
all methods.
Probably you don't care this point much at this time.

[Yuki, 2013-08-19 11:47:55]

Could you take another look?

I've addressed your review comments, and removed canvas.cc from iOS build so it
compiles.

Thanks,
Yuki Shiino

[Alexei Svitkine (slow), 2013-08-19 14:17:28]

Looks great, but please address my nits below. LGTM.

https://codereview.chromium.org/22835002/diff/88004/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

https://codereview.chromium.org/22835002/diff/88004/ui/base/text/text_elider....
ui/base/text/text_elider.cc:328: const int kPixelWidthDotsTrailer =
GetStringWidth(UTF8ToUTF16(kEllipsis),
Nit: Use GetStringWidth(string16(kEllipsisUTF16), ...

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas.h#newcode363
ui/gfx/canvas.h:363: void DrawStringWithFlagsRect(const base::string16& text,
Nit: I think DrawStringRectWithFlags() would be clearer (i.e. it's same as
DrawStringRect but WithFlags).

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas.h#newcode381
ui/gfx/canvas.h:381: void DrawStringWithShadowsRect(const base::string16& text,
Nit: DrawStringRectWithShadows

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas_ios.mm
File ui/gfx/canvas_ios.mm (right):

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas_ios.mm#newc...
ui/gfx/canvas_ios.mm:30:
sizeWithFont:font_list.GetPrimaryFont().GetNativeFont()];
Nit: Extract NativeFont into a local var so that this stays on one line.

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas_skia.cc#new...
ui/gfx/canvas_skia.cc:234: const gfx::Font& font,
Nit: No gfx:: prefix - fix throughout all the canvas*.* files, please.

[stuartmorgan, 2013-08-19 14:21:11]

On 2013/08/13 19:02:48, Alexei Svitkine wrote:
> The reason I was suggesting the canvas_ios.mm approach is that it would make
> the platforms consistent and would avoid ugly hacks like the above.

Well, I would argue that forcing iOS code down the path of pretending it uses
Skia Canvas for all font code *is* an ugly hack. The whole idea of moving
GetStringWidth here as I understand it is that you are saying that fundamentally
the string width is a property of the text rendering system. And now you are
saying that if you want to know how wide a string is in iOS's native text
rendering, you have to ask a *completely different* rendering system that isn't
the one that will draw the text. That's clearly a bad abstraction.

> I guess the crux of the problem is that iOS uses only selective parts of
> ui/ and gfx/ with some classes not existing, so any change to ui/, gfx/ or
> code that depends on those has to be careful not to break iOS since it
> works differently from every other platform. This is bad - it puts extra
> burden on developers that they shouldn't have to bear. Ideally, we should
> split 'ui' into two - one subset that iOS depends on that's fully
> implemented and another target that all the other platforms depend on that
> has the extra stuff that iOS doesn't implement - such as Skia utilities.

Agreed, this is a problem, but I think blurring the lines of what parts of ui/
use Skia on iOS will make the kind of clear division you are talking about very
hard to achieve. And making developers remember that iOS doesn't use Skia for
font rendering but does (appear to) use it for font measurement seems like a
higher cognitive burden than just remembering that it doesn't use it for fonts
at all.

FWIW, I wouldn't expect that division to be all that hard to define. Outside of
the parts of Skia we don't use (which may just be fonts and JPEG, but I forget),
IIRC the ui/ code excluded on iOS pretty much falls into two categories:
1) Code that's nonsensical on mobile (e.g., keycodes)
2) Code that we just haven't needed

Excluding 1 can be done on a mobile/non-mobile switch, and shouldn't really be a
problem in practice in most cases since it would be unlikely that someone would
try to call that code on mobile. Code in 2 can be added as needed. If it would
help, I'm happy to rework the iOS part of ui.gyp to clarify this. And I'm happy
to meet with ui/ owners to discuss how to improve the situation.

Most of that's outside the scope of this CL though. If ui/ owners feel that the
shimming through Canvas is the right solution for now, I'm okay with it, but i
don't think this is the right long-term approach.

[msw, 2013-08-19 19:05:42]

I don't have much of an opinion on the iOS/Mac API quandry.
Stuart, do you have specific recommendations for this CL?

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas_ios.mm
File ui/gfx/canvas_ios.mm (right):

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas_ios.mm#newc...
ui/gfx/canvas_ios.mm:30:
sizeWithFont:font_list.GetPrimaryFont().GetNativeFont()];
Could this implementation be merged with canvas_mac.mm's? Or must we use
sizeWithFont on iOS because sizeWithAttributes is only supported on iOS7?
http://stackoverflow.com/questions/18154345/sizewithattributes-causes-crash-o...

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas_skia.cc#new...
ui/gfx/canvas_skia.cc:233: void Canvas::SizeStringInt(const base::string16&
text,
Why not define this SizeStringInt and the two GetStringWidth implementations in
canvas.cc? It seems like these are valid for cross-platform use regardless of
the underlying SizeStringInt with FontList implementation (or lack thereof).
(though I guess you'll need to redefine them as needed anyway in canvas_ios.mm,
if canvas.cc still winds up excluded from that config)

https://codereview.chromium.org/22835002/diff/88004/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/22835002/diff/88004/ui/ui.gyp#newcode626
ui/ui.gyp:626: ['exclude', '^gfx/canvas\\.cc$'],
What specifically doesn't compile in canvas.cc on iOS with this change? It seems
like the canvas.cc impls should apply there too, and then any iOS-specific impls
can be handled in canvas_ios.mm (i.e. canvas.cc should build on iOS too).

[stuartmorgan, 2013-08-19 22:08:10]

Comment 10 and the end of comment 11 discusses what I would suggest for this CL
specifically.

(I'm just not clear on why having an abstraction with just the methods all
platforms need, with platform-specific impls (one Canvas, one iOS, and maybe one
Android with NOTIMPLEMENTED) is more complexity than doing essentially the same
thing but shoehorning that abstraction it into Canvas, which means something
else, and having to add iOS stubbing of Canvas to compensate. But I lack a lot
of the larger context.)

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas_ios.mm
File ui/gfx/canvas_ios.mm (right):

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas_ios.mm#newc...
ui/gfx/canvas_ios.mm:30:
sizeWithFont:font_list.GetPrimaryFont().GetNativeFont()];
On 2013/08/19 19:05:43, msw wrote:
> Could this implementation be merged with canvas_mac.mm's? Or must we use
> sizeWithFont on iOS because sizeWithAttributes is only supported on iOS7?
>
http://stackoverflow.com/questions/18154345/sizewithattributes-causes-crash-o...

This is the public documentation for the iOS equivalent of the additions:
https://developer.apple.com/library/ios/documentation/uikit/reference/NSStrin...
and sizeWithAttributes isn't there, so we can't use it.

Whether or not it exists in betas of an as-yet-unreleased SDK for iOS 7 would be
subject to NDA. (But it's irrelevant, because we need to continue to run on
older OS versions.)

[Yuki, 2013-08-20 15:05:46]

I've introduced layout_text.h and gfx::LayoutText::GetStringWidth(), and also
addressed review comments.

Could you take a look?

https://codereview.chromium.org/22835002/diff/88004/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

https://codereview.chromium.org/22835002/diff/88004/ui/base/text/text_elider....
ui/base/text/text_elider.cc:328: const int kPixelWidthDotsTrailer =
GetStringWidth(UTF8ToUTF16(kEllipsis),
On 2013/08/19 14:17:28, Alexei Svitkine wrote:
> Nit: Use GetStringWidth(string16(kEllipsisUTF16), ...

Done.

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas.h#newcode363
ui/gfx/canvas.h:363: void DrawStringWithFlagsRect(const base::string16& text,
On 2013/08/19 14:17:28, Alexei Svitkine wrote:
> Nit: I think DrawStringRectWithFlags() would be clearer (i.e. it's same as
> DrawStringRect but WithFlags).

Done.

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas.h#newcode381
ui/gfx/canvas.h:381: void DrawStringWithShadowsRect(const base::string16& text,
On 2013/08/19 14:17:28, Alexei Svitkine wrote:
> Nit: DrawStringRectWithShadows

Done.

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas_ios.mm
File ui/gfx/canvas_ios.mm (right):

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas_ios.mm#newc...
ui/gfx/canvas_ios.mm:30:
sizeWithFont:font_list.GetPrimaryFont().GetNativeFont()];
On 2013/08/19 14:17:28, Alexei Svitkine wrote:
> Nit: Extract NativeFont into a local var so that this stays on one line.

Done.

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas_skia.cc#new...
ui/gfx/canvas_skia.cc:233: void Canvas::SizeStringInt(const base::string16&
text,
On 2013/08/19 19:05:43, msw wrote:
> Why not define this SizeStringInt and the two GetStringWidth implementations
in
> canvas.cc? It seems like these are valid for cross-platform use regardless of
> the underlying SizeStringInt with FontList implementation (or lack thereof).
> (though I guess you'll need to redefine them as needed anyway in
canvas_ios.mm,
> if canvas.cc still winds up excluded from that config)

I was struggling to remove dependency-breakage on iOS and moved those methods
here.  However, I'm now getting better understanding and also introducing
layout_text.h, so I've moved them back to canvas.cc.

https://codereview.chromium.org/22835002/diff/88004/ui/gfx/canvas_skia.cc#new...
ui/gfx/canvas_skia.cc:234: const gfx::Font& font,
On 2013/08/19 14:17:28, Alexei Svitkine wrote:
> Nit: No gfx:: prefix - fix throughout all the canvas*.* files, please.

Done.

[Yuki, 2013-08-20 15:16:50]

erg@: Could you review the following files as an owner?
    chrome/browser/ui/gtk/content_setting_bubble_gtk.cc
    chrome/browser/ui/gtk/status_bubble_gtk.cc

vandebo@: Could you review the following file as an owner?
    printing/print_settings_initializer.cc

avi@: Could you review the following file as an owner?
    ui/base/cocoa/menu_controller.mm

sadrul@: Could you review the following file as an owner?
    ui/views/widget/tooltip_manager.cc

[Alexei Svitkine (slow), 2013-08-20 15:17:20]

https://codereview.chromium.org/22835002/diff/26004/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

https://codereview.chromium.org/22835002/diff/26004/ui/base/text/text_elider....
ui/base/text/text_elider.cc:37: return gfx::LayoutText::GetStringWidth(text,
font_list);
I think if we remove LayoutText wrapper, then this anon function can be removed
and gfx::GetStringWidth() can be used directly.

https://codereview.chromium.org/22835002/diff/26004/ui/gfx/layout_text.h
File ui/gfx/layout_text.h (right):

https://codereview.chromium.org/22835002/diff/26004/ui/gfx/layout_text.h#newc...
ui/gfx/layout_text.h:18: class UI_EXPORT LayoutText {
I don't think this needs its own class. Just make GetStringWidth a free-standing
function under gfx.

[Yuki, 2013-08-20 15:22:24]

On 2013/08/20 15:16:50, Yuki wrote:
> erg@: Could you review the following files as an owner?
>     chrome/browser/ui/gtk/content_setting_bubble_gtk.cc
>     chrome/browser/ui/gtk/status_bubble_gtk.cc
> 
> vandebo@: Could you review the following file as an owner?
>     printing/print_settings_initializer.cc
> 
> avi@: Could you review the following file as an owner?
>     ui/base/cocoa/menu_controller.mm
> 
> sadrul@: Could you review the following file as an owner?
>     ui/views/widget/tooltip_manager.cc

To: erg, vandebo, avi, and sadrul

I've made a change in this CL that ui/base/text/text_elider.h declares gfx::Font
as a forward-declaration, so the files above need to #include <ui/gfx/font.h>. 
This is the reason why I need you guys' review.

Could you guys review for this point?

Thanks in advance,
Yuki Shiino

[sadrul, 2013-08-20 16:05:26]

> sadrul@: Could you review the following file as an owner?
>     ui/views/widget/tooltip_manager.cc

LGTM

[vandebo (ex-Chrome), 2013-08-20 16:28:16]

On 2013/08/20 15:16:50, Yuki wrote:
> vandebo@: Could you review the following file as an owner?
>     printing/print_settings_initializer.cc

LGTM

[msw, 2013-08-20 16:37:37]

LGTM with LayoutText class removal for gfx::GetStringWidth, etc.

[Yuki, 2013-08-20 16:45:41]

To stuartmorgan@

Are you okay to define gfx::GetStringWidth(text, font_list) in layout_text.h?

Please let me know if anyone has any preferences so I can make (hopefully) final
change.

https://codereview.chromium.org/22835002/diff/26004/ui/gfx/layout_text.h
File ui/gfx/layout_text.h (right):

https://codereview.chromium.org/22835002/diff/26004/ui/gfx/layout_text.h#newc...
ui/gfx/layout_text.h:18: class UI_EXPORT LayoutText {
On 2013/08/20 15:17:21, Alexei Svitkine wrote:
> I don't think this needs its own class. Just make GetStringWidth a
free-standing
> function under gfx.

If everyone is okay to define gfx::GetStringWidth(), I'll remove LayoutText
class.

By the way, in that case, is it okay to define gfx::GetStringWidth() in
layout_text.h?
If you'd like to define the function in another file (or another name), please
let me know.

[Alexei Svitkine (slow), 2013-08-20 16:52:19]

Maybe just put the declaration in ui/gfx/text_utils.h and then you can make
text_utils_skia.cc and text_utils_ios.mm files?


On Tue, Aug 20, 2013 at 12:45 PM, <yukishiino@chromium.org> wrote:

> To stuartmorgan@
>
> Are you okay to define gfx::GetStringWidth(text, font_list) in
> layout_text.h?
>
> Please let me know if anyone has any preferences so I can make (hopefully)
> final
> change.
>
>
>
>
> https://codereview.chromium.**org/22835002/diff/26004/ui/**
>
gfx/layout_text.h<https://codereview.chromium.org/22835002/diff/26004/ui/gfx/layout_text.h>
> File ui/gfx/layout_text.h (right):
>
> https://codereview.chromium.**org/22835002/diff/26004/ui/**
>
gfx/layout_text.h#newcode18<https://codereview.chromium.org/22835002/diff/26004/ui/gfx/layout_text.h#newcode18>
> ui/gfx/layout_text.h:18: class UI_EXPORT LayoutText {
> On 2013/08/20 15:17:21, Alexei Svitkine wrote:
>
>> I don't think this needs its own class. Just make GetStringWidth a
>>
> free-standing
>
>> function under gfx.
>>
>
> If everyone is okay to define gfx::GetStringWidth(), I'll remove
> LayoutText class.
>
> By the way, in that case, is it okay to define gfx::GetStringWidth() in
> layout_text.h?
> If you'd like to define the function in another file (or another name),
> please let me know.
>
>
https://codereview.chromium.**org/22835002/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stuartmorgan, 2013-08-20 20:47:57]

On 2013/08/20 16:45:41, Yuki wrote:
> Are you okay to define gfx::GetStringWidth(text, font_list) in layout_text.h?

You mean declare? It shouldn't be *defined* in the header.

> If you'd like to define the function in another file (or another name), please
> let me know.

I have no opinion on the details of helper class vs helper static function,
exact util file name, etc., it was just the folding it into Canvas that was
problematic for iOS.

[Elliot Glaysher, 2013-08-20 21:05:06]

gtk lgtm

[Yuki, 2013-08-21 03:42:21]

I've moved the GetStringWidth function to text_utils.h as gfx::GetStringWidth().


On 2013/08/20 20:47:57, stuartmorgan wrote:
> On 2013/08/20 16:45:41, Yuki wrote:
> > Are you okay to define gfx::GetStringWidth(text, font_list) in
layout_text.h?
> 
> You mean declare? It shouldn't be *defined* in the header.

Sorry for confusing you.  I meant declaring the function in .h and defining it
in each text_utils_{skia,android}.cc and text_utils_ios.mm.

[Yuki, 2013-08-21 03:42:44]

https://codereview.chromium.org/22835002/diff/26004/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

https://codereview.chromium.org/22835002/diff/26004/ui/base/text/text_elider....
ui/base/text/text_elider.cc:37: return gfx::LayoutText::GetStringWidth(text,
font_list);
On 2013/08/20 15:17:21, Alexei Svitkine wrote:
> I think if we remove LayoutText wrapper, then this anon function can be
removed
> and gfx::GetStringWidth() can be used directly.

Done.

https://codereview.chromium.org/22835002/diff/26004/ui/gfx/layout_text.h
File ui/gfx/layout_text.h (right):

https://codereview.chromium.org/22835002/diff/26004/ui/gfx/layout_text.h#newc...
ui/gfx/layout_text.h:18: class UI_EXPORT LayoutText {
On 2013/08/20 15:17:21, Alexei Svitkine wrote:
> I don't think this needs its own class. Just make GetStringWidth a
free-standing
> function under gfx.

Done.

[Yuki, 2013-08-21 03:48:13]

+rsesek@

Could you review the following file as an owner?
    ui/base/cocoa/menu_controller.mm

I've made a change in this CL that ui/base/text/text_elider.h declares gfx::Font
class as a forward-declaration, so menu_controller.mm need to #include
<ui/gfx/font.h> to use gfx::Font.

Thanks in advance,
Yuki Shiino

[Alexei Svitkine (slow), 2013-08-21 12:59:54]

lgtm

[Robert Sesek, 2013-08-21 14:02:01]

lgtm

[Yuki, 2013-08-22 01:00:45]

Will commit this CL.
Feel free to put comments, I'll work on them in a separate CL.

[commit-bot: I haz the power, 2013-08-22 01:05:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/22835002/169001

[commit-bot: I haz the power, 2013-08-22 07:20:11]

Change committed as 218926