MacViews: Enable word lookup for selectable views::Labels and multi-line text.

MacViews: Enable word lookup for selectable views::Labels and multi-line text.

r423419 added support for word lookups to textfields on MacViews and r441290
added support for multi-line text selection to RenderTextHarfBuzz. This CL
enables word lookup for selectable views::Labels. Changes:
  - Made views::Label inherit from WordLookupClient.
  - Moved GetLineContainingYCoord and GetLineSegmentContainingXCoord from
    RenderTextHarfBuzz to the base RenderText class.
  - Added multi-line support to RenderText::GetDecoratedWordAtPoint.

BUG=640502, 671055
TEST=Run out/Default/views_examples_with_content_exe with the flag --enable-
harfbuzz-rendertext. Go to the Label section. Ensure word lookup works correctly
on pressing Ctrl+Command+D for selectable labels.

Review-Url: https://codereview.chromium.org/2639493002
Cr-Commit-Position: refs/heads/master@{#445313}
Committed: https://chromium.googlesource.com/chromium/src/+/666e4f0298298427d2026c2896466951f199f5bf

[karandeepb, 2017-01-17 04:05:33]

Description was changed from

==========
MacViews: Enable word lookup for selectable views::Labels and multi-line text.
==========

to

==========
MacViews: Enable word lookup for selectable views::Labels and multi-line text.

r423419 added support for word lookups to textfields on MacViews and r441290
added support for multi-line text selection to RenderTextHarfBuzz. This CL
enables word lookup for selectable views::Labels. Changes:
  - Made views::Label inherit from WordLookupClient.
  - Moved GetLineContainingYCoord and GetLineSegmentContainingXCoord from
    RenderTextHarfBuzz to the base RenderText class.
  - Added multi-line support to RenderText::GetDecoratedWordAtPoint.

BUG=640502,671055

TEST=Run out/Default/views_examples_with_content_exe with the flag
--enable-harfbuzz-rendertext. Go to the Label section. Ensure word lookup works 
correctly on pressing Ctrl+Command+D for selectable labels.
==========

[karandeepb, 2017-01-17 04:05:52]

Description was changed from

==========
MacViews: Enable word lookup for selectable views::Labels and multi-line text.

r423419 added support for word lookups to textfields on MacViews and r441290
added support for multi-line text selection to RenderTextHarfBuzz. This CL
enables word lookup for selectable views::Labels. Changes:
  - Made views::Label inherit from WordLookupClient.
  - Moved GetLineContainingYCoord and GetLineSegmentContainingXCoord from
    RenderTextHarfBuzz to the base RenderText class.
  - Added multi-line support to RenderText::GetDecoratedWordAtPoint.

BUG=640502,671055

TEST=Run out/Default/views_examples_with_content_exe with the flag
--enable-harfbuzz-rendertext. Go to the Label section. Ensure word lookup works 
correctly on pressing Ctrl+Command+D for selectable labels.
==========

to

==========
MacViews: Enable word lookup for selectable views::Labels and multi-line text.

r423419 added support for word lookups to textfields on MacViews and r441290
added support for multi-line text selection to RenderTextHarfBuzz. This CL
enables word lookup for selectable views::Labels. Changes:
  - Made views::Label inherit from WordLookupClient.
  - Moved GetLineContainingYCoord and GetLineSegmentContainingXCoord from
    RenderTextHarfBuzz to the base RenderText class.
  - Added multi-line support to RenderText::GetDecoratedWordAtPoint.

BUG=640502,671055

TEST=Run out/Default/views_examples_with_content_exe with the flag --enable-
harfbuzz-rendertext. Go to the Label section. Ensure word lookup works correctly
on pressing Ctrl+Command+D for selectable labels.
==========

[karandeepb, 2017-01-17 04:07:29]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 04:07:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[karandeepb, 2017-01-17 04:20:12]

karandeepb@chromium.org changed reviewers:
+ tapted@chromium.org

[karandeepb, 2017-01-17 04:20:13]

PTAL Trent. I am assuming we want this eventually? Also, I think it makes sense
to do this only for selectable labels, wdyt?

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

https://codereview.chromium.org/2639493002/diff/1/ui/gfx/render_text.cc#newco...
ui/gfx/render_text.cc:1059: if (line_index == -1 || line_index ==
static_cast<int>(lines().size()))
Don't think this should happen, but still added the check to stay safe.

[tapted, 2017-01-17 20:49:00]

Looks pretty good - do you think there's good test coverage of this already? I
got the impression: not. We probably need a GetDecoratedWordAtPoint_Multiline
test or similar

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

https://codereview.chromium.org/2639493002/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1031: bool RenderText::GetDecoratedWordAtPoint(const
Point& point,
Note we also need a solution for doing a lookup via context menu from a
gfx::Range selection -- see
https://codereview.chromium.org/2164483006/diff/850001/ui/gfx/render_text.cc

https://codereview.chromium.org/2639493002/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1407: int RenderText::GetLineSegmentContainingXCoord(const
internal::Line& line,
can this just go in an anonymous namespace? I can't see any data members being
accessed.

[karandeepb, 2017-01-19 03:19:46]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 03:20:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 04:20:50]

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

[commit-bot: I haz the power, 2017-01-19 04:20:51]

Dry run: This issue passed the CQ dry run.

[karandeepb, 2017-01-19 04:38:36]

PTAL Trent.

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

https://codereview.chromium.org/2639493002/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1031: bool RenderText::GetDecoratedWordAtPoint(const
Point& point,
On 2017/01/17 20:48:59, tapted wrote:
> Note we also need a solution for doing a lookup via context menu from a
> gfx::Range selection -- see
> https://codereview.chromium.org/2164483006/diff/850001/ui/gfx/render_text.cc

Yeah shouldn't be difficult to incorporate these changes.

https://codereview.chromium.org/2639493002/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1407: int RenderText::GetLineSegmentContainingXCoord(const
internal::Line& line,
On 2017/01/17 20:48:59, tapted wrote:
> can this just go in an anonymous namespace? I can't see any data members being
> accessed.

It's similar in concept to GetLineContainingYCoord, so prefer to keep it here
with it. If you want, can move it to an anon namespace in
render_text_harfbuzz.cc.

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

https://codereview.chromium.org/2639493002/diff/40001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1059: GetLineOffset(0).y());
The logic in the previous patchset was indeed incorrect. Existing tests did
catch this in Patchset 1, but didn't see it earlier.

[tapted, 2017-01-20 06:24:50]

I'll probably be in transit, so don't wait for me again to loop in owners - it
looks pretty good

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

https://codereview.chromium.org/2639493002/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1407: int RenderText::GetLineSegmentContainingXCoord(const
internal::Line& line,
On 2017/01/19 04:38:36, karandeepb wrote:
> On 2017/01/17 20:48:59, tapted wrote:
> > can this just go in an anonymous namespace? I can't see any data members
being
> > accessed.
> 
> It's similar in concept to GetLineContainingYCoord, so prefer to keep it here
> with it. If you want, can move it to an anon namespace in
> render_text_harfbuzz.cc.

I see it as more of a helper function. I think having them both as members is
misleading, it suggests to me that GetLineSegmentContainingXCoord should also
look in all lines in lines(), rather than just the provided |line|

https://codereview.chromium.org/2639493002/diff/60001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2639493002/diff/60001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:480: }
Can we keep an overload?
i.e.

Rect GetSelectionBoundsUnion() {
  return GetSubstringBoundsUnionForRange(render_text_->selection());
}

https://codereview.chromium.org/2639493002/diff/60001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:4215: SCOPED_TRACE(base::StringPrintf("Query to
the left of first line."));
This can just be a comment I think -- SCOPED_TRACE is useful if there is a loop
of function call, since then you can't tell just from the line number which test
case is failing.

[karandeepb, 2017-01-20 07:25:41]

Patchset #5 (id:80001) has been deleted

[karandeepb, 2017-01-20 07:29:11]

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

https://codereview.chromium.org/2639493002/diff/20001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1407: int RenderText::GetLineSegmentContainingXCoord(const
internal::Line& line,
On 2017/01/20 06:24:50, tapted wrote:
> On 2017/01/19 04:38:36, karandeepb wrote:
> > On 2017/01/17 20:48:59, tapted wrote:
> > > can this just go in an anonymous namespace? I can't see any data members
> being
> > > accessed.
> > 
> > It's similar in concept to GetLineContainingYCoord, so prefer to keep it
here
> > with it. If you want, can move it to an anon namespace in
> > render_text_harfbuzz.cc.
> 
> I see it as more of a helper function. I think having them both as members is
> misleading, it suggests to me that GetLineSegmentContainingXCoord should also
> look in all lines in lines(), rather than just the provided |line|

Done.

https://codereview.chromium.org/2639493002/diff/60001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2639493002/diff/60001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:480: }
On 2017/01/20 06:24:50, tapted wrote:
> Can we keep an overload?
> i.e.
> 
> Rect GetSelectionBoundsUnion() {
>   return GetSubstringBoundsUnionForRange(render_text_->selection());
> }

Done.

https://codereview.chromium.org/2639493002/diff/60001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:4215: SCOPED_TRACE(base::StringPrintf("Query to
the left of first line."));
On 2017/01/20 06:24:50, tapted wrote:
> This can just be a comment I think -- SCOPED_TRACE is useful if there is a
loop
> of function call, since then you can't tell just from the line number which
test
> case is failing.

Done.

[karandeepb, 2017-01-20 07:31:05]

karandeepb@chromium.org changed reviewers:
+ msw@chromium.org

[karandeepb, 2017-01-20 07:31:06]

PTAL msw@. Thanks.

[msw, 2017-01-20 16:45:33]

lgtm with nits

https://codereview.chromium.org/2639493002/diff/100001/ui/gfx/render_text_uni...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2639493002/diff/100001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:476: Rect GetSubstringBoundsUnionForRange(const
Range& range) {
optional nit: drop |ForRange|

https://codereview.chromium.org/2639493002/diff/100001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:4169: const base::string16 text = ASCIIToUTF16("a
b\n..\nc.");
nit: make at least one of these words multi-character

https://codereview.chromium.org/2639493002/diff/100001/ui/views/controls/label.h
File ui/views/controls/label.h (right):

https://codereview.chromium.org/2639493002/diff/100001/ui/views/controls/labe...
ui/views/controls/label.h:26: public WordLookupClient,
nit: it'd be nice if we could use composition instead of inheritance, but it's
not a blocker.

[tapted, 2017-01-20 20:19:17]

lgtm - thanks!

[karandeepb, 2017-01-23 00:20:25]

https://codereview.chromium.org/2639493002/diff/100001/ui/gfx/render_text_uni...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2639493002/diff/100001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:476: Rect GetSubstringBoundsUnionForRange(const
Range& range) {
On 2017/01/20 16:45:32, msw (out 1-20) wrote:
> optional nit: drop |ForRange|

Done.

https://codereview.chromium.org/2639493002/diff/100001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:4169: const base::string16 text = ASCIIToUTF16("a
b\n..\nc.");
On 2017/01/20 16:45:32, msw (out 1-20) wrote:
> nit: make at least one of these words multi-character

Done.

https://codereview.chromium.org/2639493002/diff/100001/ui/views/controls/label.h
File ui/views/controls/label.h (right):

https://codereview.chromium.org/2639493002/diff/100001/ui/views/controls/labe...
ui/views/controls/label.h:26: public WordLookupClient,
On 2017/01/20 16:45:32, msw (out 1-20) wrote:
> nit: it'd be nice if we could use composition instead of inheritance, but it's
> not a blocker.

That may turn out to be a bit more verbose.

[karandeepb, 2017-01-23 00:20:46]

The CQ bit was checked by karandeepb@chromium.org

[karandeepb, 2017-01-23 00:20:47]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2639493002/#ps120001
(title: "Address nits.")

[commit-bot: I haz the power, 2017-01-23 00:20:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 01:57:31]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485130846883430,
"parent_rev": "3e95a159520fb979684df2276ab86f1da74c3704", "commit_rev":
"666e4f0298298427d2026c2896466951f199f5bf"}

[commit-bot: I haz the power, 2017-01-23 01:58:25]

Description was changed from

==========
MacViews: Enable word lookup for selectable views::Labels and multi-line text.

r423419 added support for word lookups to textfields on MacViews and r441290
added support for multi-line text selection to RenderTextHarfBuzz. This CL
enables word lookup for selectable views::Labels. Changes:
  - Made views::Label inherit from WordLookupClient.
  - Moved GetLineContainingYCoord and GetLineSegmentContainingXCoord from
    RenderTextHarfBuzz to the base RenderText class.
  - Added multi-line support to RenderText::GetDecoratedWordAtPoint.

BUG=640502,671055

TEST=Run out/Default/views_examples_with_content_exe with the flag --enable-
harfbuzz-rendertext. Go to the Label section. Ensure word lookup works correctly
on pressing Ctrl+Command+D for selectable labels.
==========

to

==========
MacViews: Enable word lookup for selectable views::Labels and multi-line text.

r423419 added support for word lookups to textfields on MacViews and r441290
added support for multi-line text selection to RenderTextHarfBuzz. This CL
enables word lookup for selectable views::Labels. Changes:
  - Made views::Label inherit from WordLookupClient.
  - Moved GetLineContainingYCoord and GetLineSegmentContainingXCoord from
    RenderTextHarfBuzz to the base RenderText class.
  - Added multi-line support to RenderText::GetDecoratedWordAtPoint.

BUG=640502,671055

TEST=Run out/Default/views_examples_with_content_exe with the flag --enable-
harfbuzz-rendertext. Go to the Label section. Ensure word lookup works correctly
on pressing Ctrl+Command+D for selectable labels.

Review-Url: https://codereview.chromium.org/2639493002
Cr-Commit-Position: refs/heads/master@{#445313}
Committed:
https://chromium.googlesource.com/chromium/src/+/666e4f0298298427d2026c289646...
==========

[commit-bot: I haz the power, 2017-01-23 01:58:26]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/666e4f0298298427d2026c289646...