Change NS_STYLE_CONTENT macro definitions to scoped enum class StyleContent

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hrdktg, Assigned: hrdktg)

Tracking

(Blocks: 1 bug)

55 Branch
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

2 years ago
Posted patch StyleContent.diff (obsolete) — Splinter Review
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
(Assignee)

Updated

2 years ago
Attachment #8896666 - Flags: review?(manishearth)
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
(Assignee)

Updated

2 years ago
Attachment #8896666 - Attachment is obsolete: true
Attachment #8896666 - Flags: review?(manishearth)
(Assignee)

Comment 1

2 years ago
Posted patch StyleContent_int.diff (obsolete) — Splinter Review
Attachment #8896693 - Flags: review?(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
Attachment #8896693 - Flags: review?(manishearth) → review+
(Assignee)

Comment 3

2 years ago
(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?
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.
(Assignee)

Comment 5

2 years ago
(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?
No, just cast it.
(Assignee)

Comment 7

2 years ago
Posted patch StyleContent_int_casted.diff (obsolete) — Splinter Review
changed
>MOZ_ASSERT(contentValue->GetIntValue() == StyleContent::AltContent,
to
>MOZ_ASSERT(contentValue->GetIntValue() == int32_t(StyleContent::AltContent),
Attachment #8896693 - Attachment is obsolete: true
Attachment #8896828 - Flags: review?(manishearth)
(Assignee)

Comment 8

2 years ago
changed
>MOZ_ASSERT(contentValue->GetIntValue() == StyleContent::AltContent,
to
>MOZ_ASSERT(contentValue->GetIntValue() == int32_t(StyleContent::AltContent),
Attachment #8896828 - Attachment is obsolete: true
Attachment #8896828 - Flags: review?(manishearth)
Attachment #8896846 - Flags: review?(manishearth)
(Assignee)

Updated

2 years ago
Blocks: 1277133
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
Attachment #8896846 - Flags: review?(manishearth) → review+
Assignee: nobody → hrdktg
Keywords: checkin-needed

Comment 10

2 years ago
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/576d9884e315
Change NS_STYLE_CONTENT macro definitions to scoped enum class StyleContent; r=manishearth
Keywords: checkin-needed

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/576d9884e315
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment hidden (mozreview-request)
You need to log in before you can comment on or make changes to this bug.