Add absoluteSelectionBoundsOf() to GranularityStrategy class

Add absoluteSelectionBoundsOf() to GranularityStrategy class

This function is added because the caret rect that will be
calculated by using the line top value instead of the
selection top in LayoutText::localCaretRect(). This change
will be done in https://codereview.chromium.org/2517383002/

BUG=581004
Review-Url: https://codereview.chromium.org/2616103003
Cr-Commit-Position: refs/heads/master@{#442688}
Committed: https://chromium.googlesource.com/chromium/src/+/85eb48a8bd8ebac0047002d2e2ba266fc7cdaae5

[joone, 2017-01-06 19:31:36]

Description was changed from

==========
Add absoluteSelectionBoundsOf() to GranularityStrategy class

This function is added because the selection rect will be different
from the caret rect that will be calcaulated by using the line top
value instead of the selection top in LayoutText::localCaretRect().

This change will be done in https://codereview.chromium.org/2517383002/

BUG=581004
==========

to

==========
Add absoluteSelectionBoundsOf() to GranularityStrategy class

This function is added because the caret rect that will be
calculated by using the line top value instead of the
selection top in LayoutText::localCaretRect().

This change will be done in https://codereview.chromium.org/2517383002/

BUG=581004
==========

[joone, 2017-01-06 19:48:16]

joone.hur@intel.com changed reviewers:
+ yosin@chromium.org

[joone, 2017-01-06 19:48:16]

Hi yosin, could you review this CL?
It was separated from https://codereview.chromium.org/2517383002/

[joone, 2017-01-06 20:42:16]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 20:42:45]

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-06 22:42:23]

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

[commit-bot: I haz the power, 2017-01-06 22:42:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[joone, 2017-01-06 23:39:15]

Description was changed from

==========
Add absoluteSelectionBoundsOf() to GranularityStrategy class

This function is added because the caret rect that will be
calculated by using the line top value instead of the
selection top in LayoutText::localCaretRect().

This change will be done in https://codereview.chromium.org/2517383002/

BUG=581004
==========

to

==========
Add absoluteSelectionBoundsOf() to GranularityStrategy class

This function is added because the caret rect that will be
calculated by using the line top value instead of the
selection top in LayoutText::localCaretRect(). This change
will be done in https://codereview.chromium.org/2517383002/

BUG=581004
==========

[yosin_UTC9, 2017-01-10 04:20:01]

lgtm w/ small nits

Sorry for late response. I'm back!

https://codereview.chromium.org/2616103003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right):

https://codereview.chromium.org/2616103003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2572:
rect.setHeight(box->root().selectionHeight());
nit: early return style is better

if (layoutObject->style()->isHorizontalWritingMode()) {
  rect.setY ...
  rect.stHeight ...
  return rect;
}

rect.setX(box->root().selectionTop());
rect.setWidth(box->root().selectionHeight());
return rect;

[joone, 2017-01-10 18:14:17]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 18:14:49]

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

[joone, 2017-01-10 18:19:46]

Thanks for review!

https://codereview.chromium.org/2616103003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right):

https://codereview.chromium.org/2616103003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2572:
rect.setHeight(box->root().selectionHeight());
On 2017/01/10 04:20:01, Yosi_UTC9 wrote:
> nit: early return style is better
> 
> if (layoutObject->style()->isHorizontalWritingMode()) {
>   rect.setY ...
>   rect.stHeight ...
>   return rect;
> }
> 
> rect.setX(box->root().selectionTop());
> rect.setWidth(box->root().selectionHeight());
> return rect;

Done.

[commit-bot: I haz the power, 2017-01-10 20:16:47]

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

[commit-bot: I haz the power, 2017-01-10 20:16:50]

Dry run: This issue passed the CQ dry run.

[joone, 2017-01-10 21:11:43]

The CQ bit was checked by joone.hur@intel.com

[joone, 2017-01-10 21:11:44]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2616103003/#ps40001
(title: "apply early return")

[commit-bot: I haz the power, 2017-01-10 21:12:04]

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-10 21:16:31]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484082703696990,
"parent_rev": "73b5ae5acf62a62a0782eef02086bd7b2b2b3fe4", "commit_rev":
"85eb48a8bd8ebac0047002d2e2ba266fc7cdaae5"}

[commit-bot: I haz the power, 2017-01-10 21:17:03]

Description was changed from

==========
Add absoluteSelectionBoundsOf() to GranularityStrategy class

This function is added because the caret rect that will be
calculated by using the line top value instead of the
selection top in LayoutText::localCaretRect(). This change
will be done in https://codereview.chromium.org/2517383002/

BUG=581004
==========

to

==========
Add absoluteSelectionBoundsOf() to GranularityStrategy class

This function is added because the caret rect that will be
calculated by using the line top value instead of the
selection top in LayoutText::localCaretRect(). This change
will be done in https://codereview.chromium.org/2517383002/

BUG=581004

Review-Url: https://codereview.chromium.org/2616103003
Cr-Commit-Position: refs/heads/master@{#442688}
Committed:
https://chromium.googlesource.com/chromium/src/+/85eb48a8bd8ebac0047002d2e2ba...
==========

[commit-bot: I haz the power, 2017-01-10 21:17:04]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/85eb48a8bd8ebac0047002d2e2ba...

[yosin_UTC9, 2017-01-19 06:28:21]

It seems to computeInlineBoxPosition() returns null-InlineBox with DIV with
collapsed whitespace and pass a position in collapsed whitespace or at BR?

<div>   foo  </div>
...........^


  if (!layoutObject->isText()) {
    inlineBox = 0;
    if (canHaveChildrenForEditing(anchorNode) &&
        layoutObject->isLayoutBlockFlow() &&
        hasRenderedNonAnonymousDescendantsWithHeight(layoutObject)) {
      // Try a visually equivalent position with possibly opposite
      // editability. This helps in case |this| is in an editable block
      // but surrounded by non-editable positions. It acts to negate the
      // logic at the beginning of
      // |LayoutObject::createPositionWithAffinity()|.
      PositionTemplate<Strategy> equivalent =
          downstreamIgnoringEditingBoundaries(position);
      if (equivalent == position) {
        equivalent = upstreamIgnoringEditingBoundaries(position);
        if (equivalent == position ||
            downstreamIgnoringEditingBoundaries(equivalent) == position)
          return InlineBoxPosition(inlineBox, caretOffset);
      }

      return computeInlineBoxPosition(equivalent, TextAffinity::Upstream,
                                      primaryDirection);
    }
    if (layoutObject->isBox()) {
      inlineBox = toLayoutBox(layoutObject)->inlineBoxWrapper();
      if (!inlineBox || (caretOffset > inlineBox->caretMinOffset() &&
                         caretOffset < inlineBox->caretMaxOffset()))
        return InlineBoxPosition(inlineBox, caretOffset);
    }

https://codereview.chromium.org/2616103003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/VisibleUnits.cpp (right):

https://codereview.chromium.org/2616103003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:2567:
computeInlineBoxPosition(position.position(), position.affinity());
This is second call of |computeInlineBoxPosition()| which is also called in
|localCaretRectOfPositionTemplate(position, layoutObject)| at L2561.
Since |computeInlineBoxPosition()| is expensive function we should avoid to call
twice.
Also, in ||localCaretRectOfPositionTemplate()|, we check nullability of
boxPosition.inlineBox

  InlineBoxPosition boxPosition =
      computeInlineBoxPosition(position.position(), position.affinity());
  if (boxPosition.inlineBox)
    layoutObject = LineLayoutAPIShim::layoutObjectFrom(
        boxPosition.inlineBox->getLineLayoutItem());

Given that, localSelectionRectOfPositionTemplate() should be something like
below:
Expand localCaretRectOfPositionTemplate() into
localSelectionRectOfPositionTemplate() and calculate BoxPosition once.
I'm not sure when BoxPosition.inlineBox isn't InlineTextBox.

  if (position.isNull()) {
    layoutObject = nullptr;
    return LayoutRect();
  }
  Node* node = position.anchorNode();
  layoutObject = node->layoutObject();
  if (!layoutObject)
    return LayoutRect();
  InlineBoxPosition boxPosition =
      computeInlineBoxPosition(position.position(), position.affinity());
  if (boxPosition.inlineBox)
    layoutObject = LineLayoutAPIShim::layoutObjectFrom(
        boxPosition.inlineBox->getLineLayoutItem());
  Rect rect = layoutObject->localCaretRect(boxPosition.inlineBox,
                                      boxPosition.offsetInBox);

  // What should we do if |boxPosition.inlineBox| isn't InlineTextBox?
  InlineTextBox* box = toInlineTextBox(boxPosition.inlineBox);

  if (layoutObject->style()->isHorizontalWritingMode()) {
    rect.setY(box->root().selectionTop());
    rect.setHeight(box->root().selectionHeight());
    return rect;
  }
  rect.setX(box->root().selectionTop());
  rect.setWidth(box->root().selectionHeight());
  return rect;
}

[joone, 2017-01-20 01:56:57]

yosin@ I've tested your code. It works fine without crash so I will update the
CL soon