Replace clip-path, box-shadow, float-edge, and user-focus consts with enum classes

RESOLVED FIXED in Firefox 50

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: manishearth, Assigned: manishearth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
(Will upload patches shortly, need a bug number)
(Assignee)

Comment 1

2 years ago
Created attachment 8773288 [details]
Bug 1288383 - Replace NS_STYLE_CLIP_PATH_* with an enum class;

Review commit: https://reviewboard.mozilla.org/r/66036/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66036/
Attachment #8773288 - Flags: review?(cam)
Attachment #8773289 - Flags: review?(cam)
Attachment #8773290 - Flags: review?(cam)
Attachment #8773291 - Flags: review?(cam)
(Assignee)

Comment 2

2 years ago
Created attachment 8773289 [details]
Bug 1288383 - Replace NS_STYLE_BOX_SHADOW_* and NS_STYLE_BASIC_SHAPE_* with enum classes;

Review commit: https://reviewboard.mozilla.org/r/66038/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66038/
(Assignee)

Comment 3

2 years ago
Created attachment 8773290 [details]
Bug 1288383 - Replace NS_STYLE_FLOAT_EDGE_* with an enum class;

Review commit: https://reviewboard.mozilla.org/r/66040/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66040/
(Assignee)

Comment 4

2 years ago
Created attachment 8773291 [details]
Bug 1288383 - Replace NS_STYLE_USER_FOCUS_* with an enum class;

Review commit: https://reviewboard.mozilla.org/r/66042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66042/
(Assignee)

Updated

2 years ago
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Summary: Replace more nsStyleConsts with enums → Replace clip-path, box-shadow, float-edge, and user-focus consts with enum classes
(Assignee)

Comment 5

2 years ago
Comment on attachment 8773291 [details]
Bug 1288383 - Replace NS_STYLE_USER_FOCUS_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66042/diff/1-2/
(Assignee)

Comment 6

2 years ago
Comment on attachment 8773288 [details]
Bug 1288383 - Replace NS_STYLE_CLIP_PATH_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66036/diff/1-2/
(Assignee)

Comment 7

2 years ago
Comment on attachment 8773289 [details]
Bug 1288383 - Replace NS_STYLE_BOX_SHADOW_* and NS_STYLE_BASIC_SHAPE_* with enum classes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66038/diff/1-2/
(Assignee)

Comment 8

2 years ago
Comment on attachment 8773290 [details]
Bug 1288383 - Replace NS_STYLE_FLOAT_EDGE_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66040/diff/1-2/
(Assignee)

Comment 9

2 years ago
Comment on attachment 8773291 [details]
Bug 1288383 - Replace NS_STYLE_USER_FOCUS_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66042/diff/2-3/
(Assignee)

Comment 10

2 years ago
Doesn't fully compile yet, I made a mistake at some point in the process.

Also, TIL you can r?heycam instead of putting r=heycam in the commit message; still used to the old splinter workflow :)
(Assignee)

Comment 11

2 years ago
Compiles now.
(Assignee)

Comment 12

2 years ago
Comment on attachment 8773288 [details]
Bug 1288383 - Replace NS_STYLE_CLIP_PATH_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66036/diff/2-3/
(Assignee)

Comment 13

2 years ago
Comment on attachment 8773289 [details]
Bug 1288383 - Replace NS_STYLE_BOX_SHADOW_* and NS_STYLE_BASIC_SHAPE_* with enum classes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66038/diff/2-3/
(Assignee)

Comment 14

2 years ago
Comment on attachment 8773290 [details]
Bug 1288383 - Replace NS_STYLE_FLOAT_EDGE_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66040/diff/2-3/
(Assignee)

Comment 15

2 years ago
Comment on attachment 8773291 [details]
Bug 1288383 - Replace NS_STYLE_USER_FOCUS_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66042/diff/3-4/
https://reviewboard.mozilla.org/r/66036/#review63078

There is a |using namespace mozilla;| at the top of most .cpp files (including all the .cpp files you're touching in this patch), so no need to qualify things with "mozilla::" in those files.  (I see there are existing, extraneous "mozilla::"s in the files.)

::: layout/style/nsStyleConsts.h:75
(Diff revision 3)
>      Fill,
>      Stroke,
>      View,
>  };
>  
> +// clip-path type

This file isn't sorted in any particular way, but perhaps as we convert these #defines to enum classes we can keep the enum classes sorted.  Let's sort them by their name, and put StyleClipPathType before StyleClipShapeSizing.

::: layout/style/nsStyleConsts.h:78
(Diff revision 3)
>  };
>  
> +// clip-path type
> +enum class StyleClipPathType : uint8_t {
> +  None,
> +  Url,

Gecko style would be to name this as "URL".

::: layout/style/nsStyleStruct.h:3535
(Diff revision 3)
>  
>  private:
>    void ReleaseRef();
>    void* operator new(size_t) = delete;
>  
> -  int32_t mType; // see NS_STYLE_CLIP_PATH_* constants in nsStyleConsts.h
> +  mozilla::StyleClipPathType mType;

Now that this type is smaller, let's move it to just before mSizingBox so that they pack together.
Comment on attachment 8773288 [details]
Bug 1288383 - Replace NS_STYLE_CLIP_PATH_* with an enum class;

https://reviewboard.mozilla.org/r/66036/#review63084
Attachment #8773288 - Flags: review?(cam) → review+
(Assignee)

Comment 18

2 years ago
Comment on attachment 8773288 [details]
Bug 1288383 - Replace NS_STYLE_CLIP_PATH_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66036/diff/3-4/
(Assignee)

Comment 19

2 years ago
Comment on attachment 8773289 [details]
Bug 1288383 - Replace NS_STYLE_BOX_SHADOW_* and NS_STYLE_BASIC_SHAPE_* with enum classes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66038/diff/3-4/
(Assignee)

Comment 20

2 years ago
Comment on attachment 8773290 [details]
Bug 1288383 - Replace NS_STYLE_FLOAT_EDGE_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66040/diff/3-4/
(Assignee)

Comment 21

2 years ago
Comment on attachment 8773291 [details]
Bug 1288383 - Replace NS_STYLE_USER_FOCUS_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66042/diff/4-5/
(Assignee)

Comment 22

2 years ago
Comment on attachment 8773288 [details]
Bug 1288383 - Replace NS_STYLE_CLIP_PATH_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66036/diff/4-5/
(Assignee)

Comment 23

2 years ago
Comment on attachment 8773289 [details]
Bug 1288383 - Replace NS_STYLE_BOX_SHADOW_* and NS_STYLE_BASIC_SHAPE_* with enum classes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66038/diff/4-5/
(Assignee)

Comment 24

2 years ago
Comment on attachment 8773290 [details]
Bug 1288383 - Replace NS_STYLE_FLOAT_EDGE_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66040/diff/4-5/
(Assignee)

Comment 25

2 years ago
Comment on attachment 8773291 [details]
Bug 1288383 - Replace NS_STYLE_USER_FOCUS_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66042/diff/5-6/
(Assignee)

Comment 26

2 years ago
Comment on attachment 8773288 [details]
Bug 1288383 - Replace NS_STYLE_CLIP_PATH_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66036/diff/5-6/
(Assignee)

Comment 27

2 years ago
Comment on attachment 8773289 [details]
Bug 1288383 - Replace NS_STYLE_BOX_SHADOW_* and NS_STYLE_BASIC_SHAPE_* with enum classes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66038/diff/5-6/
(Assignee)

Comment 28

2 years ago
Comment on attachment 8773290 [details]
Bug 1288383 - Replace NS_STYLE_FLOAT_EDGE_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66040/diff/5-6/
(Assignee)

Comment 29

2 years ago
Comment on attachment 8773291 [details]
Bug 1288383 - Replace NS_STYLE_USER_FOCUS_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66042/diff/6-7/
(Assignee)

Comment 30

2 years ago
Comment on attachment 8773288 [details]
Bug 1288383 - Replace NS_STYLE_CLIP_PATH_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66036/diff/6-7/
(Assignee)

Comment 31

2 years ago
Comment on attachment 8773289 [details]
Bug 1288383 - Replace NS_STYLE_BOX_SHADOW_* and NS_STYLE_BASIC_SHAPE_* with enum classes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66038/diff/6-7/
(Assignee)

Comment 32

2 years ago
Comment on attachment 8773290 [details]
Bug 1288383 - Replace NS_STYLE_FLOAT_EDGE_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66040/diff/6-7/
(Assignee)

Comment 33

2 years ago
Comment on attachment 8773291 [details]
Bug 1288383 - Replace NS_STYLE_USER_FOCUS_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66042/diff/7-8/
(Assignee)

Comment 34

2 years ago
Comment on attachment 8773288 [details]
Bug 1288383 - Replace NS_STYLE_CLIP_PATH_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66036/diff/7-8/
(Assignee)

Comment 35

2 years ago
Comment on attachment 8773289 [details]
Bug 1288383 - Replace NS_STYLE_BOX_SHADOW_* and NS_STYLE_BASIC_SHAPE_* with enum classes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66038/diff/7-8/
(Assignee)

Comment 36

2 years ago
Comment on attachment 8773290 [details]
Bug 1288383 - Replace NS_STYLE_FLOAT_EDGE_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66040/diff/7-8/
(Assignee)

Comment 37

2 years ago
Comment on attachment 8773291 [details]
Bug 1288383 - Replace NS_STYLE_USER_FOCUS_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66042/diff/8-9/
(Assignee)

Comment 38

2 years ago
Comment on attachment 8773288 [details]
Bug 1288383 - Replace NS_STYLE_CLIP_PATH_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66036/diff/8-9/
(Assignee)

Comment 39

2 years ago
Comment on attachment 8773289 [details]
Bug 1288383 - Replace NS_STYLE_BOX_SHADOW_* and NS_STYLE_BASIC_SHAPE_* with enum classes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66038/diff/8-9/
(Assignee)

Comment 40

2 years ago
Comment on attachment 8773290 [details]
Bug 1288383 - Replace NS_STYLE_FLOAT_EDGE_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66040/diff/8-9/
(Assignee)

Comment 41

2 years ago
Comment on attachment 8773291 [details]
Bug 1288383 - Replace NS_STYLE_USER_FOCUS_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66042/diff/9-10/
Comment on attachment 8773289 [details]
Bug 1288383 - Replace NS_STYLE_BOX_SHADOW_* and NS_STYLE_BASIC_SHAPE_* with enum classes;

https://reviewboard.mozilla.org/r/66038/#review63448

::: layout/style/nsRuleNode.cpp:4256
(Diff revision 9)
>                          aConditions);
>        NS_ASSERTION(unitOK, "unexpected unit");
>      }
>  
>      if (aIsBoxShadow && arr->Item(5).GetUnit() == eCSSUnit_Enumerated) {
> -      NS_ASSERTION(arr->Item(5).GetIntValue() == NS_STYLE_BOX_SHADOW_INSET,
> +      NS_ASSERTION(arr->Item(5).GetIntValue() 

Nit: trailing space.
Attachment #8773289 - Flags: review?(cam) → review+
Comment on attachment 8773290 [details]
Bug 1288383 - Replace NS_STYLE_FLOAT_EDGE_* with an enum class;

https://reviewboard.mozilla.org/r/66040/#review63454

::: layout/style/nsStyleStruct.h:1273
(Diff revision 9)
> -  uint8_t        mFloatEdge;          // [reset]
> +  mozilla::StyleFloatEdge 
> +                 mFloatEdge;          // [reset]

Nit: I wouldn't try too hard to keep the field names aligned, so all on one line is fine.

::: layout/style/nsStyleStruct.cpp:2858
(Diff revision 9)
>    mProperty = aProperty;
>    mUnknownProperty = NS_Atomize(aPropertyString);
>  }
>  
>  bool
> -StyleTransition::operator==(const mozilla::StyleTransition& aOther) const
> +StyleTransition::operator==(const StyleTransition& aOther) const

(This is fine, though unrelated cleanups would be better in a separate patch.)
Attachment #8773290 - Flags: review?(cam) → review+
Comment on attachment 8773291 [details]
Bug 1288383 - Replace NS_STYLE_USER_FOCUS_* with an enum class;

https://reviewboard.mozilla.org/r/66042/#review63458

::: dom/events/EventStateManager.cpp:2957
(Diff revision 10)
>            // we click on a visibility: none element.
>            // We can't use nsIContent::IsFocusable() because we want to blur when
>            // we click on a non-focusable element like a <div>.
>            // We have to use |aEvent->mTarget| to not make sure we do not check
>            // an anonymous node of the targeted element.
> -          suppressBlur = (ui->mUserFocus == NS_STYLE_USER_FOCUS_IGNORE);
> +          suppressBlur = (ui->mUserFocus == mozilla::StyleUserFocus::Ignore);

Drop the "mozilla::", since we're inside a "namespace mozilla { ... }" here.
Attachment #8773291 - Flags: review?(cam) → review+
(Assignee)

Comment 45

2 years ago
Comment on attachment 8773288 [details]
Bug 1288383 - Replace NS_STYLE_CLIP_PATH_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66036/diff/9-10/
(Assignee)

Comment 46

2 years ago
Comment on attachment 8773289 [details]
Bug 1288383 - Replace NS_STYLE_BOX_SHADOW_* and NS_STYLE_BASIC_SHAPE_* with enum classes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66038/diff/9-10/
(Assignee)

Comment 47

2 years ago
Comment on attachment 8773290 [details]
Bug 1288383 - Replace NS_STYLE_FLOAT_EDGE_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66040/diff/9-10/
(Assignee)

Comment 48

2 years ago
Comment on attachment 8773291 [details]
Bug 1288383 - Replace NS_STYLE_USER_FOCUS_* with an enum class;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66042/diff/10-11/

Comment 49

2 years ago
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/90370c9e5d43
Replace NS_STYLE_CLIP_PATH_* with an enum class; r=heycam
https://hg.mozilla.org/integration/autoland/rev/9e0a6977e3fe
Replace NS_STYLE_BOX_SHADOW_* and NS_STYLE_BASIC_SHAPE_* with enum classes; r=heycam
https://hg.mozilla.org/integration/autoland/rev/c12169cb6b29
Replace NS_STYLE_FLOAT_EDGE_* with an enum class; r=heycam
https://hg.mozilla.org/integration/autoland/rev/4aa9625fa66d
Replace NS_STYLE_USER_FOCUS_* with an enum class; r=heycam
(Assignee)

Comment 50

2 years ago
Created attachment 8774271 [details] [diff] [review]
Bustage fix

Fixes a bustage in autoland, can't land directly
(Assignee)

Comment 51

2 years ago
Created attachment 8774273 [details] [diff] [review]
Interdiff for bustage fix
Comment on attachment 8774273 [details] [diff] [review]
Interdiff for bustage fix

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

r=me if Tomcat adds an appropriate commit message like "bustage fix". :-)
Attachment #8774273 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8774271 - Attachment is obsolete: true

Comment 53

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d69f9992ba40
"Replace clip-path, box-shadow, float-edge, and user-focus consts with enum classes" bustage fix. a=tomcat
(Assignee)

Updated

2 years ago
Blocks: 1277133
You need to log in before you can comment on or make changes to this bug.