471875 - Refactoring: in contentAreaUtils.js, create a new internalPersist function

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 (.NET CLR 3.5.30729)

While working on the Mozilla Archive Format extension to integrate it
with the "Save Page As" dialog, I took the opportunity to clean up the
code of the internalSave function in contentAreaUtils.js. The attached
patch is the result of this work, and its behavior should be exactly the
same as the current one.

The patch is against trunk, and I tested it in Firefox 3.0.5 using an overlay.
I tracked my changes as a series of elementary refactoring steps whose log
cas be found at http://hg.mozdev.org/maf/shortlog/ed5826350ddf (mixed with MAF-specific code).

If this patch is OK, the next step might be to make minor functional changes.
Before attempting them, I'd like to know:
* Is there a reason why the original file URL object returned from the file
   picker is preserved, instead of recreating it from the returned file object?
   The latter option would simplify the code somewhat.
* I think that the initialization of filesFolder in internalPersist can
   be safely moved after the transfer object is initialized (that would also
   fix the variable's declaration position that I intentionally left inside
   the if block for the moment). Is it correct?
* When a document that cannot be saved as a "complete" (for example a PNG file)
   is displayed in a tab, and the "Save Page As" menu command is invoked, the
   POST data associated with the original HTTP request is not passed to the
   persist function (the isDocument variable controls this). Wouldn't it be
   better to resend the POST data regardless of whether the document can be
   saved as complete?

Reproducible: Always

Comment 1

[:Paolo Amadini]

Attachment: Refactoring of internalSave

Comment 2

[Boris Zbarsky [:bzbarsky]]

So... I probably _could_ review this if gavin is ok with it, but I'm technically not a peer for this module.  Gavin, thoughts?

In any case, an obvious question: is the idea to make it possible for other callers to call internalPersist with their own pre-built args if desired?  This is changing nothing about existing Firefox code so far, according to comment 0.

To answer your questions:

1)  No idea about the file url issue, other than perhaps concerns about
    round-tripping through the file not preserving something...  On Windows and
    Mac, that URL comes directly from the file in any case in the filepicker
    code.  On Linux it's the other way around, with the url being what the OS
    hands back.  It's probably fine to just get this from the file in all cases.
2)  Sounds ok to me.
3)  That's bug 300032, right?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #2)
> So... I probably _could_ review this if gavin is ok with it, but I'm
> technically not a peer for this module.  Gavin, thoughts?

By all means! If you're willing and able to review, go ahead.

Comment 4

[:Paolo Amadini]

(In reply to comment #2)
> In any case, an obvious question: is the idea to make it possible for other
> callers to call internalPersist with their own pre-built args if desired?
> This is changing nothing about existing Firefox code so far, according to
> comment 0.

The idea here is to make the code more manageable, by splitting the various
steps involved in the save process and documenting what every function expects
from its callers. Unneeded variables are left out of scope. The first patch
I wrote doesn't change anything, but now that some points have been addressed,
I plan to write another one that actually makes some simplifications, like
leaving behind the file URL.

The idea is that understanding the second patch would be easier if it is
diffed with the first one rather than the original code.

> To answer your questions:
> 
> 1)  No idea about the file url issue, other than perhaps concerns about
>     round-tripping through the file not preserving something...  On Windows
>     and
>     Mac, that URL comes directly from the file in any case in the filepicker
>     code.  On Linux it's the other way around, with the url being what the OS
>     hands back.  It's probably fine to just get this from the file in all
>     cases.
> 2)  Sounds ok to me.

Perfect.

> 3)  That's bug 300032, right?

Exactly; I didn't notice it when I searched for bugs related to the save
process.

Comment 5

[:Paolo Amadini]

Attachment: Request for feedback - Refactoring of some save-related functions

Here is an updated patch, whose purpose is only to get some feedback about the
general idea. I haven't even checked it for syntax errors at present.

Comment 6

[:Paolo Amadini]

Now I tested the patch manually, as suggested in the source code:

// Try saving each of these types:
// - A complete webpage using File->Save Page As, and Context->Save Page As
// - A webpage as HTML only using the above methods
// - A webpage as Text only using the above methods
// - An image with an extension (e.g. .jpg) in its file name, using
//   Context->Save Image As...
// - An image without an extension (e.g. a banner ad on cnn.com) using
//   the above method.
// - A linked document using Save Link As...
// - A linked document using Alt-click Save Link As...

It works fine, it seems.

Comment 7

[:Paolo Amadini]

Attachment: Refactoring of some save-related functions, plus a fix

This patch consists in the refactoring of internalSave, to disentangle
some of the function's logic, and also includes the fix for bug 300032.
It is similar to the previous attachment 367491 [details] [diff] [review], except that now the
isDocument variable is not needed anymore.

This is the reasoning that shows that the changes do not affect the
current behavior for existing callers (except for the fix, of course):

GetSaveModeForContentType can return a value different from
SAVEMODE_FILEONLY only if aContentType is present and is a document type
(in particular, not an image type). In turn, aContentType can be present
only when this function is called from saveDocument or saveImageURL, but
in the latter case aContentType is an image type. The saveDocument
function always provides aDocument, while other callers never provide it.

Thus:

saveMode != SAVEMODE_FILEONLY => aDocument != null

aDocument == null => aContentType != "<any-type-in-GetSaveModeForContentType>"
                  => saveMode == SAVEMODE_FILEONLY

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 369501 [details] [diff] [review]
Refactoring of some save-related functions, plus a fix

>+  var useSaveDocument = (aDocument != null) &&

Just test aDocument, not (aDocument != null).

With that, looks very nice.  r=bzbarsky.  I'll land this with that change.

Can you file a followup bug on getting tests generally written here to exercise the various codepaths?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/47787fdae363

Comment 10

[:Paolo Amadini]

(In reply to comment #9)
> Pushed http://hg.mozilla.org/mozilla-central/rev/47787fdae363

Thank you very much for getting this in the tree :-)

(In reply to comment #8)
> Can you file a followup bug on getting tests generally written here to exercise
> the various codepaths?

I filed bug 486342; I already have some ideas on how to structure the test
suite, but before starting the actual work I'd like to determine what could
be the plan on the unforking of "contentAreaUtils.js" (bug 484616).

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #10)

> I'd like to determine what could

Will this bug land in m-1.9.1?

> be the plan on the unforking of "contentAreaUtils.js" (bug 484616).

If not, then I guess that would better wait for comm-central to branch.

Comment 12

[:Paolo Amadini]

(In reply to comment #11)
> (In reply to comment #10)
> 
> > I'd like to determine what could
> 
> Will this bug land in m-1.9.1?
> 
> > be the plan on the unforking of "contentAreaUtils.js" (bug 484616).
> 
> If not, then I guess that would better wait for comm-central to branch.

This bug and a few others related to "contentAreaUtils.js" have not been
backported to 1.9.1 yet. However, they've not been backported to the version
of "contentAreaUtils.js" used by Mozilla Suite either.

Therefore, I think the unforking of "contentAreaUtils.js" may proceed
independently from these bugfixes or the fact that comm-central branches,
if this was your question. See also my recent comment on bug 484616, that
describes how the unforking can be done in practice.