Closed
Bug 1389862
Opened 7 years ago
Closed 7 years ago
Change NS_STYLE_CONTENT macro definitions to scoped enum class StyleContent
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: hrdktg, Assigned: hrdktg)
References
Details
Attachments
(2 files, 3 obsolete files)
4.67 KB,
patch
|
manishearth
:
review+
|
Details | Diff | Splinter Review |
59 bytes,
text/x-review-board-request
|
Details |
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•7 years ago
|
Attachment #8896666 -
Flags: review?(manishearth)
Updated•7 years ago
|
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
Assignee | ||
Updated•7 years ago
|
Attachment #8896666 -
Attachment is obsolete: true
Attachment #8896666 -
Flags: review?(manishearth)
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8896693 -
Flags: review?(manishearth)
Comment 2•7 years ago
|
||
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•7 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?
Comment 4•7 years ago
|
||
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•7 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?
Comment 6•7 years ago
|
||
No, just cast it.
Assignee | ||
Comment 7•7 years ago
|
||
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•7 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)
Comment 9•7 years ago
|
||
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+
Updated•7 years ago
|
Assignee: nobody → hrdktg
Keywords: checkin-needed
Comment 10•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/576d9884e315
Status: UNCONFIRMED → RESOLVED
Closed: 7 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.
Description
•