Closed Bug 321517 Opened 19 years ago Closed 17 years ago

A text file in a frame is not properly saved

Categories

(Core Graveyard :: File Handling, defect)

Product:
Core Graveyard
Component:
File Handling
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: BijuMailList, Assigned: sciguyryan)

References

Details

Attachments

(5 files, 6 obsolete files)

aa4.html
217 bytes, text/html
Details
a_textfile.txt
74 bytes, text/plain
Details
aa5.html test case with a_textfile.txt
171 bytes, text/html
Details
[checked in] UI Patch v2
4.54 KB, patch
neil
: review+
bzbarsky
: superreview+
bzbarsky
: approval1.9+
Details | Diff | Splinter Review
[checked in] Patch v3.3
9.83 KB, patch
Biesinger
: review+
Biesinger
: superreview+
bzbarsky
: approval1.9+
Details | Diff | Splinter Review 
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
Attached file aa4.html — 
Summary: A text file in a fame is not properly saved → A text file in a frame is not properly saved

    
    

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

iframes
Status: UNCONFIRMED → NEW
Ever confirmed: true

    
    

    
  
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>

    
    

    
  
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"> 




    
    

    
  

      
      
      
      

        Attached file
          
        a_textfile.txt
        — 
      

    


  

    
    

    
  


  

    
    

    
  
Assigning to me while I look at this.
Assignee: file-handling → sciguyryan

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v1 (obsolete)
        — 
          — Splinter Review
      

    


  
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)

    
  
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?


    
    

    
  
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)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2.0 (obsolete)
        — 
          — Splinter Review
      

    


  
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....

    
    

    
  
(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?

    
  

        Attachment #256177 -
        Flags: superreview?(bzbarsky)

        Attachment #256177 -
        Flags: review?(bzbarsky)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Backend Patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  
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)

    
    

    
  


  
UI Patch v2

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

    
    

    
  
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 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+

    
    

    
  
(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-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Backend patch v3.0 (obsolete)
        — 
          — Splinter Review
      

    


  
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-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v3.1 (obsolete)
        — 
          — Splinter Review
      

    


  
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?

    
    

    
  
(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.

    
  

        Attachment #274150 -
        Flags: superreview?(cbiesinger)

        Attachment #274150 -
        Flags: review?(cbiesinger)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v3.2 (obsolete)
        — 
          — Splinter Review
      

    


  
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-

    
    

    
  


  
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+

    
  

        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+

    
    

    
  
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

    
    

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

    
    

    
  
(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?

    
    

    
  
OK, you are right. But the UI patch contains what looks like SeaMonkey stuff to me and they require sr...

    
    

    
  
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 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

    
    

    
  
So now it's fixed, right?
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED

    
  
Blocks: 475621
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: