A text file in a frame is not properly saved

RESOLVED FIXED

Status

RESOLVED FIXED
13 years ago
2 years ago

People

(Reporter: BijuMailList, Assigned: sciguyryan)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 6 obsolete attachments)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051219 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051219 Firefox/1.6a1

If the one of the frame content in a <frameset> or <iframe> is a text file while doing File > Save Page As ... <frame> content is saved with extension *.txt but the content in the file as html!!! 

ie, in the attached example show

<html><head></head><body><pre>bla bla blah</pre></body></html>


Reproducible: Always

Steps to Reproduce:
1. open aa4.html attachment.
2. save it localy by File > Save Page As ..
3. go to the directory 
4. you will see a *.txt file 
5. open it you will see 
<html><head></head><body><pre>bla bla blah</pre></body></html>




Actual Results:  
<html><head></head><body><pre>bla bla blah</pre></body></html>



Expected Results:  
bla bla blah

or file name as *.html
(Reporter)

Comment 1

13 years ago
Created attachment 206827 [details]
aa4.html
Summary: A text file in a fame is not properly saved → A text file in a frame is not properly saved

Comment 2

13 years ago
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051225 SeaMonkey/1.5a

iframes
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

13 years ago
Sorry, bugtext was lost when I went back.

according to http://www.ietf.org/rfc/rfc2397
<iframe src="data:,bla bla blah"></iframe> is the same as
<iframe src="data:text/plain,bla bla blah"></iframe>

save as webpage complete saves the text according to the DOM as preformatted text in a html file. according to the MIME-type, this file is name a.txt, and this is wrong, as loading the saved frame now is showing the HTML in source-view, instead of preformatted text.

no problems if iframe is <iframe src="data:text/html,bla bla blah"></iframe>
(Reporter)

Comment 4

13 years ago
I used data url in aa4.html make creating testcase easy.
This is also a problem on an actual text file...

like <frame src="bill.txt"> and <iframe src="bill.txt"> 


(Reporter)

Comment 5

13 years ago
Created attachment 206864 [details]
a_textfile.txt
(Reporter)

Comment 6

13 years ago
Created attachment 206866 [details]
aa5.html test case with a_textfile.txt
(Assignee)

Comment 7

12 years ago
Assigning to me while I look at this.
Assignee: file-handling → sciguyryan
(Assignee)

Comment 8

12 years ago
Created attachment 255933 [details] [diff] [review]
Patch v1

Patch v1

* This fixes the problem with gif files and such in a frame/iframe also. I considered checking if a document encoder type existed for a particular content type but I decided it wouldn't work correctly an encoder exists for text/plain and so the check would still pass the document too |SaveDocumentInternal| and thus the original problem here would remain.
We may want to add more types too the |kValidDocumentTypes| list if this idea works but for an outline check the set ones pretty much cover the basics that came to mind.
Attachment #255933 - Flags: superreview?(bzbarsky)
Attachment #255933 - Flags: review?(bzbarsky)
(Assignee)

Updated

12 years ago
Blocks: 115634
Status: NEW → ASSIGNED
OS: Windows XP → All
Wait.  The text/plain encoder should do the right thing.  Why isn't it?

Note that SaveDocumentInternal depends on what mContentType is set to.  Does clearing mContentType before the SaveDocumentInternal call in SaveSubframeContent (and restoring it after that call returns) help with text/plain?
(Assignee)

Comment 10

12 years ago
Comment on attachment 255933 [details] [diff] [review]
Patch v1

Cancelling r/sr requests on this patch.
Attachment #255933 - Flags: superreview?(bzbarsky)
Attachment #255933 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

12 years ago
Created attachment 256177 [details] [diff] [review]
Patch v2.0

Patch v2

* Updated the code fix to check the presence of a document encoder. If a document encoder is present for a content type then it may be saved with |SaveDocumentInternal| otherwise we use |SaveURIInternal|. This patch also includes an update to some of the callers so they do not always pass a content type. This prevents the text files being fixed up incorrectly (tested this under toolkit from a local Firefox build).
Attachment #255933 - Attachment is obsolete: true
Attachment #256177 - Flags: superreview?(bzbarsky)
Attachment #256177 - Flags: review?(bzbarsky)
The UI changes need review from the relevant UI owners.

As for the back end, I'd really like biesi to review, since I feel that I'm not in a good position to evaluate an approach I suggested....
(Assignee)

Comment 13

12 years ago
(In reply to comment #12)
> The UI changes need review from the relevant UI owners.
> 
> As for the back end, I'd really like biesi to review, since I feel that I'm not
> in a good position to evaluate an approach I suggested....
> 

OK then in that case how about I split the code in two; one for the back end code and one for the UI related code and then request separate reviewers for each?
(Assignee)

Updated

12 years ago
Attachment #256177 - Flags: superreview?(bzbarsky)
Attachment #256177 - Flags: review?(bzbarsky)
(Assignee)

Comment 14

12 years ago
Created attachment 256187 [details] [diff] [review]
Backend Patch v2

Backend Patch v2

* The code changes too nsWebBrowserPersist.cpp and nsWebBrowserPersist.h
Attachment #256177 - Attachment is obsolete: true
Attachment #256187 - Flags: superreview?(cbiesinger)
Attachment #256187 - Flags: review?(cbiesinger)
(Assignee)

Comment 15

12 years ago
Created attachment 256188 [details] [diff] [review]
[checked in] UI Patch v2

UI Patch v2

* Changes to the front end code as a follow up too the back end changes.
(Assignee)

Comment 16

12 years ago
Comment on attachment 256188 [details] [diff] [review]
[checked in] UI Patch v2

Requesting Neil to review the changes too xpfe, suite and toolkit as the changes are identical over all three files.
Attachment #256188 - Flags: review?(neil)

Comment 17

12 years ago
Comment on attachment 256188 [details] [diff] [review]
[checked in] UI Patch v2

So, this appears to fix the test case; I assume the other patch is to deal with more complex cases e.g. text documents in nested frames?
Attachment #256188 - Flags: review?(neil) → review+
(Assignee)

Comment 18

12 years ago
(In reply to comment #17)
> (From update of attachment 256188 [details] [diff] [review])
> So, this appears to fix the test case; I assume the other patch is to deal with
> more complex cases e.g. text documents in nested frames?
> 

As far as I've tested this fix works as expected for frames and sub-frames :) This path also addresses the saving of any sub-frame types, such as images, which would previously break.

Thanks also to Josh Aas for giving a r+ on the Cocoa changes via IRC (assuming that the backend code changes are acceptable).
Comment on attachment 256187 [details] [diff] [review]
Backend Patch v2

+    // of frames that are not documents e.g. images.

missing comma before e.g.

SaveURIInternal seems like the wrong thing to call here, I think something more like StoreURI would be better here (perhaps that needs a version that takes an nsIURI)
Attachment #256187 - Flags: superreview?(cbiesinger)
Attachment #256187 - Flags: superreview-
Attachment #256187 - Flags: review?(cbiesinger)
Attachment #256187 - Flags: review-
(Assignee)

Comment 20

11 years ago
Created attachment 274000 [details] [diff] [review]
Backend patch v3.0

Backend Patch v3.0

Changes:
- Calls StoreURI in place of SaveURIInternal.
- Added a new version of StoreURI that takes a nsIURI argument.
Attachment #256187 - Attachment is obsolete: true
Attachment #274000 - Flags: superreview?(cbiesinger)
Attachment #274000 - Flags: review?(cbiesinger)
Comment on attachment 274000 [details] [diff] [review]
Backend patch v3.0

don't copy code. instead, make the other StoreURI version call this one.
Attachment #274000 - Flags: superreview?(cbiesinger)
Attachment #274000 - Flags: review?(cbiesinger)
Attachment #274000 - Flags: review-
(Assignee)

Comment 22

11 years ago
Created attachment 274150 [details] [diff] [review]
Patch v3.1

Patch v3.1

Updated to comments. Thanks for a quick review.
Attachment #274000 - Attachment is obsolete: true
Attachment #274150 - Flags: superreview?(cbiesinger)
Attachment #274150 - Flags: review?(cbiesinger)
Comment on attachment 274150 [details] [diff] [review]
Patch v3.1

+    contractID.AppendWithConversion(aContentType);

sorry, haven't noticed this before, but don't introduce new callers of AppendWithConversion.

Use:
  AppendUTF16toUTF8(aContentType, contractID);

does this patch store the file with the right filename?
(Assignee)

Comment 24

11 years ago
(In reply to comment #23)
> (From update of attachment 274150 [details] [diff] [review])
> +    contractID.AppendWithConversion(aContentType);
> 
> sorry, haven't noticed this before, but don't introduce new callers of
> AppendWithConversion.
> 
> Use:
>   AppendUTF16toUTF8(aContentType, contractID);

Fixed. Thanks for pointing it out.

> 
> does this patch store the file with the right filename?
> 

Now that the code passes the URL's to be fixed up as it would do any other URL it does yes. I've done tests with frames, multiple frames etc. and it seems to work just fine.

Updated patch coming up.
(Assignee)

Updated

11 years ago
Attachment #274150 - Flags: superreview?(cbiesinger)
Attachment #274150 - Flags: review?(cbiesinger)
(Assignee)

Comment 25

11 years ago
Created attachment 274386 [details] [diff] [review]
Patch v3.2

Patch v3.2

More nits addressed.
Attachment #274150 - Attachment is obsolete: true
Attachment #274386 - Flags: superreview?(cbiesinger)
Attachment #274386 - Flags: review?(cbiesinger)
Comment on attachment 274386 [details] [diff] [review]
Patch v3.2

you're still using AppendWithConversion...
Attachment #274386 - Flags: superreview?(cbiesinger)
Attachment #274386 - Flags: review?(cbiesinger)
Attachment #274386 - Flags: review-
(Assignee)

Comment 27

11 years ago
Created attachment 274940 [details] [diff] [review]
[checked in] Patch v3.3

Patch v3.3

Uploaded wrong patch last time. Sorry!
Attachment #274386 - Attachment is obsolete: true
Attachment #274940 - Flags: superreview?(cbiesinger)
Attachment #274940 - Flags: review?(cbiesinger)
Attachment #274940 - Flags: superreview?(cbiesinger)
Attachment #274940 - Flags: superreview+
Attachment #274940 - Flags: review?(cbiesinger)
Attachment #274940 - Flags: review+
(Assignee)

Updated

11 years ago
Attachment #274940 - Flags: approval1.9?
Comment on attachment 274940 [details] [diff] [review]
[checked in] Patch v3.3

a=bzbarsky
Attachment #274940 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed

Comment 29

11 years ago
Comment on attachment 274940 [details] [diff] [review]
[checked in] Patch v3.3

Checked this patch into trunk.
Attachment #274940 - Attachment description: Patch v3.3 → [checked in] Patch v3.3

Comment 30

11 years ago
The UI-patch needs some sr or at least approval for 1.9, so I didn't check that in yet...
Keywords: checkin-needed
(Assignee)

Comment 31

11 years ago
(In reply to comment #30)
> The UI-patch needs some sr or at least approval for 1.9, so I didn't check that
> in yet...
> 

As I understand it UI stuff doesn't require 1.9 approval yet does it?

Comment 32

11 years ago
OK, you are right. But the UI patch contains what looks like SeaMonkey stuff to me and they require sr...
(Assignee)

Comment 33

11 years ago
BZ: Do you by any chance happen to know if the included UI stuff here needs a sr or is it fine to checkin as it is?
Comment on attachment 256188 [details] [diff] [review]
[checked in] UI Patch v2

I have no clue.  sr=bzbarsky, and a= for good measure.  ;)
Attachment #256188 - Flags: superreview+
Attachment #256188 - Flags: approval1.9+

Comment 35

11 years ago
Comment on attachment 256188 [details] [diff] [review]
[checked in] UI Patch v2

OK, I'm convinced by bz's power. :-) Checked this into trunk, too.
Attachment #256188 - Attachment description: UI Patch v2 → [checked in] UI Patch v2

Comment 36

11 years ago
So now it's fixed, right?
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

10 years ago
Blocks: 475621
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.