312084 - Printing shouldn't use a tempfile

Status: VERIFIED FIXED

(Calendar :: Printing, defect)

Description

[Joey Minta]

followup from bug 303951.

Comment 1

[Matthew (lilmatt) Willis]

Reassigning all automatically assigned bugs from Mostafa to nobody@m.o

Bugspam filter: TorontoMostafaMove

Comment 2

[Daniel Boelzle [:dbo]]

generating html without temp file works, but the innerHTML stuff doesn't:

            var html;
            try {
                var pipe = Components.classes["@mozilla.org/pipe;1"]
                                     .createInstance(Components.interfaces.nsIPipe);
                pipe.init(true, true, 0, 0, null);
                printformatter.formatToHtml(pipe.outputStream,
                                            settings.start,
                                            settings.end,
                                            settings.eventList.length,
                                            settings.eventList,
                                            settings.title);
                pipe.outputStream.close();
                var scriptableInputStream = Components.classes["@mozilla.org/scriptableinputstream;1"]
                                                      .createInstance(Components.interfaces.nsIScriptableInputStream);
                scriptableInputStream.init(pipe.inputStream);
                html = scriptableInputStream.read(-1);
            } catch (e) {
                html = ("printDialog::refreshHtml:" + e + "\n");
                dump(html);
            }

            document.getElementById('content').contentWindow.innerHTML = html;

I have varied a couple of things, but didn't get innerHTML to actually show something in the iframe. Joey?

Comment 3

[Daniel Boelzle [:dbo]]

document.getElementById('content')
        .contentDocument.documentElement.innerHTML = html;
works.

Comment 4

[Daniel Boelzle [:dbo]]

Attachment: no temp file use

This patch is built on top of patch 249253 (from bug 351957).

Comment 5

[Stefan Sitter [:ssitter]]

I did a quick test with this patch and the patch from Bug 351957 using one storage and six ics calendars (on local disk):

- Clicking Print button forces high CPU usage for some seconds but nothing happens afterwards, neither does the preview dialog closes nor does the printer selection dialog shows up.

- In preview the text is not properly UTF-8 encoded, e.g. 'ö' shows up as 'ö'.

- Compared to current solution the preview update seems about two times slower and runs very often even if nothing changed in settings (only focus changed)

Maybe we need some kind of caching for the settings so that the html will only be rebuild if the settings changed? But I guess this might be a followup bug.

Comment 6

[Daniel Boelzle [:dbo]]

(In reply to comment #5)
> - Clicking Print button forces high CPU usage for some seconds but nothing
> happens afterwards, neither does the preview dialog closes nor does the printer
> selection dialog shows up.
I will have a look at that. What I already found out is that listening for onLoad as done in the mentioned patch doesn't work anymore. Setting innerHTML doesn't fire that. I am currently working out a different notification. I need to rework the patch.

> - In preview the text is not properly UTF-8 encoded, e.g. 'ö' shows up as
> 'ö'.
I will have a look at that.

Comment 7

[Daniel Boelzle [:dbo]]

(In reply to comment #5)
> - In preview the text is not properly UTF-8 encoded, e.g. 'ö' shows up as
> 'ö'.

Problem is that the print formatter doesn't encode umlauts propertly, e.g. ö -> "ö". This is already wrong in the current temp file based version. However when loading from html file, the control seems to be more tolerant.

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

The print formatter doesn't have to encode, as long as it states that the file is utf-8, by adding the header. afaik, you can use utf-8 chars in html without encoding them.
I think the control ignores the charset meta-header when you just set the html string. Is there a different way to set the encoding? There likely is, because firefox has a menu to do it.

Comment 9

[Daniel Boelzle [:dbo]]

Attachment: no temp file use, v2

- using the stream converter service now
- again, this patch is built on top of patch 249253 (from bug 351957)

Comment 10

[Matthew (lilmatt) Willis]

mvl's comment regarding UTF-8 is correct.  If the document is specified as being in UTF-8, you should be able to put characters in without using entities, the exceptions of course being & < and >. 

>-                stream.close();
>+                pipe.outputStream.close();
>+                // byte-array to utf8 string:
>+                var convStream =
>+                    Components.classes["@mozilla.org/intl/converter-input-stream;1"]
>+                              .createInstance(Components.interfaces.nsIConverterInputStream);
>+                convStream.init(pipe.inputStream, "UTF-8", 0, 63/* i.e. '?' */);
>+                var portion = {};
>+                while (convStream.readString(-1, portion)) {
>+                    html += portion.value;
>+                }
Could you explain what's going on here in more detail?  I'm not real clear on the inline comment.


>             } catch (e) {
>-                dump("printDialog::refreshHtml:" + e + "\n");
>+                html = ("printDialog::refreshHtml:" + e + "\n");
>+                dump(html);
Why are we changing the dump contents?

Comment 11

[Daniel Boelzle [:dbo]]

(In reply to comment #10)
> mvl's comment regarding UTF-8 is correct.  If the document is specified as
> being in UTF-8, you should be able to put characters in without using entities,
> the exceptions of course being & < and >. 
already fixed that in this patch, v2.

> >+                pipe.outputStream.close();
> >+                // byte-array to utf8 string:
> >+                var convStream =
> >+                    Components.classes["@mozilla.org/intl/converter-input-stream;1"]
> >+                              .createInstance(Components.interfaces.nsIConverterInputStream);
> >+                convStream.init(pipe.inputStream, "UTF-8", 0, 63/* i.e. '?' */);
> >+                var portion = {};
> >+                while (convStream.readString(-1, portion)) {
> >+                    html += portion.value;
> >+                }
> Could you explain what's going on here in more detail?  I'm not real clear on
> the inline comment.

1) using a pipe pushing in the data from the html formatter on one end (it's output stream)
2) then reading out the data from the other end (input stream) into a stream converter
3) converting that UTF-8 byte stream into a string for innerHTML.

> >             } catch (e) {
> >-                dump("printDialog::refreshHtml:" + e + "\n");
> >+                html = ("printDialog::refreshHtml:" + e + "\n");
> >+                dump(html);
> Why are we changing the dump contents?
I have set html in case of errors to dump them into the preview frame, too; IMO better than currently swallowing all errors silently away (in production use). Alternatively an error box would be appropriate. But I think this is a different bug/RFE.

Comment 12

[Matthew (lilmatt) Willis]

What's the 63/* i.e. '?' */ bit for?

Comment 13

[Stefan Sitter [:ssitter]]

Comment on attachment 252041 [details] [diff] [review]
no temp file use, v2

>+      // byte-array to utf8 string:
>+      var convStream =
>+          Components.classes["@mozilla.org/intl/converter-input-stream;1"]
>+                    .createInstance(Components.interfaces.nsIConverterInputStream);
>+      convStream.init(pipe.inputStream, "UTF-8", 0, 63/* i.e. '?' */);

Should this be Components.interfaces.nsIConverterInputStream.DEFAULT_REPLACEMENT_CHARACTER instead?

>+      var portion = {};
>+      while (convStream.readString(-1, portion)) {
>+          html += portion.value;
>+      }
>   } catch (e) {

Shouldn't there be a convStream.close(); ?

Comment 14

[Daniel Boelzle [:dbo]]

(In reply to comment #13)
> >+      convStream.init(pipe.inputStream, "UTF-8", 0, 63/* i.e. '?' */);
> 
> Should this be
> Components.interfaces.nsIConverterInputStream.DEFAULT_REPLACEMENT_CHARACTER
> instead?

Yes, the official unicode replacement character is better.

> >+      var portion = {};
> >+      while (convStream.readString(-1, portion)) {
> >+          html += portion.value;
> >+      }
> >   } catch (e) {
> 
> Shouldn't there be a convStream.close(); ?

Yes, there should be (though not strongly necessary).

Comment 15

[Matthew (lilmatt) Willis]

Comment on attachment 252041 [details] [diff] [review]
no temp file use, v2

> function unloadCalendarPrintDialog()
> {
>-    gTempFile.remove(false);
> }
If we no longer need to call this function, let's remove it altogether, and from the XUL.

>-            var stream = Components.classes["@mozilla.org/network/file-output-stream;1"]
>-                                   .createInstance(Components.interfaces.nsIFileOutputStream);
>-
>+            
This line has trailing spaces ^^^

>+                // byte-array to utf8 string:
Please use "UTF-8" in the comment.

>+                var convStream =
>+                    Components.classes["@mozilla.org/intl/converter-input-stream;1"]
>+                              .createInstance(Components.interfaces.nsIConverterInputStream);
>+                convStream.init(pipe.inputStream, "UTF-8", 0, 63/* i.e. '?' */);
Please use Components.interfaces.nsIConverterInputStream.DEFAULT_REPLACEMENT_CHARACTER instead of 63.

>+                var portion = {};
>+                while (convStream.readString(-1, portion)) {
>+                    html += portion.value;
>+                }
Please close the stream

>             } catch (e) {
>-                dump("printDialog::refreshHtml:" + e + "\n");
>+                html = ("printDialog::refreshHtml:" + e + "\n");
>+                dump(html);
>             }
I'd prefer that we throw() or LOG() the error information, rather than displaying it in the preview box.

>-
>-            document.getElementById("content").contentWindow.location = tempUri.spec;
>+            document.getElementById('content').contentDocument.documentElement.innerHTML = html;
>+            if (finishFunc)
>+                finishFunc();
Add curly braces around this

I'd like to see one more iteration on this patch.  r-

Comment 16

[Daniel Boelzle [:dbo]]

(In reply to comment #15)
> >             } catch (e) {
> >-                dump("printDialog::refreshHtml:" + e + "\n");
> >+                html = ("printDialog::refreshHtml:" + e + "\n");
> >+                dump(html);
> >             }
> I'd prefer that we throw() or LOG() the error information, rather than
> displaying it in the preview box.

Then we could either remove the whole try/catch block, don't we?

Comment 17

[Daniel Boelzle [:dbo]]

Comment on attachment 252041 [details] [diff] [review]
no temp file use, v2

merging printing fixes: temp file fix into fix for bug 351957.

Comment 18

[Daniel Boelzle [:dbo]]

checked in with fix for bug 351957.

Comment 19

[Daniel Boelzle [:dbo]]

verified no temp file in use anymore