stylo: Move mRefCnt, Destroy() and HasSingleReference to GeckoStyleContext.

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: emilio, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

10 months ago
Such a mess.
Comment hidden (mozreview-request)
(Reporter)

Comment 2

10 months ago
Bug 1367904 is the gift that never stops giving...
Blocks: 1367904

Comment 3

10 months ago
mozreview-review
Comment on attachment 8889882 [details]
Bug 1384114: Move mRefCnt, Destroy() and HasSingleReference() to GeckoStyleContext.

https://reviewboard.mozilla.org/r/160948/#review166338

(In reply to Emilio Cobos Álvarez [:emilio] from comment #0)
> Such a mess.
>
> ...
>
> Bug 1367904 is the gift that never stops giving...

Please tone down the snark on this topic. The implication of your comments here (and in bug 1383307 comment 7, and in bug 1381764 comment 0) is that the work and review were shoddy, and that the followup work indicates that it landed half-baked.

I don't think that's a fair assessment. Bug 1367904 was an enormous project, and difficult to do incrementally. Keeping it out-of-tree was costing us a lot, both in terms of the rebasing cost, and in terms of the things that depended on it. Leaving some cleanups and visited-correctness issues as followups was a pretty reasonable thing to do, and I don't think there's much harm done in doing improvements like this after the fact. The two critical regressions from landing that bug (the null-deref, and the win32 alignment issue) wouldn't have been avoided by adding more refactoring to the landing, and I'm glad we discovered them as early as possible. 

You may disagree, or feel that you would have done it better yourself (which might be true!). But you can't do everything yourself, and so you need colleagues, and to keep those colleagues happy and productive you should keep your criticism of them contructive.

Anyway, this patch looks great. r=me. :-)

Comment 4

10 months ago
mozreview-review
Comment on attachment 8889882 [details]
Bug 1384114: Move mRefCnt, Destroy() and HasSingleReference() to GeckoStyleContext.

https://reviewboard.mozilla.org/r/160948/#review166356
Attachment #8889882 - Flags: review?(bobbyholley) → review+
Yeah, this was one of the followups I'd noted down (but kept low priority since there were nightly fixes to do).

The rebase was taking up more than half the time spent on this bug (since rebased introduced new test failures) near the end so deferring to followups was really the only thing we could do.
(Reporter)

Comment 6

10 months ago
(In reply to Manish Goregaokar [:manishearth] from comment #5)
> Yeah, this was one of the followups I'd noted down (but kept low priority
> since there were nightly fixes to do).
> 
> The rebase was taking up more than half the time spent on this bug (since
> rebased introduced new test failures) near the end so deferring to followups
> was really the only thing we could do.

Yeah, that's fair enough.

I still think that that bug should have landed which much more review / polish than what it did. I'd rather close the tree for a day and polish that patch that land it half-backed. But sorry for that, I should stop complaining about that bug :)

Comment 7

10 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ec0d5f928c2b
Move mRefCnt, Destroy() and HasSingleReference() to GeckoStyleContext. r=bholley

Comment 8

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ec0d5f928c2b
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.