182152 - File picker should always offer a *.* filter

Status: VERIFIED FIXED

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

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Is this correctly handled by the code that comes later on that determines the
save format based on which filter was selected?

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Oh, bah.  Bug 116951

And the code to watch out for is
http://lxr.mozilla.org/mozilla/source/xpfe/communicator/resources/content/conte
ntAreaUtils.js#317 and
http://lxr.mozilla.org/mozilla/source/xpfe/communicator/resources/content/conte
ntAreaUtils.js#320 and
http://lxr.mozilla.org/mozilla/source/xpfe/communicator/resources/content/conte
ntAreaUtils.js#342 (yes, this code is incredibly fragile...)

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Ummm, new patch

Okay, so why was this code so broken to begin with?  This sucks.  Here's a new
patch which should fare much better.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Nevermind, bryner.  For now anyway....	Apparantly this code was so horrible
before, that this patch exposed a few other bugs.  They are fixed in my newest
patch, but I'll get an r before I ask for your sr.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 107573 [details] [diff] [review]
Ummm, new patch

Let's try this again, shall we?  :)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 107573 [details] [diff] [review]
Ummm, new patch

>-    
>-  appendFiltersForContentType(fp, contentType,
>-                              isDocument ? MODE_COMPLETE : MODE_FILEONLY);  
>+
>+  appendFiltersForContentType(fp, contentType, saveMode);  

This change is wrong.  If isDocument is false, the mode _must_ be
MODE_FILEONLY.	For example, "save link as" should not offer things like "web
page complete" and "text" -- it should just offer MODE_FILEONLY.

>+  // XXX We depend on this holding true in appendFiltersForContentType():
>+  // If we can save the DOM as complete, it's index is 0.

"If we should save as a complete page, the filterIndex is 0."

>+  // If we can save the text as complete, it's index is 2.

"If we should save as text, the filterIndex is 2."

>+  var isSaveComplete = isDocument &&

I'd call this var "useSaveDocument", maybe....

>+const MODE_FILEONLY      = 0x00;
>+const MODE_COMPLETE_TEXT = 0x01;

const MODE_TEXT = 0x01;

>+const MODE_COMPLETE_DOM  = 0x10;

const MODE_COMPLETE_PAGE = 0x02;

> function appendFiltersForContentType(aFilePicker, aContentType, aSaveMode)

Add a comment here that references that other XXX comment you added (or just
state the invariant that must be observed up front).

The rest looks nice.  ;)

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Using hex, not binary this time.

/me teaches his brain to not think in different bases than his fingers

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111017 [details] [diff] [review]
Using hex, not binary this time.

Since I am in nitpick mode....

1)  Do you need the isDocument boolean?  It looks like aData.document and
useSaveDocument are what you really need and there is no need for isDocument.

2) GetSaveModeForContentType should not be adding SAVEMODE_COMPLETE_TEXT for
XML, should it?  (the case order needs to be switched....

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: :)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111021 [details] [diff] [review]
:)

OK, I suck.  You _do_ need isDocument; a lot of those places really care about
"aData.document && saveMode"....

Please revert that change, and r/sr=bzbarsky

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

I only found one thing that cared about it, but there are three that bz pointed
out.  For the record:

<caillon> why?  what cares about the saveMode?
<bz> +  if (!aData.document && !shouldDecode && contentEncodingType) {
<bz> that needs to look at saveMode
<caillon> right that's the one place I found
<bz>        fp.filterIndex = prefs.getIntPref("save_converter_index");
<bz> there too...
<bz> since we don't want to save the *.* thing or anything....
<caillon> ah
<bz> and more to the point, in cases when we have a doc but are _not_ doing
saveMode games (eg a js doc)
<bz> we don't want to be restoring that pref
<bz> those three places are it


isDocument is now back in.  :)

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Tested without any regressions on HTML, XML, XHTML, MathML, plaintext, and PNG,
GIF, and JPG image files.

I then landed this.

Comment 14

[Chris Petersen]

Verified in the Linux 2003-01-23-05 trunk build