1291667 - Replace NS_USER_STYLE_SELECT_* with enum classes

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Ravi Shankar [:waffles]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160727180356

Comment 1

[Ravi Shankar [:waffles]]

Attachment: enum_classes

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8777304 [details] [diff] [review]
enum_classes

Seems like you should be changing the type of nsStyleUIReset::mUserSelect to be the new enum class; then you won't need casts when reading it.  (Switching that type, and making its users use more typesafe mechanisms, is really the point of doing this.)

Also, please don't mix whitespace cleanups and code changes in the same patch.  They should be separate.

Comment 3

[Ravi Shankar [:waffles]]

Manish addressed those in IRC. And yeah, my editor did those changes unfortunately. I'll redo the changes and upload a new patch soon :)

Comment 4

[Ravi Shankar [:waffles]]

Attachment: style_user_select.patch

Comment 5

[Manish Goregaokar [:manishearth]]

Comment on attachment 8777361 [details] [diff] [review]
style_user_select.patch

Review of attachment 8777361 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good, just some style nits. Thanks!

I can't formally sign off on this, bouncing to :heycam.

In the meantime, could you do a try push of these? Let me know if you need help with that; since you haven't contributed in a while it's possible that your credentials have expired and need to be re-enabled.

The try syntax I was using for these was `try: -b o -p linux -u reftest,reftest-1,reftest-2,jsreftest -t none`, though `try: -b d -p linux -u none -t none` is also okay.

::: layout/generic/nsFrame.cpp
@@ +3128,5 @@
>  
>  nsresult
> +nsFrame::IsSelectable(bool* aSelectable, StyleUserSelect* aSelectStyle) const
> +{
> +  if (!aSelectable)   //it's ok if aSelectStyle is null

nit: don't change this line

@@ +3152,5 @@
>    //    _MOZ_ALL -> TEXT      -> AUTO -> AUTO,      the returned value is ALL
>    //    _MOZ_ALL -> _MOZ_TEXT -> AUTO -> AUTO,      the returned value is TEXT.
>    //    AUTO     -> CELL      -> TEXT -> AUTO,      the returned value is TEXT
>    //
> +  StyleUserSelect selectStyle  = StyleUserSelect::Auto;

nit: Try to keep the =s in alignment (it's already off for the third, fix that too)

@@ +3932,5 @@
>    // control, but the focus didn't work right anyway; it'd probably be enough
>    // if the left and right arrows could enter textboxes (which I don't believe
>    // they can at the moment)
>    return !aFrame->IsGeneratedContentFrame() &&
> +         style != StyleUserSelect::All  &&

nit: Alignment of &&

::: layout/generic/nsFrame.h
@@ +199,5 @@
>    virtual nsIFrame* GetNextInFlowVirtual() const override;
>    virtual void SetNextInFlow(nsIFrame*) override;
>    virtual nsIAtom* GetType() const override;
>  
> +  virtual nsresult  IsSelectable(bool* aIsSelectable, mozilla::StyleUserSelect* aSelectStyle) const override;

nit: Make this shorter by wrapping the argument list (as is done in PeekOffsetCharacter for example)

::: layout/style/nsCSSProps.cpp
@@ +2143,5 @@
>    { eCSSKeyword_UNKNOWN,    -1 }
>  };
>  
>  const KTableEntry nsCSSProps::kUserSelectKTable[] = {
> +  { eCSSKeyword_none,       uint8_t(StyleUserSelect::None_) },

nit: These casts aren't necessary anymore after bug 1289710. See how it's done in this file for eg. StyleClipShape.

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8777361 [details] [diff] [review]
style_user_select.patch

Review of attachment 8777361 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these and Manish's comments addressed.

::: layout/generic/nsFrame.cpp
@@ +3189,3 @@
>    else
> +  if (selectStyle == StyleUserSelect::MozAll)
> +    selectStyle = StyleUserSelect::All;

These lines have (pre-existing) strange formatting.  While you're touching them, you could fix them up to use this style:

  if (...) {
    ...
  } else {
    ...
  }

::: layout/generic/nsIFrame.h
@@ +2688,5 @@
>     *                       (i.e. is not affected by user-select: none)
>     *  @param aSelectStyle  out param. Returns the type of selection style found
>     *                        (using values defined in nsStyleConsts.h).
>     */
> +  virtual nsresult  IsSelectable(bool* aIsSelectable, mozilla::StyleUserSelect* aSelectStyle) const = 0;

Nit: As with Manish's comment in nsFrame.h, let's wrap this to 80 columns while we're touching it.

::: layout/style/nsStyleConsts.h
@@ +113,5 @@
>    SelectMenu,
>  };
>  
>  // user-select
> +enum class StyleUserSelect: uint8_t {

Nit: space before ":".

::: layout/style/nsStyleStruct.h
@@ +3040,5 @@
>             nsChangeHint_ReflowChangesSizeOrPosition |
>             nsChangeHint_ClearAncestorIntrinsics;
>    }
>  
> +  mozilla::StyleUserSelect   mUserSelect; // [reset] (selection-style)

Nit: can drop the extra spaces before "mUserSelect", now that we're not trying to align with other lines.

Comment 7

[Ravi Shankar [:waffles]]

Attachment: style_user_select.patch

Comment 8

[Ravi Shankar [:waffles]]

I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1292036 for re-enabling my try-access :)

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8777670 [details] [diff] [review]
style_user_select.patch

Review of attachment 8777670 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a couple of minor comments.

::: layout/generic/nsFrame.cpp
@@ +3206,5 @@
>      *aSelectable = false;
> +  } else {
> +    *aSelectable = allowSelection && (selectStyle != StyleUserSelect::None_);
> +  }
> +  

Drop trailing spaces.

@@ +3936,5 @@
>    // if the left and right arrows could enter textboxes (which I don't believe
>    // they can at the moment)
>    return !aFrame->IsGeneratedContentFrame() &&
> +             style != StyleUserSelect::All  &&
> +            style != StyleUserSelect::None_ &&

Let's leave the existing alignment (i.e., have these two lines starting under the "!aFrame").  I think the existing code was just being a bit clever aligning the "&&"s since it was only one space away.

::: layout/style/nsStyleConsts.h
@@ +123,5 @@
> +  Toggle,
> +  TriState,
> +  Auto,     // internal value - please use nsFrame::IsSelectable()
> +  MozAll,   // force selection of all children, unless an ancestor has NONE set - bug 48096
> +  MozNone,  // Like NONE, but doesn't change selection behavior for descendants whose user-select is not AUTO.

It looks like this constant isn't actually used.  In the nsCSSProps table we map -moz-none to the same constant as none.  So I think we can remove it.  Can you do that in a separate patch?

Comment 10

[Ravi Shankar [:waffles]]

Attachment: style_user_select.patch

Addressed comments. Thanks for the review :)

Comment 11

[Ravi Shankar [:waffles]]

Attachment: remove_moz_none.patch

New patch for removing the unused variant.

Comment 12

[Cameron McCormack (:heycam)]

One final thing: please update your commit messages to:

Bug 1291667 - blah blah blah. r=heycam

That will make life easier for the sheriffs when they land your patch on your behalf.

Comment 13

[Ravi Shankar [:waffles]]

Attachment: style_user_select.patch

Comment 14

[Ravi Shankar [:waffles]]

Attachment: remove_moz_none.patch

Comment 15

[Ravi Shankar [:waffles]]

Fixed commit messages. Thanks heycam and Manish :)

Comment 16

[Ravi Shankar [:waffles]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=efe72cbcd8ea

Comment 17

[Manish Goregaokar [:manishearth]]

> /builds/slave/try-lx-d-000000000000000000000/build/src/layout/style/nsRuleNode.cpp:1391:48: error: no matching function for call to 'SetValueHelper<mozilla::StyleUserSelect>::SetEnumeratedValue(mozilla::StyleUserSelect&, const nsCSSValue&)'

You'll need to put DEFINE_ENUM_CLASS_SETTER somewhere at the top similar to what's done for StyleBoxSizing for this call to work.

Comment 18

[Ravi Shankar [:waffles]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78601cb91e75

Comment 19

[Ravi Shankar [:waffles]]

Attachment: style_user_select.patch

Comment 20

[Ravi Shankar [:waffles]]

Attachment: remove_moz_none.patch

Comment 21

[Ravi Shankar [:waffles]]

Try build succeeds :)

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff84b85aa1cb
Change NS_STYLE_USER_SELECT_* constants to enum classes; r=Manishearth,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/4453b10529a2
Removed unused variant 'MozNone' from StyleUserSelect enum class; r=heycam

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for asserting at nsRuleNode.cpp:1322 and failing mochitests and reftests:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d92407a320cb79fde6d1d365557aa20d5cc9db4
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec0d7c532c6908e936fe19b1ba21dd9dfd6c816b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4453b10529a2f717b3fe14e582d823d88b0d8bff
A failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=33624856&repo=mozilla-inbound

Assertion failure: value >= static_cast<decltype(value)>(StyleUserSelect::None_) && value <= static_cast<decltype(value)>(StyleUserSelect::All) (inappropriate value), at /home/worker/workspace/build/src/layout/style/nsRuleNode.cpp:1322

Comment 24

[Ravi Shankar [:waffles]]

Attachment: style_user_select.patch

Sorry about that. I've no idea how I did that change.
I've updated the patch.

Comment 25

[Manish Goregaokar [:manishearth]]

New patch looks good. Please re-push to try, though.

Comment 26

[Ravi Shankar [:waffles]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58bc8f70057c

Comment 27

[Carsten Book [:Tomcat]]

has problems to apply:

Hunk #1 FAILED at 118
1 out of 1 hunks FAILED -- saving rejects to file layout/style/nsStyleConsts.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh remove_moz_none.patch

Comment 28

[Manish Goregaokar [:manishearth]]

Heh, TYlin's focus patch broke it.

While you're at it, could you push the new patches through mozreview? I'll help you set it up on IRC if you need it.

Comment 29

[Ravi Shankar [:waffles]]

Attachment: Bug 1291667 - Change NS_STYLE_USER_SELECT_* constants to enum classes;

Review commit: https://reviewboard.mozilla.org/r/70712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70712/

Comment 30

[Ravi Shankar [:waffles]]

Attachment: Bug 1291667 - Removed unused variant 'MozNone' from StyleUserSelect enum class;

Review commit: https://reviewboard.mozilla.org/r/70714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70714/

Comment 31

[Ravi Shankar [:waffles]]

Fixed the patch, marked the previous patches as obsolete, and submitted for mozreview'ing :)

Comment 32

[Manish Goregaokar [:manishearth]]

Comment on attachment 8779779 [details]
Bug 1291667 - Change NS_STYLE_USER_SELECT_* constants to enum classes;

https://reviewboard.mozilla.org/r/70712/#review68058

Comment 33

[Manish Goregaokar [:manishearth]]

Comment on attachment 8779780 [details]
Bug 1291667 - Removed unused variant 'MozNone' from StyleUserSelect enum class;

https://reviewboard.mozilla.org/r/70714/#review68062

(Note that this still needs heycam's review to land)

Comment 34

[Cameron McCormack (:heycam)]

Comment on attachment 8779779 [details]
Bug 1291667 - Change NS_STYLE_USER_SELECT_* constants to enum classes;

https://reviewboard.mozilla.org/r/70712/#review68266

Comment 35

[Cameron McCormack (:heycam)]

Comment on attachment 8779780 [details]
Bug 1291667 - Removed unused variant 'MozNone' from StyleUserSelect enum class;

https://reviewboard.mozilla.org/r/70714/#review68268

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7ce03021e06
Change NS_STYLE_USER_SELECT_* constants to enum classes; r=heycam,manishearth
https://hg.mozilla.org/integration/autoland/rev/8bf7ed14b115
Removed unused variant 'MozNone' from StyleUserSelect enum class; r=heycam,manishearth

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c7ce03021e06
https://hg.mozilla.org/mozilla-central/rev/8bf7ed14b115