Closed Bug 1611041 Opened 9 months ago Closed 9 months ago

Convert image-rendering #defines to an enum class.

Categories

(Core :: CSS Parsing and Computation, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox74 --- fixed

People

(Reporter: emilio, Assigned: thomas.dolezal, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [lang=c++][lang=rust])

Attachments

(1 file, 8 obsolete files)

https://searchfox.org/mozilla-central/rev/e878e5b81bb319c141900ce9cfcde732df5c8449/layout/style/nsStyleConsts.h#746

See all the already-fixed bugs blocking bug 1277133 for existing examples of this. But let me know if you need concrete steps and I'm happy to provide them.

Hi, I am a beginner and wanted to work on this. What steps to follow?

Flags: needinfo?(emilio)

So you need to replace this set of #defines by something like:

enum class StyleImageRendering : uint8_t {
    Auto,
    Optimizespeed,
    Optimizequality,
    CrispEdges,
}

Then you need to add gecko_enum_prefix="StyleImageRendering" to this entry.

Then you need to add mozilla::StyleImageRendering to this list.

Change this line to be:

mozilla::StyleImageRendering mImageRendering;

And then replace all the usages of NS_STYLE_IMAGE_RENDERING_* by the relevant constant (there's only one).

Flags: needinfo?(emilio)
Attached patch bug1611041.patch (obsolete) — Splinter Review

I took a try at this issue based on how Emilio described. Looking for any feedback.

Attached file Replaced as Emilio described (obsolete) —

Sorry I believe I missed a usage in nsLayoutUtils.cpp

Attachment #9123425 - Attachment is obsolete: true
Attached patch out.patch (obsolete) — Splinter Review
Attachment #9123429 - Attachment is obsolete: true
Attached patch 1611041.patch (obsolete) — Splinter Review
Attachment #9123430 - Attachment is obsolete: true

Sorry I had trouble with the patch file

Attached patch Patch for 1611041 (obsolete) — Splinter Review

This patch file should have all my changes

Attachment #9123431 - Attachment is obsolete: true
Attached patch 1611041 fix (obsolete) — Splinter Review

I think this patch file should have the formatting properly since i noticed in previous was not looking good, sorry for all the spam

Attachment #9123436 - Attachment is obsolete: true
Comment on attachment 9123438 [details] [diff] [review]
1611041 fix

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

This looks generally sane, but it doesn't build.

It'd also be great to submit the patch via phabricator, which is the supported system in theory: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

I don't mind reviewing attachments, but as you cannot tag me as a reviewer it is easy to miss.

Thank you for the patch, it looks really close!

::: layout/style/nsStyleConsts.h
@@ +89,5 @@
>    /// XUL boxes.
>    MozBox =
>        StyleDisplayFrom(StyleDisplayOutside::Block, StyleDisplayInside::MozBox),
> +  MozInlineBox =
> +      StyleDisplayFrom(StyleDisplayOutside::Inline, StyleDisplayInside::MozBox),

This change looks unrelated?

::: layout/style/nsStyleStruct.h
@@ +1261,4 @@
>    mozilla::StyleImageOrientation mImageOrientation;
>    uint8_t mDirection;  // NS_STYLE_DIRECTION_*
>    mozilla::StyleVisibility mVisible;
> +  mozilla::StyleImageRendering;  // NS_STYLE_IMAGE_RENDERING_*

This doesn't compile, needs to be `mozilla::StyleImageRendering mImageRendering;` (and the comment is no longer needed).

I fixed the line to include mImageRendering, I cannot get it too successfully build since the build says something is wrong with the servo style bindings in the toml file. Could there be another place I need to look?

Could you attach the full error?

Attached file error.log (obsolete) —

0:17.37 cargo:rerun-if-changed=/home/tom/src/mozilla-central/layout/style/ServoBindings.toml
0:17.37 --- stderr

Attachment #9123547 - Attachment mime type: text/x-log → text/plain
Comment on attachment 9123438 [details] [diff] [review]
1611041 fix

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

::: layout/style/nsStyleConsts.h
@@ +748,5 @@
> +  Auto,
> +  Optimizespeed,
> +  Optimizequality,
> +  CrispEdges,
> +}

You need a semicolon here. Relevant part of that error is:

>  0:17.38 /home/tom/src/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsStyleConsts.h:752:2: error: expected ';' after enum

A bit unfortunate that the rust-error parsing the C++ came before than the C++ error itself, which would've probably been more helpful.
Attached file 1611041: fixed errors (obsolete) —

Have fixed the two mistakes in this patch hopefully. It builds successfully

Attachment #9123438 - Attachment is obsolete: true
Attachment #9123547 - Attachment is obsolete: true

Patch for the fix

Attachment #9123567 - Attachment is obsolete: true
Comment on attachment 9123568 [details] [diff] [review]
patch for 1611041

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

Thanks for the fix, this looks great! :)

Please try to follow the documented process for submitting patches[1] next time? That is generally easier and better than attaching patches to bugzilla.

I can check-in this for you this time though. Thank you again!

[1]: https://firefox-source-docs.mozilla.org/contributing/how_to_contribute_firefox.html#to-submit-a-patch
Attachment #9123568 - Flags: review+
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/c06f42caa508
Convert image-rendering #defines to an enum class. r=emilio
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
Assignee: nobody → thomas.dolezal
You need to log in before you can comment on or make changes to this bug.