Open Bug 515494 Opened 15 years ago Updated 1 year ago

Remove xlink support in nsGenericElement

Categories

(Core :: DOM: Core & HTML, defect, P5)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Milestone:
mozilla1.9.3a1

People

(Reporter: sdwilsh, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

v1.1
9.92 KB, patch
sicking
: review+
jst
: superreview+
Details | Diff | Splinter Review 
I talked this over with jst and sicking, and this should be OK to do.  I want to remove support for xlinks on an arbitrary html element.  This will help out a lot with bug 461199.
Summary: Remove xlink support in nsGenericHTMLElement → Remove xlink support in nsGenericElement
This may be a duplicate of bug 332773.
Attached patch v1.0 (obsolete) — — Splinter Review 
As far as I can tell, this is all that needs to be done.
Attachment #399616 - Flags: superreview?(jst)
Attachment #399616 - Flags: review?(jonas)
Whiteboard: [needs review sicking][needs sr jst]
Wanna remove nsXMLContentSink::mAllowAutoXLinks and nsIContent::MaybeTriggerAutoLink as well?

It's strictly speaking a different feature, but it's still xlink.
So would this break SVG? Or do we handle xlink differently there?
SVG doesn't use xlink for links as I understand it. (this is one of the confusing parts about xlink)
Especially, with the patch we don't call the ForgetLink if xlink:href
is removed.
Ok, if so we need to add code to nsSVGAElement specifically.
(In reply to comment #8)
> Ok, if so we need to add code to nsSVGAElement specifically.
Pointing me in the right direction would help there.  I'm well outside of my normal coding areas with this patch.
Btw, may I ask how does this help with bug 461199?
(In reply to comment #10)
> Btw, may I ask how does this help with bug 461199?
Everything needs to cache it's visited state with my work in that bug.  I could add caching in nsGenericElement, or we could drop support for it.  I talked it over with some folks and we felt that dropping it made more sense.
(In reply to comment #3)
> Wanna remove nsXMLContentSink::mAllowAutoXLinks and
> nsIContent::MaybeTriggerAutoLink as well?
Can we save that for bug 332773?
Blocks: 332773
Attached patch v1.1 — — Splinter Review 
This won't break MathML or SVG A elements.
Attachment #399616 - Attachment is obsolete: true
Attachment #399812 - Flags: superreview?(jst)
Attachment #399812 - Flags: review?(jonas)
Attachment #399616 - Flags: superreview?(jst)
Attachment #399616 - Flags: review?(jonas)
Attachment #399812 - Flags: superreview?(jst) → superreview+
Whiteboard: [needs review sicking][needs sr jst] → [needs review sicking]
Pushed this to the try server to make sure I didn't break anything.
Whiteboard: [needs review sicking]
So which elements still have XLinks?
http://hg.mozilla.org/mozilla-central/rev/50be346999f6

(In reply to comment #15)
> So which elements still have XLinks?
svg:a and mathml
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Shouldn't svg:view element have Xlink, too?
http://www.w3.org/TR/SVG/linking.html#hyperlinking-mod
Lots (or at least a few) of svg elements use an xlink:href attribute. However they are not anchors which is the type of links debated here. I.e. you can't click them in order to navigate to a different page.
Note that even before this patch (and that's not changed after) MathML's xlink stuff was broken.  See bug 427990.

Also, doesn't this add hiding warnings due to not overriding both SetAttr signatures (yes, I hate C++).

I really wish we could share all that code somehow for html/svg/mathml.  Would it make sense to put it in nsStyledElement?
Actually, looking at this more carefully. I don't see how this is helping anything.

All the code in nsGenericElement did was to call ForgetLink at various points. That doesn't add xlink support to nsGenericElement.

Why was this patch needed?
Also implicated in a Ts regression:

Regression: Ts increase 3.98% on Vista Firefox

Previous results: 526.684 from build 20090911132549 of revision 19413240b354 at 2009-09-11 15:23:00 on talos-rev2-vista15 run # 0

New results: 547.632 from build 20090911141246 of revision 50be346999f6 at 2009-09-11 15:55:00 on talos-rev2-vista04 run # 0

Suspected checkin range: from revision 19413240b354 to revision 50be346999f6
(In reply to comment #20)
> All the code in nsGenericElement did was to call ForgetLink at various points.
> That doesn't add xlink support to nsGenericElement.
> 
> Why was this patch needed?
So maybe my summary of this bug was incorrect (I thought that was the extent of xlink support on nsGenericElement).  Those calls need to disappear for my work in bug 461199 where SetLinkState and GetLinkState manage this.
(In reply to comment #21)
> Also implicated in a Ts regression:
I highly doubt this was the cause of that, but someone should back it out if they want to prove this.
I'd sort of like to second comment 20.  This left XLink support in the tree for nsXMLElement (the only place it was supported, really), but made it so we no longer restyle them on navigation.  We didn't have tests to test such restyling, apparently....

We need to either back this out and reopen or get a bug filed on fixing this correctly.
(In reply to comment #20)
> Actually, looking at this more carefully. I don't see how this is helping
> anything.
> 
> All the code in nsGenericElement did was to call ForgetLink at various points.
> That doesn't add xlink support to nsGenericElement.
> 
> Why was this patch needed?
I missed this comment before.  I'm going to back this patch out and revisit this bug when I get my blockers taken care of.
> but made it so we no longer restyle them on navigation.

Er, sorry.  That's wrong.  We made it so that we never call ForgetLink on such nodes, which means the document will always keep them alive via its link hashtable until it gets destroyed...

I guess we have a similar issue for nodes that are not in the document but get added to the table, actually.  :(
Note bug 516906 where I remove XLink support from nsXMLElement. However I still think the right thing to do here is to back out the patch in this bug. I don't see how it's helping us now other than duplicate code in subclasses instead.

If it helps us later, lets land it then.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
So, should I pull the current patch into bug 461199?  That patch, plus part 19 of bug 461199 should fix the issues that were raised with the current patch.
No longer blocks: async-visited-check
Assignee: sdwilsh → nobody
Status: ASSIGNED → NEW
Is this bug still relevant?
(In reply to comment #30)
> Is this bug still relevant?
I don't think so, but I'm not sure at this point either :/
https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven't been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.
Priority: -- → P5
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: