MacViews: Use RenderTextHarfBuzz only for Textfields

MacViews: Use RenderTextHarfBuzz only for Textfields

Using RenderTextHarfBuzz for string sizing and eliding on Mac is problematic
because it may not match when drawn by Cocoa, so we use RenderTextMac.
But RenderTextMac does not have support for editing that is needed by
views::Textfield, so use RenderTextHarfBuzz for MacViews textfields.

Committed: https://crrev.com/62fd79bb7907a559887866bbdba869d25f72b011
Cr-Commit-Position: refs/heads/master@{#307396}

[Andre, 2014-12-05 22:50:32]

Patchset #2 (id:20001) has been deleted

[Andre, 2014-12-06 00:02:47]

andresantoso@chromium.org changed reviewers:
+ asvitkine@chromium.org, msw@chromium.org

[Andre, 2014-12-06 00:02:48]

asvitkine, msw, PTAL.

[Alexei Svitkine (slow), 2014-12-08 15:53:10]

https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (left):

https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.cc#ol...
ui/gfx/render_text.cc:401: #if defined(OS_MACOSX) && !defined(TOOLKIT_VIEWS)
Is this change needed anymore?

[Andre, 2014-12-08 18:33:46]

https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (left):

https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.cc#ol...
ui/gfx/render_text.cc:401: #if defined(OS_MACOSX) && !defined(TOOLKIT_VIEWS)
On 2014/12/08 15:53:10, Alexei Svitkine wrote:
> Is this change needed anymore?

I think so.
TOOLKIT_VIEWS will be turned on soon for Mac, and we don't want that change to
affect the Cocoa
browser.

[Alexei Svitkine (slow), 2014-12-08 18:43:14]

lgtm

[msw, 2014-12-08 19:27:50]

https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:412: return new RenderTextHarfBuzz;
Shouldn't this respect kDisableHarfBuzzRenderText?

https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:197: virtual scoped_ptr<RenderText> NewInstance() const =
0;
nit: CreateInstanceOfSameType?

[Andre, 2014-12-08 19:51:32]

https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.cc#ne...
ui/gfx/render_text.cc:412: return new RenderTextHarfBuzz;
On 2014/12/08 19:27:50, msw wrote:
> Shouldn't this respect kDisableHarfBuzzRenderText?

Done.

https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:197: virtual scoped_ptr<RenderText> NewInstance() const =
0;
On 2014/12/08 19:27:50, msw wrote:
> nit: CreateInstanceOfSameType?

I thought about calling it Clone(), but it doesn't clone the attributes.
How about CreateCloneInstance or CreateInstanceForClone?

[msw, 2014-12-08 19:57:52]

lgtm with CreateInstanceForClone rename nit.

https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:197: virtual scoped_ptr<RenderText> NewInstance() const =
0;
On 2014/12/08 19:51:32, Andre wrote:
> On 2014/12/08 19:27:50, msw wrote:
> > nit: CreateInstanceOfSameType?
> 
> I thought about calling it Clone(), but it doesn't clone the attributes.
> How about CreateCloneInstance or CreateInstanceForClone?

Sure, I slightly prefer CreateInstanceForClone.

[Alexei Svitkine (slow), 2014-12-08 20:01:17]

I don't like using the word "clone" because it does imply copied
properties, which as you say is not the case. Mike's suggestion SGTM.

On Mon, Dec 8, 2014 at 2:51 PM, <andresantoso@chromium.org> wrote:

>
> https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.cc
> File ui/gfx/render_text.cc (right):
>
> https://codereview.chromium.org/779793006/diff/40001/ui/
> gfx/render_text.cc#newcode412
> ui/gfx/render_text.cc:412: return new RenderTextHarfBuzz;
> On 2014/12/08 19:27:50, msw wrote:
>
>> Shouldn't this respect kDisableHarfBuzzRenderText?
>>
>
> Done.
>
> https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.h
> File ui/gfx/render_text.h (right):
>
> https://codereview.chromium.org/779793006/diff/40001/ui/
> gfx/render_text.h#newcode197
> ui/gfx/render_text.h:197: virtual scoped_ptr<RenderText> NewInstance()
> const = 0;
> On 2014/12/08 19:27:50, msw wrote:
>
>> nit: CreateInstanceOfSameType?
>>
>
> I thought about calling it Clone(), but it doesn't clone the attributes.
> How about CreateCloneInstance or CreateInstanceForClone?
>
> https://codereview.chromium.org/779793006/
>

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

[Andre, 2014-12-08 20:35:28]

On 2014/12/08 20:01:17, Alexei Svitkine wrote:
> I don't like using the word "clone" because it does imply copied
> properties, which as you say is not the case. Mike's suggestion SGTM.
> 
> On Mon, Dec 8, 2014 at 2:51 PM, <mailto:andresantoso@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.cc
> > File ui/gfx/render_text.cc (right):
> >
> > https://codereview.chromium.org/779793006/diff/40001/ui/
> > gfx/render_text.cc#newcode412
> > ui/gfx/render_text.cc:412: return new RenderTextHarfBuzz;
> > On 2014/12/08 19:27:50, msw wrote:
> >
> >> Shouldn't this respect kDisableHarfBuzzRenderText?
> >>
> >
> > Done.
> >
> > https://codereview.chromium.org/779793006/diff/40001/ui/gfx/render_text.h
> > File ui/gfx/render_text.h (right):
> >
> > https://codereview.chromium.org/779793006/diff/40001/ui/
> > gfx/render_text.h#newcode197
> > ui/gfx/render_text.h:197: virtual scoped_ptr<RenderText> NewInstance()
> > const = 0;
> > On 2014/12/08 19:27:50, msw wrote:
> >
> >> nit: CreateInstanceOfSameType?
> >>
> >
> > I thought about calling it Clone(), but it doesn't clone the attributes.
> > How about CreateCloneInstance or CreateInstanceForClone?
> >
> > https://codereview.chromium.org/779793006/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Done, CreateInstanceOfSameType it is.

[Andre, 2014-12-08 21:21:12]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-12-08 21:21:51]

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

[commit-bot: I haz the power, 2014-12-08 21:28:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-08 21:28:21]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Andre, 2014-12-08 21:38:01]

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

[Andre, 2014-12-08 21:38:01]

sky@ PTAL for views/examples.

[sky, 2014-12-09 00:56:15]

LGTM

[Andre, 2014-12-09 01:30:19]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-12-09 01:31:00]

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

[commit-bot: I haz the power, 2014-12-09 01:34:09]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2014-12-09 01:35:06]

Patchset 4 (id:??) landed as
https://crrev.com/62fd79bb7907a559887866bbdba869d25f72b011
Cr-Commit-Position: refs/heads/master@{#307396}