1611041 - Convert image-rendering #defines to an enum class.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 1

[shrinivaschowdhary97]

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

Comment 2

[Emilio Cobos Álvarez (: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).

Comment 3

[Thomas Dolezal]

Attachment: bug1611041.patch

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

Comment 4

[Thomas Dolezal]

Attachment: Replaced as Emilio described

Sorry I believe I missed a usage in nsLayoutUtils.cpp

Comment 5

[Thomas Dolezal]

Attachment: out.patch

Comment 6

[Thomas Dolezal]

Attachment: 1611041.patch

Comment 7

[Thomas Dolezal]

Sorry I had trouble with the patch file

Comment 8

[Thomas Dolezal]

Attachment: Patch for 1611041

This patch file should have all my changes

Comment 9

[Thomas Dolezal]

Attachment: 1611041 fix

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

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).

Comment 11

[Thomas Dolezal]

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?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Could you attach the full error?

Comment 13

[Thomas Dolezal]

Attachment: error.log

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

Comment 14

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 15

[Thomas Dolezal]

Attachment: 1611041: fixed errors

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

Comment 16

[Thomas Dolezal]

Attachment: patch for 1611041

Patch for the fix

Comment 17

[Emilio Cobos Álvarez (:emilio)]

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c06f42caa508
Convert image-rendering #defines to an enum class. r=emilio

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c06f42caa508