Closed
Bug 1209405
Opened 9 years ago
Closed 7 years ago
High CPU load on Kickstarter, even without JS, CSS and images
Categories
(Core :: SVG, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: a3357618, Assigned: daisuke)
References
Details
(Keywords: perf, power)
Attachments
(7 files)
62.31 KB,
application/x-zip
|
Details | |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
hiro
:
review+
|
Details |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0 Build ID: 20150917150946 Steps to reproduce: Go to any Kickstarter campaign page, such as https://www.kickstarter.com/projects/899401530/fed-up-food-education-for-every-classroom/description Check CPU load, stuck at a steady 50%. I have a dual core CPU, so it could well be a steady 25% for a quad core. Worth noting that only my second CPU core is going nuts, the first one stays at ~5% I tried with and without JavaScript (blocked with NoScript 2.6.9.37), with and without CSS (disabled with ALT > Display > Page style > No style), with and without images (blocked with Adblock Plus 2.6.11), and the problem is there in all cases. I tried with a fresh default profile and also in safe mode. I noticed it on Firefox 41, but I don't visit Kickstarter that often. It could be a bug introduced sometime between, say, Firefox 35 and 41, but that's just a guess. That or the website was updated and now triggers a bug that has been present in Firefox even prior to 35. Actual results: ~95% CPU load on the second CPU core out of two, ~5% on the first one. Total 50% load. Expected results: Barely any CPU power used.
CPU is Intel Core2Duo P8400 Windows 7 64-bit clean install, almost up to date
Comment 2•9 years ago
|
||
The high CPU is caused a lot of invisible button SVG animation as follows <button class="btn btn--green btn--block"> <span class="btn-text"> Continue </span> <div class="loader-dots"> <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 56 16" version="1.1"> <g> <circle r="8" cy="8" cx="8" class="loader-dot-1"> <animate values="1; .3; .3; .3;" repeatCount="indefinite" dur="1s" begin="0s" attributeType="XML" attributeName="opacity"/> </circle> <circle r="8" cy="8" cx="28" class="loader-dot-2"> <animate values="1; .3; .3; .3;" repeatCount="indefinite" dur="1s" begin="0.33s" attributeType="XML" attributeName="opacity"/> </circle> <circle r="8" cy="8" cx="48" class="loader-dot-3"> <animate values="1; .3; .3; .3;" repeatCount="indefinite" dur="1s" begin="0.66s" attributeType="XML" attributeName="opacity"/> </circle> </g> </svg> </div> </button>
Status: UNCONFIRMED → NEW
Component: General → Layout: View Rendering
Ever confirmed: true
Keywords: power
Product: Firefox → Core
Comment 3•9 years ago
|
||
Comment 5•8 years ago
|
||
Profiler: https://cleopatra.io/#report=6d7c0a5371b55782edf2710d54db8bf08bf4c12d
Component: Layout: View Rendering → SVG
Keywords: perf
Comment 6•8 years ago
|
||
Seen on https://www.kickstarter.com/projects/revivalprod/overload-the-ultimate-six-degree-of-freedom-shoote There’s also an invisible audio volume animation, hidden by default (display:none) causing this too. <svg version="1.1" viewBox="0 0 18 17.2" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns="http://www.w3.org/2000/svg"><g> <polygon class="audio-indicator-bar" points="0,0 2,0 2,11.5 2,17.2 0,17.2"> <animate attributeName="points" begin="0s" calcMode="spline" dur="1.2s" keySplines="0.18 0.01 0.37 0.99;0.18 0.01 0.37 0.99;0.18 0.01 0.37 0.99" keyTimes="0; 0.3; 0.9; 1" repeatCount="indefinite" values="0,0 2,0 2,11.5 2,17.2 0,17.2;0,2.6 2,2.6 2,8.2 2,17.2 0,17.2;0,12.1 2,12.1 2,14 2,17.2 0,17.2;0,0 2,0 2,11.5 2,17.2 0,17.2"></animate> </polygon> … snip … </g> </svg> Platform should be tagged as all.
Updated•8 years ago
|
OS: Windows 7 → All
Comment 7•8 years ago
|
||
Related bug 962594 for *CSS* animations in display:none elements, https://bugzilla.mozilla.org/show_bug.cgi?id=962594#c57
Comment 8•8 years ago
|
||
Brian, can we throttle SVG animations on display:none elements?
Flags: needinfo?(bbirtles)
Comment 9•8 years ago
|
||
(In reply to Cameron McCormack (:heycam) (away Feb 27 – Mar 13) from comment #8) > Brian, can we throttle SVG animations on display:none elements? Not easily, I think. For CSS, we completely stop animations in display:none subtrees. That's ok because spec says animations in display:none subtrees don't run. But SVG doesn't say that (and, in most cases, SVG doesn't treat display:none specially--gradients etc. defined in display:none trees are still useable, etc.). So throttling, like you say, is probably a better idea. We could try to use the same setup as we have for OMTA where we mark style as needing a flush but don't post a restyle? I'm not sure how much work that would be but is probably possible, at least for SVG animations that target CSS properties. I'll see if I can find someone to look at this in Q2.
Flags: needinfo?(bbirtles)
Comment 10•8 years ago
|
||
I think this is probably easier to fix on the animation side rather than the timing side. That is, rather than trying to throttle the refresh driver, we should simply try to update the animation values lazily when we are in a display:none subtree. That's what we do for CSS animations etc. I think the easiest way might be to exploit the mForceCompositing member in nsSMILCompositor. I think what we want to do is avoid setting mForceCompositing to true if nsSMILAnimationFunction::HasChanged() returns true (see nsSMILCompositor::GetFirstFuncToAffectSandwich) when the target element is in a display:none subtree. However, we *might* need to ensure that set call nsDocument::SetNeedStyleFlush in that case, I'm not sure. Perhaps we need to extend mForceCompositing to a three-state enum like we do for CSS: no change, throttled update, regular update. We'll need to tweak at least ComposeAttribute and GetFirstFuncToAffectSandwich but I think that might work. My earlier notes (which can be ignored): At first I thought we could deal with this at the nsISMILType layer. The difficulty here is that there are two ways we are performing animation. In comment 2 we are animating CSS properties. For that we would probably need to either: * tweak nsSMILMappedAttribute::FlushChangesToTargetAttr to *not* call nsIPresShell::RestyleForAnimation when we're in a display:none subtree but call nsDocument::SetNeedStyleFlush instead. * tweak the call sites of FlushChangesToTargetAttr to not end up calling it in this case. However, in comment 6 it seems like we hit this same issue for animations of SVG attributes and profiling this seems to confirm the problem. We have a mechanism for animated attributes to mark themselves as needing an update. AutoChangePointListNotifier calls mPointList->Element()->AnimationNeedsResample() in its destructor. This calls doc->GetAnimationController()->SetResampleNeeded() which does *two* things: 1. Calls FlagDocumentNeedsFlush() 2. Sets mResampleNeeded = true; At first I thought we could just do (1) for the display:none subtree case (i.e. call SetNeedStyleFlush but leave mResampleNeeded alone so that if we get a call to FlushResampleRequests we can ignore it) but I think that's not quite right. Doing that would probably produce the opposite result--i.e. it will cause us to *not* do the update in the few cases where we really need to, but still do it on regular ticks. Hence why I started looking into mForceCompositing.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → daisuke
Comment 11•8 years ago
|
||
Hi Cameron, I've been looking into this with Daisuke and I'm think we might change how animVal works when we're in display:none. For CSS properties we can mark the style as needing a flush and only update it when getComputedStyle etc. is called. However, for our animVal interface, we don't have any code there that, e.g. checks a dirty flag and requests a sample if needed. So if we want to stop updating animated values on elements in display:none subtrees I think we can either: a) Add machinery to each of the animVal interfaces that checks an 'needs resample' flag and calculates the animated value on-demand when the flag is set. or b) Simply not update animVal values when in display:none subtrees. (We might need to add a few exceptions such as when the 'display' property itself is animated.) I'm leaning towards (b) because in SVG2 animVal is going to become an alias for baseVal anyway. I'd also rather not add a whole lot of new code just for supporting SMIL interfaces in display:none if SMIL is on the way out. Does that seem ok to you?
Flags: needinfo?(cam)
Comment 12•8 years ago
|
||
Also, as per comment 6, we are seeing this bug for animations of the 'points' attribute, so we can't just say, for example, "we only throttle animation of CSS properties".
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de477dade0bd
Comment 14•8 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #11) > Does that seem ok to you? In general I am sympathetic to that plan, but will this be a problem for animating resource elements in a <defs>? I think that's not uncommon, so if it breaks SMIL animations targetting say paint servers inside a <defs>, then we will need to go with (a) to keep that working.
Flags: needinfo?(cam)
Assignee | ||
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3ce5fca4787
Assignee | ||
Comment 16•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fb7bbb59ac5
Assignee | ||
Comment 17•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d44799a4a5f8
Assignee | ||
Comment 18•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84b012680e7b
Assignee | ||
Comment 19•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64e7e847e8c0
Assignee | ||
Comment 20•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=04c765aa292c
Assignee | ||
Comment 21•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=792fdb434f06
Assignee | ||
Comment 22•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e145aee091d
Comment 23•8 years ago
|
||
I had a look at the test failures and observed that the reason we successfully update forwards-filling animations that use 'em' units[1] is that when we parse values that depend on context, we return a "aPreventCachingOfSandwich" value. Inside nsSMILAnimationFunction that causes us to set mValueNeedsReparsingEverySample which in turn means that nsSMILAnimationFunction::HasChanged() always returns true. In effect that means that for forwards-filling animations that us 'em' units, we recompose the animation *every* tick. That's not great, but that's the existing behavior. For the test that was previously failing, we need to make sure to flush pending notifications on each tick when we have one of these animations that needs to be recomposed on every sample--at least so long as we're not in a display:none subtree. I think we can do that fairly easily because: 1. Compositors are created every tick 2. When we set them up in nsSMILAnimationController::AddAnimationToCompositorTable we call nsSMILAnimationFunction::HasChanged() on each animation function associated with the compositor. Now, HasChanged() will return true for any function that needs to be composed on every sample. 3. If HasChanged() returns true for any animation function, we call nsSMILCompositor::ToggleForceCompositing() which will set mForceCompositing to true. So, what I *think* we could do, is when deciding if we should flush pending notifications or not, check if *either* isResampleNeeded is true, or if nsSMILCompositor::mForceCompositing is true for any of the compositors in the table. nsSMILCompositor::mForceCompositing is not public so we'll need to add an accessor for that, maybe something like nsSMILCompositor::WillComposite() which returns mForceCompositing && !mAnimationFunctionsIsEmpty(). I think that would work, but it's a little confusing since "will composite" will return false sometimes even when we will actually compositor. To be more accurate, we should name it something like "DefinitelyNeedsToComposite". We could probably approach this from one of a few different ways: 1. Do the above and deal with the fact that we have a slightly confusing setup and confusing names. 2. Actually expose the mValueNeedsReparsingEverySample value from nsSMILAnimationFunction (e.g. add a ValueNeedsReparsingEverySample() method); Query that method in nsSMILAnimationController::AddAnimationToCompositorTable and pass it back to nsSMILAnimationController::DoSample 3. Rework nsSMILCompositor::GetFirstFuncToAffectSandwich such that we can expose an nsSMILCompositor::WillComposite method that actually accurately returns whether or not we're going to compose anything; Make that method also cache the first function to affect the sandwich, sort the functions etc. (We might need some extra state to be able to assert that we've already done the initial setup work.) Use WillComposite() to control if we flush pending notifications or not and drop GetFirstFuncToAffectSandwich. I think 3 is a bit of work and carries the risk of introducing new bugs so I'm probably leaning towards 2 at the moment, but 1 might be ok too. [1] e.g. this test case http://codepen.io/anon/pen/VaOGyL
Assignee | ||
Comment 24•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d8c0f388f40
Assignee | ||
Comment 25•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b3adc532685
Assignee | ||
Comment 26•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53794/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53794/
Attachment #8754170 -
Flags: review?(bbirtles)
Attachment #8754171 -
Flags: review?(bbirtles)
Attachment #8754172 -
Flags: review?(bbirtles)
Attachment #8754173 -
Flags: review?(bbirtles)
Attachment #8754174 -
Flags: review?(bbirtles)
Attachment #8754175 -
Flags: review?(hiikezoe)
Assignee | ||
Comment 27•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53796/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53796/
Assignee | ||
Comment 28•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53798/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53798/
Assignee | ||
Comment 29•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53800/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53800/
Assignee | ||
Comment 30•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53802/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53802/
Assignee | ||
Comment 31•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/53804/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53804/
Comment 32•8 years ago
|
||
Comment on attachment 8754175 [details] MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL animation. r=hiro https://reviewboard.mozilla.org/r/53804/#review50496 ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:13 (Diff revision 1) > + <svg version="1.1" xmlns="http://www.w3.org/2000/svg" > + xmlns:xlink="http://www.w3.org/1999/xlink"> > + <rect width="100%" height="100%" fill="lime"> > + <animate attributeType="XML" > + attributeName="fill" > + values="red;lime" > + dur="1s" > + repeatCount="indefinite"/> > + </rect> > + </svg> Can we generate this SVG element dynamically? Once this test file has other test cases, this static element will be an obstacle. ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:28 (Diff revision 1) > +</div> > + > +<script> > +"use strict"; > + > +function waitForAnimationFrames(frameCount) { I wonder why we can't use the same function in testcommons.js here. ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:41 (Diff revision 1) > + } > + window.requestAnimationFrame(handleFrame); > + }); > +} > + > +function observeStyling(frameCount, onFrame) { I believe we can now move this function into testcommon.js. ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:61 (Diff revision 1) > + resolve(stylingMarkers); > + }); > + }); > +} > + > +function ensureElementRemoval(aElement) { This function can be moved into testcommon.js too? ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:70 (Diff revision 1) > +// We need to wait for all paints before running tests to avoid contaminations > +// from styling of this document itself. > +waitForAllPaints(function() { I don't think we need to wait for the paints in case of SVG animations. Our browser has SVG animations on UI running on mochitest? Oops! Now I noticed that ensureElementRemoval() is not used at all. It should be removed. And I think test test can be run with testharness.js instead of SimpleTest.js. ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:76 (Diff revision 1) > +// from styling of this document itself. > +waitForAllPaints(function() { > + add_task(function* no_restyling_if_smil_is_display_none() { > + var displayMarkers = yield observeStyling(5); > + is(displayMarkers.length, 5, > + "Bug 1209405: should restyle in every frame"); I don't think Bug XXX is necessary here. ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:78 (Diff revision 1) > + document.getElementById("target").style.display = "none"; > + getComputedStyle(document.getElementById("target")).display; > + var displayNoneMarkers = yield observeStyling(5); > + is(displayNoneMarkers.length, 0, > + "Bug 1209405: should never restyle if display:none"); We don't need to check after removal of display:none style?
Attachment #8754175 -
Flags: review?(hiikezoe)
Comment 33•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #32) > Comment on attachment 8754175 [details] > MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL > animation. r?hiro > > https://reviewboard.mozilla.org/r/53804/#review50496 > > ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:13 > (Diff revision 1) > > + <svg version="1.1" xmlns="http://www.w3.org/2000/svg" > > + xmlns:xlink="http://www.w3.org/1999/xlink"> > > + <rect width="100%" height="100%" fill="lime"> > > + <animate attributeType="XML" > > + attributeName="fill" > > + values="red;lime" > > + dur="1s" > > + repeatCount="indefinite"/> > > + </rect> > > + </svg> > > Can we generate this SVG element dynamically? Once this test file has other > test cases, this static element will be an obstacle. > > ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:28 > (Diff revision 1) > > +</div> > > + > > +<script> > > +"use strict"; > > + > > +function waitForAnimationFrames(frameCount) { > > I wonder why we can't use the same function in testcommons.js here. Ah, I thought the test in dom/animation/test/.
Assignee | ||
Comment 34•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #32) > Comment on attachment 8754175 [details] > MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL > animation. r?hiro > > https://reviewboard.mozilla.org/r/53804/#review50496 > > ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:13 > (Diff revision 1) > > + <svg version="1.1" xmlns="http://www.w3.org/2000/svg" > > + xmlns:xlink="http://www.w3.org/1999/xlink"> > > + <rect width="100%" height="100%" fill="lime"> > > + <animate attributeType="XML" > > + attributeName="fill" > > + values="red;lime" > > + dur="1s" > > + repeatCount="indefinite"/> > > + </rect> > > + </svg> > > Can we generate this SVG element dynamically? Once this test file has other > test cases, this static element will be an obstacle. Thank you very much, Hiro! Ok, I'll do that. Ah, also we don't need xmlns:xlink="http://www.w3.org/1999/xlink" namespace. > ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:28 > (Diff revision 1) > > +</div> > > + > > +<script> > > +"use strict"; > > + > > +function waitForAnimationFrames(frameCount) { > > I wonder why we can't use the same function in testcommons.js here. As you said, I brought the code since this test is under layout/style/test/chrome/. Is it better to make such a testcommon.js? > ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:70 > (Diff revision 1) > > +// We need to wait for all paints before running tests to avoid contaminations > > +// from styling of this document itself. > > +waitForAllPaints(function() { > > I don't think we need to wait for the paints in case of SVG animations. Our > browser has SVG animations on UI running on mochitest? Yes, running on mochitest. > Oops! Now I noticed that ensureElementRemoval() is not used at all. It > should be removed. I'll use this function after revising to generate SVG element dynamically. > And I think test test can be run with testharness.js instead of > SimpleTest.js. OK, change to use that. > ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:76 > (Diff revision 1) > > +// from styling of this document itself. > > +waitForAllPaints(function() { > > + add_task(function* no_restyling_if_smil_is_display_none() { > > + var displayMarkers = yield observeStyling(5); > > + is(displayMarkers.length, 5, > > + "Bug 1209405: should restyle in every frame"); > > I don't think Bug XXX is necessary here. OK. > ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:78 > (Diff revision 1) > > + document.getElementById("target").style.display = "none"; > > + getComputedStyle(document.getElementById("target")).display; > > + var displayNoneMarkers = yield observeStyling(5); > > + is(displayNoneMarkers.length, 0, > > + "Bug 1209405: should never restyle if display:none"); > > We don't need to check after removal of display:none style? Thanks! I'll append the test.
Comment 35•8 years ago
|
||
Comment on attachment 8754170 [details] MozReview Request: Bug 1209405 - Part 1: Modify tests so as not use display:none. r=birtles https://reviewboard.mozilla.org/r/53794/#review50508 r=me with hideSubtree removed from smilTestUtils.js ::: dom/smil/test/test_smilCSSFromBy.xhtml (Diff revision 1) > - // Set "display:none" on everything and run the tests again > - SMILUtil.hideSubtree(SMILUtil.getSVGRoot(), false, false); > - testBundleList(gFromByBundles, new SMILTimingData(1.0, 1.0)); > - It looks like this patch removes all references to SMILUtils.hideSubtree. If that's the case, please remove it from smilTestUtils.js.
Attachment #8754170 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 36•8 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #35) > Comment on attachment 8754170 [details] > MozReview Request: Bug 1209405 - Part 1: Modify tests so as not use > display:none. r?birtles > > https://reviewboard.mozilla.org/r/53794/#review50508 > > r=me with hideSubtree removed from smilTestUtils.js > > ::: dom/smil/test/test_smilCSSFromBy.xhtml > (Diff revision 1) > > - // Set "display:none" on everything and run the tests again > > - SMILUtil.hideSubtree(SMILUtil.getSVGRoot(), false, false); > > - testBundleList(gFromByBundles, new SMILTimingData(1.0, 1.0)); > > - > > It looks like this patch removes all references to SMILUtils.hideSubtree. If > that's the case, please remove it from smilTestUtils.js. Thank you very much, Brian! Ok!
Comment 37•8 years ago
|
||
(In reply to Daisuke Akatsuka (:daisuke) from comment #34) > > ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:28 > > (Diff revision 1) > > > +</div> > > > + > > > +<script> > > > +"use strict"; > > > + > > > +function waitForAnimationFrames(frameCount) { > > > > I wonder why we can't use the same function in testcommons.js here. > > As you said, I brought the code since this test is under > layout/style/test/chrome/. > Is it better to make such a testcommon.js? We should avoid spreading this out somehow. Any ideas? And now I think this test can be run without chrome privileges. > > ::: layout/style/test/chrome/test_restyles_in_smil_animation.html:70 > > (Diff revision 1) > > > +// We need to wait for all paints before running tests to avoid contaminations > > > +// from styling of this document itself. > > > +waitForAllPaints(function() { > > > > I don't think we need to wait for the paints in case of SVG animations. Our > > browser has SVG animations on UI running on mochitest? > > Yes, running on mochitest. > > > Oops! Now I noticed that ensureElementRemoval() is not used at all. It > > should be removed. > > I'll use this function after revising to generate SVG element dynamically. > > > And I think test test can be run with testharness.js instead of > > SimpleTest.js. > > OK, change to use that. Sorry, if the test is in layout, we don't need to use testharness.js there.
Updated•8 years ago
|
Attachment #8754171 -
Flags: review?(bbirtles) → review+
Comment 38•8 years ago
|
||
Comment on attachment 8754171 [details] MozReview Request: Bug 1209405 - Part 2: High CPU load on Kickstarter, even without JS, CSS and images. r=birtles https://reviewboard.mozilla.org/r/53796/#review50518 r=me with changes addressed ::: dom/smil/nsSMILCompositor.cpp:148 (Diff revision 1) > + bool canThrottle = mKey.mAttributeName != nsGkAtoms::display && > + styleContext && styleContext->IsInDisplayNoneSubtree(); I think this would be easier to read as: bool canThrottle = mKey.mAttributeName != nsGkAtoms::display && styleContext && styleContext->IsInDisplayNoneSubtree(); (I wonder if we could actually make it return true if styleContext is null, but perhaps it's safer not to for now.)
Comment 39•8 years ago
|
||
Comment on attachment 8754172 [details] MozReview Request: Bug 1209405 - Part 3: Save updating style. r=birtles https://reviewboard.mozilla.org/r/53798/#review50510 Looks good but I'd like to have another look with the changes to AddAnimationToCompositorTable made. ::: dom/smil/nsSMILAnimationController.h:145 (Diff revision 1) > - static void AddAnimationToCompositorTable( > + // Returns true if the value that AnimationFunction handles needs to reparse every sample. > + static bool AddAnimationToCompositorTable( Instead of returning true, I think it would be more clear to pass a bool reference here. ::: dom/smil/nsSMILAnimationFunction.h:252 (Diff revision 1) > } > > + /** > + * Returns true if needs to reparse every sample. > + */ > + bool ValueNeedsReparsingEverySample() { We should make this method const. ::: dom/smil/nsSMILCompositor.cpp:68 (Diff revision 1) > - return; > + // needs to update for such as following structure > + // layout/reftests/svg/smil/smil-transitions-interaction-3a.svg > + // layout/reftests/svg/smil/smil-transitions-interaction-3b.svg > + return true; I think this comment should explain why we need to update style, not just point to test cases that fail.
Attachment #8754172 -
Flags: review?(bbirtles)
Comment 40•8 years ago
|
||
Comment on attachment 8754173 [details] MozReview Request: Bug 1209405 - Part 4: Add defs element tests. r=birtles https://reviewboard.mozilla.org/r/53800/#review50520 r=me with comments addressed Please make sure to retrigger these tests several times on try before landing. We should make sure we're not introducing a new intermittent failure. ::: layout/reftests/svg/smil/anim-defs-fill.svg:5 (Diff revision 1) > +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" > + xmlns:xlink="http://www.w3.org/1999/xlink" > + class="reftest-wait"> > + > + <title>Test animation using 'defs' element with 'fill' attribute</title> Nit: Test animation element in 'defs' element with 'fill' property ::: layout/reftests/svg/smil/anim-defs-fill.svg:19 (Diff revision 1) > + dur="100s"/> > + </defs> > + > + <script> > + window.addEventListener("MozReftestInvalidate", function() { > + setTimeAndWaitToSnapshot(49.999, 0.001); Let's make the wait time greater than 1 standard frame (=16ms), e.g. setTimeAndWaitToSnapshot(49.95, 0.05). That way, if setting the current time triggers a change in style that is enacted on the next tick, we won't accidentally pass. Likewise for the other tests in this file. ::: layout/reftests/svg/smil/anim-defs-gradient-attribute.svg:5 (Diff revision 1) > +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" > + xmlns:xlink="http://www.w3.org/1999/xlink" > + class="reftest-wait"> > + > + <title>Test animation using 'defs' element with attribute of gradient</title> Nit: Test animation of gradient attribute in 'defs' element ::: layout/reftests/svg/smil/anim-defs-gradient-property.svg:5 (Diff revision 1) > +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" > + xmlns:xlink="http://www.w3.org/1999/xlink" > + class="reftest-wait"> > + > + <title>Test animation using 'defs' element with property of gradient</title> Nit: Test animation of gradient property in 'defs' element ::: layout/reftests/svg/smil/anim-defs-width.svg:5 (Diff revision 1) > +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" > + xmlns:xlink="http://www.w3.org/1999/xlink" > + class="reftest-wait"> > + > + <title>Test animation using 'defs' element with 'width' attribute</title> Nit: Test animation element in 'defs' element with 'width' attribute ::: layout/reftests/svg/smil/anim-defs-width.svg:8 (Diff revision 1) > + class="reftest-wait"> > + > + <title>Test animation using 'defs' element with 'width' attribute</title> > + <script xlink:href="smil-util.js" type="text/javascript"/> > + > + <rect id="target" fill="lime" width="100%" height="100%"/> Can we make this so that if the animation does not run at all the test fails? e.g. see the initial width to 0%? ::: layout/reftests/svg/smil/smil-util.js:13 (Diff revision 1) > svg.setCurrentTime(timeInSeconds); > svg.removeAttribute("class"); > } > + > +// Seeks to the given time and then removes the SVG document's class to trigger > +// a reftest snapshot after wait minWaitTimeInSeconds Nit: '... after waiting at least minWaitTimeInSeconds.' ::: layout/reftests/svg/smil/smil-util.js:19 (Diff revision 1) > + var takeSnapshot = function(currentTime) { > + if (currentTime > timeToTakeSnapshot) { > + svg.removeAttribute("class"); > + } else { > + window.requestAnimationFrame(takeSnapshot); > + } > + }; > + window.requestAnimationFrame(takeSnapshot); We can write this more clearly as something like: requestAnimationFrame(function takeSnapshot(currentTime) { if (currentTime > timeToTakeSnapshot) { svg.removeAttribute("class"); } else { requestAnimationFrame(takeSnapshot); } });
Attachment #8754173 -
Flags: review?(bbirtles) → review+
Comment 41•8 years ago
|
||
Comment on attachment 8754174 [details] MozReview Request: Bug 1209405 - Part 5: Add animation test that changes 'display' attribute. r=birtles https://reviewboard.mozilla.org/r/53802/#review50526 Looks good, but I should check the added test too. ::: layout/reftests/svg/smil/anim-display.svg:8 (Diff revision 1) > + <rect display="none" width="100%" height="100%" fill="lime"> > + <animate attributeName="display" > + values="none;inline" > + calcMode="discrete" > + dur="100s"/> > + </rect> Can we add a variation on this that does something like: <g display="none" id="g"> <rect ....> <animate xlink:href="#g" attributeName="display" ... /> </rect> </g> ::: layout/reftests/svg/smil/anim-display.svg:17 (Diff revision 1) > + dur="100s"/> > + </rect> > + > + <script> > + window.addEventListener("MozReftestInvalidate", function() { > + setTimeAndWaitToSnapshot(49.999, 0.001); As with the previous patch, let's wait a bit longer here, e.g. 49.9, 0.1
Attachment #8754174 -
Flags: review?(bbirtles)
Assignee | ||
Comment 42•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdc150f813f1
Assignee | ||
Comment 43•8 years ago
|
||
Comment on attachment 8754170 [details] MozReview Request: Bug 1209405 - Part 1: Modify tests so as not use display:none. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53794/diff/1-2/
Attachment #8754170 -
Attachment description: MozReview Request: Bug 1209405 - Part 1: Modify tests so as not use display:none. r?birtles → MozReview Request: Bug 1209405 - Part 1: Modify tests so as not use display:none. r=birtles
Attachment #8754171 -
Attachment description: MozReview Request: Bug 1209405 - Part 2: High CPU load on Kickstarter, even without JS, CSS and images. r?birtles → MozReview Request: Bug 1209405 - Part 2: High CPU load on Kickstarter, even without JS, CSS and images. r=birtles
Attachment #8754173 -
Attachment description: MozReview Request: Bug 1209405 - Part 4: Add defs element tests. r?birtles → MozReview Request: Bug 1209405 - Part 4: Add defs element tests. r=birtles
Attachment #8754172 -
Flags: review?(bbirtles)
Attachment #8754174 -
Flags: review?(bbirtles)
Attachment #8754175 -
Flags: review?(hiikezoe)
Assignee | ||
Comment 44•8 years ago
|
||
Comment on attachment 8754171 [details] MozReview Request: Bug 1209405 - Part 2: High CPU load on Kickstarter, even without JS, CSS and images. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53796/diff/1-2/
Assignee | ||
Comment 45•8 years ago
|
||
Comment on attachment 8754172 [details] MozReview Request: Bug 1209405 - Part 3: Save updating style. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53798/diff/1-2/
Assignee | ||
Comment 46•8 years ago
|
||
Comment on attachment 8754173 [details] MozReview Request: Bug 1209405 - Part 4: Add defs element tests. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53800/diff/1-2/
Assignee | ||
Comment 47•8 years ago
|
||
Comment on attachment 8754174 [details] MozReview Request: Bug 1209405 - Part 5: Add animation test that changes 'display' attribute. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53802/diff/1-2/
Assignee | ||
Comment 48•8 years ago
|
||
Comment on attachment 8754175 [details] MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL animation. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53804/diff/1-2/
Updated•8 years ago
|
Attachment #8754175 -
Flags: review?(hiikezoe) → review+
Comment 49•8 years ago
|
||
Comment on attachment 8754175 [details] MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL animation. r=hiro https://reviewboard.mozilla.org/r/53804/#review50790 ::: layout/style/test/test_restyles_in_smil_animation.html:28 (Diff revision 2) > + } > + window.requestAnimationFrame(handleFrame); > + }); > +} > + > +function observeStyling(frameCount, onFrame) { nit: onFrame is not used at all. It should be removed. ::: layout/style/test/test_restyles_in_smil_animation.html:39 (Diff revision 2) > + > + docShell.recordProfileTimelineMarkers = true; > + docShell.popProfileTimelineMarkers(); > + > + return new Promise(function(resolve) { > + return waitForAnimationFrames(frameCount, onFrame).then(function() { nit: waitForAnimationFrames() does not have the second argument. ::: layout/style/test/test_restyles_in_smil_animation.html:83 (Diff revision 2) > + return div; > +} > + > +SimpleTest.waitForExplicitFinish(); > + > +add_task(function* no_restyling_if_smil_is_display_none() { I think this test is for smil which is in display:none subtree. So, smil_is_in_display_none_subtree? ::: layout/style/test/test_restyles_in_smil_animation.html:92 (Diff revision 2) > + var displayNoneMarkers = yield observeStyling(5); > + is(displayNoneMarkers.length, 0, "should never restyle if display:none"); > + > + div.style.display = "block"; > + getComputedStyle(div).display; > + var displayNoneMarkers = yield observeStyling(5); nit: displayNoneMarkers is re-defined here, and the second one is not for display:none actually. ::: layout/style/test/test_restyles_in_smil_animation.html:95 (Diff revision 2) > + div.style.display = "none"; > + getComputedStyle(div).display; > + var displayNoneMarkers = yield observeStyling(5); > + is(displayNoneMarkers.length, 0, "should never restyle if display:none"); > + > + div.style.display = "block"; div.style.display = "" is a proper way, I think. A big reason why I don't want to spread observeStyling() out is that it has currently caused an intermittent failure (bug 1244897). I hope you help me a lot to solve that issue. ;-)
Comment 50•8 years ago
|
||
Comment on attachment 8754172 [details] MozReview Request: Bug 1209405 - Part 3: Save updating style. r=birtles https://reviewboard.mozilla.org/r/53798/#review50798 ::: dom/smil/nsSMILAnimationController.h:149 (Diff revision 2) > TimeContainerHashtable* aActiveContainers); > + > static void AddAnimationToCompositorTable( > - mozilla::dom::SVGAnimationElement* aElement, nsSMILCompositorTable* aCompositorTable); > + mozilla::dom::SVGAnimationElement* aElement, > + nsSMILCompositorTable* aCompositorTable, > + bool& aFlushStyleNeeded); Let's call this aStyleFlushNeeded. ::: dom/smil/nsSMILAnimationController.cpp:322 (Diff revision 2) > + bool isResampleNeeded = mResampleNeeded; > mResampleNeeded = false; Why did you split this out into isResampleNeeded and isFlushStyleNeeded? Couldn't we just keep one variable, 'isStyleFlushNeeded'? ::: dom/smil/nsSMILAnimationController.cpp:383 (Diff revision 2) > - AddAnimationToCompositorTable(animElem, currentCompositorTable); > + AddAnimationToCompositorTable(animElem, > + currentCompositorTable, isFlushStyleNeeded); isFlushStyleNeeded should go on a new line ::: dom/smil/nsSMILAnimationController.cpp:442 (Diff revision 2) > // STEP 5: Compose currently-animated attributes. > // XXXdholbert: This step traverses our animation targets in an effectively > // random order. For animation from/to 'inherit' values to work correctly > // when the inherited value is *also* being animated, we really should be > // traversing our animated nodes in an ancestors-first order (bug 501183) > + bool isUpdateStyleNeeded = false; Let's call this mightHavePendingStyleUpdates. In fact, we could even just reset mMightHavePendingStyleUpdates and pass that, I think? ::: dom/smil/nsSMILAnimationController.cpp:640 (Diff revision 2) > // We've now made sure that |func|'s inactivity will be reflected as of > // this sample. We need to clear its HasChanged() flag so that it won't > // trigger this same clause in future samples (until it changes again). > func.ClearHasChanged(); > } > + aFlushStyleNeeded = func.ValueNeedsReparsingEverySample(); This should be |= right? ::: dom/smil/nsSMILAnimationFunction.h:250 (Diff revision 2) > void SetWasSkipped() { > mWasSkippedInPrevSample = true; > } > > + /** > + * Returns true if needs to reparse every sample. 'Returns true if we need to recalculate the animation value on every sample (e.g. because it depends on context like the font-size)' ::: dom/smil/nsSMILCompositor.h:55 (Diff revision 2) > - // attribute > - void ComposeAttribute(); > + // attribute. > + void ComposeAttribute(bool& aUpdateStyleNeeded); We need to document what aUpdateStyleNeeded means (and let's call it aMightHavePendingStyleUpdates). e.g. 'If a change is made that might produce style updates, aMightHavePendingStyleUpdates is set to true. Otherwise it is not modified.' ::: dom/smil/nsSMILCompositor.cpp:68 (Diff revision 2) > } > if (mAnimationFunctions.IsEmpty()) { > // No active animation functions. (We can still have a nsSMILCompositor in > // that case if an animation function has *just* become inactive) > smilAttr->ClearAnimValue(); > + // needs to update when SMIL animation effects are removed. 'Removing the animation effect may require a style update.'
Attachment #8754172 -
Flags: review?(bbirtles)
Comment 51•8 years ago
|
||
Comment on attachment 8754174 [details] MozReview Request: Bug 1209405 - Part 5: Add animation test that changes 'display' attribute. r=birtles https://reviewboard.mozilla.org/r/53802/#review50800 ::: layout/reftests/svg/smil/anim-display-in-g-element.svg:5 (Diff revision 2) > +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" > + xmlns:xlink="http://www.w3.org/1999/xlink" > + class="reftest-wait"> > + > + <title>Test animation that changes 'display' attribute in g element</title> 'Test animation that changes 'display' attribute on an element that is not the immediate parent'
Attachment #8754174 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 52•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=01cafd1ea9d1
Assignee | ||
Comment 53•8 years ago
|
||
Comment on attachment 8754170 [details] MozReview Request: Bug 1209405 - Part 1: Modify tests so as not use display:none. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53794/diff/2-3/
Attachment #8754174 -
Attachment description: MozReview Request: Bug 1209405 - Part 5: Add animation test that changes 'display' attribute. r?birtles → MozReview Request: Bug 1209405 - Part 5: Add animation test that changes 'display' attribute. r=birtles
Attachment #8754175 -
Attachment description: MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL animation. r?hiro → MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL animation. r=hiro
Attachment #8754172 -
Flags: review?(bbirtles)
Assignee | ||
Comment 54•8 years ago
|
||
Comment on attachment 8754171 [details] MozReview Request: Bug 1209405 - Part 2: High CPU load on Kickstarter, even without JS, CSS and images. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53796/diff/2-3/
Assignee | ||
Comment 55•8 years ago
|
||
Comment on attachment 8754172 [details] MozReview Request: Bug 1209405 - Part 3: Save updating style. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53798/diff/2-3/
Assignee | ||
Comment 56•8 years ago
|
||
Comment on attachment 8754173 [details] MozReview Request: Bug 1209405 - Part 4: Add defs element tests. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53800/diff/2-3/
Assignee | ||
Comment 57•8 years ago
|
||
Comment on attachment 8754174 [details] MozReview Request: Bug 1209405 - Part 5: Add animation test that changes 'display' attribute. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53802/diff/2-3/
Assignee | ||
Comment 58•8 years ago
|
||
Comment on attachment 8754175 [details] MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL animation. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53804/diff/2-3/
Comment 59•8 years ago
|
||
Comment on attachment 8754172 [details] MozReview Request: Bug 1209405 - Part 3: Save updating style. r=birtles https://reviewboard.mozilla.org/r/53798/#review50858
Attachment #8754172 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 60•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=967d42fceddd
Assignee | ||
Comment 61•8 years ago
|
||
Comment on attachment 8754170 [details] MozReview Request: Bug 1209405 - Part 1: Modify tests so as not use display:none. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53794/diff/3-4/
Attachment #8754172 -
Attachment description: MozReview Request: Bug 1209405 - Part 3: Save updating style. r?birtles → MozReview Request: Bug 1209405 - Part 3: Save updating style. r=birtles
Assignee | ||
Comment 62•8 years ago
|
||
Comment on attachment 8754171 [details] MozReview Request: Bug 1209405 - Part 2: High CPU load on Kickstarter, even without JS, CSS and images. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53796/diff/3-4/
Assignee | ||
Comment 63•8 years ago
|
||
Comment on attachment 8754172 [details] MozReview Request: Bug 1209405 - Part 3: Save updating style. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53798/diff/3-4/
Assignee | ||
Comment 64•8 years ago
|
||
Comment on attachment 8754173 [details] MozReview Request: Bug 1209405 - Part 4: Add defs element tests. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53800/diff/3-4/
Assignee | ||
Comment 65•8 years ago
|
||
Comment on attachment 8754174 [details] MozReview Request: Bug 1209405 - Part 5: Add animation test that changes 'display' attribute. r=birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53802/diff/3-4/
Assignee | ||
Comment 66•8 years ago
|
||
Comment on attachment 8754175 [details] MozReview Request: Bug 1209405 - Part 6: Add restyle test for SMIL animation. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53804/diff/3-4/
Assignee | ||
Comment 67•8 years ago
|
||
Hi Hiro, Could you review Part 6 patch again since the test sometime failed on Linux debug environment. https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f1ba1dc9d3a&selectedJob=21154522 To solve this, I'm trying to use waitForAllPaintsFlushed instead of waitForAnimationFrames(1). https://reviewboard.mozilla.org/r/53804/diff/3-4/ It looks ok now. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f578f090e27
Comment 68•8 years ago
|
||
(In reply to Daisuke Akatsuka (:daisuke) from comment #67) > Hi Hiro, > Could you review Part 6 patch again since the test sometime failed on Linux > debug environment. > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=6f1ba1dc9d3a&selectedJob=21154522 > > To solve this, I'm trying to use waitForAllPaintsFlushed instead of > waitForAnimationFrames(1). > https://reviewboard.mozilla.org/r/53804/diff/3-4/ > It looks ok now. > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f578f090e27 I am OK with that change for now, but you should check later that where the one extra restyle for SMIL comes from or waitForPaintFlushed() is something broken.
Comment 69•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #68) > (In reply to Daisuke Akatsuka (:daisuke) from comment #67) > > Hi Hiro, > > Could you review Part 6 patch again since the test sometime failed on Linux > > debug environment. > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=6f1ba1dc9d3a&selectedJob=21154522 > > > > To solve this, I'm trying to use waitForAllPaintsFlushed instead of > > waitForAnimationFrames(1). > > https://reviewboard.mozilla.org/r/53804/diff/3-4/ > > It looks ok now. > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f578f090e27 > > I am OK with that change for now, but you should check later that where the > one extra restyle for SMIL comes from or waitForPaintFlushed() is something > broken. Oops. My comment was wrong. Forget about waitForPaintFlushed().
Assignee | ||
Comment 70•8 years ago
|
||
Thanks, Hiro! I'll request to check-in.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 71•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b08ce7055a https://hg.mozilla.org/integration/mozilla-inbound/rev/b373151148d6 https://hg.mozilla.org/integration/mozilla-inbound/rev/16f55c99f2cf https://hg.mozilla.org/integration/mozilla-inbound/rev/e83d50725302 https://hg.mozilla.org/integration/mozilla-inbound/rev/66b0510d6f02 https://hg.mozilla.org/integration/mozilla-inbound/rev/37d3edd296e2
Keywords: checkin-needed
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 72•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c2b08ce7055a https://hg.mozilla.org/mozilla-central/rev/b373151148d6 https://hg.mozilla.org/mozilla-central/rev/16f55c99f2cf https://hg.mozilla.org/mozilla-central/rev/e83d50725302 https://hg.mozilla.org/mozilla-central/rev/66b0510d6f02 https://hg.mozilla.org/mozilla-central/rev/37d3edd296e2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Reporter | ||
Comment 73•7 years ago
|
||
I'm reopening this bug because it appears that the patch did not fix it. Maybe you guys fixed a different bug, like perhaps the simplified case was wrong, or only part of the problem on the actual Kickstarter website ? Either way, the problem is exactly the same. Using Firefox 50.1 with e10s enabled, for more other report details see comment #1. PS: For future reference, when something like that occurs, is it up to me to reopen the bug ? I'm not that acquainted to Bugzilla protocols. At least the issue initially described in this bug is not fixed, that's the certain part.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 74•7 years ago
|
||
Sorry, it's not comment #1, it's the bug description. By which I meant to say, among other things, that the bug occurs even with no CSS. (Mentioning it because this bug has been marked as blocking Bug https://bugzilla.mozilla.org/show_bug.cgi?id=1315874
Comment 75•7 years ago
|
||
(In reply to Steph from comment #73) > I'm reopening this bug because it appears that the patch did not fix it. > Maybe you guys fixed a different bug, like perhaps the simplified case was > wrong, or only part of the problem on the actual Kickstarter website ? Right. We seemed to fix a part of problem that causes high CPU usage. We've been tackling remaining problem(s) in bug 1322970. Please move to that bug. Thank you,
Comment 76•7 years ago
|
||
We don't reopen bugs unless the code concerned is backed out. If there are still issues here they need to be addressed in a different bug.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•