402458 - saveDocument func calls internalSave with aSkipPrompt param in the wrong position @ contentAreaUtils.js:168

Status: RESOLVED FIXED

(Firefox :: File Handling, defect, P2)

Description

[CamilleBlardone]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en) AppleWebKit/419.3 (KHTML, like Gecko) Safari/419.3
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9

saveDocument(aDocument, aSkipPrompt) 

calls 

internalSave(aDocument.location.href, aDocument, null, contentDisposition,
               aDocument.contentType, false, null, null, aSkipPrompt);

with 9 parameters instead of 10. As a result, aSkipPrompt which should be the 10th parameters is always interpreted as "undefined" by "internalSave(...)".

A a consequence, the file prompt is always asked for every code that is using saveDocument(aDocument, aSkipPrompt)  to save a document. 

Which is the answer to some bugs.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Jeremy Baron]

(In reply to comment #0)
> saveDocument(aDocument, aSkipPrompt) 
> 
> calls 
> 
> internalSave(aDocument.location.href, aDocument, null, contentDisposition,
>                aDocument.contentType, false, null, null, aSkipPrompt);
In the future, please provide a filename / line no for the file you're talking about or just link to it at bonsai/mxr (mxr.mozilla.org and bonsai.mozilla.org)

> with 9 parameters instead of 10. As a result, aSkipPrompt which should be the
> 10th parameters is always interpreted as "undefined" by "internalSave(...)".
> 
> A a consequence, the file prompt is always asked for every code that is using
> saveDocument(aDocument, aSkipPrompt)  to save a document. 
The aSkipPrompt in saveDocument's call to internalSave was added while porting code from xpfe to toolkit in bug 294759.  blocking that bug.

> Which is the answer to some bugs.
I'm personally wondering as I've just looked at this code for the first time today and it might be helpful: which ones?
Also of course please mention any related bugs which have been filed.

I don't have time to look much more at this now but I'll provide links if necessary tonight.

I wanted to move this to toolkit as that's where the code is but can't find a relevant component and the bug blocking this was done in the current component. 

Comment 2

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

Camille, can you attach a patch?

Comment 3

[CamilleBlardone]

Promise to be more precise about file/line. (chrome/toolkit.jar) (toolkit/content/global/contentAreaUtils.js - line 201)

I'll post a patch soon and try to fix this.

Comment 4

[CamilleBlardone]

Attachment: (Av1-TK) a Path that correct the error in saveDocument in contentAreaUtils.js

The only thing it chances, is that its adds "null," just before ", aSkipPrompt);" in

  internalSave(aDocument.location.href, aDocument, null, contentDisposition,
               aDocument.contentType, false, null, null, null, aSkipPrompt);

line 202

The consequence is that if aSkipPrompt happens to be "true" then the save as file dialog won't appear if a default path is defined for download in firefox preferences.

Comment 5

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

Thanks Camille!

That appears to be the full file; it would be better if you could attach a patch. You can read http://developer.mozilla.org/en/docs/Creating_a_patch for details on doing that from CVS (preferred way), or you could also just use diff -u (not optimal, but easier).

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

We totally want this for Firefox3, I assert.

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a-TK) Add |null| value as |aReferrer| parameter

Is there a "referrer URI object" to get/use instead of |null| ?

FTR: Av1-TK was a modified MacOSX v1.8 branch packaged file.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 288201 [details] [diff] [review]
(Av1a-TK) Add |null| value as |aReferrer| parameter

We should pass aDocument.referrer as a uri object, I think.

Something like |aDocument.referrer ? IO.newURI(aDocument.referrer) : null|.

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Av2-TK) Adds |aDocument.referrer| value as |aReferrer| parameter

Av1a-TK, with comment 8 update.
(untested)

Comment 10

[Mike Connor [:mconnor]]

Yeah, we want this ASAP.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 288390 [details] [diff] [review]
(Av2-TK) Adds |aDocument.referrer| value as |aReferrer| parameter

r=mano

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-SM) Adds |aDocument.referrer| value as |aReferrer| parameter

Same fix as Av2-TK.

Comment 13

[[e-mail]]

Comment on attachment 289851 [details] [diff] [review]
(Bv1-SM) Adds |aDocument.referrer| value as |aReferrer| parameter

>+               aDocument.referrer
>+                 ? makeURI(aDocument.referrer, getCharsetforSave(aDocument))
>+                 : null
Nit: I think aDocument.referrer is always ASCII, so you don't need a charset
Nit: use && instead of ? : null

Comment 14

[Reed Loden [:reed]]

It would be nice to keep these in sync, so if Neil has problems with the patch, address his comments for both SeaMonkey and Toolkit. :)

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Adds |aDocument.referrer| value as |aReferrer| parameter

Merges Av2-TK and Bv1-SM,
with comment 13 update.

Comment 16

[[e-mail]]

Comment on attachment 289948 [details] [diff] [review]
(Cv1) Adds |aDocument.referrer| value as |aReferrer| parameter

So, I was 50% right - document.referrer seems to be escaped UTF8 but you can't use &&, it has to be ? : null or whatever you had before, I forget.

Comment 17

[Asaf Romano (gone)]

Comment on attachment 289948 [details] [diff] [review]
(Cv1) Adds |aDocument.referrer| value as |aReferrer| parameter

gah

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to comment #16)
> (From update of attachment 289948 [details] [diff] [review])

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b2pre) Gecko/2007112602 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

> So, I was 50% right - document.referrer seems to be escaped UTF8 but you can't

I had a look at
<http://mxr.mozilla.org/seamonkey/source/netwerk/base/public/nsIProtocolHandler.idl#67>
but I don't know how to check this.

With |aDocument.referrer = "http://www.yahoo.co.jp/"|, |getCharsetforSave(aDocument)| returns |"EUC-JP"| :-|

> use &&, it has to be ? : null or whatever you had before, I forget.

Right ... because when there is no actual referrer |aDocument.referrer| is an empty sting and not the |null| value (which we want to use) :-|

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Cv2) Adds |aDocument.referrer| value as |aReferrer| parameter [Checkin: Comment 20]

Cv1, with comment 16 update.

Comment 20

[Reed Loden [:reed]]

Checking in toolkit/content/contentAreaUtils.js;
/cvsroot/mozilla/toolkit/content/contentAreaUtils.js,v  <--  contentAreaUtils.js
new revision: 1.99; previous revision: 1.98
done
Checking in suite/common/contentAreaUtils.js;
/cvsroot/mozilla/suite/common/contentAreaUtils.js,v  <--  contentAreaUtils.js
new revision: 1.149; previous revision: 1.148
done

Comment 21

[Serge Gautherie (:sgautherie)]

Camille, thanks for your detailed report !