136718 - FixupNode does not handle XML stylesheet processing instructions

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect, P2)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

While investigating bug 128326 I noticed that saving XML as complete does not
fix the XML stylesheet processing instructions (webbrowserpersist). To test,
open the URL and save it. Please note that I intend to fix bug 128326 by
disabling save as complete for XML until this bug is fixed, so you may need to
do changes in your tree to test this.

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

XML Stylesheet PI spec: http://www.w3.org/TR/xml-stylesheet/

Comment 2

[Bill Law]

nsIWebBrowserPersist issues go to Adam

Comment 3

[Adam Lock]

Attachment: Work in progress

Argh, this is just horrific! Patch demonstrates work in progress - I have
nabbed some code from the XML content sink to parse the href out of the
xml-stylesheet processing instruction, I have still to figure how I can fix it
up and put it back. 

One question - can processing instructions be comma-seperated? The code I stole
from the content sink seems to think so.

Comment 4

[Adam Lock]

Attachment: Patch

Patch parses xml-stylesheet, fixes up the href and reconstructs it including
the other pseudo-attributes. This patch is large but much of it is
reorganization, moving common fixup stuff into shared functions and so on.

Testing must be done in mfcEmbed because Mozilla won't allow XML files to be
saved with their linked content. XML serialization is still broken, but the
xml-stylesheet stuff appears to be fixed up correctly.

Comment 5

[Adam Lock]

Heikki, can you review this patch for me please?

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 90511 [details] [diff] [review]
Patch

>Index: mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp
>===================================================================
>+// Content types that indicate linked documents should also be saved
>+const char *kPersistableLinkTypes[] = {
>+    "text/css"
>+};
>+const PRUint32 kPersistableLinkTypesSize = sizeof(kPersistableLinkTypes) / 

The above are not used anywhere.

>+// Schemes that cannot be saved because they contain no useful content
>+const char *kNonpersistableSchemes[] = {
>+    "about:",

Borrowing from security, it might be better to list schemes that CAN be saved,
but since this is copied from old code and I don't think there are security
issues, I am fine with this.

>+PRBool
>+nsWebBrowserPersist::GetQuotedAttributeValue(
>+    const nsAString &aSource, const nsAString &aAttribute, nsAString &aValue)
>+{  
>+    // NOTE: This code was lifted verbatim from nsParserUtils.cpp

Could this perhaps reuse the one in parser, or would that introduce a new
dependency? Would it matter?

>+nsresult nsWebBrowserPersist::FixupXMLStyleSheetLink(nsIDOMProcessingInstruction *aPI, nsAString &aHref)

Null check aPI?

>+            newData += NS_LITERAL_STRING("href=\"");
>+            newData += aHref; // Use the supplied href
>+            newData += PRUnichar('\"');
>+            newData += PRUnichar(' ');

Could you replace the above pattern with:

>+            newData += NS_LITERAL_STRING("href=\"")
>+                    + aHref // Use the supplied href
>+                    + NS_LITERAL_STRING("\" ");

This same pattern appears elsewhere as well. Also, the last NS_LITERAL_STRING
should probably be made into a named literal since it appears several times in
the same function.

>+        if (target.Equals(NS_LITERAL_STRING("xml-stylesheet").get()))

I think you can remove |.get()|.

>+        rawPathURL.Assign(data->mRelativePathToData);
>+        rawPathURL.Append(filename);

Again, would it compile if you did (would be more efficient):

>+        rawPathURL = data->mRelativePathToData + filename;

>Index: mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.h
>===================================================================

Done.

Please note that you can test this in the browser if you reverse patch this
diff:

http://bugzilla.mozilla.org/attachment.cgi?id=78804&action=view

(then you can save XML docs only as complete).

Comment 7

[Adam Lock]

Attachment: Patch mk II

All issues fixed with the exception of the nsParserUtils issue. That helper
class is private to the content lib so for the sake of 40 or so lines I thought
it better to lift the method I was interested in.

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 90642 [details] [diff] [review]
Patch mk II

r=heikki

Comment 9

[kinmoz]

Comment on attachment 90642 [details] [diff] [review]
Patch mk II

sr=[e-mail]


+    // NOTE: This code was lifted verbatim from nsParserUtils.cpp

Like heikki, this bugs me ... we really should have some string parsing utility
classes that can be shared amongst the parser, editor and the various other
modules like this one that need it.

Comment 10

[Adam Lock]

Fix is checked in. I raised bug 156539 to deal with the parser code reuse issue.

Comment 11

[Adam Lock]

Marking fixed

Comment 12

[Heikki Toivonen (remove -bugzilla when emailing directly)]

This is actually not fixed. There is an error in FixupXMLStyleSheetLink() such
that we always write out the old href instead of the fixed href. I will soon
attach a patch to bug 146062 that will also fix this. 

Reopening.

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

taking...

Comment 14

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Fixed again.

Comment 15

[Chris Petersen]

Verified in the 2003-01-23-04 Win32 and 2003-01-23-03 OS X trunk builds.