XML content sink doesn't split rel value properly before looking for dns-prefetch

RESOLVED FIXED in mozilla9



8 years ago
6 years ago


(Reporter: hsivonen, Assigned: daniloshiga)



Firefox Tracking Flags

(Not tracked)



(1 attachment, 2 obsolete attachments)



8 years ago
The HTML content sink has this code:
2793       // look for <link rel="next" href="url">
2794       nsAutoString relVal;
2795       element->GetAttr(kNameSpaceID_None, nsGkAtoms::rel, relVal);
2796       if (!relVal.IsEmpty()) {
2797         // XXX seems overkill to generate this string array
2798         nsAutoTArray<nsString, 4> linkTypes;
2799         nsStyleLinkElement::ParseLinkTypes(relVal, linkTypes);
2800         PRBool hasPrefetch = linkTypes.Contains(NS_LITERAL_STRING("prefetch"));
2801         if (hasPrefetch || linkTypes.Contains(NS_LITERAL_STRING("next"))) {
2802           nsAutoString hrefVal;
2803           element->GetAttr(kNameSpaceID_None, nsGkAtoms::href, hrefVal);
2804           if (!hrefVal.IsEmpty()) {
2805             PrefetchHref(hrefVal, element, hasPrefetch);
2806           }
2807         }
2808         if (linkTypes.Contains(NS_LITERAL_STRING("dns-prefetch"))) {
2809           nsAutoString hrefVal;
2810           element->GetAttr(kNameSpaceID_None, nsGkAtoms::href, hrefVal);
2811           if (!hrefVal.IsEmpty()) {
2812             PrefetchDNS(hrefVal);
2813           }
2814         }
2815       }

But the XML content sink only has this code:
661     // Look for <link rel="dns-prefetch" href="hostname">
662     if (nodeInfo->Equals(nsGkAtoms::link, kNameSpaceID_XHTML)) {
663       nsAutoString relVal;
664       aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::rel, relVal);
665       if (relVal.EqualsLiteral("dns-prefetch")) {
666         nsAutoString hrefVal;
667         aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::href, hrefVal);
668         if (!hrefVal.IsEmpty()) {
669           PrefetchDNS(hrefVal);
670         }
671       }
672     }

The XML content sink should have the same code that the HTML content sink has here.
Keywords: helpwanted, student-project

Comment 1

6 years ago
Created attachment 547683 [details] [diff] [review]
Copied the code that handles rel value from HTML content sink.

If I understood the bug correctly, this should fix it.
Attachment #547683 - Flags: review?(hsivonen)

Comment 2

6 years ago
Comment on attachment 547683 [details] [diff] [review]
Copied the code that handles rel value from HTML content sink.

Looks ok.
Attachment #547683 - Flags: review?(hsivonen) → review+
Whiteboard: checkin-needed
Keywords: checkin-needed
Whiteboard: checkin-needed
Assignee: nobody → daniloshiga
the patch doesn't apply anymore, could you please update it and then reask for c-n? be sure to put checkin-needed in the keywords field
Keywords: checkin-needed

Comment 4

6 years ago
Created attachment 554511 [details] [diff] [review]
new version of the patch, since older one doesn't apply anymore

just made the same changes, based on the current code.
Attachment #547683 - Attachment is obsolete: true


6 years ago
Keywords: checkin-needed

Comment 5

6 years ago
What name do you want for the patch author? I've set it to just daniloshiga for now, but if you let me know before this gets pushed to inbound, I'll adjust for you.

Sent to try:

Assuming all green, will push to inbound after.

I'm happy to fix it this time, but for new patches could you make sure the patch format (context, commit message, author etc) is correct when requesting checkin-needed - thanks! :-)
Keywords: checkin-needed

Comment 6

6 years ago
Failed try:
/builds/slave/try-lnx64-dbg/build/content/xml/document/src/nsXMLContentSink.cpp: In member function 'virtual nsresult nsXMLContentSink::CloseElement(nsIContent*)':
/builds/slave/try-lnx64-dbg/build/content/xml/document/src/nsXMLContentSink.cpp:652:10: error: 'relVal' was not declared in this scope
make[8]: *** [nsXMLContentSink.o] Error 1

Comment 7

6 years ago
Created attachment 556514 [details] [diff] [review]
new version of patch, this time done right.

I'm really sorry about the last patch, done it in a hurry and used the wrong command, lesson learned, here comes the right patch.
Attachment #554511 - Attachment is obsolete: true
Attachment #556514 - Flags: checkin+


6 years ago
Attachment #556514 - Flags: checkin+


6 years ago
Keywords: checkin-needed

Comment 8

6 years ago
That's ok - thanks for fixing it up :-)

The patch is now in my queue with a few other bits that are being sent to try first and then landing on mozilla-inbound. Once there it will be merged to mozilla-central within a day or so. At each stage the relevant changeset URLs will be posted back here and whomever does the merges will mark resolved fixed at the appropriate point.

Thanks again for your contribution - looking forward to seeing the next one - ask on IRC if you'd like any guidance/suggestions! :-)
Keywords: checkin-needed

Comment 9

6 years ago
Keywords: helpwanted, student-project
Target Milestone: --- → mozilla9
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.