1273424 - Rename some style keyword constants to be consistent with style keywords

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Keith Yeung [:KiChjang]]

Attachment: 0001-Rename-moz-float-edge-constants.patch

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160503215307




Expected results:

Style keyword constants like NS_STYLE_FLOAT_EDGE_CONTENT should be renamed as NS_STYLE_FLOAT_EDGE_CONTENT_BOX in order to be consistent with eCSSKeyword_content_box (necessary for stylo).

Comment 1

[Cameron McCormack (:heycam)]

Comment on attachment 8753257 [details] [diff] [review]
0001-Rename-moz-float-edge-constants.patch

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

Patch looks good, thanks!

Can you attach a patch in hg format?  Since someone will need to check in the patch on your behalf, it's good to get it as close as possible to the final patch that the sheriffs will import.  I use the git-patch-to-hg-patch tool from https://github.com/mozilla/moz-git-tools to do this, when working from a git gecko-dev checkout, to take my top commit and turn it into an hg patch:

  $ git format-patch -U8 -pk --stdout HEAD~1 | git-patch-to-hg-patch > a.patch

(You'd think I'd have an alias for this but I don't.)  The commit line should be of the form "Bug NNNNNNN - Rename -moz-float [...] with keywords. r=heycam" though in general you don't need to fill in the bug number and r=foo (the sherrifs can fill those in).

Also, please try to find a reviewer for the patch when attaching it.  Unlike in the servo repository, a random reviewer will not be assigned automatically, so it's possible for patches to slip through the cracks without an explicit reviewer.  The bugzilla interface can suggest appropriate reviewers for the component the bug is filed in.

Comment 2

[Cameron McCormack (:heycam)]

When you have your updated patch attached, set the checkin-needed keyword in the bug and the sheriffs will land the patch for you.

I've pushed the patch to try (another thing which Servo's infrastructure will do automatically for you but here in Gecko-land needs to be done automatically, unless you use mozreview): https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1749fdfbd8e

Comment 3

[Keith Yeung [:KiChjang]]

Just a heads-up, this still requires a bit more renaming for -moz-appearance, and I'm working on it. Can my first patch be submitted to the main repo in the meantime though?

Comment 4

[Cameron McCormack (:heycam)]

Yes, that's fine.  If you want to attach a few separate patches to this bug you can add the leave-open keyword and keep working in this bug.  Or a separate bug is fine, too.  (I tend to use separate bugs if I have part work that's landable, just to avoid confusion about the bug status.)

Comment 5

[Keith Yeung [:KiChjang]]

Attachment: Rename-moz-float-edge-constants.patch

Comment 6

[Keith Yeung [:KiChjang]]

Attachment: Rename-moz-appearance-constants.patch

Comment 7

[Keith Yeung [:KiChjang]]

Attachment: Rename-moz-appearance-constants.patch

Comment 8

[Keith Yeung [:KiChjang]]

Attachment: Rename-moz-appearance-constants.patch

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8753400 [details] [diff] [review]
Rename-moz-appearance-constants.patch

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

I kinda skimmed this with some spot-checking, but assuming you did this in some sort of automated way (i.e. find+replace) it should be fine. Thanks for doing this!

Comment 10

[Bobby Holley (:bholley)]

Doing an all-platform try build for the platform-specific -moz-appearance stuff. I'll push to inbound when it's green.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9406eccae2f3

Comment 11

[Keith Yeung [:KiChjang]]

Attachment: Rename-moz-appearance-constants.patch

Comment 12

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a0da84e4f00

Comment 13

[Bobby Holley (:bholley)]

Looks like this just collided with bug 1192053, which is pretty unlucky. Needs a rebase.

Comment 14

[Keith Yeung [:KiChjang]]

Attachment: Rename-moz-appearance-constants.patch

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e995b22f325e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c7f405a2cb6

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e995b22f325e
https://hg.mozilla.org/mozilla-central/rev/9c7f405a2cb6

Comment 17

[Keith Yeung [:KiChjang]]

Sorry, I seem to have missed 1 property and mistaken another 1. Reopening.

Comment 18

[Keith Yeung [:KiChjang]]

Attachment: Bug 1273424 - Align-resizerpanel-and-caret.patch

Comment 19

[Keith Yeung [:KiChjang]]

Superseded by 1274339.

Comment 20

[Bobby Holley (:bholley)]

Well, not quite a dupe because parts of this already landed.