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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

Such a mess.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1384114: Move mRefCnt, Destroy() and HasSingleReference() to GeckoStyleContext.

Review commit: https://reviewboard.mozilla.org/r/160948/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160948/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Bug 1367904 is the gift that never stops giving...

Comment 3

[Bobby Holley (:bholley)]

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

[Bobby Holley (:bholley)]

Comment on attachment 8889882 [details]
Bug 1384114: Move mRefCnt, Destroy() and HasSingleReference() to GeckoStyleContext.

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

Comment 5

[Manish Goregaokar [:manishearth]]

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.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec0d5f928c2b
Move mRefCnt, Destroy() and HasSingleReference() to GeckoStyleContext. r=bholley

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec0d5f928c2b