[blink/mac] Support getting baseline point for showing dictionary lookup bubble.

[blink/mac] Support getting baseline point for showing dictionary lookup bubble.

TBR=jochen@chromium.org
BUG=121917
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=202504

[Shu Chen, 2015-09-09 04:46:20]

shuchen@chromium.org changed reviewers:
+ jochen@chromium.org

[Shu Chen, 2015-09-09 04:46:20]

jochen@, can you please help to review this cl? Thanks

[Shu Chen, 2015-09-09 04:47:34]

On 2015/09/09 04:46:20, Shu Chen wrote:
> jochen@, can you please help to review this cl? Thanks

Btw, the newly added function will be used in this cl:
https://codereview.chromium.org/1318483007.

[jochen (gone - plz use gerrit), 2015-09-09 14:24:44]

jochen@chromium.org changed reviewers:
+ eae@chromium.org, esprehn@chromium.org

[jochen (gone - plz use gerrit), 2015-09-09 14:24:44]

sorry, I don't know enough about this code to review it. Elliot or Emil, is this
something you could review?

[eae, 2015-09-09 16:56:05]

https://codereview.chromium.org/1329103002/diff/40001/Source/web/mac/WebSubst...
File Source/web/mac/WebSubstringUtil.mm (right):

https://codereview.chromium.org/1329103002/diff/40001/Source/web/mac/WebSubst...
Source/web/mac/WebSubstringUtil.mm:116: // TODO(yosin) We shold avoid to create
|Range| object.
Could you please file a bug about this and add a link here?

https://codereview.chromium.org/1329103002/diff/40001/Source/web/mac/WebSubst...
Source/web/mac/WebSubstringUtil.mm:119: stringPoint.setY(frameView->height() -
stringPoint.y());
You take the bottom left corner of the bounding rect for the range and then
adjust it by the height of the frame view. What type of coordinate does that
give you? I know this code was just moved but an explanation here would help.

[Shu Chen, 2015-09-10 01:14:07]

https://codereview.chromium.org/1329103002/diff/40001/Source/web/mac/WebSubst...
File Source/web/mac/WebSubstringUtil.mm (right):

https://codereview.chromium.org/1329103002/diff/40001/Source/web/mac/WebSubst...
Source/web/mac/WebSubstringUtil.mm:116: // TODO(yosin) We shold avoid to create
|Range| object.
On 2015/09/09 16:56:04, eae wrote:
> Could you please file a bug about this and add a link here?

Done.

https://codereview.chromium.org/1329103002/diff/40001/Source/web/mac/WebSubst...
Source/web/mac/WebSubstringUtil.mm:119: stringPoint.setY(frameView->height() -
stringPoint.y());
On 2015/09/09 16:56:04, eae wrote:
> You take the bottom left corner of the bounding rect for the range and then
> adjust it by the height of the frame view. What type of coordinate does that
> give you? I know this code was just moved but an explanation here would help.

Actually, I don't understand why adjust y() by the height of the frame view.
The y() will be adjusted back when using it at here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re....

The point type is changed as IntPoint -> WebPoint -> gfx::Point -> CGPoint ->
NSPoint.
I suspect the computation on certain point type requires that strange adjust.

Do you have any ideas?

[esprehn, 2015-09-10 03:01:48]

Why does this take a pointer but the other one take a reference? I'd be
nice if this was consistent.

Also this seems like a strange API to expose out of blink. Should this be
in base?

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

[Shu Chen, 2015-09-10 03:23:28]

On 2015/09/10 03:01:48, esprehn wrote:
> Why does this take a pointer but the other one take a reference? I'd be
> nice if this was consistent.
> 
> Also this seems like a strange API to expose out of blink. Should this be
> in base?

I am not familiar with blink, so I try to minimize the change as much as
possible.
So I am not sure whether it is legal to "expose out of blink" in non-base.

For the reference/point issue, the purpose is to share the code and avoid
breakage.
attributedSubstringInRange() is called in content::TextInputClientObserver.
So the method definition need to remain the same to avoid breakage.
However, we need to support giving out the baseline point from range.
To share the code with the original attributedSubstringInRange() method, I made
the
parameter as pointer so that the new attributedSubstringInRange method can
simply pass nil.

This cl cannot just change the other method attributedWordAtPoint() to use
pointer parameter to avoid breakage.

Please refer to cl https://codereview.chromium.org/1318483007 for the usage of
this blink change.

[Shu Chen, 2015-09-10 03:26:44]

On 2015/09/10 03:23:28, Shu Chen wrote:
> On 2015/09/10 03:01:48, esprehn wrote:
> > Why does this take a pointer but the other one take a reference? I'd be
> > nice if this was consistent.
> > 
> > Also this seems like a strange API to expose out of blink. Should this be
> > in base?
> 
> I am not familiar with blink, so I try to minimize the change as much as
> possible.
> So I am not sure whether it is legal to "expose out of blink" in non-base.
> 
> For the reference/point issue, the purpose is to share the code and avoid
> breakage.
> attributedSubstringInRange() is called in content::TextInputClientObserver.
> So the method definition need to remain the same to avoid breakage.
> However, we need to support giving out the baseline point from range.
> To share the code with the original attributedSubstringInRange() method, I
made
> the
> parameter as pointer so that the new attributedSubstringInRange method can
> simply pass nil.
> 
> This cl cannot just change the other method attributedWordAtPoint() to use
> pointer parameter to avoid breakage.
> 
> Please refer to cl https://codereview.chromium.org/1318483007 for the usage of
> this blink change.

Btw, being pointer for |baselinePoint| is a temporary solution. Eventually,
there
should be only one attributedSubstringInRange() method, and the |baselinePoint|
param should be in reference type.

[Shu Chen, 2015-09-15 01:16:29]

Pinging...

[Shu Chen, 2015-09-18 01:53:55]

shuchen@chromium.org changed reviewers:
- jochen@chromium.org

[Shu Chen, 2015-09-18 01:53:56]

Pinging for reivew. 

eae@/esprehn@, mind taking another look? Thanks

[eae, 2015-09-18 02:02:16]

LGTM but please allow Elliot to comment before landing.

[esprehn, 2015-09-18 02:34:00]

lgtm w/ nits.

https://codereview.chromium.org/1329103002/diff/60001/Source/web/mac/WebSubst...
File Source/web/mac/WebSubstringUtil.mm (right):

https://codereview.chromium.org/1329103002/diff/60001/Source/web/mac/WebSubst...
Source/web/mac/WebSubstringUtil.mm:125: if (font)
We normally write this as a single statement.

if (NSFont* font = [attributes objectForKey:NSFontAttributeName])
  stringPoint.move(0, ceil(-[font descender]));

https://codereview.chromium.org/1329103002/diff/60001/Source/web/mac/WebSubst...
Source/web/mac/WebSubstringUtil.mm:172: NSAttributedString* ret =
attributedSubstringFromRange(ephemeralRange);
don't abbreviate. 

NSAttributedString* result = ...;

[Shu Chen, 2015-09-18 02:39:37]

Thanks for you review!

https://codereview.chromium.org/1329103002/diff/60001/Source/web/mac/WebSubst...
File Source/web/mac/WebSubstringUtil.mm (right):

https://codereview.chromium.org/1329103002/diff/60001/Source/web/mac/WebSubst...
Source/web/mac/WebSubstringUtil.mm:125: if (font)
On 2015/09/18 02:34:00, esprehn wrote:
> We normally write this as a single statement.
> 
> if (NSFont* font = [attributes objectForKey:NSFontAttributeName])
>   stringPoint.move(0, ceil(-[font descender]));

Done.

https://codereview.chromium.org/1329103002/diff/60001/Source/web/mac/WebSubst...
Source/web/mac/WebSubstringUtil.mm:172: NSAttributedString* ret =
attributedSubstringFromRange(ephemeralRange);
On 2015/09/18 02:34:00, esprehn wrote:
> don't abbreviate. 
> 
> NSAttributedString* result = ...;

Done.

[Shu Chen, 2015-09-18 02:39:57]

The CQ bit was checked by shuchen@chromium.org

[Shu Chen, 2015-09-18 02:39:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1329103002/#ps80001
(title: "nits.")

[commit-bot: I haz the power, 2015-09-18 02:40:36]

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

[commit-bot: I haz the power, 2015-09-18 02:51:37]

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

[commit-bot: I haz the power, 2015-09-18 02:51:38]

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

[Shu Chen, 2015-09-18 02:59:43]

TBR'ing jochen@ for rubber stamp on WebSubstringUtil.h.

[Shu Chen, 2015-09-18 02:59:47]

The CQ bit was checked by shuchen@chromium.org

[Shu Chen, 2015-09-18 02:59:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1329103002/#ps100001
(title: "avoid |const NSClass*|")

[commit-bot: I haz the power, 2015-09-18 03:00:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1329103002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1329103002/100001

[commit-bot: I haz the power, 2015-09-18 04:41:50]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202504