SVG Consolidate functionality to get primary frame

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

12 years ago
Lots of methods in svg/content classes need to get the primary frame. We should have a single implementation as nsGenericHTMLElement does.
(Assignee)

Comment 1

12 years ago
Created attachment 224573 [details] [diff] [review]
patch modelled on nsGenericHTMLElement

Followup from bug 339807 comment 5.
Assignee: general → longsonr
Status: NEW → ASSIGNED
Attachment #224573 - Flags: review?(tor)
(Assignee)

Comment 2

12 years ago
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 3

12 years ago
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?
(Assignee)

Comment 4

12 years ago
Created attachment 224576 [details] [diff] [review]
2nd attempt

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)
(Assignee)

Comment 5

12 years ago
Created attachment 224578 [details] [diff] [review]
without unrelated marker changes
Attachment #224576 - Attachment is obsolete: true
Attachment #224578 - Flags: review?(tor)
Attachment #224576 - Flags: review?(tor)

Updated

12 years ago
Attachment #224578 - Flags: review?(tor) → review+
(Assignee)

Updated

12 years ago
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-
(Assignee)

Comment 7

12 years ago
Created attachment 224681 [details] [diff] [review]
possible solution

> 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.
(Assignee)

Comment 9

12 years ago
Created attachment 224839 [details] [diff] [review]
attempt to address sr comments

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+
(Assignee)

Comment 11

12 years ago
Created attachment 225280 [details] [diff] [review]
version checked in

> (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
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Blocks: 341261
You need to log in before you can comment on or make changes to this bug.