Closed Bug 1312173 Opened 4 years ago Closed 4 years ago

Convert NS_STYLE_USER_INPUT_* to an enum class

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: adamtomasv, Assigned: adamtomasv, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161013141419
Blocks: 1277133
Mentor: manishearth
Attachment #8803602 - Flags: review?(xidorn+moz)
Comment on attachment 8803602 [details]
Bug 1312173 - Convert NS_STYLE_USER_INPUT_* to an enum class;

https://reviewboard.mozilla.org/r/87802/#review86792

Needs sign off from a peer to land, but looks good on my end. I've triggered a try build, we can land this if that passes and xidorn gives r+. Thanks!
Attachment #8803602 - Flags: review?(manishearth) → review+
Assignee: nobody → adamtomasv
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8803602 [details]
Bug 1312173 - Convert NS_STYLE_USER_INPUT_* to an enum class;

https://reviewboard.mozilla.org/r/87802/#review86794

Please address the issues below. I'd like to review the updated patch.

::: layout/forms/nsComboboxControlFrame.cpp:1166
(Diff revision 1)
>  #endif
>  
>    // If we have style that affects how we are selected, feed event down to
>    // nsFrame::HandleEvent so that selection takes place when appropriate.
>    const nsStyleUserInterface* uiStyle = StyleUserInterface();
> -  if (uiStyle->mUserInput == NS_STYLE_USER_INPUT_NONE || uiStyle->mUserInput == NS_STYLE_USER_INPUT_DISABLED)
> +  if (uiStyle->mUserInput == StyleUserInput::None_ || uiStyle->mUserInput == StyleUserInput::Disabled)

Please break this line into two lines to make line length not exceed 80 chars. I know it exceeds currently, but it makes sense to fix this kind of code style when we update code.

And as far as you're here, please also wrap the body of the if statement with {}.

::: layout/forms/nsFormControlFrame.cpp:189
(Diff revision 1)
>                                  nsEventStatus* aEventStatus)
>  {
>    // Check for user-input:none style
>    const nsStyleUserInterface* uiStyle = StyleUserInterface();
> -  if (uiStyle->mUserInput == NS_STYLE_USER_INPUT_NONE ||
> -      uiStyle->mUserInput == NS_STYLE_USER_INPUT_DISABLED)
> +  if (uiStyle->mUserInput == StyleUserInput::None_ ||
> +      uiStyle->mUserInput == StyleUserInput::Disabled)

Please also wrap with {}.

::: layout/forms/nsGfxButtonControlFrame.cpp:232
(Diff revision 1)
>    // from being called. The nsFrame::HandleEvent causes the button label
>    // to be selected (Drawn with an XOR rectangle over the label)
>  
>    // do we have user-input style?
>    const nsStyleUserInterface* uiStyle = StyleUserInterface();
> -  if (uiStyle->mUserInput == NS_STYLE_USER_INPUT_NONE || uiStyle->mUserInput == NS_STYLE_USER_INPUT_DISABLED)
> +  if (uiStyle->mUserInput == StyleUserInput::None_ || uiStyle->mUserInput == StyleUserInput::Disabled)

Break line and wrap with {}.

::: layout/forms/nsImageControlFrame.cpp:155
(Diff revision 1)
>      return NS_OK;
>    }
>  
>    // do we have user-input style?
>    const nsStyleUserInterface* uiStyle = StyleUserInterface();
> -  if (uiStyle->mUserInput == NS_STYLE_USER_INPUT_NONE || uiStyle->mUserInput == NS_STYLE_USER_INPUT_DISABLED)
> +  if (uiStyle->mUserInput == StyleUserInput::None_ || uiStyle->mUserInput == StyleUserInput::Disabled)

Ditto.

::: layout/forms/nsListControlFrame.cpp:926
(Diff revision 1)
>      return NS_OK;
>  
>    // do we have style that affects how we are selected?
>    // do we have user-input style?
>    const nsStyleUserInterface* uiStyle = StyleUserInterface();
> -  if (uiStyle->mUserInput == NS_STYLE_USER_INPUT_NONE || uiStyle->mUserInput == NS_STYLE_USER_INPUT_DISABLED)
> +  if (uiStyle->mUserInput == StyleUserInput::None_ || uiStyle->mUserInput == StyleUserInput::Disabled)

Ditto.

::: layout/style/nsStyleConsts.h:216
(Diff revision 1)
>    MozText,  // Like TEXT, except that it won't get overridden by ancestors having ALL.
>  };
>  
>  // user-input
> -#define NS_STYLE_USER_INPUT_NONE      0
> -#define NS_STYLE_USER_INPUT_ENABLED   1
> +enum class StyleUserInput : uint8_t {
> +  None_,

Could you explain why do you need the "_" suffix for None? It doesn't seem to me it is some thing useful.
Attachment #8803602 - Flags: review?(xidorn+moz)
Hi Xidorn, thanks for the review, I will update the coding style. I used the tips from here: https://bugzilla.mozilla.org/show_bug.cgi?id=1277133#c10 to work with the issue. There the "_" in None_ is recommended. 

Let me know if I should remove it.

Also, I would like to fix order of kUserInputKTable to match the new enum (so that the auto value is the last). Would that be ok?
Flags: needinfo?(xidorn+moz)
Re comment 4:

Using None_ was to workaround the None #define in X11 header. It had been fixed in bug 1300337, so we should just use None.
Flags: needinfo?(xidorn+moz)
(In reply to adamtomasv from comment #4)
> Hi Xidorn, thanks for the review, I will update the coding style. I used the
> tips from here: https://bugzilla.mozilla.org/show_bug.cgi?id=1277133#c10 to
> work with the issue. There the "_" in None_ is recommended. 
> 
> Let me know if I should remove it.

Please remove it if not needed.

> Also, I would like to fix order of kUserInputKTable to match the new enum
> (so that the auto value is the last). Would that be ok?

That's ok.
Please squash the two commits together. You can use "hg histedit" and roll the second patch into the first patch. If you are using git, you can use "git rebase -i".
Attachment #8803663 - Attachment is obsolete: true
Attachment #8803663 - Flags: review?(xidorn+moz)
Comment on attachment 8803602 [details]
Bug 1312173 - Convert NS_STYLE_USER_INPUT_* to an enum class;

https://reviewboard.mozilla.org/r/87802/#review86802

Looks good, thanks.
Attachment #8803602 - Flags: review?(xidorn+moz) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3d0ad7bb854d
Convert NS_STYLE_USER_INPUT_* to an enum class;  r=manishearth,xidorn
We should ideally switch all the None_s over at once to make or easy for servo to switch. Adam, want to work on this?
(In reply to Manish Goregaokar [:manishearth] from comment #12)
> We should ideally switch all the None_s over at once to make or easy for
> servo to switch. Adam, want to work on this?

Hi Manish, yes. Let me know the bug number. //A
Flags: needinfo?(manishearth)
Oh, looks like I fixed them all already, in bug 1300337. My bad.
Flags: needinfo?(manishearth)
https://hg.mozilla.org/mozilla-central/rev/3d0ad7bb854d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.