Closed
Bug 340542
Opened 18 years ago
Closed 18 years ago
SVG Consolidate functionality to get primary frame
Categories
(Core :: SVG, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: longsonr, Assigned: longsonr)
References
Details
Attachments
(3 files, 3 obsolete files)
14.36 KB,
patch
|
tor
:
review+
bzbarsky
:
superreview-
|
Details | Diff | Splinter Review |
29.45 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
29.57 KB,
patch
|
Details | Diff | Splinter Review |
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•18 years ago
|
||
Followup from bug 339807 comment 5.
Assignee | ||
Comment 2•18 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 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•18 years ago
|
||
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•18 years ago
|
||
Attachment #224576 -
Attachment is obsolete: true
Attachment #224578 -
Flags: review?(tor)
Attachment #224576 -
Flags: review?(tor)
Attachment #224578 -
Flags: review?(tor) → review+
Assignee | ||
Updated•18 years ago
|
Attachment #224578 -
Flags: superreview?(bzbarsky)
Comment 6•18 years ago
|
||
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•18 years ago
|
||
> 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?
Comment 8•18 years ago
|
||
> 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•18 years ago
|
||
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 10•18 years ago
|
||
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•18 years ago
|
||
> (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•18 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•