Jonathan Chan [:jyc] Assignee
	Description • 8 years ago

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

	Daniel Holbert [:dholbert]
	Comment 3 • 8 years ago

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

	Jonathan Chan [:jyc] Assignee
	Comment 6 • 8 years ago

Hi! As discussed on IRC, I have added the required commands to the patch. Commenting to clear the needinfo.

	Jonathan Chan [:jyc] Assignee
	Comment 7 • 8 years ago

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c379b2be0e67

	Daniel Holbert [:dholbert]
	Comment 8 • 8 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 9 • 8 years ago
mozreview-review

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.

	Daniel Holbert [:dholbert]
	Comment 10 • 8 years ago
mozreview-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.)

	Daniel Holbert [:dholbert]
	Comment 11 • 8 years ago

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

	Daniel Holbert [:dholbert]
	Comment 12 • 8 years ago

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

	Pulsebot
	Comment 13 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 14 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/702bca0deee2
https://hg.mozilla.org/mozilla-central/rev/dcdb85fc5517

	Pulsebot
	Comment 15 • 8 years ago

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

	Gregory Szorc [:gps]
	Comment 16 • 8 years ago

I had to back out ad3c62c42039, dcdb85fc5517, and 702bca0deee2 from central because of build bustage. Sorry.

	Pulsebot
	Comment 17 • 8 years ago

Backout by gszorc@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/8c3529c5f60a
Backed out changesets dcdb85fc5517, 702bca0deee2, 9cKX8gC1ATA for build bustage; a=bustage

	Daniel Holbert [:dholbert]
	Comment 18 • 8 years ago

OK, thanks. I'll redo the search-and-replace on central and reland there - hope that's OK.

	Brian Birtles (:birtles)
	Comment 19 • 8 years ago

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!

	Gregory Szorc [:gps]
	Comment 20 • 8 years ago

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.

	Pulsebot
	Comment 21 • 8 years ago

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

	Daniel Holbert [:dholbert]
	Comment 22 • 8 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 23 • 8 years ago

> Hence, re-landed on central (per comment 20).

er, comment 21

	Daniel Holbert [:dholbert]
	Comment 24 • 8 years ago

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

	Brian Birtles (:birtles)
	Comment 25 • 8 years ago

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