[LayoutNG] Implement CSS 'text-indent' property

[LayoutNG] Implement CSS 'text-indent' property

There are a few minor differences from the current layout engine.
1. Tab position is not affected by 'text-indent' in NG. This matches to
   Edge/Gecko.
2. When 'unicode-bidi: plaintext', the heuristic base direction is used
   rather than the specified by the 'direction' property. This matches
   to Gecko (Edge does not implement plaintext).

There are several tests in wpt/css/CSS2/text, but importing them is
blocked by a script problem (crbug.com/727923). I'll look into them
once import restarted.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng

Review-Url: https://codereview.chromium.org/2943983002
Cr-Commit-Position: refs/heads/master@{#481420}
Committed: https://chromium.googlesource.com/chromium/src/+/67f72caa837f24bbb1595c26363daf09e0200f14

[kojii, 2017-06-19 10:25:14]

Description was changed from

==========
indent

BUG=
==========

to

==========
indent

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[kojii, 2017-06-19 13:50:02]

Description was changed from

==========
indent

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Implement CSS 'text-indent' property

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[kojii, 2017-06-21 15:19:38]

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

[commit-bot: I haz the power, 2017-06-21 15:19:50]

Dry run: CQ is trying da patch.

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

[kojii, 2017-06-21 15:20:36]

Patchset #6 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-06-21 15:21:27]

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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kojii, 2017-06-21 15:51:44]

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

[commit-bot: I haz the power, 2017-06-21 15:51:58]

Dry run: CQ is trying da patch.

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

[kojii, 2017-06-21 15:56:51]

Description was changed from

==========
[LayoutNG] Implement CSS 'text-indent' property

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Implement CSS 'text-indent' property

There are a few minor differences from the current layout engine.
1. Tab position is not affected by 'text-indent' in NG. This matches to
   Edge/Gecko.
2. When 'unicode-bidi: plaintext', the heuristic base direction is used
   rather than the specified by the 'direction' property. This matches
   to Gecko (Edge does not implement plaintext).

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[commit-bot: I haz the power, 2017-06-21 16:00:34]

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

[commit-bot: I haz the power, 2017-06-21 16:00:35]

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

[kojii, 2017-06-21 16:01:07]

Description was changed from

==========
[LayoutNG] Implement CSS 'text-indent' property

There are a few minor differences from the current layout engine.
1. Tab position is not affected by 'text-indent' in NG. This matches to
   Edge/Gecko.
2. When 'unicode-bidi: plaintext', the heuristic base direction is used
   rather than the specified by the 'direction' property. This matches
   to Gecko (Edge does not implement plaintext).

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Implement CSS 'text-indent' property

There are a few minor differences from the current layout engine.
1. Tab position is not affected by 'text-indent' in NG. This matches to
   Edge/Gecko.
2. When 'unicode-bidi: plaintext', the heuristic base direction is used
   rather than the specified by the 'direction' property. This matches
   to Gecko (Edge does not implement plaintext).

There are several tests in wpt/css/CSS2/text, but importing them is
blocked by the script problem (crbug.com/727923). I'll look into them
once import restarted.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[kojii, 2017-06-21 16:05:08]

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

[kojii, 2017-06-21 16:05:09]

PTAL.

[eae, 2017-06-21 16:29:50]

LGTM

[kojii, 2017-06-21 18:19:05]

Description was changed from

==========
[LayoutNG] Implement CSS 'text-indent' property

There are a few minor differences from the current layout engine.
1. Tab position is not affected by 'text-indent' in NG. This matches to
   Edge/Gecko.
2. When 'unicode-bidi: plaintext', the heuristic base direction is used
   rather than the specified by the 'direction' property. This matches
   to Gecko (Edge does not implement plaintext).

There are several tests in wpt/css/CSS2/text, but importing them is
blocked by the script problem (crbug.com/727923). I'll look into them
once import restarted.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Implement CSS 'text-indent' property

There are a few minor differences from the current layout engine.
1. Tab position is not affected by 'text-indent' in NG. This matches to
   Edge/Gecko.
2. When 'unicode-bidi: plaintext', the heuristic base direction is used
   rather than the specified by the 'direction' property. This matches
   to Gecko (Edge does not implement plaintext).

There are several tests in wpt/css/CSS2/text, but importing them is
blocked by a script problem (crbug.com/727923). I'll look into them
once import restarted.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[kojii, 2017-06-22 02:10:04]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2017-06-22 02:10:20]

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-22 03:05:58]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1498097404070050,
"parent_rev": "454746093cd6f8d587a0c28818c500846c311cc9", "commit_rev":
"67f72caa837f24bbb1595c26363daf09e0200f14"}

[commit-bot: I haz the power, 2017-06-22 03:06:18]

Description was changed from

==========
[LayoutNG] Implement CSS 'text-indent' property

There are a few minor differences from the current layout engine.
1. Tab position is not affected by 'text-indent' in NG. This matches to
   Edge/Gecko.
2. When 'unicode-bidi: plaintext', the heuristic base direction is used
   rather than the specified by the 'direction' property. This matches
   to Gecko (Edge does not implement plaintext).

There are several tests in wpt/css/CSS2/text, but importing them is
blocked by a script problem (crbug.com/727923). I'll look into them
once import restarted.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] Implement CSS 'text-indent' property

There are a few minor differences from the current layout engine.
1. Tab position is not affected by 'text-indent' in NG. This matches to
   Edge/Gecko.
2. When 'unicode-bidi: plaintext', the heuristic base direction is used
   rather than the specified by the 'direction' property. This matches
   to Gecko (Edge does not implement plaintext).

There are several tests in wpt/css/CSS2/text, but importing them is
blocked by a script problem (crbug.com/727923). I'll look into them
once import restarted.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng

Review-Url: https://codereview.chromium.org/2943983002
Cr-Commit-Position: refs/heads/master@{#481420}
Committed:
https://chromium.googlesource.com/chromium/src/+/67f72caa837f24bbb1595c26363d...
==========

[commit-bot: I haz the power, 2017-06-22 03:06:19]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/67f72caa837f24bbb1595c26363d...

[Nico, 2017-09-15 19:33:55]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2017-09-15 19:33:58]

https://codereview.chromium.org/2943983002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2943983002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1304: return
TextIndentType() == TextIndentType::kNormal ? should_use : !should_use;
Is this meant to call GetTextIndentType()? As-is, this default-constructs an
instance of the TextIndentType() enum (which is always kNormal), so this always
returns should_use.

Maybe this is missing some test coverage?

(Found by this warning which I'm currently evaluating:

../../third_party/WebKit/Source/core/style/ComputedStyle.cpp(1259,70):  error:
code will never be executed [-Werror,-Wunreachable-code]
  return TextIndentType() == TextIndentType::kNormal ? should_use : !should_use;
                                                                     ^~~~~~~~~~
)

[kojii, 2017-09-16 05:25:21]

https://codereview.chromium.org/2943983002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2943983002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1304: return
TextIndentType() == TextIndentType::kNormal ? should_use : !should_use;
On 2017/09/15 at 19:33:57, Nico wrote:
> Is this meant to call GetTextIndentType()? As-is, this default-constructs an
instance of the TextIndentType() enum (which is always kNormal), so this always
returns should_use.

Thanks, you're right, I'll send a fix.

> Maybe this is missing some test coverage?

We do cover, but I was implementing CSS2 and this is in CSS3, and CSS3 tests are
still failing for other reasons.

Still it was helpful to find this out, thank you!