Comment 117 for bug 747197

Created attachment 282460
mostly complete patch (actually not nearly complete)

Updated patch attached. It correctly serializes CSS images. I actually had most of this work done before your review; I should have uploaded what I had earlier, since this resulted in some amount of wasted effort for you, bz. Mea culpa.

I think I removed all the changes related to bug 293834 in this patch.

(In reply to comment #102)
> (From update of attachment 279129 [details])
> >Index: layout/style/nsCSSStyleSheet.cpp
> >+ aContent.Append(NS_LITERAL_STRING("/* Rules from linked stylesheet, original URL: */\n"));
>
> As I said, this is no good if the linked stylesheets have @namespace rules (or
> @charset or @import, but skipping those won't necessarily break things, while
> skipping @namespace most definitely will). Please make sure to write tests for
> this case.
>
> What you probably want to do instead is to serialize the rules, and have
> @import serialization start the serialization of the imported sheet. I believe
> that's what dbaron suggested too.
>

I believe the code should now do this, but haven't written tests yet.

> >+ for (i = 0; i < styleRules; ++i) {
> >+ rv = GetStyleRuleAt(i, *getter_AddRefs(rule));
>
> Declare |rule| here, not up at the beginning somewhere?
done

>
> >Index: layout/style/nsHTMLCSSStyleSheet.cpp
> >+ NS_IMETHOD Serialize(nsAString& aContent, nsIDocumentEncoderFixup *aFixup);
>
> Why not just put this method on nsICSSStyleSheet, since those are all you
> serialize?
>
> >Index: layout/style/nsICSSRule.h
>
> And probably put this on nsICSSStyleRule.
>
Done, though I had Serialize on nsICSSRule and not nsICSSStyleRule because the non-style rules in nsCSSRules.h require Serialize too. But it's not much different either way, I guess.

> I don't see any rule implementations of Serialize() in this patch.
Added.

> Some of the rules will require fixup too, of course
> (@document rules come to mind).
>
> Of course such rules in user sheets won't work right once saving has happened,
> but getting that to work is food for another bug, I think.
Haven't tested/accounted for this yet.

> >Index: embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp
> >+#include "nsIFormControl.h"
> >+#include "nsIDOM3Node.h"
>
> This looks like part of another patch, right?
>

Yes, they're from bug 293834, which this bug depends on to correctly persist serialized style elements.

> >+ // Leave mCurrentDataPath alone so that CSS fixup knows where to save
> >+ //mCurrentDataPath = oldDataPath;
>
> This is pretty questionable. I'd have to dig to make sure, but I would be it's
> wrong.
Yeah, that was leftover code from experimentation that I neglected to delete. Removed.

>
> >@@ -2504,17 +2522,37 @@ nsWebBrowserPersist::EnumPersistURIs(nsH
> >+ rv = cos->Init(outputStream, nsnull, 0, 0);
> >+ NS_ENSURE_SUCCESS(rv, rv);
>
> This is wrong if the sheet has an @charset rule. Unless you plan to strip
> those, in which case it's wrong if the main document is not UTF-8. Again,
> please add tests.

What would be the correct way of persisting @charset rules? Extract the @charset and write the stream with that encoding?

>
> >+ PRBool wroteFullString = PR_FALSE;
> >+ rv = cos->WriteString(data->mContents, &wroteFullString);
>
> Why is wroteFullString being ignored? Perhaps this should be called something
> else?
Er, because WriteString requires a boolean out param, and I couldn't think of what we might do if the full string wasn't written?

>
> >+ rv = cos->Flush();
> >+ rv = outputStream->Flush();
> >+ rv = cos->Close();
>
> Why bother assigning to rv if you plan to ignore it?
rv's removed.

>
> >+ nodeAsLink->GetType(type);
> >+ if (type.EqualsLiteral("text/css")) {
>
> This is wrong. |type| could be an empty string for a CSS style sheet.
> Further, if type is "text/css" there ould be no DOMStyleSheet attached to the
> node. You probably want to just GetSheet() and then null-check it. Again, add
> tests.
Code fixed, tests later.

>
> >+ nsAutoString content;
> >+ ss->Serialize(content, mFixup);
> ...
> >+ data->mContents.Assign(content);
>
> Why not get the |data| first and just pass data->mContents to Serialize()?

done

>
> I haven't reviewed the URI-munging details.
>
> CSS fixup should probably also be applied to "style" attributes. Might be a
> separate bug.

Yeah, I think leaving that as a separate bug will be better than squeezing it in this one.

>
> To answer your quesions in the bug:
>
> > I've tried to avoid gratuitous refactoring of nsWebBrowserPersist
>
> If doing said refactoring (in a separate patch prior to implementing this)
> would make things better, please go for it! Unless you think you want to get
> this into 1.9 and that would make it harder; in that case please file a
> followup on the refactoring.
>

> For images in particular, it does if they were used and we've gotten far enough
> in downloading them to know the type. More precisely, given an
> nsCSSValue::Image you can get its imgIRequest and get the type from that.
>
> But really, if we're persisting those images (which we should be, right?),
> we'll know the type because those we _do_ get via an nsIChannel.
>

Yes, but the problem is that, due to the way webbrowserpersist is structured, we need to know the type of what we're going to download before we download it. The "normal" flow for webbrowserpersist is this:

SaveDocument
  SaveDocumentInternal
    OnWalkDOMNode results in calls to StoreURI
  SaveGatheredURIs
    EnumPersistURIs results in fixed-up filenames based on download channel MIME type
    SaveDocuments results in calls to FixupURI, which maps original URIs to fixed-up filenames.

In essence, it boils down to webbrowserpersist expecting to do two passes, and to download files before determining the filename to replace the URI with, whereas dbaron's writeup is a one-pass system, where a the URI is immediately replaced by a filename. It works if you do a little munging with MIME types, it's just not pretty (thus the musing about refactoring).