Rename some functions about layout locations

Rename some functions about layout locations

After the renaming, we'll have the following terms:

- physicalLocation: physical location of a box or an inline in the
  containing block.  (0,0) is the top-left corner of the containing
  block.

  Renamed LayoutBox::topLeftLocation() and
  InlineBox::locationIncludingFlipping() to physicalLocation().

- location: location in "physical coordinates with flipped blocks
  direction". (0,0) is the top-left corner of the containing block
  for writing-mode in normal blocks direction (horizontal-tb and
  vertical-lr), and is the top-right corner of the containing block
  for writing-mode in flipped block direction (vertical-rl).

  Renamed InlineBox::topLeft() to location() to keep consistence
  with LayoutBox.

Removed InlineBox::top(), left(), right(), bottom().

BTW removed LayoutInline::logicalPositionToPhysicalPoint() which was
not properly named because the input is not "logical" position but
"physical position in flipped blocks direction". Also it was redundant.
Replaced the callsites with flipForWritingMode().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=

Committed: https://crrev.com/6085e846342287cc5a8531abe99c838687a10620
Cr-Commit-Position: refs/heads/master@{#434900}

[Xianzhu, 2016-11-26 02:08:45]

Description was changed from

==========
Rename some functions about layout locations

After the renaming, we'll have the following terms:

- physicalLocation: physical location of a box or an inline in the
  containing block.  (0,0) is the top-left corner of the containing
  block.

  Renamed LayoutBox::topLeftLocation() and
  InlineBox::locationIncludingFlipping() to physicalLocation().

- location: location in "physical coordinates with flipped blocks
  direction". (0,0) is the top-left corner of the containing block
  for writing-mode in normal blocks direction (horizontal-tb and
  vertical-lr), and is the top-right corner of the containing block
  for writing-mode in flipped block direction (vertical-rl).

  Renamed InlineBox::topLeft() to location() to keep consistence
  with LayoutBox.

BTW removed LayoutInline::logicalPositionToPhysicalPoint() which was
not properly named because the input is not "logical" position but
"physical position in flipped blocks direction". Also it was redundant.
Replaced the callsites with flipForWritingMode().
==========

to

==========
Rename some functions about layout locations

After the renaming, we'll have the following terms:

- physicalLocation: physical location of a box or an inline in the
  containing block.  (0,0) is the top-left corner of the containing
  block.

  Renamed LayoutBox::topLeftLocation() and
  InlineBox::locationIncludingFlipping() to physicalLocation().

- location: location in "physical coordinates with flipped blocks
  direction". (0,0) is the top-left corner of the containing block
  for writing-mode in normal blocks direction (horizontal-tb and
  vertical-lr), and is the top-right corner of the containing block
  for writing-mode in flipped block direction (vertical-rl).

  Renamed InlineBox::topLeft() to location() to keep consistence
  with LayoutBox.

BTW removed LayoutInline::logicalPositionToPhysicalPoint() which was
not properly named because the input is not "logical" position but
"physical position in flipped blocks direction". Also it was redundant.
Replaced the callsites with flipForWritingMode().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-11-26 02:08:52]

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

[commit-bot: I haz the power, 2016-11-26 02:08:59]

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

[Xianzhu, 2016-11-26 02:39:00]

Description was changed from

==========
Rename some functions about layout locations

After the renaming, we'll have the following terms:

- physicalLocation: physical location of a box or an inline in the
  containing block.  (0,0) is the top-left corner of the containing
  block.

  Renamed LayoutBox::topLeftLocation() and
  InlineBox::locationIncludingFlipping() to physicalLocation().

- location: location in "physical coordinates with flipped blocks
  direction". (0,0) is the top-left corner of the containing block
  for writing-mode in normal blocks direction (horizontal-tb and
  vertical-lr), and is the top-right corner of the containing block
  for writing-mode in flipped block direction (vertical-rl).

  Renamed InlineBox::topLeft() to location() to keep consistence
  with LayoutBox.

BTW removed LayoutInline::logicalPositionToPhysicalPoint() which was
not properly named because the input is not "logical" position but
"physical position in flipped blocks direction". Also it was redundant.
Replaced the callsites with flipForWritingMode().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Rename some functions about layout locations

After the renaming, we'll have the following terms:

- physicalLocation: physical location of a box or an inline in the
  containing block.  (0,0) is the top-left corner of the containing
  block.

  Renamed LayoutBox::topLeftLocation() and
  InlineBox::locationIncludingFlipping() to physicalLocation().

- location: location in "physical coordinates with flipped blocks
  direction". (0,0) is the top-left corner of the containing block
  for writing-mode in normal blocks direction (horizontal-tb and
  vertical-lr), and is the top-right corner of the containing block
  for writing-mode in flipped block direction (vertical-rl).

  Renamed InlineBox::topLeft() to location() to keep consistence
  with LayoutBox.

Removed InlineBox::top(), left(), right(), bottom().

BTW removed LayoutInline::logicalPositionToPhysicalPoint() which was
not properly named because the input is not "logical" position but
"physical position in flipped blocks direction". Also it was redundant.
Replaced the callsites with flipForWritingMode().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=
==========

[Xianzhu, 2016-11-26 02:40:52]

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

[commit-bot: I haz the power, 2016-11-26 02:41:02]

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, 2016-11-26 04:17:48]

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

[commit-bot: I haz the power, 2016-11-26 04:17:48]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Xianzhu, 2016-11-27 04:31:07]

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

[commit-bot: I haz the power, 2016-11-27 04:31:19]

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, 2016-11-27 06:03:57]

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

[commit-bot: I haz the power, 2016-11-27 06:03:57]

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_...)

[Xianzhu, 2016-11-27 17:56:24]

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

[commit-bot: I haz the power, 2016-11-27 17:56:38]

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

[Xianzhu, 2016-11-27 18:02:01]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, szager@chromium.org

[commit-bot: I haz the power, 2016-11-27 20:36:01]

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

[commit-bot: I haz the power, 2016-11-27 20:36:02]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-11-28 03:36:27]

pdr@chromium.org changed reviewers:
+ wkorman@chromium.org

[chrishtr, 2016-11-28 15:13:28]

chrishtr@chromium.org changed reviewers:
+ eae@chromium.org

[chrishtr, 2016-11-28 15:13:29]

A nice cleanup. Also adding eae@ because this renames some core methods in
layout positioning.

https://codereview.chromium.org/2536453002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineBox.cpp (left):

https://codereview.chromium.org/2536453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.cpp:351: return
LayoutPoint(point.x(),
This old code looks backward to me now...why is it messing with the y coordinate
in horizontal mode, rather
than x? Was this a bug?

https://codereview.chromium.org/2536453002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineBox.cpp (right):

https://codereview.chromium.org/2536453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.cpp:345: void
InlineBox::logicalRectToPhysicalRect(LayoutRect& current) const {
Please unittest this.

[Xianzhu, 2016-11-28 18:42:36]

https://codereview.chromium.org/2536453002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineBox.cpp (left):

https://codereview.chromium.org/2536453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.cpp:351: return
LayoutPoint(point.x(),
On 2016/11/28 15:13:29, chrishtr wrote:
> This old code looks backward to me now...why is it messing with the y
coordinate
> in horizontal mode, rather
> than x? Was this a bug?

This seems for horizontal-bt which is not supported elsewhere.

https://codereview.chromium.org/2536453002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineBox.cpp (right):

https://codereview.chromium.org/2536453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.cpp:345: void
InlineBox::logicalRectToPhysicalRect(LayoutRect& current) const {
On 2016/11/28 15:13:29, chrishtr wrote:
> Please unittest this.

Done.

[chrishtr, 2016-11-28 19:16:01]

lgtm

Please wait for someone from layout to approve also.

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineBoxTest.cpp (right):

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBoxTest.cpp:32:
EXPECT_EQ(LayoutRect(14, 11, 44, 33), rect);
Could you confirm quickly that this did not change with your patch?

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBoxTest.cpp:43:
EXPECT_EQ(LayoutRect(22, 11, 44, 33), rect);
Same here. Just a little paranoid because I tried last year to do something
similar to this change and it didn't
work.

[Xianzhu, 2016-11-28 20:10:07]

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineBoxTest.cpp (right):

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBoxTest.cpp:32:
EXPECT_EQ(LayoutRect(14, 11, 44, 33), rect);
On 2016/11/28 19:16:01, chrishtr wrote:
> Could you confirm quickly that this did not change with your patch?

Confirmed that these tests passed without the change. I guess that you added
logicalPositionToPhysicalPoint(LayoutPoint, LayoutSize) because
flipForWritingMode(LayoutPoint) didn't work for you. With this change we use
flipForWritingMode(LayoutRect) for the purpose.

[chrishtr, 2016-11-28 21:35:50]

On 2016/11/28 at 20:10:07, wangxianzhu wrote:
>
https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/line/InlineBoxTest.cpp (right):
> 
>
https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/line/InlineBoxTest.cpp:32:
EXPECT_EQ(LayoutRect(14, 11, 44, 33), rect);
> On 2016/11/28 19:16:01, chrishtr wrote:
> > Could you confirm quickly that this did not change with your patch?
> 
> Confirmed that these tests passed without the change. I guess that you added
logicalPositionToPhysicalPoint(LayoutPoint, LayoutSize) because
flipForWritingMode(LayoutPoint) didn't work for you. With this change we use
flipForWritingMode(LayoutRect) for the purpose.

Maybe. I'm not sure. At the time I did it, I didn't understand vertical text
nearly as well as I do now.

[szager1, 2016-11-28 23:03:09]

Most lg, but needs a few more breadcrumbs for the next person...

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.h:227: // coordinates with
flipped blocks direction".
This needs a more detailed explanation.

[wkorman, 2016-11-29 00:02:13]

lgtm

This does look like a nice cleanup. It's always so hard to wrap my head around
these. I think this makes it clearer.

Please update references to topLeftLocation() in core/layout/README.md, and
consider adding a brief paragraph explaining both location() and
physicalLocation() there, as you have them in the CL description.

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.h:227: // coordinates with
flipped blocks direction".
On 2016/11/28 23:03:09, szager1 wrote:
> This needs a more detailed explanation.

Agree, we can add a brief description here and could also refer reader to
core/layout/README.md for more? Or could ref that .md file in the class header
if it's not already there, since it's also relevant for the other methods below.

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.h:232: // coordinates.
Comment should fit with the one for x() above, is this not also in "...with
flipped blocks direction"?

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.h:237: const LayoutPoint&
location() const { return m_location; }
And we should comment this similarly.

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.h:361: LayoutPoint
physicalLocation() const;
And comment this.

[wkorman, 2016-11-29 00:17:00]

Just wanted to note for other readers --

I spent a long time reading this change (probably went through it three times),
mainly due to concern that we might be mixing up location/physicalLocation()
between Inline and Box in some manner. But after a while I realized in code, and
saw again in your change description, that all we're really doing is renaming
Inline m_topLeft and associated methods to be consistent with the new
nomenclature, and then things made sense.

I also hesitated re: removing top(), bottom(), left(), right() since those seem
like appropriate ways to refer to logical positions. See also the section in 

  core/layout/README.md ## Glossaries

which touches on fact that there is a general desire to move to using
spec-relevant nomenclature in method names where we can. I think this change is
fine as-is but eae@/szager@ should consider.

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.h:235: LayoutUnit top()
const { return m_location.y(); }
Do we still need this? Why do we need this but not bottom(), and can we just
change callers to use y()?

[Xianzhu, 2016-11-29 00:48:03]

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

[commit-bot: I haz the power, 2016-11-29 00:49:08]

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

[Xianzhu, 2016-11-29 01:44:18]

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.h:227: // coordinates with
flipped blocks direction".
On 2016/11/29 00:02:13, wkorman wrote:
> On 2016/11/28 23:03:09, szager1 wrote:
> > This needs a more detailed explanation.
> 
> Agree, we can add a brief description here and could also refer reader to
> core/layout/README.md for more? Or could ref that .md file in the class header
> if it's not already there, since it's also relevant for the other methods
below.

Done.

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.h:232: // coordinates.
On 2016/11/29 00:02:13, wkorman wrote:
> Comment should fit with the one for x() above, is this not also in "...with
> flipped blocks direction"?

Added:
It's actually in the same coordinate space as x() but for y() physical
coordinates and "physical coordinates with flipped block-flow direction" are the
same.

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.h:235: LayoutUnit top()
const { return m_location.y(); }
On 2016/11/29 00:17:00, wkorman wrote:
> Do we still need this? Why do we need this but not bottom(), and can we just
> change callers to use y()?

Removed.

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.h:237: const LayoutPoint&
location() const { return m_location; }
On 2016/11/29 00:02:13, wkorman wrote:
> And we should comment this similarly.

Done.

https://codereview.chromium.org/2536453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.h:361: LayoutPoint
physicalLocation() const;
On 2016/11/29 00:02:13, wkorman wrote:
> And comment this.

Done.

[szager1, 2016-11-29 02:07:03]

lgtm

[commit-bot: I haz the power, 2016-11-29 02:54:52]

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

[commit-bot: I haz the power, 2016-11-29 02:54:53]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2016-11-29 03:10:52]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-11-29 03:10:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2536453002/#ps100001
(title: "Update comments")

[Xianzhu, 2016-11-29 03:11:04]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2016-11-29 03:36:11]

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

[commit-bot: I haz the power, 2016-11-29 03:36:34]

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

[Xianzhu, 2016-11-29 04:14:20]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2016-11-29 04:14:20]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-11-29 04:14:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, szager@chromium.org, wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2536453002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-11-29 04:14:34]

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, 2016-11-29 05:13:05]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1480392860918440,
"parent_rev": "4b60955f487bc0077aa798fcba5e9ea69a4b6922", "commit_rev":
"212e46a15c602b4a271b0498d24cbbe9835caa0a"}

[commit-bot: I haz the power, 2016-11-29 05:14:12]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-29 05:19:55]

Description was changed from

==========
Rename some functions about layout locations

After the renaming, we'll have the following terms:

- physicalLocation: physical location of a box or an inline in the
  containing block.  (0,0) is the top-left corner of the containing
  block.

  Renamed LayoutBox::topLeftLocation() and
  InlineBox::locationIncludingFlipping() to physicalLocation().

- location: location in "physical coordinates with flipped blocks
  direction". (0,0) is the top-left corner of the containing block
  for writing-mode in normal blocks direction (horizontal-tb and
  vertical-lr), and is the top-right corner of the containing block
  for writing-mode in flipped block direction (vertical-rl).

  Renamed InlineBox::topLeft() to location() to keep consistence
  with LayoutBox.

Removed InlineBox::top(), left(), right(), bottom().

BTW removed LayoutInline::logicalPositionToPhysicalPoint() which was
not properly named because the input is not "logical" position but
"physical position in flipped blocks direction". Also it was redundant.
Replaced the callsites with flipForWritingMode().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=
==========

to

==========
Rename some functions about layout locations

After the renaming, we'll have the following terms:

- physicalLocation: physical location of a box or an inline in the
  containing block.  (0,0) is the top-left corner of the containing
  block.

  Renamed LayoutBox::topLeftLocation() and
  InlineBox::locationIncludingFlipping() to physicalLocation().

- location: location in "physical coordinates with flipped blocks
  direction". (0,0) is the top-left corner of the containing block
  for writing-mode in normal blocks direction (horizontal-tb and
  vertical-lr), and is the top-right corner of the containing block
  for writing-mode in flipped block direction (vertical-rl).

  Renamed InlineBox::topLeft() to location() to keep consistence
  with LayoutBox.

Removed InlineBox::top(), left(), right(), bottom().

BTW removed LayoutInline::logicalPositionToPhysicalPoint() which was
not properly named because the input is not "logical" position but
"physical position in flipped blocks direction". Also it was redundant.
Replaced the callsites with flipForWritingMode().
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=

Committed: https://crrev.com/6085e846342287cc5a8531abe99c838687a10620
Cr-Commit-Position: refs/heads/master@{#434900}
==========

[commit-bot: I haz the power, 2016-11-29 05:19:57]

Patchset 7 (id:??) landed as
https://crrev.com/6085e846342287cc5a8531abe99c838687a10620
Cr-Commit-Position: refs/heads/master@{#434900}