Changed TextDirection to an enum class and renamed its members

Changed TextDirection to an enum class and renamed its members

Changed TextDirection to an enum class and gave it an unsigned
underlying type. Also renamed its members to use proper CamelCase
instead of all-caps.

Changing it to an enum class enforces better namespacing and code
practices. Adding the unsigned underlying type is pre-work for when the
class is eventually stored as an enum bitfield (it would be done in this
patch, except a presubmit warning already exists that prevents that. The
presubmit warning needs to be updated before that change can occur.)

Unlike the other enums in ComputedStyleBase, TextDirection will not be
generated. Instead, the enum from platform/ will be included and used.

This patch has no logic changes -- only renames, braces added to long
if-statements, static_casts to unsigned where the old enum type was
implicitly converted, and ASSERTs changed to DCHECKs.

BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/6ed7bd96eb762abd8c2ea7821ee5ec9d5948a592
Cr-Commit-Position: refs/heads/master@{#440043}

[sashab, 2016-12-06 23:55:15]

Description was changed from

==========
Changed TextDirection to an enum class and renamed its members

Changed TextDirection to an enum class and gave it an unsigned
underlying type. Also renamed its members to use proper CamelCase
instead of all-caps.

Changing it to an enum class enforces better namespacing and code
practices. Adding the unsigned underlying type is pre-work for when the
class is eventually stored as an enum bitfield (it would be done in this
patch, except a presubmit warning already exists that prevents that. The
presubmit warning needs to be updated before that change can occur.)

Unlike the other enums in ComputedStyleBase, TextDirection will not be
generated. Instead, the enum from platform/ will be included and used.

This patch has no logic changes -- only renames, braces added to long
if-statements, static_casts to unsigned where the old enum type was
implicitly converted, and ASSERTs changed to DCHECKs.

BUG=628043
==========

to

==========
Changed TextDirection to an enum class and renamed its members

Changed TextDirection to an enum class and gave it an unsigned
underlying type. Also renamed its members to use proper CamelCase
instead of all-caps.

Changing it to an enum class enforces better namespacing and code
practices. Adding the unsigned underlying type is pre-work for when the
class is eventually stored as an enum bitfield (it would be done in this
patch, except a presubmit warning already exists that prevents that. The
presubmit warning needs to be updated before that change can occur.)

Unlike the other enums in ComputedStyleBase, TextDirection will not be
generated. Instead, the enum from platform/ will be included and used.

This patch has no logic changes -- only renames, braces added to long
if-statements, static_casts to unsigned where the old enum type was
implicitly converted, and ASSERTs changed to DCHECKs.

BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[sashab, 2016-12-06 23:55:42]

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

[commit-bot: I haz the power, 2016-12-06 23:56:56]

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-12-07 00:04:27]

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

[commit-bot: I haz the power, 2016-12-07 00:04:29]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[sashab, 2016-12-07 01:54:05]

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

[commit-bot: I haz the power, 2016-12-07 01:54:36]

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

[sashab, 2016-12-07 01:59:41]

sashab@chromium.org changed reviewers:
+ haraken@chromium.org

[sashab, 2016-12-07 01:59:43]

Hi haraken,

Sorry this is a huge patch -- as discussed, this changes TextDirection to an
enum class and gives its members proper naming as per the style guide.

TextDirection will not be generated by ComputedStyle since it will stay in
platform/ but the generator will still need to know the enum's available values.

See the patch description for more details.

Thanks,

Sasha

https://codereview.chromium.org/2555923002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/text/TextDirection.h (right):

https://codereview.chromium.org/2555923002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/TextDirection.h:31: enum class
TextDirection : unsigned { Rtl, Ltr };
Main change is here

https://codereview.chromium.org/2555923002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/text/TextRun.h (right):

https://codereview.chromium.org/2555923002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/text/TextRun.h:244: bool ltr() const { return
direction() == TextDirection::Ltr; }
Changed these to function calls instead of static_casts

[haraken, 2016-12-07 02:08:41]

LGTM

[sashab, 2016-12-07 02:11:07]

sashab@chromium.org changed reviewers:
+ napper@chromium.org

[sashab, 2016-12-07 02:11:08]

9 minutes to review an 800 line patch, I like it! :D

napper@ for follow-up review, lets see if you can beat 100 lines/minute ;)

[commit-bot: I haz the power, 2016-12-07 02:19:12]

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

[commit-bot: I haz the power, 2016-12-07 02:19:12]

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

[sashab, 2016-12-07 02:52:35]

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

[commit-bot: I haz the power, 2016-12-07 02:52:58]

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-12-07 03:37:07]

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

[commit-bot: I haz the power, 2016-12-07 03:37:08]

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

[napper, 2016-12-07 05:27:13]

lgtm

https://codereview.chromium.org/2555923002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2555923002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1200: unsigned
textDirectionIndex = static_cast<unsigned>(textDirection);
const unsigned textDirectionIndex = static_cast<unsigned>(textDirection);

https://codereview.chromium.org/2555923002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTextTest.cpp (right):

https://codereview.chromium.org/2555923002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTextTest.cpp:75: float width =
getBasicText()->width(23u, 4294967282u, LayoutUnit(2.59375),
const float width = ...

https://codereview.chromium.org/2555923002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTextTest.cpp:91: float width =
const float width = ...

[sashab, 2016-12-07 05:39:40]

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

[commit-bot: I haz the power, 2016-12-07 05:39:58]

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

[sashab, 2016-12-07 05:40:26]

Thanks napper :) The mac bots were failing so I added 2 more files to the
change, I've added comments to help you find them, ptal at those

https://codereview.chromium.org/2555923002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2555923002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1200: unsigned
textDirectionIndex = static_cast<unsigned>(textDirection);
On 2016/12/07 at 05:27:13, napper wrote:
> const unsigned textDirectionIndex = static_cast<unsigned>(textDirection);

Done

https://codereview.chromium.org/2555923002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTextTest.cpp (right):

https://codereview.chromium.org/2555923002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTextTest.cpp:75: float width =
getBasicText()->width(23u, 4294967282u, LayoutUnit(2.59375),
On 2016/12/07 at 05:27:13, napper wrote:
> const float width = ...

Done

https://codereview.chromium.org/2555923002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTextTest.cpp:91: float width =
On 2016/12/07 at 05:27:13, napper wrote:
> const float width = ...

Done

https://codereview.chromium.org/2555923002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutThemeMac.mm (left):

https://codereview.chromium.org/2555923002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutThemeMac.mm:488: if (IsOS10_9() &&
style.direction() == RTL)
ptal

https://codereview.chromium.org/2555923002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/ThemePainterMac.mm (left):

https://codereview.chromium.org/2555923002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ThemePainterMac.mm:138: bool isRTL =
o.styleRef().direction() == RTL;
 here

[napper, 2016-12-07 05:45:26]

lgtm

[commit-bot: I haz the power, 2016-12-07 06:16:41]

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

[commit-bot: I haz the power, 2016-12-07 06:16:42]

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

[sashab, 2016-12-07 06:20:09]

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

[commit-bot: I haz the power, 2016-12-07 06:20:23]

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-12-07 06:45:07]

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

[commit-bot: I haz the power, 2016-12-07 06:45:08]

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

[sashab, 2016-12-07 23:11:53]

The CQ bit was checked by sashab@chromium.org

[sashab, 2016-12-07 23:11:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, napper@chromium.org
Link to the patchset: https://codereview.chromium.org/2555923002/#ps120001
(title: "One more tiny fix")

[commit-bot: I haz the power, 2016-12-07 23:12:24]

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-12-08 00:56:26]

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

[commit-bot: I haz the power, 2016-12-08 00:56:28]

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

[sashab, 2016-12-08 23:29:52]

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

[commit-bot: I haz the power, 2016-12-08 23:30:31]

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-12-08 23:45:47]

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

[commit-bot: I haz the power, 2016-12-08 23:45:48]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[sashab, 2016-12-08 23:59:05]

Napper/haraken: Decided not to land this, since I'd rather move towards a
ComputedStyle object that has its own private internal storage data types and
mapping functions to convert to platform/ types. So this patch is not needed...
for now.

[sashab, 2016-12-21 03:28:34]

Re-opening this - decided to support using platform/ enums without mapping
methods after all :) Since we don't need custom storage functionality yet.

[sashab, 2016-12-21 03:42:35]

sashab@chromium.org changed reviewers:
+ alancutter@chromium.org

[sashab, 2016-12-21 03:42:36]

Alancutter@ - I rebased the previous patch and fixed a few minor conflicts.
PTAQL and let me know if all looks generally OK and I'll land it. :)

FYI these were the conflicting files, they should be the only changes from the
previous patch:
#	both modified:   third_party/WebKit/Source/core/layout/LayoutMenuList.cpp
#	both modified:   third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc
#	both modified:   third_party/WebKit/Source/core/layout/ng/ng_inline_node.h
#	both modified:  
third_party/WebKit/Source/core/layout/ng/ng_inline_node_test.cc
#	both modified:   third_party/WebKit/Source/core/paint/ThemePainterDefault.cpp

[alancutter (OOO until 2018), 2016-12-21 04:23:06]

lgtm

[sashab, 2016-12-21 04:24:08]

The CQ bit was checked by sashab@chromium.org

[sashab, 2016-12-21 04:24:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, napper@chromium.org
Link to the patchset: https://codereview.chromium.org/2555923002/#ps140001
(title: "Rebase")

[sashab, 2016-12-21 04:24:18]

The CQ bit was unchecked by sashab@chromium.org

[sashab, 2016-12-21 04:27:22]

The CQ bit was checked by sashab@chromium.org

[sashab, 2016-12-21 04:27:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, napper@chromium.org, alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2555923002/#ps160001
(title: "Rebase after reopen")

[sashab, 2016-12-21 04:27:30]

The CQ bit was unchecked by sashab@chromium.org

[sashab, 2016-12-21 04:27:33]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2016-12-21 04:27:51]

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

[sashab, 2016-12-21 05:30:31]

The CQ bit was unchecked by sashab@chromium.org

[sashab, 2016-12-21 05:30:35]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2016-12-21 05:30:46]

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-12-21 06:24:47]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1482298235093310,
"parent_rev": "ec23bedba9327f8d79d036b86e06ec8dd5cf898d", "commit_rev":
"ccf0357832f0c10ab9fe762da16c6b8cdac02ce0"}

[commit-bot: I haz the power, 2016-12-21 06:25:21]

Description was changed from

==========
Changed TextDirection to an enum class and renamed its members

Changed TextDirection to an enum class and gave it an unsigned
underlying type. Also renamed its members to use proper CamelCase
instead of all-caps.

Changing it to an enum class enforces better namespacing and code
practices. Adding the unsigned underlying type is pre-work for when the
class is eventually stored as an enum bitfield (it would be done in this
patch, except a presubmit warning already exists that prevents that. The
presubmit warning needs to be updated before that change can occur.)

Unlike the other enums in ComputedStyleBase, TextDirection will not be
generated. Instead, the enum from platform/ will be included and used.

This patch has no logic changes -- only renames, braces added to long
if-statements, static_casts to unsigned where the old enum type was
implicitly converted, and ASSERTs changed to DCHECKs.

BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Changed TextDirection to an enum class and renamed its members

Changed TextDirection to an enum class and gave it an unsigned
underlying type. Also renamed its members to use proper CamelCase
instead of all-caps.

Changing it to an enum class enforces better namespacing and code
practices. Adding the unsigned underlying type is pre-work for when the
class is eventually stored as an enum bitfield (it would be done in this
patch, except a presubmit warning already exists that prevents that. The
presubmit warning needs to be updated before that change can occur.)

Unlike the other enums in ComputedStyleBase, TextDirection will not be
generated. Instead, the enum from platform/ will be included and used.

This patch has no logic changes -- only renames, braces added to long
if-statements, static_casts to unsigned where the old enum type was
implicitly converted, and ASSERTs changed to DCHECKs.

BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2555923002
==========

[commit-bot: I haz the power, 2016-12-21 06:25:23]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-21 06:27:28]

Description was changed from

==========
Changed TextDirection to an enum class and renamed its members

Changed TextDirection to an enum class and gave it an unsigned
underlying type. Also renamed its members to use proper CamelCase
instead of all-caps.

Changing it to an enum class enforces better namespacing and code
practices. Adding the unsigned underlying type is pre-work for when the
class is eventually stored as an enum bitfield (it would be done in this
patch, except a presubmit warning already exists that prevents that. The
presubmit warning needs to be updated before that change can occur.)

Unlike the other enums in ComputedStyleBase, TextDirection will not be
generated. Instead, the enum from platform/ will be included and used.

This patch has no logic changes -- only renames, braces added to long
if-statements, static_casts to unsigned where the old enum type was
implicitly converted, and ASSERTs changed to DCHECKs.

BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2555923002
==========

to

==========
Changed TextDirection to an enum class and renamed its members

Changed TextDirection to an enum class and gave it an unsigned
underlying type. Also renamed its members to use proper CamelCase
instead of all-caps.

Changing it to an enum class enforces better namespacing and code
practices. Adding the unsigned underlying type is pre-work for when the
class is eventually stored as an enum bitfield (it would be done in this
patch, except a presubmit warning already exists that prevents that. The
presubmit warning needs to be updated before that change can occur.)

Unlike the other enums in ComputedStyleBase, TextDirection will not be
generated. Instead, the enum from platform/ will be included and used.

This patch has no logic changes -- only renames, braces added to long
if-statements, static_casts to unsigned where the old enum type was
implicitly converted, and ASSERTs changed to DCHECKs.

BUG=628043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/6ed7bd96eb762abd8c2ea7821ee5ec9d5948a592
Cr-Commit-Position: refs/heads/master@{#440043}
==========

[commit-bot: I haz the power, 2016-12-21 06:27:30]

Patchset 9 (id:??) landed as
https://crrev.com/6ed7bd96eb762abd8c2ea7821ee5ec9d5948a592
Cr-Commit-Position: refs/heads/master@{#440043}