Stop returning nsChangeHint_NeedDirtyReflow unnecessarily from CalcDifference for SVG property changes

NEW
Unassigned

Status

()

Core
SVG
5 years ago
4 years ago

People

(Reporter: jwatt, Unassigned)

Tracking

({perf})

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [jwatt:invalidation])

In bug 875037 I discovered that it's not really possible to just return nsChangeHint_NeedReflow from a style struct's CalcDifference method, so I ended up returning nsChangeHint_NeedDirtyReflow with it. That will cause us to do excessive work though, so we should try and remove that.

The issue that I encountered is that if CalcDifference just returns the hint nsChangeHint_NeedReflow we can end up hitting:

  "ASSERTION: Shouldn't be trying to restyle non-elements directly"

under the stack:

  nsStyleChangeList::AppendChange
  CaptureChange
  nsFrameManager::ReResolveStyleContext
  nsFrameManager::ReResolveStyleContext
  nsFrameManager::ReResolveStyleContext
  nsFrameManager::ComputeStyleChangeFor
  nsCSSFrameConstructor::RestyleElement
  mozilla::css::RestyleTracker::ProcessOneRestyle
  mozilla::css::RestyleTracker::DoProcessRestyles
  mozilla::css::RestyleTracker::ProcessRestyles
  nsCSSFrameConstructor::ProcessPendingRestyles

where AppendChange is passed an nsTextFrame. (See bug 875037 comment 8 for its specifics.)

The issue seems to be that nsChangeHint_NeedReflow is not handled for descendants if the nsChangeHint_NeedDirtyReflow isn't also present:

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsChangeHint.h?rev=7a2f7a45819a&mark=195-196#182

which is why we end up processing the nsChangeHint_NeedDirtyReflow hint for an nsTextFrame and triggering the assertion above.

Any thoughts on how we should handle this, dbaron?
Flags: needinfo?(dbaron)
The only other style struct change that seems to result in only nsChangeHint_NeedReflow being returned from CalcDifference is nsStylePosition::mJustifyContent. I wonder if we just don't have any tests that cause that to trigger the assertion.
Assuming I land tomorrow, this bug will be to remove the lines:

https://mxr.mozilla.org/mozilla-central/search?string=bug%20876085
So it's a really bad idea to return a hint that's non-inherited from the CalcDifference method for an inherited struct.  (I think there's a case or two where such a bug slipped in, and an existing bug on the resulting assertions... but we don't want more.)

Is this for an inherited struct?  If so...what do you intend it to mean to want NeedReflow but not NeedDirtyReflow?

(sorry, very quick answer, but pressed for time right now)
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #3)
> So it's a really bad idea to return a hint that's non-inherited from the
> CalcDifference method for an inherited struct.  (I think there's a case or
> two where such a bug slipped in, and an existing bug on the resulting
> assertions... but we don't want more.)

I do not have a good grasp on the different style change hints yet.  If CalcDifference should not return non-inherited hints for inherited style structs, can we assert this?  Maybe in the DO_STRUCT_DIFFERENCE macro used in nsStyleContext::CalcStyleDifference?
Yes, we probably should; there's an open bug on such an assertion (relatively new, and I think the same assertion as comment 0) that I haven't gotten to yet, I think.


Anyway, what was the thing that you thought didn't need NeedDirtyReflow?
(FWIW, this bug as described is WONTFIX; we should probably add some assertions to make the situation clearer, though.)
(Reporter)

Updated

4 years ago
Whiteboard: [jwatt:invalidation]
You need to log in before you can comment on or make changes to this bug.