Closed Bug 1189307 Opened 10 years ago Closed 10 years ago

Use GetClosestFrameOfType to get svgTextFrame

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: longsonr, Assigned: longsonr)

Details

Attachments

(1 obsolete file)

No description provided.
Attached patch closest.txt (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #8641062 - Flags: review?(dholbert)
heycam is probably a better reviewer here, since he understands the invariants around SVG text frames better than I do. Can you provide some context to explain what's changing here & why? Note that this does seem to be a functional change -- the existing code just looks up 1-2 frames and stops -- in contrast, the GetClosestFrameOfType() call you're swapping in will walk up the tree indefinitely. (Maybe that's still no functional change, if we know there must be a svgTextFrame at most 2 hops away? I don't know.)
Comment on attachment 8641062 [details] [diff] [review] closest.txt One purely-stylistic nit, though: >+++ b/layout/svg/nsSVGEffects.cpp >+ frame = nsLayoutUtils::GetClosestFrameOfType >+ (frame->GetParent(), nsGkAtoms::svgTextFrame); This wrapping style is a bit unconventional. Better not to have a newline after the function-name / open-paren, unless absolutely necessary to avoid going over the 80 character line limit. And we're not at risk of hitting that limit here -- this could just be: frame = nsLayoutUtils::GetClosestFrameOfType(frame->GetParent(), nsGkAtoms::svgTextFrame); Canceling review flag; please tag heycam for review after you've given a bit more background per comment 2 (since I suspect that'll help heycam understand what's going on, too).
Attachment #8641062 - Flags: review?(dholbert)
See bug 1130888 for why I think this may be required.
Attachment #8641062 - Flags: review?(cam)
I'm not sure this is right -- will this cause <tspan fill="url(...)"> to instead use the ancestor <text> element's paint server reference instead? Can you provide an example document showing, or explain how, the existing code doesn't work? I'm not so sure the problem in bug 1130888 is analagous here.
Flags: needinfo?(longsonr)
These are the frame trees from bug 1130888 (it has text and a textPath) nsTextFrame = aTargetFrame nsInlineFrame nsInlineFrame <- the textPath frame nsBlockFrame <- an intermediate block frame SVGTextFrame <- the text frame nsTextFrame = aTargetFrame nsBlockFrame <- an intermediate block frame SVGTextFrame <- the text frame so the existing code is correct as is that for bug 113088 (since SVG state bits only exist on SVG frames)
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(longsonr)
Resolution: --- → INVALID
Attachment #8641062 - Attachment is obsolete: true
Attachment #8641062 - Flags: review?(cam)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: