Closed Bug 1295271 Opened 3 years ago Closed 3 years ago

Remove _moz_ prefix for internal constants for multicol related properties

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: neerja, Assigned: neerja)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

This bug was created as a sub part of the fix for Bug 698783 and represents a fix for part 1 from David Baron's comment on the original bug.

Copying David Baron's comment here for Bug 698783 ->

So as we discussed earlier today, I think there are a few things to do here that should be separated into a few different patches:

 (1) we should get rid of the _moz_ in the internal constants for these properties, anytime.  That's just code cleanup.

 (2) we should import the multicol reftests from the CSSWG test repository.  This has a few steps:
     (a) rerun layout/reftests/w3c-css/import-tests.py , and if there are any changes, make a patch with them and push it to try.
     (b) then, add importing of the multicol tests, and do the same.  This depends on (2a).
     (c) evaluate the failures of the multicol tests added in (b) and figure out if we're doing worse than other browsers that have unprefixed.  This depends on (2b).

 (3) write a patch to layout/style/nsCSSPropList.h to remove the -moz- prefixes there, but add them back as aliases in layout/style/nsCSSPropAliasList.h, and then make the same changes to layout/style/test/property_database.js and any other tests that show failures (probably a small number).  This depends on (2c) to decide whether we're ready to do it.

 (4) Substitute all of the rest of the tree to remove the -moz- prefixes for these properties.  This depends on (3).

 (5) Remove the aliases introduced in (3).  This depends on (3) and (4), and shouldn't be done until after we've successfully shipped (3).

All of these (1, 2a, 2b, 2c, 3, 4, and 5) could be different bugs, although it's ok if 2a and 2b are in a single bug, and it's also ok if 3 and 4 are in a single bug, or even 1, 3, and 4.  But things that happen at different times should be in separate bugs even if they could be in the same bug if done at the same time.
Could you provide the full command that you used to do the renaming, in the commit message?

(Not in the first line, but in a later line -- commit messages can be multi-line, with the first line having "Bug ### - Summary. r=whoever", and later lines providing more details/information)

That's helpful for review purposes, for assisting with possible-merge-conflicts (e.g. if the patch needs to be regenerated just before it lands due to someone else having moved code around), and for posterity (when someone's looking at the changeset in the future and wondering about how it was generated).

See e.g. https://hg.mozilla.org/mozilla-central/rev/702bca0deee2 for a recent similar search-and-replace changeset which included the command in the commit message.
Flags: needinfo?(npancholi)
(In reply to Daniel Holbert [:dholbert] from comment #2)
> Could you provide the full command that you used to do the renaming, in the
> commit message?
> 
> (Not in the first line, but in a later line -- commit messages can be
> multi-line, with the first line having "Bug ### - Summary. r=whoever", and
> later lines providing more details/information)
> 
> That's helpful for review purposes, for assisting with
> possible-merge-conflicts (e.g. if the patch needs to be regenerated just
> before it lands due to someone else having moved code around), and for
> posterity (when someone's looking at the changeset in the future and
> wondering about how it was generated).
> 
> See e.g. https://hg.mozilla.org/mozilla-central/rev/702bca0deee2 for a
> recent similar search-and-replace changeset which included the command in
> the commit message.

Done :)
Flags: needinfo?(npancholi)
Comment on attachment 8782669 [details]
Bug 1295271 - Remove all _moz_ prefixes from all instances of _moz_column

https://reviewboard.mozilla.org/r/72732/#review70812
Attachment #8782669 - Flags: review?(dbaron) → review+
Comment on attachment 8783072 [details]
Bug 1295271 - Manually fix some whitespace to correct mis-indentation after automated _moz_ prefix removal

https://reviewboard.mozilla.org/r/73038/#review70814

The commit message should probably be "Manually fix" rather than "Manually fixing".

(And, actually, the previous commit message probably shouldn't have the "Used script to " at the beginning.)
Attachment #8783072 - Flags: review?(dbaron) → review+
Ran test on treeherher: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c80ec2977ce2

Most failures are for Mochi tests and the one Reftest intermittent failure on Linux only seems unrelated to any multicol properties so I'm marking this as 'checkin-needed'
Keywords: checkin-needed
Autoland hit conflicts. Please rebase.
Keywords: checkin-needed
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/66e185d40b6c
Remove all _moz_ prefixes from all instances of _moz_column r=dbaron
https://hg.mozilla.org/integration/autoland/rev/56cc74607c72
Manually fix some whitespace to correct mis-indentation after automated _moz_ prefix removal r=dbaron
https://hg.mozilla.org/mozilla-central/rev/66e185d40b6c
https://hg.mozilla.org/mozilla-central/rev/56cc74607c72
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.