Closed Bug 1291667 Opened 3 years ago Closed 3 years ago

Replace NS_USER_STYLE_SELECT_* with enum classes

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: waffles, Assigned: waffles, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 10 obsolete files)

58 bytes, text/x-review-board-request
heycam
: review+
manishearth
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
manishearth
: review+
Details
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160727180356
Blocks: 1277133
Mentor: manishearth
Attached patch enum_classes (obsolete) — Splinter Review
Attachment #8777304 - Flags: review?(manishearth)
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.
Status: UNCONFIRMED → NEW
Component: Layout → CSS Parsing and Computation
Ever confirmed: true
Manish addressed those in IRC. And yeah, my editor did those changes unfortunately. I'll redo the changes and upload a new patch soon :)
Attached patch style_user_select.patch (obsolete) — Splinter Review
Attachment #8777304 - Attachment is obsolete: true
Attachment #8777304 - Flags: review?(manishearth)
Attachment #8777361 - Flags: review?(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.
Attachment #8777361 - Flags: review?(manishearth)
Attachment #8777361 - Flags: review?(cam)
Attachment #8777361 - Flags: feedback+
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.
Attachment #8777361 - Flags: review?(cam) → review+
Assignee: nobody → wafflespeanut
Status: NEW → ASSIGNED
Attached patch style_user_select.patch (obsolete) — Splinter Review
Attachment #8777361 - Attachment is obsolete: true
Attachment #8777670 - Flags: review?(cam)
I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1292036 for re-enabling my try-access :)
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?
Attachment #8777670 - Flags: review?(cam) → review+
Attached patch style_user_select.patch (obsolete) — Splinter Review
Addressed comments. Thanks for the review :)
Attachment #8777670 - Attachment is obsolete: true
Attachment #8777680 - Flags: review?(cam)
Attached patch remove_moz_none.patch (obsolete) — Splinter Review
New patch for removing the unused variant.
Attachment #8777681 - Flags: review?(cam)
Attachment #8777680 - Flags: review?(cam) → review+
Attachment #8777681 - Flags: review?(cam) → review+
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.
Attached patch style_user_select.patch (obsolete) — Splinter Review
Attachment #8777680 - Attachment is obsolete: true
Attachment #8777687 - Flags: review+
Attached patch remove_moz_none.patch (obsolete) — Splinter Review
Attachment #8777681 - Attachment is obsolete: true
Attachment #8777688 - Flags: review+
Fixed commit messages. Thanks heycam and Manish :)
> /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.
Attached patch style_user_select.patch (obsolete) — Splinter Review
Attachment #8777687 - Attachment is obsolete: true
Attachment #8778858 - Flags: review+
Attached patch remove_moz_none.patch (obsolete) — Splinter Review
Attachment #8777688 - Attachment is obsolete: true
Attachment #8778859 - Flags: review+
Try build succeeds :)
Pushed by cmccormack@mozilla.com:
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
Keywords: checkin-needed
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
Flags: needinfo?(wafflespeanut)
Attached patch style_user_select.patch (obsolete) — Splinter Review
Sorry about that. I've no idea how I did that change.
I've updated the patch.
Attachment #8778858 - Attachment is obsolete: true
Flags: needinfo?(wafflespeanut)
Attachment #8779717 - Flags: review+
Keywords: checkin-needed
New patch looks good. Please re-push to try, though.
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
Flags: needinfo?(wafflespeanut)
Keywords: checkin-needed
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.
Flags: needinfo?(wafflespeanut)
Attachment #8779717 - Attachment is obsolete: true
Attachment #8778859 - Attachment is obsolete: true
Fixed the patch, marked the previous patches as obsolete, and submitted for mozreview'ing :)
Comment on attachment 8779779 [details]
Bug 1291667 - Change NS_STYLE_USER_SELECT_* constants to enum classes;

https://reviewboard.mozilla.org/r/70712/#review68058
Attachment #8779779 - Flags: review?(manishearth) → review+
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)
Attachment #8779780 - Flags: review+
Comment on attachment 8779779 [details]
Bug 1291667 - Change NS_STYLE_USER_SELECT_* constants to enum classes;

https://reviewboard.mozilla.org/r/70712/#review68266
Attachment #8779779 - Flags: review?(cam) → review+
Comment on attachment 8779780 [details]
Bug 1291667 - Removed unused variant 'MozNone' from StyleUserSelect enum class;

https://reviewboard.mozilla.org/r/70714/#review68268
Attachment #8779780 - Flags: review?(cam) → review+
Pushed by Ms2ger@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/c7ce03021e06
https://hg.mozilla.org/mozilla-central/rev/8bf7ed14b115
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.