Introduce LocalCaretRect for refactoring Local{Caret,Selection}RectPosition()

Introduce LocalCaretRect for refactoring Local{Caret,Selection}RectPosition()

This patch introduces |LocalCaretRect| to use as return value of
|Local{Caret,Selection}RectPosition()| to avoid output-parameter with utilizing
const-variables for improving code health.

BUG=707656
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2926823002
Cr-Commit-Position: refs/heads/master@{#478231}
Committed: https://chromium.googlesource.com/chromium/src/+/c6c973e6a860ef254df119b535653b987d0e9d90

[yosin_UTC9, 2017-06-07 08:26:11]

Patchset #1 (id:1) has been deleted

[yosin_UTC9, 2017-06-07 08:26:16]

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

[commit-bot: I haz the power, 2017-06-07 08:26:31]

Dry run: CQ is trying da patch.

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

[yosin_UTC9, 2017-06-07 08:28:43]

Description was changed from

==========
2017-06-07T17:03:47

BUG=707656

2017-06-07T17:03:26
==========

to

==========
Introduce LocalCaretRect for refactoring Local{Caret,Selection}RectPosition()

This patch introduces |LocalCaretRect| to use as return value of
|Local{Caret,Selection}RectPosition()| to avoid output-parameter with utilizing
const-variables for improving code health.

BUG=707656
TEST=n/a; no behavior changes
==========

[commit-bot: I haz the power, 2017-06-07 09:33:03]

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

[commit-bot: I haz the power, 2017-06-07 09:33:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yosin_UTC9, 2017-06-07 10:08:17]

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

[commit-bot: I haz the power, 2017-06-07 10:08:25]

Dry run: CQ is trying da patch.

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

[yosin_UTC9, 2017-06-07 10:08:52]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-06-07 11:46:48]

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

[commit-bot: I haz the power, 2017-06-07 11:46:49]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-06-08 05:27:06]

yosin@chromium.org changed reviewers:
+ xiaochengh@chromium.org, yoichio@chromium.org

[yosin_UTC9, 2017-06-08 05:27:07]

PTAL

[Xiaocheng, 2017-06-08 21:00:42]

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

https://codereview.chromium.org/2926823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1282: if
(!node->GetLayoutObject())
We should store

LayoutObject* const layout_object = node->GetLayoutObject();

here to avoid repeated calls of Node::GetLayoutObject().

https://codereview.chromium.org/2926823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1289: return
ComputeLocalCaretRect(node->GetLayoutObject(), box_position);
We can use early return style without switching the if and else branches.

Let's do that to be git log friendly.

https://codereview.chromium.org/2926823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1312: return
LocalCaretRect();
The original code returns a non-null LayoutObject.

Let's not change that.

https://codereview.chromium.org/2926823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1321: return
LocalCaretRect();
Ditto.

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

https://codereview.chromium.org/2926823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.h:77: FloatQuad
LocalToAbsoluteQuad() const;
This function is only used within VisibleUnits.cpp.

If it's not going to be used by follow-up patches, I prefer hiding it as a
static function.

[yosin_UTC9, 2017-06-09 03:50:59]

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

[commit-bot: I haz the power, 2017-06-09 03:51:12]

Dry run: CQ is trying da patch.

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

[yosin_UTC9, 2017-06-09 03:53:06]

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

[commit-bot: I haz the power, 2017-06-09 03:53:12]

Dry run: CQ is trying da patch.

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

[Xiaocheng, 2017-06-09 05:58:39]

As discussed in crrev.com/2925323004, this patch doesn't change any behavior.

So, lgtm with a nit.

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

https://codereview.chromium.org/2926823002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1009: return
ComputeLocalCaretRect(node->GetLayoutObject(), box_position);
nit: s/node->GetLayoutObject()/layout_object/

[yosin_UTC9, 2017-06-09 06:13:29]

Thanks for reviewing!
Committing...

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

https://codereview.chromium.org/2926823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1282: if
(!node->GetLayoutObject())
On 2017/06/08 at 21:00:42, Xiaocheng wrote:
> We should store
> 
> LayoutObject* const layout_object = node->GetLayoutObject();
> 
> here to avoid repeated calls of Node::GetLayoutObject().

Done.

https://codereview.chromium.org/2926823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1289: return
ComputeLocalCaretRect(node->GetLayoutObject(), box_position);
On 2017/06/08 at 21:00:41, Xiaocheng wrote:
> We can use early return style without switching the if and else branches.
> 
> Let's do that to be git log friendly.

Done.

https://codereview.chromium.org/2926823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1312: return
LocalCaretRect();
On 2017/06/08 at 21:00:42, Xiaocheng wrote:
> The original code returns a non-null LayoutObject.
> 
> Let's not change that.

I create another patch to return nullptr for layout object.
http://crrev.com/2925323004

https://codereview.chromium.org/2926823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1321: return
LocalCaretRect();
On 2017/06/08 at 21:00:42, Xiaocheng wrote:
> Ditto.

I create another patch to return nullptr for layout object.
http://crrev.com/2925323004

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

https://codereview.chromium.org/2926823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.h:77: FloatQuad
LocalToAbsoluteQuad() const;
On 2017/06/08 at 21:00:42, Xiaocheng wrote:
> This function is only used within VisibleUnits.cpp.
> 
> If it's not going to be used by follow-up patches, I prefer hiding it as a
static function.

Done.

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

https://codereview.chromium.org/2926823002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/VisibleUnits.cpp:1009: return
ComputeLocalCaretRect(node->GetLayoutObject(), box_position);
On 2017/06/09 at 05:58:38, Xiaocheng wrote:
> nit: s/node->GetLayoutObject()/layout_object/

Done.

[yosin_UTC9, 2017-06-09 06:13:32]

The CQ bit was checked by yosin@chromium.org

[yosin_UTC9, 2017-06-09 06:13:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
xiaochengh@chromium.org
Link to the patchset: https://codereview.chromium.org/2926823002/#ps80001
(title: "2017-06-09T15:11:53")

[commit-bot: I haz the power, 2017-06-09 06:13:48]

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-06-09 07:51:59]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496988812028860,
"parent_rev": "0d263dc80d9db101a9eba1e73f44ed81e73c5609", "commit_rev":
"c6c973e6a860ef254df119b535653b987d0e9d90"}

[commit-bot: I haz the power, 2017-06-09 07:52:12]

Description was changed from

==========
Introduce LocalCaretRect for refactoring Local{Caret,Selection}RectPosition()

This patch introduces |LocalCaretRect| to use as return value of
|Local{Caret,Selection}RectPosition()| to avoid output-parameter with utilizing
const-variables for improving code health.

BUG=707656
TEST=n/a; no behavior changes
==========

to

==========
Introduce LocalCaretRect for refactoring Local{Caret,Selection}RectPosition()

This patch introduces |LocalCaretRect| to use as return value of
|Local{Caret,Selection}RectPosition()| to avoid output-parameter with utilizing
const-variables for improving code health.

BUG=707656
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2926823002
Cr-Commit-Position: refs/heads/master@{#478231}
Committed:
https://chromium.googlesource.com/chromium/src/+/c6c973e6a860ef254df119b53565...
==========

[commit-bot: I haz the power, 2017-06-09 07:52:14]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c6c973e6a860ef254df119b53565...