Added functionality in ComputedStyleBase to use platform/ types

Added functionality in ComputedStyleBase to use platform/ types

Added functionality in ComputedStyleBase to use platform/ types that
conform to naming guidelines.

Also converted the direction property to be used by this new generator,
which uses the external enum type platform/text/TextDirection.

BUG=628043
Committed: https://crrev.com/072f5a503cddfe46f59dfdc03cee69a328ca9e57
Cr-Commit-Position: refs/heads/master@{#441091}

[sashab, 2016-12-09 03:47:36]

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

[commit-bot: I haz the power, 2016-12-09 03:47: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-09 03:49:01]

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

[sashab, 2016-12-09 03:49:02]

Ignoring perf for the moment, would be great if you could take a look at this
patch, and let me know what you think. This heads in the direction of mapping
platform values to internal controllable types.

Let me know if you want me to annotate any part of this patch too.

Perf trybots are down atm, but will run them later for a solid result. Also note
that once this is generated we can sorta change our minds about the mapping
idea, or change the generated mapping methods, at any time.

[sashab, 2016-12-09 04:07:47]

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

[commit-bot: I haz the power, 2016-12-09 04:08:04]

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-09 04:17:32]

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

[commit-bot: I haz the power, 2016-12-09 04:17:33]

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

[napper, 2016-12-09 04:22:38]

Generally LGTM. My only comment is that it seems weird to have a static only
class (and one that only contains private methods) - any reason why you don't
make it a namespace? Alternatively it seems like maybe these should just be
methods (or maybe even just free functions) in ComputedStyleBase?

[sashab, 2016-12-11 23:22:08]

Thanks for the feedback Jon. I did a small test locally and moving these methods
& types to be private to ComputedStyle works. I am hesitant to move them to be
private to the CPP file since this won't allow inlining of them.

Handing over the patch to you to experiment with. Feel free to use this one as
baseline and finish the rest yourself & I'll review. If you don't get to it dw
and I'll take it over next week. :D

Sash

[sashab, 2016-12-16 04:32:33]

Description was changed from

==========
Added functionality in ComputedStyleBase to generate platform/ types

Added functionality in ComputedStyleBase to generate platform/ types
by adding a ComputedStyleTypes class, private to ComputedStyleBase,
which stores internal types used in ComputedStyleBase. Then added a
ComputedStyleTypeMappings class, also private to ComputedStyleBase,
which has the conversion functions for these types as well as
hand-written methods that do the conversion.

In the future, ComputedStyleTypeMappings will be generated but
ComputedStyleTypeMappingsImpl will always remain hand-written.
ComputedStyleTypeMappingsImpl should be designed in a way that causes a
compile error if the platform/ value changes, e.g. using a switch
statement with no default case to cause a compile error if new values
are added to the enum but not to the switch statement.

Also converted the direction property to be used by this new generator,
which uses the external enum type platform/text/TextDirection.

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

to

==========
Added functionality in ComputedStyleBase to generate platform/ types

Added functionality in ComputedStyleBase to generate platform/ types
by adding generated private internal types and mapping functions for use
only in ComputedStyleBase. Also added a hand-written
ComputedStyleTypeMappingsImpl file which contains the implementations of
these mappings.

ComputedStyleTypeMappingsImpl should be designed in a way that causes a
compile error if the platform/ value changes, e.g. using a switch
statement with no default case to cause a compile error if new values
are added to the enum but not to the switch statement.

Also converted the direction property to be used by this new generator,
which uses the external enum type platform/text/TextDirection.
==========

[sashab, 2016-12-16 04:32:49]

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

[alancutter (OOO until 2018), 2016-12-19 00:56:04]

ComputedStyleTypeMappingsImpl seems to be missing from this CL.

[sashab, 2016-12-19 03:30:30]

Sorry about that, ptal now

[sashab, 2016-12-19 03:30:34]

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

[commit-bot: I haz the power, 2016-12-19 03:30:53]

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-19 03:56:59]

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

[commit-bot: I haz the power, 2016-12-19 03:57:00]

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

[alancutter (OOO until 2018), 2016-12-19 04:09:30]

https://codereview.chromium.org/2563913002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl
(right):

https://codereview.chromium.org/2563913002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:127:
enum class {{enum_name}} : unsigned {
Why is a separate enum necessary?

[sashab, 2016-12-19 23:29:05]

On 2016/12/19 at 04:09:30, alancutter wrote:
>
https://codereview.chromium.org/2563913002/diff/80001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl
(right):
> 
>
https://codereview.chromium.org/2563913002/diff/80001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:127:
enum class {{enum_name}} : unsigned {
> Why is a separate enum necessary?

The separate enum serves two purposes:
1) It gives us control over the storage of the type, which we can later change
to anything we want - bitmask, hash map, etc. The enum is also generated from
the keywords of the property. If we use the value in platform/, we don't get
this freedom.
2) It may later become the public facing type for the ComputedStyle object,
removing the platform/ type or adding converters where needed.

I'll add haraken@ as a reviewer if you think this patch generally looks good
otherwise, and get his feedback on this part, since it's a design decision.

[sashab, 2016-12-21 04:41:28]

Description was changed from

==========
Added functionality in ComputedStyleBase to generate platform/ types

Added functionality in ComputedStyleBase to generate platform/ types
by adding generated private internal types and mapping functions for use
only in ComputedStyleBase. Also added a hand-written
ComputedStyleTypeMappingsImpl file which contains the implementations of
these mappings.

ComputedStyleTypeMappingsImpl should be designed in a way that causes a
compile error if the platform/ value changes, e.g. using a switch
statement with no default case to cause a compile error if new values
are added to the enum but not to the switch statement.

Also converted the direction property to be used by this new generator,
which uses the external enum type platform/text/TextDirection.
==========

to

==========
Added functionality in ComputedStyleBase to use platform/ types

Added functionality in ComputedStyleBase to use platform/ types that
conform to naming guidelines.

Also converted the direction property to be used by this new generator,
which uses the external enum type platform/text/TextDirection.
==========

[sashab, 2016-12-21 04:42:52]

As discussed, used TextDirection directly from platform/. Will add another patch
to use mapping functions for eg WritingMode.

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

Description was changed from

==========
Added functionality in ComputedStyleBase to use platform/ types

Added functionality in ComputedStyleBase to use platform/ types that
conform to naming guidelines.

Also converted the direction property to be used by this new generator,
which uses the external enum type platform/text/TextDirection.
==========

to

==========
Added functionality in ComputedStyleBase to use platform/ types

Added functionality in ComputedStyleBase to use platform/ types that
conform to naming guidelines.

Also converted the direction property to be used by this new generator,
which uses the external enum type platform/text/TextDirection.

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

[alancutter (OOO until 2018), 2016-12-22 04:29:15]

lgtm

[sashab, 2016-12-22 04:33:13]

The CQ bit was checked by sashab@chromium.org

[sashab, 2016-12-22 04:33:14]

The patchset sent to the CQ was uploaded after l-g-t-m from napper@chromium.org
Link to the patchset: https://codereview.chromium.org/2563913002/#ps100001
(title: "New approach with no mapping functions")

[commit-bot: I haz the power, 2016-12-22 04:33:26]

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-22 04:38:56]

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

[commit-bot: I haz the power, 2016-12-22 04:39:08]

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

[sashab, 2016-12-22 05:02:08]

The CQ bit was checked by sashab@chromium.org

[sashab, 2016-12-22 05:02:09]

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

[commit-bot: I haz the power, 2016-12-22 05:02:26]

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-22 06:54:35]

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

[commit-bot: I haz the power, 2016-12-22 06:54:39]

Failed to apply patch for third_party/WebKit/Source/core/style/ComputedStyle.h:
While running git apply --index -p1;
  error: patch failed: third_party/WebKit/Source/core/style/ComputedStyle.h:313
  error: third_party/WebKit/Source/core/style/ComputedStyle.h: patch does not
apply

Patch:       third_party/WebKit/Source/core/style/ComputedStyle.h
Index: third_party/WebKit/Source/core/style/ComputedStyle.h
diff --git a/third_party/WebKit/Source/core/style/ComputedStyle.h
b/third_party/WebKit/Source/core/style/ComputedStyle.h
index
7bd056b334216c38d8e33a48649fbec767fc9005..ccac6977784886bf54922be0fc007a323e7e492b
100644
--- a/third_party/WebKit/Source/core/style/ComputedStyle.h
+++ b/third_party/WebKit/Source/core/style/ComputedStyle.h
@@ -200,7 +200,6 @@ class CORE_EXPORT ComputedStyle : public ComputedStyleBase,
     bool operator==(const InheritedData& other) const {
       return (m_hasSimpleUnderline == other.m_hasSimpleUnderline) &&
              (m_cursorStyle == other.m_cursorStyle) &&
-             (m_direction == other.m_direction) &&
              (m_rtlOrdering == other.m_rtlOrdering) &&
              (m_insideLink == other.m_insideLink) &&
              (m_writingMode == other.m_writingMode);
@@ -213,7 +212,6 @@ class CORE_EXPORT ComputedStyle : public ComputedStyleBase,
     unsigned m_hasSimpleUnderline : 1;  // True if 'underline solid' is the
only
                                         // text decoration on this element.
     unsigned m_cursorStyle : 6;     // ECursor
-    unsigned m_direction : 1;       // TextDirection
     // 32 bits
 
     // non CSS2 inherited
@@ -313,7 +311,6 @@ class CORE_EXPORT ComputedStyle : public ComputedStyleBase,
     ComputedStyleBase::setBitDefaults();
     m_inheritedData.m_hasSimpleUnderline = false;
     m_inheritedData.m_cursorStyle = static_cast<unsigned>(initialCursor());
-    m_inheritedData.m_direction = static_cast<unsigned>(initialDirection());
     m_inheritedData.m_rtlOrdering =
static_cast<unsigned>(initialRTLOrdering());
     m_inheritedData.m_insideLink = NotInsideLink;
     m_inheritedData.m_writingMode = initialWritingMode();
@@ -2021,15 +2018,6 @@ class CORE_EXPORT ComputedStyle : public
ComputedStyleBase,
     m_inheritedData.m_cursorStyle = static_cast<unsigned>(c);
   }
 
-  // direction
-  static TextDirection initialDirection() { return TextDirection::Ltr; }
-  TextDirection direction() const {
-    return static_cast<TextDirection>(m_inheritedData.m_direction);
-  }
-  void setDirection(TextDirection v) {
-    m_inheritedData.m_direction = static_cast<unsigned>(v);
-  }
-
   // color
   static Color initialColor() { return Color::black; }
   void setColor(const Color&);

[sashab, 2017-01-03 02:53:53]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-01-03 02:53:53]

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

[commit-bot: I haz the power, 2017-01-03 02:54:03]

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-03 05:33:44]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1483412033295150,
"parent_rev": "8eda10c33eaf2c467a6ab16f8003d55945fe2229", "commit_rev":
"e6a7daf7031899e547469d1026baea9881021803"}

[commit-bot: I haz the power, 2017-01-03 05:34:24]

Description was changed from

==========
Added functionality in ComputedStyleBase to use platform/ types

Added functionality in ComputedStyleBase to use platform/ types that
conform to naming guidelines.

Also converted the direction property to be used by this new generator,
which uses the external enum type platform/text/TextDirection.

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

to

==========
Added functionality in ComputedStyleBase to use platform/ types

Added functionality in ComputedStyleBase to use platform/ types that
conform to naming guidelines.

Also converted the direction property to be used by this new generator,
which uses the external enum type platform/text/TextDirection.

BUG=628043

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

[commit-bot: I haz the power, 2017-01-03 05:34:26]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2017-01-03 05:36:29]

Description was changed from

==========
Added functionality in ComputedStyleBase to use platform/ types

Added functionality in ComputedStyleBase to use platform/ types that
conform to naming guidelines.

Also converted the direction property to be used by this new generator,
which uses the external enum type platform/text/TextDirection.

BUG=628043

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

to

==========
Added functionality in ComputedStyleBase to use platform/ types

Added functionality in ComputedStyleBase to use platform/ types that
conform to naming guidelines.

Also converted the direction property to be used by this new generator,
which uses the external enum type platform/text/TextDirection.

BUG=628043

Committed: https://crrev.com/072f5a503cddfe46f59dfdc03cee69a328ca9e57
Cr-Commit-Position: refs/heads/master@{#441091}
==========

[commit-bot: I haz the power, 2017-01-03 05:36:30]

Patchset 8 (id:??) landed as
https://crrev.com/072f5a503cddfe46f59dfdc03cee69a328ca9e57
Cr-Commit-Position: refs/heads/master@{#441091}