Last Comment Bug 548263 - XML content sink doesn't split rel value properly before looking for dns-prefetch
: XML content sink doesn't split rel value properly before looking for dns-pref...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: daniloshiga
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-24 06:05 PST by Henri Sivonen (:hsivonen)
Modified: 2011-08-30 04:41 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Copied the code that handles rel value from HTML content sink. (2.12 KB, patch)
2011-07-22 06:55 PDT, daniloshiga
hsivonen: review+
Details | Diff | Review
new version of the patch, since older one doesn't apply anymore (1.69 KB, patch)
2011-08-19 12:46 PDT, daniloshiga
no flags Details | Diff | Review
new version of patch, this time done right. (2.12 KB, patch)
2011-08-29 04:06 PDT, daniloshiga
no flags Details | Diff | Review

Description Henri Sivonen (:hsivonen) 2010-02-24 06:05:07 PST
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.
Comment 1 daniloshiga 2011-07-22 06:55:42 PDT
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.
Comment 2 Henri Sivonen (:hsivonen) 2011-07-24 23:52:44 PDT
Comment on attachment 547683 [details] [diff] [review]
Copied the code that handles rel value from HTML content sink.

Looks ok.
Comment 3 Marco Bonardo [::mak] 2011-08-18 06:06:38 PDT
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
Comment 4 daniloshiga 2011-08-19 12:46:09 PDT
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.
Comment 5 Ed Morley [:emorley] 2011-08-21 06:05:34 PDT
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:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=9a567d609584

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! :-)
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
Comment 6 Ed Morley [:emorley] 2011-08-21 06:39:55 PDT
Failed try:
http://tbpl.allizom.org/php/getParsedLog.php?id=6056159#error0
{
/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 daniloshiga 2011-08-29 04:06:41 PDT
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.
Comment 8 Ed Morley [:emorley] 2011-08-29 05:20:21 PDT
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! :-)
Comment 10 Ed Morley [:emorley] 2011-08-30 04:41:34 PDT
http://hg.mozilla.org/mozilla-central/rev/7658f61d9e2b

Note You need to log in before you can comment on or make changes to this bug.