228529 - JS Warnings in printing/print preview

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[timeless]

Warning ``assignment to undeclared variable gPrintSettingsAreGlobal'' [xs] in
file ``chrome://communicator/content/printing.js'', line 65, character 0.
Stopped for error handler.
#0: function GetPrintSettings() in <chrome://communicator/content/printing.js>
line 65
063: .getService(Components.interfaces.nsIPrefBranch);
064: if (pref) {
065: gPrintSettingsAreGlobal =
pref.getBoolPref("print.use_global_printsettings", false);
066: gSavePrintSettings = pref.getBoolPref("print.save_print_settings", false);
067: }
Continuing from error handler.
Warning ``assignment to undeclared variable gSavePrintSettings'' [xs] in file
``chrome://communicator/content/printing.js'', line 66, character 0.
Stopped for error handler.
#0: function GetPrintSettings() in <chrome://communicator/content/printing.js>
line 66
064: if (pref) {
065: gPrintSettingsAreGlobal =
pref.getBoolPref("print.use_global_printsettings", false);
066: gSavePrintSettings = pref.getBoolPref("print.save_print_settings", false);
067: }
068:

Warning ``redeclaration of var str'' [xs] in file
``chrome://global/content/printPreviewProgress.js'', line 52, character 10.

Warning ``assignment to undeclared variable gWebProgress'' [xs] in file
``chrome://messenger/content/msgPrintEngine.js'', line 149, character 0.
Stopped for error handler.
#0: function PrintPreview() in <chrome://messenger/content/msgPrintEngine.js>
line 149
147: //
148: // Doing it all from script, means it lays out before hand and we can let
printing do it's own thing
149: gWebProgress = new Object();
150:
151: var printPreviewParams = new Object();

Warning ``redeclaration of var total'' [xs] in file
``chrome://communicator/content/printPreviewBindings.xml'', line 293, character 16.
Warning ``redeclaration of var ifreq'' [xs] in file
``chrome://communicator/content/printPreviewBindings.xml'', line 402, character 16.

Comment 1

[timeless]

Attachment: (Av1) declare global, remove duplicate decls, ...

Comment 2

[timeless]

Attachment: (Av1b) declare globals, remove duplicate decls, ... [Checked in: Comment 6]

Comment 3

[Olivier Cahagne]

*** Bug 160939 has been marked as a duplicate of this bug. ***

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 137447 [details] [diff] [review]
(Av1b) declare globals, remove duplicate decls, ...
[Checked in: Comment 6]

sr=bzbarsky

Comment 5

[[e-mail]]

Comment on attachment 137447 [details] [diff] [review]
(Av1b) declare globals, remove duplicate decls, ...
[Checked in: Comment 6]

>             var total = print.printPreviewNumPages;
> 
>             // bounds check potentially user-entered number
>             if (newPageNum > 0 && newPageNum <= total) 
>             {
>               this.mPageTextBox.value = newPageNum;
>               print.printPreviewNavigate(
>                 print.PRINTPREVIEW_GOTO_PAGENUM, newPageNum);
>               validInput = true;
>             }
>           }
>           else 
>           {
>-            var total = print.printPreviewNumPages;
>+            total = print.printPreviewNumPages;
I see you didn't bother to lexically scope this one :-P

>+var gPrintSettingsAreGlobal;
>+var gSavePrintSettings;
Explicit initialization to false would be nice.

>Index: xpfe/global/resources/content/printPreviewProgress.js
You don't need this now :-)

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 137447 [details] [diff] [review]
(Av1b) declare globals, remove duplicate decls, ...
[Checked in: Comment 6]


Check in: { 12/20/2003 20:04	timeless%mozdev.org }

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) additional |var ...| cleanup

As suggested by comment 5, and a little more.

{
>Index: xpfe/global/resources/content/printPreviewProgress.js
You don't need this now :-)
}
Checked in in comment 6 anyway:
Should anything else/more be done about it ??

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 137792 [details] [diff] [review]
(Bv1) additional |var ...| cleanup


'r=?': (see comment 7)

Comment 9

[timeless]

Comment on attachment 137792 [details] [diff] [review]
(Bv1) additional |var ...| cleanup

Since ifreq isn't used beyond the next line, skip the variable and just do
.QueryInterface(IFR).QueryInterface(IDOMWindow)

Comment 10

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1b) additional |var ...| cleanup

Bv1, plus comment 9 suggestion.

Comment 11

[[e-mail]]

Comment on attachment 137800 [details] [diff] [review]
(Bv1b) additional |var ...| cleanup

As "total" is only used at most once you could replace each reference with
print.printPreviewNumPages instead.

>-          var ifreq;
>-          var webBrowserPrint;  
>           var domWin;  
>           try {
>-            ifreq = _content.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
>-            domWin = ifreq.QueryInterface(Components.interfaces.nsIDOMWindow);
>+            domWin = _content.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                             .QueryInterface(Components.interfaces.nsIDOMWindow);
>           } catch (e) {
Bah, how did I not spot this before... these QueryInterface calls have no
useful effect, the entire code sequence is for all print preview purposes
equivalent to var domWin = content; i.e you can remove this code and replace
the two uses of domWin with content.

Oh, and I'm not too happy with your use of const, at least not for user input
variables; the one I'm vaguely OK with is the print preview settings one but
only because no matter how many times you call that method you'll probably get
a similar object reference.

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1c) additional |var ...| cleanup

Bv1b, with comment 11 suggestions.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I may not get to this any time soon.

Comment 14

[Alec Flett]

has this code been compiled and tested? Please don't waste reviewers time with
untested patches.

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2) additional |var ...| cleanup

Bv1c, with more cleanup.

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 138033 [details] [diff] [review]
(Bv2) additional |var ...| cleanup


(alec)
Re comment 14:
Compile: JavaScript only, no compilation !?
Test: I successfully did some simple tests on the modified parts. (and I had
'neil: r+')
Please, explain me what I can do better. Thanks.

(neil)
'r=?': (see comment 15)
This is the "full" job, after your previous r+, as I had a closer look at it...

Comment 17

[[e-mail]]

Comment on attachment 138033 [details] [diff] [review]
(Bv2) additional |var ...| cleanup

>+          var confirmed = Components
>+                .classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                .getService(Components.interfaces.nsIPromptService)
>+                .prompt(window,
>+                        this.mCustomTitle,
>+                        this.mScaleLabel.value,
>+                        result,
>+                        null,
>+                        {value:aValue});
This is just getting silly.

>             if (!settings.shrinkToFit) {
>               settings.shrinkToFit = true;
>               this.doPrintPreview(print, settings, this.mTotalPages, this.mScaleCombobox);
>-              return;
>-            } else {
>-              return;
>             }
>+            return;
This was the only useful change from the last patch.

Comment 18

[Serge Gautherie (:sgautherie)]

Attachment: (Bv3) additional |var ...| cleanup

Bv2, revised a little.

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 138041 [details] [diff] [review]
(Bv3) additional |var ...| cleanup


Re comment 17:

I compacted the |.prompt()| line(s) of |confirmed|,
and (un)did 4 other minor changes.


For the record, there are 2 other actual code fixes included in Bv2 (and Bv3):

-  var prevPS = gPrintSettings;

-    return true;
   } catch(e) {
     return false; 
   }
   return true;

Besides these, the idea was/is to remove as many "used once" |var| as possible,
based on what you wrote about |total| in comment 11...

Comment 20

[[e-mail]]

Comment on attachment 138041 [details] [diff] [review]
(Bv3) additional |var ...| cleanup

That unused variable and the other fix are fine, thanks for pointing that out.
However, you should not try to do too much in one statement. Ideally statements
should fit on one line, except calls to methods with a large number of
arguments, which should be in their own statement, or well-known patterns such
as creating a component or getting a service.

Comment 21

[Serge Gautherie (:sgautherie)]

Attachment: (Bv4) additional |var ...| cleanup [Checked in: Comment 23]

Bv3, with 3 '*Service' |var| restored.

Comment 22

[Alec Flett]

Comment on attachment 138059 [details] [diff] [review]
(Bv4) additional |var ...| cleanup
[Checked in: Comment 23]

sr=alecf

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 138059 [details] [diff] [review]
(Bv4) additional |var ...| cleanup
[Checked in: Comment 23]


Check in: { 01/18/2004 14:50	neil%parkwaycc.co.uk }