375175 - Get rid of unused function nsSVGOuterSVGFrame::InitiateReflow

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Eli Friedman]

Attachment: Patch

Simple cleanup.

Comment 1

[Robert Longson [:longsonr]]

bug 301628 removed the code that set mNeedsReflow to PR_TRUE. I wonder if we should set this to PR_TURE in nsSVGOuterSVGFrame::AttributeChanged to see if that fixes any of: bug 369850, bug 371428 bug 349541, bug 365915.

Comment 2

[Robert Longson [:longsonr]]

(In reply to comment #1)
> bug 301628 removed the code that set mNeedsReflow to PR_TRUE. I wonder if we
> should set this to PR_TURE in nsSVGOuterSVGFrame::AttributeChanged to see if
> that fixes any of: bug 369850, bug 371428 bug 349541, bug 365915.
> 

When I looked into this some more I found that AttributeChanged already does this.

Comment 3

[Jonathan Watt [:jwatt]]

Making 'svg' a replaced element (bug 294086) fixes bug 369850 BTW. Probably also bug 371428, but it doesn't have a testcase.

Comment 4

[tor]

Comment on attachment 259496 [details] [diff] [review]
Patch

I wonder how much of this will have to be reintroduced if start using reflow more (to get more fine-grained detail about updates).

Comment 5

[Robert Longson [:longsonr]]

Comment on attachment 259496 [details] [diff] [review]
Patch

Would keeping it around really make improving SVG reflow any easier?

See also http://groups.google.com/group/mozilla.dev.tech.svg/browse_thread/thread/e51d4bb6d8f96b70/1c949381e9880a8d#1c949381e9880a8d

low risk removal of dead code.

Comment 6

[Jonathan Watt [:jwatt]]

All this code does is try to avoid updating the SVG twice if a reflow is pending when we update due to unsuspending painting. At most this is a perf issue. It's not a correctness issue. To determine if this is even a perf issue we could stick the following warning into UnsuspendRedraw:

  if (GetStateBits() & NS_FRAME_IS_DIRTY) {
    NS_WARNING("We're going to update again due to reflow!");
  }

In fact Reflow calls SuspendRedraw/UnsuspendRedraw, so we could just return from there early instead of warning and our children will have UnsuspendRedraw called on them when Reflow calls it, right? (Admittedly we'd have to cheat with mRedrawSuspendCount.)

Anyway, keeping this around won't "improve SVG reflow" in terms of fixing bugs, and doing syncronous reflow has performance issues of its own so I don't think we should be doing that anyway.

FWIW, I'd say the risk of removing dead code is about zero. ;-)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 259496 [details] [diff] [review]
Patch

a=bzbarsky

Comment 8

[Jonathan Watt [:jwatt]]

Boris is waiting on this, so I'll check it in now.

Comment 9

[Jonathan Watt [:jwatt]]

Checked in.

I also opened bug 393064 to follow up and investigate whether we have a perf issue here.