Make it easier for apps to hook into toolkit's printing component

RESOLVED FIXED

Status

()

Toolkit
Printing
--
enhancement
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Ian Neal, Assigned: Ian Neal)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

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

No it shouldn't.

(Assignee)

Comment 3

12 years ago
Created attachment 207464 [details] [diff] [review]
First pass patch v0.1

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
Attachment #207464 - Flags: second-review?(mconnor)
Attachment #207464 - Flags: first-review?(mscott)
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Attachment #207464 - Flags: second-review?(mconnor)
Attachment #207464 - Flags: first-review?(mscott)
Attachment #207464 - Flags: first-review?(mconnor)
(Assignee)

Updated

12 years ago
Attachment #207464 - Flags: first-review?(mconnor)
(Assignee)

Comment 4

12 years ago
Created attachment 214037 [details] [diff] [review]
Unbitrotted patch v0.1a

Changes since v0.1:
* Unbitrotted wrt browser.js and viewSource.js
Attachment #207464 - Attachment is obsolete: true
Attachment #214037 - Flags: first-review?
(Assignee)

Updated

12 years ago
Attachment #214037 - Flags: first-review? → first-review?(mconnor)
(Assignee)

Updated

11 years ago
Attachment #214037 - Flags: first-review?(mconnor)
(Assignee)

Comment 5

11 years ago
Created attachment 236648 [details] [diff] [review]
2nd unbitrotted patch v0.1b

Changes since v0.1a:
* Unbitrotted against current trunk
Attachment #214037 - Attachment is obsolete: true
Attachment #236648 - Flags: first-review?
(Assignee)

Updated

11 years ago
Attachment #236648 - Flags: first-review? → first-review?(mconnor)
(Assignee)

Updated

11 years ago
Attachment #236648 - Flags: first-review?(mconnor) → first-review?(gavin.sharp)
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.
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.
Attachment #236648 - Flags: first-review?(gavin.sharp) → first-review-
Created attachment 272378 [details] [diff] [review]
The minimum, v1.0

> 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.
Attachment #236648 - Attachment is obsolete: true
Attachment #272378 - Flags: review?(gavin.sharp)
Blocks: 387247
(Assignee)

Comment 9

10 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?
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?
Attachment #272378 - Flags: review?(gavin.sharp) → review+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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.

Updated

10 years ago
Blocks: 270235
(In reply to comment #12)
> followup bugs

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