Ian Neal Assignee
	Description • 19 years ago

At the moment printUtils.js is very firefox-centric making it difficult for other apps to easily hook into it. I'll be looking at trying to make it a bit easier.

	Worcester12345
	Comment 1 • 19 years ago

This bug should be marked to block 125824.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 2 • 19 years ago

(In reply to comment #1)
> This bug should be marked to block 125824.

No it shouldn't.

	Ian Neal Assignee
	Comment 3 • 19 years ago

This patch:
* Revises printUtils.js so duplicate code can be removed out of other files and provides hooks back into those files for non-duplicated / app-specific code
* Moves browser specific code out of printUtils.js into browser.js
* Tweaks browser.js to use revised printUtils.js
* Tweaks viewSource.js to use revised printUtils.js
* Hides findbar in viewsource printpreview mode
* Removes duplicate code out of calPrintEngine.js and fixes issues with printpreview from SunBird
* Some general tidying up of typos and making some variable names clearer

Once this has landed, other products (e.g. ThunderBird, Composer, Suite) could be tweaked to use revised printUtils.js

	Ian Neal Assignee
	Comment 4 • 19 years ago

Changes since v0.1:
* Unbitrotted wrt browser.js and viewSource.js

	Ian Neal Assignee
	Comment 5 • 18 years ago

Changes since v0.1a:
* Unbitrotted against current trunk

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 6 • 18 years ago

Unfortunately this is bitrotten again due to bug 185239, and that patch's addition of window arguments to most of these functions might mean reconsidering some of these changes. A few initial comments, though:

General:
1) Relying on functions like getPPBrowser() and getEngineWebBrowserPrint() being defined in the global scope seems kinda fragile. getEngineWebBrowserPrint() is unnecessary now that these methods take a window param, but I'm not really sure what to do about getNavToolbox() 

Browser.js:
2) There are a few other places in browser.js that could use getNavToolbox().
3) The |"getStripVisibility" in gBrowser| checks aren't needed (gBrowser will always be a <tabbrowser>).
4) The findbar-related changes are rotten due to bug 288254 :( (also applies to viewSource.js)

printPreviewBindings.xml:
4) '!result.value || result.value == ""' should be just "!result.value".
5) printService is really the printSettingsService, right? (this change is made in other files too)

printUtils.js:
6) Can you explain why you changed from hiding the toolbar to collapsing one of it's ancestors? At the very least, the "it will be shown in onEnterPrintPreview" part seems wrong now.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 7 • 18 years ago

Comment on attachment 236648 [details] [diff] [review]
2nd unbitrotted patch v0.1b

Since this no longer applies, I'm going to r- to get this out of my queue until it's been updated to address the questions in comment 6.

	Phil Ringnalda (:philor)
	Comment 8 • 17 years ago

> functions ... in the global scope seems kinda fragile

Fragile in some way other than an extension being psychotic enough to define a function like getPPBrowser(), in which case they'll break print preview, and need to stop doing that, just like the hundreds of other function names in browser.js? I guess we could require passing in arguments to the functions, if there's more reason I'm not seeing.

> changed from hiding the toolbar to collapsing

With the current hack of hiding the toolbar, when you change from Portrait to Landscape, the toolbar at the top of the window disappears, while the preview in Portrait orientation stays visible until it's regenerated, and then the toolbar pops back into view with the new preview - it looks weird. Collapsing the browser leaves the toolbar in place, but blanks out the window - it's more like loading a web page (where the current way would be like clicking a link, having the current page remain visible, but the browser chrome become invisible until it loads).

I tried six different approaches to changing the XPCOM variables to something that looks less like a constant, while still being accurate and clear and under 80 characters, until I finally realized that I had spent more time on it than everyone else combined will ever spend trying to follow it. They're ugly, but it's all just boilerplate.

I dropped the onTweakPrintSettings from IanN's patch, because it seems to be in the wrong place (called in enterPrintPreview, but doing what calendar currently does in its FinishPrintPreview), and doesn't seem like it ought to be needed - they do something entering print preview, and something else leaving print preview, which sounds like a job for aEnterPPCallback and aExitPPCallback.

	Ian Neal Assignee
	Comment 9 • 17 years ago

Comment on attachment 272378 [details] [diff] [review]
The minimum, v1.0

>Index: toolkit/components/printing/content/printUtils.js
>===================================================================
>     // remove the print preview toolbar
>+    var navToolbox = getNavToolbox();
>     var printPreviewTB = document.getElementById("print-preview-toolbar");
>-    getBrowser().parentNode.removeChild(printPreviewTB);
>+    navToolbox.parentNode.removeChild(printPreviewTB);

As the code has changed, you could probably just do:
>-    getBrowser().parentNode.removeChild(printPreviewTB);
>+    getNavToolbox().parentNode.removeChild(printPreviewTB);

You might still need:
     QueryInterface : function(iid)
     {
-      if (iid.equals(Components.interfaces.nsIObserver) || iid.equals(Components.interfaces.nsISupportsWeakReference))
+      if (iid.equals(Components.interfaces.nsIObserver) ||
+          iid.equals(Components.interfaces.nsISupportsWeakReference) ||
+          iid.equals(Components.interfaces.nsISupports))
         return this;   
       throw Components.results.NS_NOINTERFACE;
     }
in there.

Is there a separate bug for tidying up calendar once this patch is done with?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 10 • 17 years ago

Comment on attachment 272378 [details] [diff] [review]
The minimum, v1.0

>Index: browser/base/content/browser.js

>+function getPPBrowser()
>+{
>+  return document.getElementById("content");

Return gBrowser (or getBrowser()) directly?

>Index: toolkit/components/printing/content/printUtils.js

>-      // hide the toolbar here -- it will be shown in
>+      // collapse the browser here -- it will be shown in
>       // onEnterPrintPreview; this forces a reflow which fixes display
>       // issues in bug 267422.
>-      pptoolbar.hidden = true;
>+      var browser = getPPBrowser();
>+      if (browser)
>+        browser.collapsed = true;

Why is this change needed?

	Phil Ringnalda (:philor)
	Comment 11 • 17 years ago

browser/base/content/browser.js 1.825
toolkit/components/printing/content/printUtils.js 1.28
toolkit/components/viewsource/content/viewSource.js 1.20

and then once I realized I'd just checked in without addressing gavin's review comment (the getBrowser(), not the "why the change," which I'd already pointed out was already in comment 8),

browser/base/content/browser.js 1.826

	Phil Ringnalda (:philor)
	Comment 12 • 17 years ago

GAH!

Um, comment 9 followup bugs as soon as I get a minute. I can't believe I attached a patch to a bug without even cc:ing myself.

	Phil Ringnalda (:philor)
	Comment 13 • 17 years ago

(In reply to comment #12)
> followup bugs

Filed bug 393742 and bug 393747