1426649 - Remove nsCSSFrameConstructor updates

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

They're pretty useless now.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1426649: Stop tracking DOM changes from painting.

I'm pretty sure this is not a problem now, since we don't mutate the DOM from
layout anymore.

Just to confirm, I did a try run with a RELEASE_ASSERT(!CheckDOMModified()), and
it passed.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1426649: Remove nsCSSFrameConstructor updates.

They used to do quote updates and such but they where moved long ago, and do
nothing now.

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

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8938458 [details]
Bug 1426649: Stop tracking DOM changes from painting.

https://reviewboard.mozilla.org/r/209162/#review214976

I.. don't know anything about this code. It looks like it was added by roc and reviewed by dbaron, so I'm going to redirect the review to dbaron.

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 8938459 [details]
Bug 1426649: Remove nsCSSFrameConstructor updates.

https://reviewboard.mozilla.org/r/209164/#review214992

These code changes looks fine to me, assuming that part 1 is OK.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938458 [details]
Bug 1426649: Stop tracking DOM changes from painting.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209162/diff/1-2/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938459 [details]
Bug 1426649: Remove nsCSSFrameConstructor updates.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209164/diff/1-2/

Comment 7

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8938458 [details]
Bug 1426649: Stop tracking DOM changes from painting.

This is because Flash could modify the dom during painting, see here
https://bugzilla.mozilla.org/show_bug.cgi?id=594774#c6

So this should still be relevant until we completely disable flash, no?

Comment 8

[Mats Palmgren (inactive)]

The crashtest that bug 594774 added (modules/plugin/test/crashtests/522512-1.html)
was removed with the comment that the crash can't happen anymore:
https://hg.mozilla.org/mozilla-central/rev/671bd739dbff
(presumably as a result of bug 596451)

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It seems like it would be a little bit safer to *ship* the code as a MOZ_RELEASE_ASSERT for a bit, and check crash-stats?

(I'm also not sure who would know what our current levels of flash usage are like on nightly vs. release... or whether out-of-process plugins would have fixed this fully.)

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8938458 [details]
Bug 1426649: Stop tracking DOM changes from painting.

https://reviewboard.mozilla.org/r/209162/#review219484

Then again, given comment 8, I guess I'm fine with just doing this.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Yeah, per comment 8 I think this should be fine. Thanks for looking at the history of that test mats!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/302d28afd44f
Stop tracking DOM changes from painting. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/682a00d90865
Remove nsCSSFrameConstructor updates. r=mats

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/302d28afd44f
https://hg.mozilla.org/mozilla-central/rev/682a00d90865