1389862 - Change NS_STYLE_CONTENT macro definitions to scoped enum class StyleContent

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[hardik tyagi [:hrdktg]]

Attachment: StyleContent.diff

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

Steps to reproduce:

Steps to reproduce:

Nothing to reproduce


Actual results:

Actual results:

Currently using NS_STYLE_CONTENT macro definitions


Expected results:

Expected results:

Changed this to enum class StyleContent

Comment 1

[hardik tyagi [:hrdktg]]

Attachment: StyleContent_int.diff

Comment 2

[Manish Goregaokar [:manishearth]]

Comment on attachment 8896693 [details] [diff] [review]
StyleContent_int.diff

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

Looks good, minor issue.

I'll do a try push for you.

::: servo/components/style/gecko/generated/structs_debug.rs
@@ +299,4 @@
>      pub const NS_STYLE_BORDER_IMAGE_REPEAT_SPACE: ::std::os::raw::c_uint = 3;
>      pub const NS_STYLE_BORDER_IMAGE_SLICE_NOFILL: ::std::os::raw::c_uint = 0;
>      pub const NS_STYLE_BORDER_IMAGE_SLICE_FILL: ::std::os::raw::c_uint = 1;
> +    pub const StyleContent::OpenQuote: ::std::os::raw::c_uint = 0;

This file shouldn't be edited

Comment 3

[hardik tyagi [:hrdktg]]

(In reply to Manish Goregaokar [:manishearth] from comment #2)
> Comment on attachment 8896693 [details] [diff] [review]
> StyleContent_int.diff
> 
> Review of attachment 8896693 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, minor issue.
> 
> I'll do a try push for you.

I am trying to get into Open-source stuff and this patch the first I have ever made. Complete newbie so hoping for help along the way.
Thank you for reviewing.
 
> ::: servo/components/style/gecko/generated/structs_debug.rs
> @@ +299,4 @@
> >      pub const NS_STYLE_BORDER_IMAGE_REPEAT_SPACE: ::std::os::raw::c_uint = 3;
> >      pub const NS_STYLE_BORDER_IMAGE_SLICE_NOFILL: ::std::os::raw::c_uint = 0;
> >      pub const NS_STYLE_BORDER_IMAGE_SLICE_FILL: ::std::os::raw::c_uint = 1;
> > +    pub const StyleContent::OpenQuote: ::std::os::raw::c_uint = 0;
> 
> This file shouldn't be edited

Why that file should not be edited? What happens to the NS_STYLE_CONTENT_* in this file then?

Comment 4

[Manish Goregaokar [:manishearth]]

There were build errors in the try push, please ensure it builds and runs before uploading a patch.

https://treeherder.mozilla.org/logviewer.html#?job_id=122826980&repo=try&lineNumber=21014

> Why that file should not be edited? What happens to the NS_STYLE_CONTENT_* in this file then?


Everything under servo/ is vendored from https://github.com/servo/servo, and those are autogenerated bindings. When landing this I'll deal with the servo side changes for you since we don't have sync set up yet.

Comment 5

[hardik tyagi [:hrdktg]]

(In reply to Manish Goregaokar [:manishearth] from comment #4)
> There were build errors in the try push, please ensure it builds and runs
> before uploading a patch.
> 
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=122826980&repo=try&lineNumber=21014
> 
> > Why that file should not be edited? What happens to the NS_STYLE_CONTENT_* in this file then?
> 
> 
> Everything under servo/ is vendored from https://github.com/servo/servo, and
> those are autogenerated bindings. When landing this I'll deal with the servo
> side changes for you since we don't have sync set up yet.


Weird, On my system it builds successfully and runs too.

leaving that aside:
> error: no match for 'operator==' (operand types are 'int32_t {aka int}' and 'mozilla::StyleContent')
> MOZ_ASSERT(contentValue->GetIntValue() == StyleContent::AltContent

GetIntValue() returns int32_t
> int32_t GetIntValue() const;

So if I cast the StyleContent::AltContent into int32_t, would that resolve the error? 
or should I change the declaration of 
> enum class StyleContent : uint8_t
to int32_t?

Comment 6

[Manish Goregaokar [:manishearth]]

No, just cast it.

Comment 7

[hardik tyagi [:hrdktg]]

Attachment: StyleContent_int_casted.diff

changed
>MOZ_ASSERT(contentValue->GetIntValue() == StyleContent::AltContent,
to
>MOZ_ASSERT(contentValue->GetIntValue() == int32_t(StyleContent::AltContent),

Comment 8

[hardik tyagi [:hrdktg]]

Attachment: StyleContent_int_casted_final.diff

changed
>MOZ_ASSERT(contentValue->GetIntValue() == StyleContent::AltContent,
to
>MOZ_ASSERT(contentValue->GetIntValue() == int32_t(StyleContent::AltContent),

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8896846 [details] [diff] [review]
StyleContent_int_casted_final.diff

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

Try passes

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

Doesn't need servo side changes after all

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/576d9884e315
Change NS_STYLE_CONTENT macro definitions to scoped enum class StyleContent; r=manishearth

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/576d9884e315

Comment 12

[akriti verma]

Attachment: bug 1389862 - created enum class stylefont

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