1548341 - Convert NS_STYLE_BORDER to an enum class in nsStyleConsts.h

Status: RESOLVED FIXED

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

Description

[Jeremy Ir]

Attachment: styleBorderPatch

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

Converted #define NS_STYLE_BORDER values to an enum class in nsStyleConsts.h
Performed a recursive grep on mozilla-central to find all instances of defined constants and replaced them with their enumerated equivalents

Performed a build

Actual results:

Failed build due to the NS_STYLE_BORDER values not found in module structs by the gecko_properties.rs file

Expected results:

Successful compilation

Comment 1

[Jeremy Ir]

Attachment: styleBorderPatch.error

Comment 2

[Daniel Holbert [:dholbert]]

Thanks for taking this!

That error is in Rust code that's generated during the build process. You probably need to update some rust/C++ glue code to make this code get generated properly -- based on a recent similar change for another ALL_CAPS_KEYWORD conversion, it looks like:

• you need to add your new type to this list: https://searchfox.org/mozilla-central/rev/7944190ad1668a94223b950a19f1fffe8662d6b8/layout/style/ServoBindings.toml#59

• you need to add a gecko_enum_prefix decl to the appropriate block, probably here:
  https://searchfox.org/mozilla-central/rev/7944190ad1668a94223b950a19f1fffe8662d6b8/servo/components/style/properties/longhands/border.mako.rs#45

(See the .rs and .toml changes in the above-linked recent similar change for more.)

And you also might need to clobber your old build (./mach clobber) to get rid of the old generated code. Though I suspect if you update those ^^ files, the relevant old stuff might get cleaned up & regenerated as part of the normal build process.

Comment 3

[Daniel Holbert [:dholbert]]

Also: mozilla uses phabricator for patch upload + review these days:
https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

For now, you can still post patches as bugzilla attachments, but I think that'll stop working (at least for review-UI purposes) soon -- and if you're planning on submitting more patches (hopefully you do!), it'd probably be worth getting set up & familiar with the phabricator workflow.

Comment 4

[Daniel Holbert [:dholbert]]

( There seems to be a pretty good walkthrough of the phabricator patch-submission workflow at https://moz-conduit.readthedocs.io/en/latest/walkthrough.html BTW)

Comment 5

[Jeremy Ir]

Thank you for the thorough response. I'll update this with my changes when I am able to as it appears that the change isn't as trivial as adding that gecko_enum_prefix. That argument is explicitly handled in kwargs for the single_keyword_system definition in the helpers.mako.rs. This may be as simple as adding the same argument handling for predefined_type, but, this is my first exposure to Rust, so I'm treading more gingerly with regards to any changes in it.

I appreciate the ./mach clobber command. I kept looking through various ./mach build options looking for the more aggressive form of build clean that build systems usually have, but came up dry.

With regards to Phabricator, I'll migrate toward using that for posting patches (was expecting to do so for normal CRs). I much prefer Phabricator to generating patch attachments, but was unaware of how the workflow worked at Mozilla specifically. Thanks for the docs for that.

Comment 6

[Daniel Holbert [:dholbert]]

Sounds good! If the suggestions in comment 2 don't fully address the build warnings, and the build errors are inscrutable, I'd suggest comparing your changes against the patch for some other recent helper-bug that's chained off of bug 1277133 for ideas. But feel free to ask here, too.

Good luck!

Comment 7

[Jeremy Ir]

It looks like these two constants broke the standardized pattern that the rest of the ones in nsStyleConsts.h used, which is why things seemed so different than the changes you reference in comment 2 (as I mentioned in comment 5). The NS_STYLE_BORDER_ prefix was throwing us on the wrong trail. I cross-reference the collapse and separate values with the utilization in the code and found that they belonged to the border-collapse CSS property, so the consts should have originally been NS_STYLE_BORDER_COLLAPSE_COLLAPSE and NS_STYLE_BORDER_COLLAPSE_SEPARATE. I've made the appropriate changes with the migration to the enumerated class.

However, that was not what was causing the compilation failures. It looks like I needed to update border collapse variable type here.

I've successfully built and ran the source. As I understand it from reading some of the other similar bugs, nothing more should be required for testing besides any build/test logic Phabricator may launch . Is this correct?

Comment 8

[Jeremy Ir]

Attachment: Bug 1548341 - Convert NS_STYLE_BORDER to an enum class in nsStyleConsts.h. r?dholbert

Converting the NS_STYLE_BORDER definitions in to enumerated classes as
per bug 1277133.

The original constants broke the convention used by the rest of the
definitions as the CSS property being described is border-collapse,
so corrections were made with the migration to the enumerated class.

Comment 9

[Daniel Holbert [:dholbert]]

Thanks for the patch!

I've posted one nit on phabricator, but this looks pretty much ready aside from that.

I took the liberty of submitting it to our "Try" tree to see if it passes tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e52102549bc0e97c950013bc55dfbe99fd9befc

(Don't be too alarmed if unrelated-looking things turn up orange there -- there may be some known intermittent test failures. I'd only be concerned with failures that use <table>s and/or borders.)

Comment 10

[Daniel Holbert [:dholbert]]

The Try run looks good!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e99ec6c4418f
Convert NS_STYLE_BORDER to an enum class in nsStyleConsts.h. r=dholbert

Comment 12

[Daniel Holbert [:dholbert]]

This has now landed on our 'autoland' integration branch -- as long as it's not backed out, it should be merged to mozilla-central within a day or so, at which point this bug will automatically be closed as FIXED.

(And if you like, you can remove your local copy of the commit with hg prune or hg strip -- the latter is slightly more destructive of local state, if you make a mistake.)

Thanks again for the patch!

Comment 13

[Alexandru Michis [:malexandru]]

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