1061364 - Remove additional setting of mNeedsRefreshes = true to nsTransitionManager::WalkTransitionRule

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

In Bug 1025709, an additional setting of mNeedsRefreshes = true as added to nsTransitionManager::WalkTransitionRule:

  http://hg.mozilla.org/mozilla-central/rev/31695984cfe2#l5.65

now at:

  http://hg.mozilla.org/mozilla-central/file/738469449872/layout/style/nsTransitionManager.cpp#l655

According to bug 1048838 comment 19 this may cause unnecessary work when we have multiple style flushes.

We should see if there is a way to remove this. Also, while we're at it, as also mentioned in bug 1048838 comment 19, we should also investigate why WalkTransitionRule needs a null-check.

Comment 1

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

For what it's worth:
 * I've been running for at least a month (desktop and B2G phone) with the mNeedsRefreshes = true removed, and haven't noticed any problems.
 * there was a problem when I removed the null-check

Comment 2

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

I removed the removal from my tree because it was breaking all but the first transition in https://bug1110277.bugzilla.mozilla.org/attachment.cgi?id=8535092 , at least back when I was debugging that testcase.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Don't force transitions to refresh their style rule

This works for me, even for the test case in comment 2. Applies on top of the patch for bug 1117603.

Comment 4

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

Comment on attachment 8576461 [details] [diff] [review]
Don't force transitions to refresh their style rule

r=dbaron

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I'm waiting for bug 1117603 to land before I land this.

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Looks like this might trigger a test failure after all:

browser/components/customizableui/test/browser_989751_subviewbutton_class.js | Uncaught exception - Condition timed out: () => !PanelUI.multiView.hasAttribute("transitioning")

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

From: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d224f99253a5

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

After some changes in bug 1117603, looks like this might be ok now but I've retriggered the jobs that failed last time in case I just happened to get lucky:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c0d57b67523

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aaf374ba062f

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out because it blocked bug 1117603 from being backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5329cda711c8

Comment 11

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

Can this reland now that bug 1117603 stuck?

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #11)
> Can this reland now that bug 1117603 stuck?

Yes! (I was just running it through try over the weekend and making sure bug 1117603 really stuck this time!)

https://hg.mozilla.org/integration/mozilla-inbound/rev/7edc16a9d48f

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7edc16a9d48f