Changed ComputedStyle's EVisibility to be an enum class and fixed naming

Changed ComputedStyle's EVisibility to be an enum class and fixed naming

Changed ComputedStyle's EVisibility to be an enum class and fixed naming
to be consistent with Blink style. This is part of a larger effort to
convert ComputedStyle's enums to enum classes.

Design doc:
https://docs.google.com/document/d/1BfjMEXIzvY1WZEn_nESBxyqkazkfa0sBSEvm-F89YwM/edit#

BUG=628043
Committed: https://crrev.com/9418458ddfa80b213daa5357bfe21372dbd7565c
Cr-Commit-Position: refs/heads/master@{#410880}

[sashab, 2016-07-27 01:15:15]

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

[commit-bot: I haz the power, 2016-07-27 01:15:45]

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

[sashab, 2016-07-27 01:17:49]

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

[commit-bot: I haz the power, 2016-07-27 01:18:09]

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

[commit-bot: I haz the power, 2016-07-27 01:18:10]

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

[Bugs Nash, 2016-07-28 01:25:12]

https://codereview.chromium.org/2181953004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/CSSVisibilityInterpolationType.cpp
(right):

https://codereview.chromium.org/2181953004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/CSSVisibilityInterpolationType.cpp:98:
// TODO(sashab): Why is this being stored as a double?
Should a question be put in as a TODO? Maybe do the research now.

https://codereview.chromium.org/2181953004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2181953004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:301:
m_inheritedData.m_visibility = static_cast<unsigned>(initialVisibility());
why not make m_inheritedData.m_visibility store as a EVisibility instead? Seems
like the code would be cleaner and more type safe

[esprehn, 2016-07-29 01:53:07]

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

[esprehn, 2016-07-29 01:53:09]

https://codereview.chromium.org/2181953004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2181953004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:301:
m_inheritedData.m_visibility = static_cast<unsigned>(initialVisibility());
On 2016/07/28 at 01:25:12, Bugs Nash wrote:
> why not make m_inheritedData.m_visibility store as a EVisibility instead?
Seems like the code would be cleaner and more type safe

I'm not sure if type safe bit field enums work in all our compilers (MSVC was
bad years ago), you should do some static_assert's about the size of
ComputedStyle's various objects and then try messing with this stuff to see if
MSVC can deal with enum class bitfields. It used to realign the types which
wasted lots of space.

[sashab, 2016-07-29 06:25:50]

https://codereview.chromium.org/2181953004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/CSSVisibilityInterpolationType.cpp
(right):

https://codereview.chromium.org/2181953004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/CSSVisibilityInterpolationType.cpp:98:
// TODO(sashab): Why is this being stored as a double?
On 2016/07/28 at 01:25:12, Bugs Nash wrote:
> Should a question be put in as a TODO? Maybe do the research now.

Seems to be added by alancutter@ here:
https://codereview.chromium.org/1548873002
seems like an honest mistake since other ConversionCheckers store fractions.

Changed the TODO to be clearer I'll do it in a follow-up patch. :)

https://codereview.chromium.org/2181953004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/2181953004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:301:
m_inheritedData.m_visibility = static_cast<unsigned>(initialVisibility());
On 2016/07/29 at 01:53:09, esprehn wrote:
> On 2016/07/28 at 01:25:12, Bugs Nash wrote:
> > why not make m_inheritedData.m_visibility store as a EVisibility instead?
Seems like the code would be cleaner and more type safe
> 
> I'm not sure if type safe bit field enums work in all our compilers (MSVC was
bad years ago), you should do some static_assert's about the size of
ComputedStyle's various objects and then try messing with this stuff to see if
MSVC can deal with enum class bitfields. It used to realign the types which
wasted lots of space.

Yup; thought I replied to this, thanks Elliott. I'm happy to do that as a
follow-up. For now, the design doc contains a bunch of research I've done to
make sure these static casts don't cost us any cycles (they don't).

The packing example from MSDN seems to align unsigned int bitfields to 8-bit
boundaries:
https://msdn.microsoft.com/en-us/library/ewwyfdbe.aspx

Also, from https://msdn.microsoft.com/en-us/library/1d48zaa8.aspx, "Adjacent bit
fields are packed into the same 1-, 2-, or 4-byte allocation unit if the
integral types are the same size and if the next bit field fits into the current
allocation unit without crossing the boundary imposed by the common alignment
requirements of the bit fields.".

But then, on https://msdn.microsoft.com/en-us/library/9f53hsws.aspx, it suggests
that it doesn't do that alignment for unsigned.

This will need more investigation and I'm happy to do that in parallel, but for
now we can safely leave things as they are.

[alancutter (OOO until 2018), 2016-07-29 09:16:46]

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

[alancutter (OOO until 2018), 2016-07-29 09:16:48]

https://codereview.chromium.org/2181953004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/CSSVisibilityInterpolationType.cpp
(right):

https://codereview.chromium.org/2181953004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/CSSVisibilityInterpolationType.cpp:98:
// TODO(sashab): Why is this being stored as a double?
On 2016/07/29 at 06:25:50, sashab wrote:
> On 2016/07/28 at 01:25:12, Bugs Nash wrote:
> > Should a question be put in as a TODO? Maybe do the research now.
> 
> Seems to be added by alancutter@ here:
> https://codereview.chromium.org/1548873002
> seems like an honest mistake since other ConversionCheckers store fractions.
> 
> Changed the TODO to be clearer I'll do it in a follow-up patch. :)

This is indeed a mistake, feel free to make it store an enum instead.

[sashab, 2016-08-01 03:26:24]

On 2016/07/29 at 09:16:48, alancutter wrote:
>
https://codereview.chromium.org/2181953004/diff/20001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/core/animation/CSSVisibilityInterpolationType.cpp
(right):
> 
>
https://codereview.chromium.org/2181953004/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/animation/CSSVisibilityInterpolationType.cpp:98:
// TODO(sashab): Why is this being stored as a double?
> On 2016/07/29 at 06:25:50, sashab wrote:
> > On 2016/07/28 at 01:25:12, Bugs Nash wrote:
> > > Should a question be put in as a TODO? Maybe do the research now.
> > 
> > Seems to be added by alancutter@ here:
> > https://codereview.chromium.org/1548873002
> > seems like an honest mistake since other ConversionCheckers store fractions.
> > 
> > Changed the TODO to be clearer I'll do it in a follow-up patch. :)
> 
> This is indeed a mistake, feel free to make it store an enum instead.

Thanks Alan; did this in a separate patch (crrev.com/2195233002) and rebased
this patch onto that one, marking it as dependent.

[sashab, 2016-08-01 03:48:43]

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

[commit-bot: I haz the power, 2016-08-01 03:48:49]

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

[sashab, 2016-08-01 03:49:27]

Thanks esprehn; PTAL at ComputedStyleConstants.h now. Added a TODO (which will
later become a documenting comment) explaining why all the enums should extend
from unsigned, as per the discussion on msvc bitfields. Also made EVisibility
extend from unsigned.

[commit-bot: I haz the power, 2016-08-01 05:17:55]

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

[commit-bot: I haz the power, 2016-08-01 05:17:57]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-08-02 02:37:45]

lgtm, fwiw the E prefix is pretty dumb it just means "Enum" I think. I'd be
inclined to rename these all to CSSFoo like CSSVisibility or something, but I'd
do that separately as one big patch over all the types.

https://codereview.chromium.org/2181953004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (right):

https://codereview.chromium.org/2181953004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyleConstants.h:394: enum class
EVisibility : unsigned { Visible, Hidden, Collapse };
Do the static_asserts for size fail if you leave off the unsigned? :)

[sashab, 2016-08-02 06:02:39]

Thanks Elliott. Added a TODO for the static_asserts, have an idea of how to do
this in a follow-up patch.

As for renaming EVisibility... We *could* make it CSSVisibility, or to prevent
name conflicts with everything, we could put it in its own
ComputedStyleConstants or CSS namespace.

https://codereview.chromium.org/2181953004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (right):

https://codereview.chromium.org/2181953004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyleConstants.h:394: enum class
EVisibility : unsigned { Visible, Hidden, Collapse };
On 2016/08/02 at 02:37:44, esprehn wrote:
> Do the static_asserts for size fail if you leave off the unsigned? :)

No, ComputedStyle does have a style check but from other experiments this no
longer seems to be a problem.

Added a TODO anyway, I still want to check that this is unsigned, lots of ways
to do this so I think we should leave this for another patch. :)

[sashab, 2016-08-02 06:51:59]

The CQ bit was checked by sashab@chromium.org

[sashab, 2016-08-02 06:51:59]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2181953004/#ps100001
(title: "Added TODO")

[commit-bot: I haz the power, 2016-08-02 06:52:09]

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-08-02 06:54:24]

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

[commit-bot: I haz the power, 2016-08-02 06:54:26]

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-08-09 06:48:03]

The CQ bit was checked by sashab@chromium.org

[sashab, 2016-08-09 06:48:03]

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

[commit-bot: I haz the power, 2016-08-09 06:48: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-08-09 07:00:18]

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

[commit-bot: I haz the power, 2016-08-09 07:00:21]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[sashab, 2016-08-09 07:02:45]

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

[commit-bot: I haz the power, 2016-08-09 07:03:27]

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-08-09 08:43:38]

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

[commit-bot: I haz the power, 2016-08-09 08:43:39]

Dry run: This issue passed the CQ dry run.

[sashab, 2016-08-09 23:49:03]

The CQ bit was checked by sashab@chromium.org

[sashab, 2016-08-09 23:49:04]

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

[commit-bot: I haz the power, 2016-08-09 23:49:55]

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-08-09 23:56:59]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-09 23:59:07]

Description was changed from

==========
Changed ComputedStyle's EVisibility to be an enum class and fixed naming

Changed ComputedStyle's EVisibility to be an enum class and fixed naming
to be consistent with Blink style. This is part of a larger effort to
convert ComputedStyle's enums to enum classes.

Design doc:
https://docs.google.com/document/d/1BfjMEXIzvY1WZEn_nESBxyqkazkfa0sBSEvm-F89Y...

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

to

==========
Changed ComputedStyle's EVisibility to be an enum class and fixed naming

Changed ComputedStyle's EVisibility to be an enum class and fixed naming
to be consistent with Blink style. This is part of a larger effort to
convert ComputedStyle's enums to enum classes.

Design doc:
https://docs.google.com/document/d/1BfjMEXIzvY1WZEn_nESBxyqkazkfa0sBSEvm-F89Y...

BUG=628043

Committed: https://crrev.com/9418458ddfa80b213daa5357bfe21372dbd7565c
Cr-Commit-Position: refs/heads/master@{#410880}
==========

[commit-bot: I haz the power, 2016-08-09 23:59:09]

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

[tkent, 2016-08-10 01:11:32]

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

[tkent, 2016-08-10 01:11:34]

https://codereview.chromium.org/2181953004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (right):

https://codereview.chromium.org/2181953004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyleConstants.h:396: enum class
EVisibility : unsigned { Visible, Hidden, Collapse };
Can you drop 'E' prefix from enum type names in your project?  It's an ancient
naming convention in kHTML era.
Also, can you add 'k' prefixes to enum item names?

[sashab, 2016-08-10 01:14:51]

https://codereview.chromium.org/2181953004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (right):

https://codereview.chromium.org/2181953004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyleConstants.h:396: enum class
EVisibility : unsigned { Visible, Hidden, Collapse };
On 2016/08/10 at 01:11:33, tkent wrote:
> Can you drop 'E' prefix from enum type names in your project?  It's an ancient
naming convention in kHTML era.
> Also, can you add 'k' prefixes to enum item names?

Sure, so just to clarify:

enum class Visibility : unsigned { kVisibile, kHidden, kCollapse };

[tkent, 2016-08-10 01:18:43]

https://codereview.chromium.org/2181953004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (right):

https://codereview.chromium.org/2181953004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyleConstants.h:396: enum class
EVisibility : unsigned { Visible, Hidden, Collapse };
On 2016/08/10 at 01:14:51, sashab wrote:
> On 2016/08/10 at 01:11:33, tkent wrote:
> > Can you drop 'E' prefix from enum type names in your project?  It's an
ancient naming convention in kHTML era.
> > Also, can you add 'k' prefixes to enum item names?
> 
> Sure, so just to clarify:
> 
> enum class Visibility : unsigned { kVisibile, kHidden, kCollapse };

Exactly!