Last Comment Bug 756462 - Stop unnecessary propagating COORD_CONTEXT_CHANGED notifications to descendants of nsSVGInnerSVGFrame
: Stop unnecessary propagating COORD_CONTEXT_CHANGED notifications to descendan...
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Jonathan Watt [:jwatt]
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-18 07:47 PDT by Jonathan Watt [:jwatt]
Modified: 2012-05-19 18:32 PDT (History)
2 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.33 KB, patch)
2012-05-18 07:49 PDT, Jonathan Watt [:jwatt]
longsonr: review+
Details | Diff | Splinter Review

Description Jonathan Watt [:jwatt] 2012-05-18 07:47:08 PDT
We should stop unnecessary and wastefully propagating COORD_CONTEXT_CHANGED notifications to descendants of nsSVGInnerSVGFrame. Doing so has the potential to cause us to do much more work than necessary, mainly in the form of repainting a much larger area of the screen than we need to.
Comment 1 Jonathan Watt [:jwatt] 2012-05-18 07:49:49 PDT
Created attachment 625103 [details] [diff] [review]
patch
Comment 2 Jonathan Watt [:jwatt] 2012-05-18 07:54:54 PDT
Actually I've changed the comment to the following locally:

      // Remove COORD_CONTEXT_CHANGED, since we establish the coordinate
      // context for our descendants and this notification won't change its
      // dimensions:
Comment 3 Robert Longson 2012-05-18 08:06:26 PDT
If the inner svg originally had a width or height of 0px and we change to a non-zero px width/height or we have a viewBox and a width/height of 0% and we change to non-zero% width/height then we do need a notification I think as our children won't have had valid user units at all before. Can you add reftests to check each of these cases please and additional logic if necessary.
Comment 4 Robert Longson 2012-05-18 08:13:43 PDT
You might want to clone and adjust the reftests that went wrong when you sent your original patch for bug 413960 to try.
Comment 5 Jonathan Watt [:jwatt] 2012-05-18 08:52:28 PDT
NotifySVGChanged is never called on a frame for changes to itself. It's called on descendants of the frame that changed. So in your examples, when the width/height/viewBox of the nsSVGInnerSVGFrame change, then NotifySVGChanged will be called on all its descendants without the opportunity for nsSVGInnerSVGFrame::NotifySVGChanged to stop that, since nsSVGInnerSVGFrame::AttributeChanged calls nsSVGUtils::NotifyChildrenOfSVGChange.
Comment 6 Robert Longson 2012-05-18 09:03:51 PDT
(In reply to Jonathan Watt [:jwatt] from comment #5)
> NotifySVGChanged is never called on a frame for changes to itself. It's
> called on descendants of the frame that changed. So in your examples, when
> the width/height/viewBox of the nsSVGInnerSVGFrame change, then
> NotifySVGChanged will be called on all its descendants without the
> opportunity for nsSVGInnerSVGFrame::NotifySVGChanged to stop that, since
> nsSVGInnerSVGFrame::AttributeChanged calls
> nsSVGUtils::NotifyChildrenOfSVGChange.

OK, I guess I buy that.
Comment 7 Jonathan Watt [:jwatt] 2012-05-19 06:53:12 PDT
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/17cad7e07453
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-05-19 18:32:31 PDT
https://hg.mozilla.org/mozilla-central/rev/17cad7e07453

Note You need to log in before you can comment on or make changes to this bug.