Closed Bug 340542 Opened 18 years ago Closed 18 years ago

SVG Consolidate functionality to get primary frame

Categories

(Core :: SVG, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

Attachments

(3 files, 3 obsolete files)

Lots of methods in svg/content classes need to get the primary frame. We should have a single implementation as nsGenericHTMLElement does.
Followup from bug 339807 comment 5.
Assignee: general → longsonr
Status: NEW → ASSIGNED
Attachment #224573 - Flags: review?(tor)
I'm not absolutely sure I should include the suspend/unsuspend and getbbox methods in nsSVGSVGElement as they would return NS_ERROR_FAILURE in the intermediate steps and won't any more.
Comment on attachment 224573 [details] [diff] [review]
patch modelled on nsGenericHTMLElement

I'm not sure why you have both GetPrimaryFrame and GetPrimaryFrameFor, as only the former is used.  Why not just have the one argument form?
Attached patch 2nd attempt (obsolete) — Splinter Review
Is this better? One function although it does depend on nsGenericHTMLElement. I could copy that code although we do already have a dependency on nsGenericHTMLElement elsewhere in content.
Attachment #224573 - Attachment is obsolete: true
Attachment #224576 - Flags: review?(tor)
Attachment #224573 - Flags: review?(tor)
Attachment #224576 - Attachment is obsolete: true
Attachment #224578 - Flags: review?(tor)
Attachment #224576 - Flags: review?(tor)
Attachment #224578 - Flags: review?(tor) → review+
Attachment #224578 - Flags: superreview?(bzbarsky)
Comment on attachment 224578 [details] [diff] [review]
without unrelated marker changes

Two issues here:

1) This change breaks us flushing layout.

2) Getting the primary frame should probably just live on nsGenericElement -- there's nothing SVG- or HTML-specific about it.
Attachment #224578 - Flags: superreview?(bzbarsky) → superreview-
Attached patch possible solution (obsolete) — Splinter Review
> Two issues here:

> 1) This change breaks us flushing layout.

Mea Culpa. I didn't notice the flags were different.

> 2) Getting the primary frame should probably just live on nsGenericElement --
> there's nothing SVG- or HTML-specific about it.

I have attached something which would be about the simplest change possible as it just moves the existing static method as-is. It does not save much code space though.

An alternative would be to add Flush_None to mozFlushType and an NS_ASSERT that that is not a valid value for nsDocument::FlushPendingNotifications (or alternatively just a return if it was passed).

Then the static GetPrimaryFrameFor could be passed a mozFlushType rather than a PRBool calling FlushPendingNotifications for any value except Flush_None.

The member function GetPrimaryFrame could also be moved to the parent and given a mozFlushType argument.

What solution would you like to see? 

Do I need to split this up into an SVG part and a non-SVG part to get it reviewed and should that be as separate bugs or just different patches within this one?
> An alternative would be to add Flush_None to mozFlushType

You can just pass 0 as needed and then flush only if it's nonzero.  That seems like the way to go.

> Do I need to split this up into an SVG part and a non-SVG part 

Nope.  One patch should be fine; I can r+sr the non-SVG changes.
I could not pass 0 to a method that takes an enum as it does not compile so I have overloaded the functions instead.

There are lots of places that the new GetPrimaryFrame methods could save code space in the content/html code so I'll raise a follow up for that.
Attachment #224681 - Attachment is obsolete: true
Attachment #224839 - Flags: superreview?(bzbarsky)
Attachment #224839 - Flags: review?(bzbarsky)
Comment on attachment 224839 [details] [diff] [review]
attempt to address sr comments

>Index: base/src/nsGenericElement.h

>+  nsIFrame* GetPrimaryFrame();

Document that this does not flush?

>+   * @param aType the kind of flush to do.  EXPENSIVE.

I don't think we need the "EXPENSIVE" part.  Could document that typical mozFlushType values here would be Flush_Frames or Flush_Layout, depending on what you want out of the return value.  Not a big deal either way, though.

>+  static nsIFrame* GetPrimaryFrameFor(nsIContent* aContent,
>+                                      nsIDocument* aDocument);

Again, document that this does not flush?

>Index: svg/content/src/nsSVGGraphicElement.cpp
> NS_IMETHODIMP nsSVGGraphicElement::GetBBox(nsIDOMSVGRect **_retval)
>   NS_ASSERTION(frame, "can't get bounding box for element without frame");

Kind of unrelated, but |frame| can easily be null here.  If you want to make this a warning, sure, but it shouldn't be an assert.  Same thing happens in a few of the other files you modified.  If you want to spin that off into a separate bug, go for it.

r+sr=bzbarsky.  This is excellent stuff.  I look forward to the rest of the HTML changes too!
Attachment #224839 - Flags: superreview?(bzbarsky)
Attachment #224839 - Flags: superreview+
Attachment #224839 - Flags: review?(bzbarsky)
Attachment #224839 - Flags: review+
> (From update of attachment 224839 [details] [diff] [review] [edit])
> >Index: base/src/nsGenericElement.h

> >+  nsIFrame* GetPrimaryFrame();

> Document that this does not flush?

Done.

> >+   * @param aType the kind of flush to do.  EXPENSIVE.

> I don't think we need the "EXPENSIVE" part.  Could document that typical
> mozFlushType values here would be Flush_Frames or Flush_Layout, depending on
> what you want out of the return value.  Not a big deal either way, though.

Done.

> >+  static nsIFrame* GetPrimaryFrameFor(nsIContent* aContent,
> >+                                      nsIDocument* aDocument);

> Again, document that this does not flush?

Done.

> >Index: svg/content/src/nsSVGGraphicElement.cpp
> > NS_IMETHODIMP nsSVGGraphicElement::GetBBox(nsIDOMSVGRect **_retval)
> >   NS_ASSERTION(frame, "can't get bounding box for element without frame");

> Kind of unrelated, but |frame| can easily be null here.  If you want to make
> this a warning, sure, but it shouldn't be an assert.  Same thing happens in a
> few of the other files you modified.  If you want to spin that off into a
> separate bug, go for it.

This issue is tracked by bug 327705 and bug 326974
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Blocks: 341261
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: