1312173 - Convert NS_STYLE_USER_INPUT_* to an enum class

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Adam Velebil]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161013141419

Comment 1

[Adam Velebil]

Attachment: Bug 1312173 - Convert NS_STYLE_USER_INPUT_* to an enum class;

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

Comment 2

[Manish Goregaokar [:manishearth]]

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!

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 4

[Adam Velebil]

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?

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

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.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 7

[Adam Velebil]

Attachment: Bug 1312173 - Convert NS_STYLE_USER_INPUT_* to an enum class; CodingStandards;

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

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

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".

Comment 9

[Adam Velebil]

Comment on attachment 8803602 [details]
Bug 1312173 - Convert NS_STYLE_USER_INPUT_* to an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87802/diff/1-2/

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d0ad7bb854d
Convert NS_STYLE_USER_INPUT_* to an enum class;  r=manishearth,xidorn

Comment 12

[Manish Goregaokar [:manishearth]]

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?

Comment 13

[Adam Velebil]

(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

Comment 14

[Manish Goregaokar [:manishearth]]

Oh, looks like I fixed them all already, in bug 1300337. My bad.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d0ad7bb854d