Closed Bug 1293739 Opened 3 years ago Closed 3 years ago

Rename nsCSSProperty -> nsCSSPropertyID, nsCSSPropertySet -> nsCSSPropertyIDSet

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jyc, Assigned: jyc)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Rename nsCSSProperty to nsCSSPropertyID and nsCSSPropertyID to nsCSSPropertyIDSet in preparation for the Properties & Values patches (1273706) adding a new mozilla::CSSProperty type for representing custom and standard properties.

We discussed this on #layout and the consensus was that this would be the least confusing / wordy solution (instead of naming mozilla::CSSProperty mozilla::CSSPossiblyCustomProperty).
I imagine you used a "sed" command -- or some other sort of automated search-and-replace -- to generate these patches, right?  (plus a manual "hg mv" for the header file itself, perhaps)

If so: could you please provide the command you used?

For automated mass-renames like this, manual line-by-line patch review is kinda impractical & not the best use of resources.

The sanest way for reviewers to r+ changes like this is generally for them (i.e. me) do the following:
 (1) Review the automated-rewriting command itself, for sanity.
 (2) Run the automated-rewriting command locally and ensure that it generates the same patch. (plus maybe a few manual changes)
 (3) Skim the generated patch to see if anything jumps out as obviously-broken (but not review it in detail).

...but that can only happen if we have an automated-rewriting command to review.

(Ideally, the command that you used should be included in the extended commit message, too.)
Flags: needinfo?(jyc)
Hi! As discussed on IRC, I have added the required commands to the patch. Commenting to clear the needinfo.
Flags: needinfo?(jyc)
From part 1:
> (Note: on my computer, this somehow mishandles two symlinked files,
> testing/mozharness/configs/single_locale/linux32.py and media/libav/README -- I
> removed the changes from the changeset manually as well).

It looks like those are the only two symlinks in the source tree -- and it mishandles them because "sed -i" just stomps on symlinks and replaces them with regular files.

There's some more info about this here:
  https://www.reddit.com/r/linux/comments/kehtf/be_careful_when_using_sed_i_on_symbolic_links/
  https://unix.stackexchange.com/questions/192012/how-to-avoid-sed-i-to-destroy-symlink

It looks like the "--follow-symlinks" command-line arg for sed will avoid this problem, for future reference.
Comment on attachment 8779491 [details]
Bug 1293739 - Part 1: Rename nsCSSProperty to nsCSSPropertyID.

https://reviewboard.mozilla.org/r/70476/#review69224

r=me on the changes in part 1.

Since this will need to be rebased just before landing, I'll go ahead and land this for you (using your commands to regenerate the patch based off of latest mozilla-inbound), and I'll add --follow-symlinks and drop that parenthesized chunk from your commit message.
Attachment #8779491 - Flags: review?(dholbert) → review+
Comment on attachment 8779492 [details]
Bug 1293739 - Part 2: Rename nsCSSPropertySet to nsCSSPropertyIDSet.

https://reviewboard.mozilla.org/r/70478/#review69226

r=me on part 2. Sorry for the delay on these.

(I'll fixup the extended-commit-message's command & symlink-related-note for this one, too, and rebase and land.)
Attachment #8779492 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #9)
> Since this will need to be rebased just before landing, I'll go ahead and
> land this for you (using your commands to regenerate the patch based off of
> latest mozilla-inbound), and I'll add --follow-symlinks and drop that
> parenthesized chunk from your commit message.

Actually, to my surprise, these patches still apply cleanly -- I thought for sure we would've had some change in the past week that would've broken these patches (particularly the first one)! But, I guess not.

Anyway: I did update the commit messages at least, and I'll land when the tree reopens, after verifying that this still builds locally.
Also, I posted a PSA about this rename here, to warn people that they may need to update their local patches:
  https://groups.google.com/d/msg/mozilla.dev.platform/to9Kh57hqFA/Ay-r3hXaAgAJ
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/702bca0deee2
Part 1: Rename nsCSSProperty to nsCSSPropertyID. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcdb85fc5517
Part 2: Rename nsCSSPropertySet to nsCSSPropertyIDSet. r=dholbert
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/ad3c62c42039
Merge bustage fix between bug 1293739 and bug 1277433 a=me CLOSED TREE
I had to back out ad3c62c42039, dcdb85fc5517, and 702bca0deee2 from central because of build bustage. Sorry.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backout by gszorc@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/8c3529c5f60a
Backed out changesets dcdb85fc5517, 702bca0deee2, 9cKX8gC1ATA for build bustage; a=bustage
OK, thanks. I'll redo the search-and-replace on central and reland there - hope that's OK.
Oh, I'm really sorry about that. I saw your message on dev-platform but it didn't occur to me that the patches I was landing for Daisuke would be affected. I suppose Autoland would have helpfully rejected my push except that I pushed during the window between these patches landing on m-i and being merged to m-c -- so there was some fairly unlucky timing there. Sorry!
This is all fallout from the fact we're running multiple integration repos. In the future there will only be try, autoland, and central for the tip of development. I'd encourage everyone to stop pushing to inbound and fx-team. There's an "autoland" button in MozReview that any L3 user can use. There's not many excuses to not use autoland when commits are reviewed on MozReview (as they were in this bug).

Regarding relanding it, you may want to wait until central, autoland, and inbound are all merged. You may want to coordinate with a sheriff when landing so they can merge it around.
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/84ad59b127a8
Part 1: Rename nsCSSProperty to nsCSSPropertyID. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/fe895421dfbe
Part 2: Rename nsCSSPropertySet to nsCSSPropertyIDSet. r=dholbert
Sorry, only just now saw comment 20.

This is already on inbound (and stuff is layering on top of it there), so I figured that any delays in re-landing would only compound the potential merge issues. Hence, re-landed on central (per comment 20).

@birtles: no worries! I should have planned better and coordinated with sheriffs to merge this around when originally landing.
Flags: needinfo?(dholbert)
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
blocking-b2g: --- → 2.6?
blocking-b2g: 2.6? → ---
(In reply to Gregory Szorc [:gps] from comment #20)
> I'd encourage everyone to stop pushing to inbound and fx-team.
> There's an "autoland" button in MozReview that any L3 user can use. There's
> not many excuses to not use autoland when commits are reviewed on MozReview
> (as they were in this bug).

(Replying to this -- in this case, my excuse for pushing to inbound was: I'd made local tweaks to jyc's patches during review (tweaking his commit message and re-running his script to regenerate the patch on an up-to-date tree in case of merge conflicts).  In order to use Autoland, I would've had to push my tweaked versions of jyc's patches to MozReview, without actually intending to get review but just in order to allow myself to autoland my fixed versions. I don't even know if that workflow [pushing someone else's patches] is supported in MozReview, but in any case it was more complex than just directly landing my tweaked versions of the patches.)
Yes, last I checked there are still a few cases where we can't use Autoland (e.g. when you get "r+ but please split this patch into two" -- last I checked MozReview would think one of the updated patches was not reviewed and would not let you push it without getting another review; another being trivial whitespace-only changes.)
You need to log in before you can comment on or make changes to this bug.