650960 - Replace Print Progress dialogs with events fired at the tab + doorhangers

Status: RESOLVED WORKSFORME

(Toolkit :: Printing, enhancement)

Description

[Zack Weinberg (:zwol)]

Bug 629500 expunges the old XUL print and page setup dialogs but leaves us with XUL, app-modal print progress dialog boxes.  The UX team is of the opinion that we don't need to show print progress at all.

Comment 1

[Zack Weinberg (:zwol)]

It occurs to me now that I don't know whether printing a large document freezes the UI in general or even just the tab being printed.  If it does, we oughta keep the progress bars until that can be fixed.

Comment 2

[Zack Weinberg (:zwol)]

We discussed this a bit on dev.platform a couple months ago and I did some experiments.  I now think there _should_ be some indication that Firefox's piece of the print process is not yet finished, on a per-tab basis, if only so that users don't get confused when they can't close tabs being printed.  However, I still want to kill the app-modal print progress boxes, because app-modal is never the Right Thing nowadays.

I think the right way to do this is to have the print engine fire chrome events at the tab being printed at approximately the same times it currently updates the existing print progress boxes (i.e. roughly after each page).  This gets the notifications into JS land, where we can use doorhangers.

The doorhanger I have in mind is just a printer icon by default, but if you click on it it tells you how many pages are left to go, and it pops up a full notification "Page sent to printer" (or something like that) when we're all done.  Or if there's a problem, that gets reported as a notification too, eliminating the app-modal prompt box that's currently used for that.

Feedback most welcome.  Note that I do not plan to work on this until bug 629500 is done.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Zack Weinberg (:zwol) from comment #2)
> We discussed this a bit on dev.platform a couple months ago and I did some
> experiments.  I now think there _should_ be some indication that Firefox's
> piece of the print process is not yet finished, on a per-tab basis, if only
> so that users don't get confused when they can't close tabs being printed. 
Yes please. Printing pages like HTML5 spec takes a lot of time.

> However, I still want to kill the app-modal print progress boxes, because
> app-modal is never the Right Thing nowadays.
Sounds right.


> I think the right way to do this is to have the print engine fire chrome
> events at the tab being printed at approximately the same times it currently
> updates the existing print progress boxes (i.e. roughly after each page). 
> This gets the notifications into JS land, where we can use doorhangers.
Sounds like a possible solution. Make sure you dispatch trusted event from printengine code,
so that chrome listeners gets it by default.

Comment 4

[Zack Weinberg (:zwol)]

It occurred to me that this change doesn't need to wait for bug 629500, since there's nothing platform-specific about the print progress code, and in fact it would simplify the patch series in 629500 quite a bit, since it deletes a ton of code that I had to carefully preserve there.  So, here goes.

This works in my testing, modulo problems described below, but it touches quite a bit of code that I'm not fully up to speed on, and so I would appreciate careful review.  In particular this is the first time I've tried to add a DOM event or do anything in browser/.

Comment 5

[Zack Weinberg (:zwol)]

Attachment: 1/4: add new DOM event for print status

This is strictly the addition of the new DOM event that the new scheme will use.  Nothing uses it at this point.  Please review carefully, there is an excellent chance that I missed one of the many places that has to be updated for a new event type.

Comment 6

[Zack Weinberg (:zwol)]

Attachment: 2/4: change the print engine to generate the new events

This piece disconnects the print engine from the old print progress windows and wires it up to generate the new MozPrintStatus events instead.  Nothing is listening for those events yet; that will be part 3.

I have fairly high confidence in this piece.

Comment 7

[Zack Weinberg (:zwol)]

Attachment: 3/4: consume the new events in PrintUtils / browser.js

Add a new method to PrintUtils to consume the print status events and fly doorhanger notifications; wire that up in browser.js.

A slight modification to PopupNotifications.jsm was required.  The print-progress doorhanger is dismissed by default, and repeatedly updated throughout the print job.  If the user clicks on the notification icon, the doorhanger should become visible and stay that way until they hide it again or cancel the job.  PopupNotifications was hiding the doorhanger as soon as the next update came in.  Besides being irritating this made it almost impossible to click the cancel button.  So I changed it so that if you replace a currently-visible doorhanger with a dismissed-by-default doorhanger, it stays visible.

This appears to work correctly for a regular print job.  However, for Print Preview, the doorhanger does not appear until *after* the print preview is dismissed, and it then claims that we are still preparing the print preview, and you can't get rid of it.  I do not know what is wrong here.  In extremis, perhaps we could just not do any notifications for print preview.

Icons are acceptable for GTK, but moz-icon://stock/gtk-print?size=48 does not correctly pick up the 48x48 icon from the system theme, and I couldn't find the code for that, so instead I copied the icon that it should have used into gnomestripe.  It's CC-BY-SA3 or LGPL3.  I don't know if that's considered compatible with MPL2, but I figure we ought to fix the stock-icon handler anyway.

I attempted to update the Mac and Windows themes as well, but I can't test the small icons (so they may not look quite right) and I could not find large printer icons consistent with the theme in general, so those are stubbed out.  I'm inclined to suggest that this be addressed in a follow-up bug assigned to someone who knows more about graphic design than me.  In any case it would be nice to have "success" and "error" variants of the base icon in all the stock themes, which is again not something I can easily do myself (except possibly if I can figure out how to persuade moz-icon: to support Gtk badged icons).

Comment 8

[Zack Weinberg (:zwol)]

Attachment: 4/4: remove old print progress dialogs and related cruft

Finally, here we get rid of the old print progress dialogs, and some related junk (e.g. nsIPrintStatusFeedback, nsIPrintingPrompt, and nsIContentViewerFile).

Because nsIPrintingPromptService is slated to be removed in bug 629500, I did not change its compile-time interface at all; I just gutted all the implementations of the methods that are no longer used.

In addition to the concerns described above re icons and print preview, this patchset may have consequences for comm-central applications.  They will, at a minimum, lose the old progress dialogs, need to connect up the new event to the new PrintUtils method to get the doorhangers, and provide appropriate graphic elements.  However, I don't even know whether Thunderbird, Seamonkey, et al *have* doorhangers at the moment.  I would appreciate advice on how to proceed.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 627821 [details] [diff] [review]
1/4: add new DOM event for print status

Review of attachment 627821 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/interfaces/events/nsIDOMPrintStatusEvent.idl
@@ +62,5 @@
> +  /**
> +   * Details of the error.
> +   * Only accessible when |status| == |ERROR|.
> +   */
> +  readonly attribute DOMString      errorMessage;

Is this localized?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 627822 [details] [diff] [review]
2/4: change the print engine to generate the new events

Review of attachment 627822 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on the non-toolkit code, with those fixed.

Someone else needs to review the toolkit changes.

::: layout/printing/nsPrintEngine.cpp
@@ +538,5 @@
>      // Check prefs for a default setting as to whether we should print silently
>      printSilently =
>        Preferences::GetBool("print.always_print_silent", printSilently);
>  
> +    // If not printing silently, fly the platform print dialog

"fly"? I think "show"

::: layout/printing/nsPrintEngine.h
@@ +149,5 @@
>                               PRUnichar**       aURLStr,
>                               eDocTitleDefault  aDefType);
> +
> +  static void FirePrintStatusEvent(nsIDocument    *aDoc,
> +                                   bool            aIsPrinting,

Use a flags word instead of a bool parameter.

@@ +152,5 @@
> +  static void FirePrintStatusEvent(nsIDocument    *aDoc,
> +                                   bool            aIsPrinting,
> +                                   PRUint32        aStatus,
> +                                   PRUint32        aPageNo     = 0,
> +                                   PRUint32        aPageMax    = 0,

aPagesComplete, aPagesTotal to match the IDL

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 627827 [details] [diff] [review]
4/4: remove old print progress dialogs and related cruft

Review of attachment 627827 [details] [diff] [review]:
-----------------------------------------------------------------

oooh so good!

::: layout/tools/layout-debug/src/nsRegressionTester.cpp
@@ +94,5 @@
>      docShell->GetContentViewer(getter_AddRefs(viewer));
>      if (viewer){
> +      nsCOMPtr<nsIDocumentViewerPrint> viewerPrint = do_QueryInterface(viewer);
> +      if (viewerPrint) {
> +         viewerPrint->DebugPrint(fp);

Fix indent

Comment 12

[Zack Weinberg (:zwol)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> > +  /**
> > +   * Details of the error.
> > +   * Only accessible when |status| == |ERROR|.
> > +   */
> > +  readonly attribute DOMString      errorMessage;
> 
> Is this localized?

Ooh, good catch.  It is *not* localized; it is the stringification of the NS_ERROR_whatever constant.  This is because printUtils.js needs to special-case NS_ERROR_ABORT.  (Originally it *was* localized; I didn't realize the implication of consolidating the NS_ERROR_ABORT special case into printUtils until later.)  I would actually like to expose this as an nsresult value and do the string conversion (which is necessary in order to look up the real error message in the string bundle) in JS instead, but I don't know how to check whether all the necessary constants are reflected into Components.results (the print engine has a lot of its own error codes).

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> > +    // If not printing silently, fly the platform print dialog
> 
> "fly"? I think "show"

Preexisting terminology in print-dialog-related code, but I will change it anyway; whoever wrote most of the comments in these files was not on good terms with the muse of language.

> > +  static void FirePrintStatusEvent(nsIDocument    *aDoc,
> > +                                   bool            aIsPrinting,
> 
> Use a flags word instead of a bool parameter.

What future flags do you think we will need to add?

> aPagesComplete, aPagesTotal to match the IDL

Check.

> > +      if (viewerPrint) {
> >  +         viewerPrint->DebugPrint(fp);
> Fix indent

Check.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Zack Weinberg (:zwol) from comment #12)
> Ooh, good catch.  It is *not* localized; it is the stringification of the
> NS_ERROR_whatever constant.  This is because printUtils.js needs to
> special-case NS_ERROR_ABORT.  (Originally it *was* localized; I didn't
> realize the implication of consolidating the NS_ERROR_ABORT special case
> into printUtils until later.)  I would actually like to expose this as an
> nsresult value and do the string conversion (which is necessary in order to
> look up the real error message in the string bundle) in JS instead, but I
> don't know how to check whether all the necessary constants are reflected
> into Components.results (the print engine has a lot of its own error codes).

Seems to me exposing the numeric error code makes sense. If we need to add new constants to Components.results, then we should do that.

> > > +  static void FirePrintStatusEvent(nsIDocument    *aDoc,
> > > +                                   bool            aIsPrinting,
> > 
> > Use a flags word instead of a bool parameter.
> 
> What future flags do you think we will need to add?

I don't know but even a single flag makes for more readable callers than passing true/false.

Comment 14

[Zack Weinberg (:zwol)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> 
> Seems to me exposing the numeric error code makes sense. If we need to add
> new constants to Components.results, then we should do that.

OK, I found xpc.msg and I'll see what I can do.

> > What future flags do you think we will need to add?
> 
> I don't know but even a single flag makes for more readable callers than
> passing true/false.

All callers in this case have an "is printing" boolean, so having to write "isPrinting ? FLAG_IS_PRINTING : FLAG_PRINT_PREVIEW" would actually hurt readability.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I saw at least one place you're passing false. But OK, I'll let it slide :-)

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 627824 [details] [diff] [review]
3/4: consume the new events in PrintUtils / browser.js

>--- a/toolkit/components/printing/content/printUtils.js
>+++ b/toolkit/components/printing/content/printUtils.js

>+    PopupNotifications.show(browser, _id, message, _anchor,
>+                            action, null,
>+                            { persistence: 0,
>+                              persistWhileVisible: false,
>+                              dismissed: !done,
>+                              removeOnDismissal: done });

PopupNotifications isn't guaranteed to exist here. printUtils.js is used in way more contexts than just browser.xul.

This also needs UI review. I'm not convinced that this is going to provide a better user experience.

Comment 17

[[e-mail]]

(In reply to Dão Gottwald from comment #16)
> (From update of attachment 627824 [details] [diff] [review])
> PopupNotifications isn't guaranteed to exist here. printUtils.js is used in
> way more contexts than just browser.xul.
In SeaMonkey alone for instance, I quickly found the following places:
* In the browser, where popup notifications can be disabled (!)
* In view source
* In web composer
* In the mail reader
* In mail composition
* In address book

Comment 18

[Zack Weinberg (:zwol)]

Comment on attachment 627824 [details] [diff] [review]
3/4: consume the new events in PrintUtils / browser.js

PopupNotifications only gets used if you install printStatusEventsToPopups as a  listener for MozPrintStatus events.  Currently, the only place that does that is Firefox's browser.js.  The theory is that any other Gecko application that also had the PopupNotifications interface can also use printStatusEventsToPopups (which is why it's not in browser.js).

Applications that don't have PopupNotifications need to write their own MozPrintStatus event listener, and (assuming this patch series lands) will lose print progress updates until they do.  But they won't crash.

I am happy to discuss UI adjustments and to assist with the writing of appropriate event listeners for comm-central applications.

Comment 19

[Dão Gottwald [:dao]]

(In reply to Zack Weinberg (:zwol) from comment #18)
> The theory is that any other Gecko
> application that also had the PopupNotifications interface can also use
> printStatusEventsToPopups (which is why it's not in browser.js).

PopupNotifications is a random variable in browser.js, not part of any interface.
I'm also not sure that it's reasonable to expect windows without a tabbrowser to use PopupNotifications.jsm.

> Applications that don't have PopupNotifications need to write their own MozPrintStatus
> event listener, and (assuming this patch series lands) will lose print progress updates
> until they do.

These alternatives don't seem reasonable, given that (1) writing custom UIs for this isn't trivial and (2) we've already determined that we don't want to remove progress updates as the printed documents need to remain open until the progress finished (which, by the way, your popup notification doesn't really ensure, does it?).

> I am happy to discuss UI adjustments and to assist with the writing of appropriate
> event listeners for comm-central applications.

Note that this also affects features on mozilla-central and add-ons.

Comment 20

[Philip Chee]

> Note that this also affects features on mozilla-central and add-ons.
CC jorgev

Comment 21

[Zack Weinberg (:zwol)]

(In reply to Dão Gottwald [:dao] from comment #19)
> (In reply to Zack Weinberg (:zwol) from comment #18)
> > The theory is that any other Gecko
> > application that also had the PopupNotifications interface can also use
> > printStatusEventsToPopups (which is why it's not in browser.js).
> 
> PopupNotifications is a random variable in browser.js, not part of any
> interface.

Is it?  The documentation on MDN ( https://developer.mozilla.org/en/Using_popup_notifications ) says "[PopupNotifications.jsm] is imported by the browser, you don't have to do it yourself."  An earlier revision of this patch had a a Cu.import() call inside printStatusEventsToPopups, for great defensiveness, but that produced nonsense errors which went away when I removed it.

> I'm also not sure that it's reasonable to expect windows without a
> tabbrowser to use PopupNotifications.jsm.

Indeed, there may be a problem even in Firefox with (at least) view source windows.  I am, however, inclined to fix this by making it work somehow and/or by finding an alternative to the use of PopupNotifications.jsm but preserving the architectural scheme of DOM events fired at the <browser> instead of this modal fake web progress listener **** that we have now.

> > Applications that don't have PopupNotifications need to write their own MozPrintStatus
> > event listener, and (assuming this patch series lands) will lose print progress updates
> > until they do.
> 
> These alternatives don't seem reasonable, given that (1) writing custom UIs
> for this isn't trivial

What would you suggest?  I do not want to give up on the architectural change (parts 1, 2, and 4).

> and (2) we've already determined that we don't want
> to remove progress updates as the printed documents need to remain open
> until the progress finished (which, by the way, your popup notification
> doesn't really ensure, does it?).

I haven't looked into what (if anything) keeps the document open *now*, but I think that is properly the document viewer's job (or the docshell or something in the same general ballpark; the document viewer in particular is aware of when print operations begin and end), and affects the UI only insofar as a window/tab that cannot be closed right now needs to have some visible marker so the user doesn't get confused.  I will investigate and possibly file more bugs.
 
> > I am happy to discuss UI adjustments and to assist with the writing of appropriate
> > event listeners for comm-central applications.
> 
> Note that this also affects features on mozilla-central and add-ons.

Are we reaching the point where this needs to go back to the newsgroups?  Not that I got very far trying to discuss this on the newsgroups last time.  It is also possible for me to turn up at the MV office if we need to work out a plan in person.

Comment 22

[Jorge Villalobos [:jorgev] (he/him)]

There are almost no add-ons related to printing, but it's still good to communicate this to add-on devs (specially theme devs) when it lands.

Comment 23

[Dão Gottwald [:dao]]

(In reply to Zack Weinberg (:zwol) from comment #21)
> (In reply to Dão Gottwald [:dao] from comment #19)
> > PopupNotifications is a random variable in browser.js, not part of any
> > interface.
> 
> Is it?  The documentation on MDN (
> https://developer.mozilla.org/en/Using_popup_notifications ) says
> "[PopupNotifications.jsm] is imported by the browser, you don't have to do
> it yourself."

"the browser" means browser.js here, so this is more or less in line with what I said. browser.js imports PopupNotifications.jsm, creates a PopupNotifications instance and happens to name it PopupNotifications.

> > I'm also not sure that it's reasonable to expect windows without a
> > tabbrowser to use PopupNotifications.jsm.
> 
> Indeed, there may be a problem even in Firefox with (at least) view source
> windows.  I am, however, inclined to fix this by making it work somehow
> and/or by finding an alternative to the use of PopupNotifications.jsm but
> preserving the architectural scheme of DOM events fired at the <browser>
> instead of this modal fake web progress listener crap that we have now.

Can printPreviewProgress.js be changed to using an event listener?

> > and (2) we've already determined that we don't want
> > to remove progress updates as the printed documents need to remain open
> > until the progress finished (which, by the way, your popup notification
> > doesn't really ensure, does it?).
> 
> I haven't looked into what (if anything) keeps the document open *now*, but
> I think that is properly the document viewer's job (or the docshell or
> something in the same general ballpark; the document viewer in particular is
> aware of when print operations begin and end), and affects the UI only
> insofar as a window/tab that cannot be closed right now needs to have some
> visible marker so the user doesn't get confused.  I will investigate and
> possibly file more bugs.

Are you saying that the tab or window couldn't be closed even without the modal dialog? I was under the impression that it could be closed and that the print job would fail.

> Are we reaching the point where this needs to go back to the newsgroups?

I'll leave that to you. We can keep discussing here if you think it won't hinder the actual work too much.

> It is also possible for me to turn up at the MV office if we need to work
> out a plan in person.

I'm in Germany...

(In reply to Jorge Villalobos [:jorgev] from comment #22)
> There are almost no add-ons related to printing, but it's still good to
> communicate this to add-on devs (specially theme devs) when it lands.

Not sure what you mean by "related to printing"; I was talking about add-ons that simply display stuff and allow the user to print it.

Comment 24

[Zack Weinberg (:zwol)]

(In reply to Dão Gottwald [:dao] from comment #23)
> "the browser" means browser.js here, so this is more or less in line with
> what I said. browser.js imports PopupNotifications.jsm, creates a
> PopupNotifications instance and happens to name it PopupNotifications.

The documentation gives the strong and IMHO reasonable impression that code in Firefox can count on that instance being named PopupNotifications, i.e., it's not a "just happens to", it's part of browser.js's contract with extensions.

> Can printPreviewProgress.js be changed to using an event listener?

Well, it would amount to rewriting the thing from scratch, but I suppose we could have a stock event listener for MozPaintEvent that put up modal dialog boxes, but I would like to at least *try* to find an alternative non-modal presentation.  How ubiquitous are the slide-down notification bars?

> > I haven't looked into what (if anything) keeps the document open *now*, but
> > I think that is properly the document viewer's job (or the docshell or
> > something in the same general ballpark...)
> 
> Are you saying that the tab or window couldn't be closed even without the
> modal dialog? I was under the impression that it could be closed and that
> the print job would fail.

I am saying that if the tab or window can be closed in the absence of the modal dialog, we should fix that rather than treating it as a reason to keep the modal dialog box around.

> I'll leave that to you. We can keep discussing here if you think it won't
> hinder the actual work too much.

I'd actually prefer here...

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 627821 [details] [diff] [review]
1/4: add new DOM event for print status

A bit swamped right now, and Olli would make a better reviewer anyway.

Comment 26

[Dão Gottwald [:dao]]

(In reply to Zack Weinberg (:zwol) from comment #24)
> (In reply to Dão Gottwald [:dao] from comment #23)
> > "the browser" means browser.js here, so this is more or less in line with
> > what I said. browser.js imports PopupNotifications.jsm, creates a
> > PopupNotifications instance and happens to name it PopupNotifications.
> 
> The documentation gives the strong and IMHO reasonable impression that code
> in Firefox can count on that instance being named PopupNotifications, i.e.,
> it's not a "just happens to", it's part of browser.js's contract with
> extensions.

browser.js is one PopupNotifications.jsm consumer. It exposes its PopupNotifications instance with a certain name, which code living in the same window as browser.js can count on. This isn't according to any "PopupNotifications interface".

> > Can printPreviewProgress.js be changed to using an event listener?
> 
> Well, it would amount to rewriting the thing from scratch, but I suppose we
> could have a stock event listener for MozPaintEvent that put up modal dialog
> boxes, but I would like to at least *try* to find an alternative non-modal
> presentation.  How ubiquitous are the slide-down notification bars?

They aren't ubiquitous, and they would have the same flaw as the popup notification:

> I am saying that if the tab or window can be closed in the absence of the
> modal dialog, we should fix that rather than treating it as a reason to keep
> the modal dialog box around.

Fix it how? We shouldn't introduce footguns, so until we've found a better way, it's a reason to keep the modal dialog around. There's a good chance that users won't actually be interested in tracking the progress; just showing it somewhere isn't going to prevent them from thinking that they can close the document now that they've triggered the print job.

Comment 27

[Zack Weinberg (:zwol)]

(In reply to Dão Gottwald [:dao] from comment #26)
> (In reply to Zack Weinberg (:zwol) from comment #24)
> > The documentation gives the strong and IMHO reasonable impression that code
> > in Firefox can count on that instance being named PopupNotifications, i.e.,
> > it's not a "just happens to", it's part of browser.js's contract with
> > extensions.
> 
> browser.js is one PopupNotifications.jsm consumer. It exposes its
> PopupNotifications instance with a certain name, which code living in the
> same window as browser.js can count on. This isn't according to any
> "PopupNotifications interface".

I feel like we're talking past each other.  Are you trying to say that the code that's currently in printUtils.js needs to get at PopupNotifications.jsm some other way?  If so, how?

> > I am saying that if the tab or window can be closed in the absence of the
> > modal dialog, we should fix that rather than treating it as a reason to keep
> > the modal dialog box around.
> 
> Fix it how? We shouldn't introduce footguns, so until we've found a better
> way, it's a reason to keep the modal dialog around.

I won't have time to look into this properly in the next few days, but I suspect it is a matter of adding a few lines of code to the document viewer.  It is aware of when printing begins and ends, and it certainly ought to be in a position to cancel a window close.

Comment 28

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Dão Gottwald [:dao] from comment #23)
> (In reply to Jorge Villalobos [:jorgev] from comment #22)
> > There are almost no add-ons related to printing, but it's still good to
> > communicate this to add-on devs (specially theme devs) when it lands.
> 
> Not sure what you mean by "related to printing"; I was talking about add-ons
> that simply display stuff and allow the user to print it.

I was referring to add-ons that make changes to the print flow. I know of one that allows you to modify the webpage and remove some elements before the page is printed.

I've never seen an add-on that gives users the possibility to print custom stuff it is displaying, but it's possible there are a few.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 627821 [details] [diff] [review]
1/4: add new DOM event for print status

>+nsresult
>+NS_NewDOMPrintStatusEvent(nsIDOMEvent** aInstancePtrResult,
>+                          nsPresContext* aPresContext,
>+                          bool     aIsPrinting,
>+                          PRUint32 aStatus,
>+                          PRUint32 aPagesComplete,
>+                          PRUint32 aPagesTotal,
>+                          nsresult aErrorCode)
>+{
>+  /* sanity checks */
>+  if (aStatus == nsIDOMPrintStatusEvent::ERROR && NS_SUCCEEDED(aErrorCode))
>+    return NS_ERROR_INVALID_ARG;
if (expr) {
  stmt;
}
same also elsewhere.


>+nsDOMPrintStatusEvent::GetIsPrinting(bool *aIsPrinting)
Nit, bool*
Same also elsewhere


>+/* readonly attribute unsigned long pagesComplete; */
>+NS_IMETHODIMP
>+nsDOMPrintStatusEvent::GetPagesComplete(PRUint32 *aPagesComplete)
>+{
>+  if (mStatus != nsIDOMPrintStatusEvent::PAGE_COMPLETE)
>+    return NS_ERROR_NOT_AVAILABLE;
APIs which throw even in somewhat normal case are annoying.
returning 0 is cases we don't know the answer should be ok.
Same also elsewhere.


+
>+/* readonly attribute DOMString errorMessage; */
>+NS_IMETHODIMP
>+nsDOMPrintStatusEvent::GetErrorMessage(nsAString &aErrorMessage)
>+{
So this should return some error value, not string.

Comment 30

[Zack Weinberg (:zwol)]

Attachment: part 1a: new DOM events for print progress and error reporting

I'm still tinkering with the code that will _use_ these events, but since events seem to be in flux, I thought it might be a good idea to post them for review now.

Major change from the previous iteration of this patch is that there are three event types instead of a primary status code.  This bakes a bunch of consistency checking into the type system -- all MozPrintErrorEvents, and only MozPrintErrorEvents, expose an .errorCode property, for instance.

I'd appreciate particular attention by reviewers to

Comment 31

[Zack Weinberg (:zwol)]

(Sorry about the truncated message, I hit return on the wrong field and bang.)

On attachment 723258 [details] [diff] [review], I'd appreciate particular attention by reviewers to details of the DOMClassInfo stuff, which I have not had to muck with to this extent before, and opinions as to whether the moz prefixes are actually necessary.  (Note that plain MozPrintEvent will be fired at content (as 'beforeprint'/'afterprint'), but the others will not; however, all three of them are exposed as global window properties, as all event prototypes are.)

Comment 32

[Zack Weinberg (:zwol)]

Attachment: part 1b: generalize nsContentUtils::Dispatch*Event

nsContentUtils::Dispatch*Event can't be used if you need a non-generic event type, which is exactly the situation part 2 will be in.  So, add overloads that allow you to create the event object yourself.  Seems straightforward enough, however I'd like to declare myself completely baffled as to why so many event constructors take *another* event as an argument; it's quite likely there is something subtle about event creation I do not understand, and that might affect this patch.

Comment 33

[Zack Weinberg (:zwol)]

Attachment: part 0: remove nsIContentViewerFile

This was originally another piece of bug 629500, and was r+ed over there by roc, but it dawned on me that part 2 of this bug presently touches nsIContentViewerFile methods, and if we just go ahead and zap it now, it won't have to.

Comment 34

[Zack Weinberg (:zwol)]

While I'm in here, lemme address a query from way back when:

(In reply to Zack Weinberg (:zwol) from comment #21)
> (In reply to Dão Gottwald [:dao] from comment #19)
> > and (2) we've already determined that we don't want
> > to remove progress updates as the printed documents need to remain open
> > until the progress finished (which, by the way, your popup notification
> > doesn't really ensure, does it?).
> 
> I haven't looked into what (if anything) keeps the document open *now*, but
> I think that is properly the document viewer's job (or the docshell or
> something in the same general ballpark; the document viewer in particular is
> aware of when print operations begin and end), and affects the UI only
> insofar as a window/tab that cannot be closed right now needs to have some
> visible marker so the user doesn't get confused.  I will investigate and
> possibly file more bugs.

It turns out that nsDocumentViewer and nsGlobalWindow already contain code to prevent a window from being closed while it is being printed.  See nsGlobalWindow::Close, nsGlobalWindow::CanClose, 
nsDocumentViewer::RequestWindowClose, and the related nsDocumentViewer:PermitUnload.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Note, I have several huge patches in my review queue, so I can't promise very fast reviews here. Sorry.
Ping me if I haven't reviewed within a week.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 723258 [details] [diff] [review]
part 1a: new DOM events for print progress and error reporting

The event implementations should be autogenerated.
Just have the .idl per event, each interface should have corrent [noscript] init*Event, add *EventInit and add the event name to
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/event_impl_gen.conf.in
That will create the event implementation based on the .idl.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 723265 [details] [diff] [review]
part 1b: generalize nsContentUtils::Dispatch*Event


>   /**
>-   * This method creates and dispatches a untrusted event.
>+   * As above, but caller provides the event object.
>+   *
>+   * @param aDoc           The document containing the event target.
>+   * @param aTarget        The target of the event, should be QIable to
>+   *                       nsIDOMEventTarget.
>+   * @param aEvent         The event object.  Should be fresh from
>+   *                       NS_NewDOM*Event; do not call InitEvent.
>+   * @param aEventName     The name of the event.
>+   * @param aCanBubble     Whether the event can bubble.
>+   * @param aCancelable    Is the event cancelable.
>+   * @param aDefaultAction Set to true if default action should be taken,
>+   *                       see nsIDOMEventTarget::DispatchEvent.
>+   */
>+  static nsresult DispatchTrustedEvent(nsIDocument* aDoc,
>+                                       nsISupports* aTarget,
>+                                       nsIDOMEvent* aEvent,
>+                                       const nsAString& aEventName,
>+                                       bool aCanBubble,
>+                                       bool aCancelable,
>+                                       bool *aDefaultAction = nullptr);
Makes no sense to take aEvenName, aCanBubble, aCancelable params.
The event must be initialized before calling this


>+   * @param aDoc           The document which will be used to create the event.
>+   * @param aTarget        The target of the event, should be QIable to
>+   *                       nsIDOMEventTarget.
>+   * @param aEvent         The event object.  Should be fresh from
>+   *                       NS_NewDOM*Event; do not call InitEvent.
>+   * @param aEventName     The name of the event.
>+   * @param aCanBubble     Whether the event can bubble.
>+   * @param aCancelable    Is the event cancelable.
>+   * @param aDefaultAction Set to true if default action should be taken,
>+   *                       see nsIDOMEventTarget::DispatchEvent.
>+   */
>+  static nsresult DispatchUntrustedEvent(nsIDocument* aDoc,
>+                                         nsISupports* aTarget,
>+                                         nsIDOMEvent* aEvent,
>+                                         const nsAString& aEventName,
>+                                         bool aCanBubble,
>+                                         bool aCancelable,
>+                                         bool *aDefaultAction = nullptr);
ditto


>+  static nsresult DispatchChromeEvent(nsIDocument* aDoc,
>+                                      nsISupports* aTarget,
>+                                      nsIDOMEvent* aEvent,
>+                                      const nsAString& aEventName,
>+                                      bool aCanBubble,
>+                                      bool aCancelable,
>+                                      bool *aDefaultAction = nullptr);
ditto

Comment 38

[Zack Weinberg (:zwol)]

(In reply to Olli Pettay [:smaug] from comment #36)
> Comment on attachment 723258 [details] [diff] [review]
> part 1a: new DOM events for print progress and error reporting
> 
> The event implementations should be autogenerated.
> Just have the .idl per event, each interface should have corrent [noscript]
> init*Event, add *EventInit and add the event name to
> http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/
> event_impl_gen.conf.in
> That will create the event implementation based on the .idl.

Blech, you want me to move event parameters from the constructor to the init method?  That's backwards.  We should be moving in the direction of eliminating init methods in favor of constructors.  But it does get me out of writing a bunch of glue, so I'll do it anyway.

(In reply to Olli Pettay [:smaug] from comment #37)
> >+   * As above, but caller provides the event object.
> >+   *
> >+   * @param aDoc           The document containing the event target.
> >+   * @param aTarget        The target of the event, should be QIable to
> >+   *                       nsIDOMEventTarget.
> >+   * @param aEvent         The event object.  Should be fresh from
> >+   *                       NS_NewDOM*Event; do not call InitEvent.
> >+   * @param aEventName     The name of the event.
> >+   * @param aCanBubble     Whether the event can bubble.
> >+   * @param aCancelable    Is the event cancelable.
> >+   * @param aDefaultAction Set to true if default action should be taken,
> >+   *                       see nsIDOMEventTarget::DispatchEvent.
> >+   */
> >+  static nsresult DispatchTrustedEvent(nsIDocument* aDoc,
> >+                                       nsISupports* aTarget,
> >+                                       nsIDOMEvent* aEvent,
> >+                                       const nsAString& aEventName,
> >+                                       bool aCanBubble,
> >+                                       bool aCancelable,
> >+                                       bool *aDefaultAction = nullptr);
> Makes no sense to take aEvenName, aCanBubble, aCancelable params.
> The event must be initialized before calling this

I think you missed the part of the docstring where it specifically says you do *not* call InitEvent before you call this method.  Intended use pattern is

   nsCOMPtr<nsIDOMEvent> event;
   if (NS_SUCCEEDED(NS_NewWhateverEvent(getter_AddRefs(event),
                                        owner, ctxt, nullptr, 
                                        other arguments))
     DispatchTrustedEvent(doc, target, event, "name", bubbles, cancelable);

That said, I'm just going to scrap this patch, it was less useful than I expected it to be and I think it won't work at all with your requested change to the new events.

While you're here, quick question: What exactly is the difference between the "owner" of an event (as in the second argument to all NS_New*Event calls since bug 822399) and the "target" of an event (as in the second argument to nsContentUtils::Dispatch*Event)?

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

From owner we get to the js global in which the event is wrapped.
Target can be technically in some other window, and before dispatching events don't have target at all.

Comment 40

[Zack Weinberg (:zwol)]

Attachment: part 1: new DOM events for print status

Here's the new revision of the event-creation patch.  I'm impressed by how much less DOM glue I had to write.  (I did have to add support for "attribute nsresult" to a couple code generators, but that wasn't terribly hard.)

I wonder, though, whether it's possible to make (nsIDOM|Moz)PrintErrorEvent and (nsIDOM|Moz)PrintProgressEvent inherit from (nsIDOM|Moz)PrintEvent.  There's three copies of all the isPrintPreview-related code in this iteration.

Also I'm not sure where if at all the NS_PRINTERROR and NS_PRINTPROGRESS constants will get used (see next patch).

Comment 41

[Zack Weinberg (:zwol)]

Attachment: part 2: generate new events from print engine

Here's where we generate the new events.  In addition to the new MozPrintError and MozPrintProgress events, which are chrome-only, the semi-standard "beforeprint" and "afterprint" events now get explicitly sent to the chrome event listener for the outermost window as well as the window tree, and have a .mozIsPrintPreview property.  I haven't yet revised part 3 so I don't know for sure whether this will be useful, but I suspect it will be.

I am *not* impressed with the amount of code required to fire an event with attached data from C++ (compared to the nsContentUtils::Dispatch*Event API that I tried to augment last time) and the way the variable part lands right in the middle, making a helper function impractical.  Switching to generated event code actually makes it *worse*, because I have to call a special init method on the object returned from NS_NewDOMWhateverEvent, and I have to QI it back to its true type to do that.  I hope the "Paris bindings" plan for events will address this.

There is a subtle behavioral change in here: setting the showPrintProgress attribute on a print-settings object to false didn't actually do anything, to the chagrin of several tests.  Now it does.  (However, it only suppresses the per-page progress events, not the beforeprint, afterprint, or error events; the print-settings object is not reliably available when those events are triggered.  Changing that would require significant changes to the timing of the beforeprint event especially.  And the aforementioned tests do expect that beforeprint and afterprint events aren't affected.)

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

beforeprint and afterprint are very much standard. Nothing "semi" there.
http://www.whatwg.org/specs/web-apps/current-work/#dom-print
Don't change how they work. If you want to add some new property to before/afterprint events, 
please file a spec bug first.

And Paris bindings don't affect how events are dispatched.
I agree it would be nice to generate methods which
create/initialize/dispatch events.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 724921 [details] [diff] [review]
part 1: new DOM events for print status

># HG changeset patch
># Parent de810bfae908b1944cd8dc0aa59a0ffd22e542a3
># User Zack Weinberg <[e-mail]>
>Bug 650960, part 1: Add DOM events for reporting print status.  r=smaug
>
>diff --git a/content/events/src/nsEventDispatcher.cpp b/content/events/src/nsEventDispatcher.cpp
>--- a/content/events/src/nsEventDispatcher.cpp
>+++ b/content/events/src/nsEventDispatcher.cpp
>@@ -855,14 +855,19 @@ nsEventDispatcher::CreateEvent(mozilla::
>       nsDOMTouchEvent::PrefEnabled())
>     return NS_NewDOMTouchEvent(aDOMEvent, aOwner, aPresContext, nullptr);
>   if (aEventType.LowerCaseEqualsLiteral("hashchangeevent"))
>     return NS_NewDOMHashChangeEvent(aDOMEvent, aOwner, aPresContext, nullptr);
>   if (aEventType.LowerCaseEqualsLiteral("customevent"))
>     return NS_NewDOMCustomEvent(aDOMEvent, aOwner, aPresContext, nullptr);
>   if (aEventType.LowerCaseEqualsLiteral("mozsmsevent"))
>     return NS_NewDOMMozSmsEvent(aDOMEvent, aOwner, aPresContext, nullptr);
>-  if (aEventType.LowerCaseEqualsLiteral("storageevent")) {
>+  if (aEventType.LowerCaseEqualsLiteral("storageevent"))
>     return NS_NewDOMStorageEvent(aDOMEvent, aOwner, aPresContext, nullptr);
>-  }
>+  if (aEventType.LowerCaseEqualsLiteral("mozprintevent"))
>+    return NS_NewDOMMozPrintEvent(aDOMEvent, aOwner, aPresContext, nullptr);
>+  if (aEventType.LowerCaseEqualsLiteral("mozprintprogressevent"))
>+    return NS_NewDOMMozPrintProgressEvent(aDOMEvent, aOwner, aPresContext, nullptr);
>+  if (aEventType.LowerCaseEqualsLiteral("mozprinterrorevent"))
>+    return NS_NewDOMMozPrintErrorEvent(aDOMEvent, aOwner, aPresContext, nullptr);
> 
Don't add support for document.createEvent(). That is for legacy events only.

>+++ b/dom/tests/mochitest/general/test_interfaces.html
>@@ -531,27 +531,37 @@ var interfaceNamesInGlobalScope =
>     "RTCIceCandidate",
>     "RTCPeerConnection",
>     "LocalMediaStream",
>     "CSSConditionRule",
>     "CSSGroupingRule",
>     "AsyncScrollEventDetail",
>     "MozSmsSegmentInfo",
>     "DOMCursor",
>-    "BlobEvent"
>+    "BlobEvent",
>+    "MozPrintEvent",
>+    "MozPrintErrorEvent",
>+    "MozPrintProgressEvent",
>+    "MozActivity",
>+    "MozActivityHandlerDescription",
>+    "MozActivityOptions",
>+    "MozActivityRequestHandler",
>+    "MozNavigatorActivities",
>+    "NavigatorSystemMessages",
>+    "SystemMessageCallback"
>   ]
Why you add so many interfaces here?


I think it is better to wait still few days so that event codegen can deal with webidl.
Then we can implement new events without exposing them to web pages.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 724924 [details] [diff] [review]
part 2: generate new events from print engine


> 
>+static void
>+DispatchSinglePrintEvent(nsIDocument *aDoc,
>+                         nsString aEventName,
>+                         bool aIsPrintPreview,
>+                         bool aToChrome)
>+{
>+  nsIPresShell *shell = aDoc->GetShell();
>+  nsPresContext *ctxt = shell ? shell->GetPresContext() : nullptr;
>+  nsPIDOMWindow *wdow = aDoc->GetWindow();
>+  nsCOMPtr<mozilla::dom::EventTarget> target = do_QueryInterface(wdow);
>+  nsCOMPtr<mozilla::dom::EventTarget> owner =
>+    aToChrome ? do_QueryInterface(wdow->GetParentTarget()) : target;
>+
>+  nsCOMPtr<nsIDOMEvent> event;
>+  nsresult rv = NS_NewDOMMozPrintEvent(getter_AddRefs(event), owner, ctxt,
>+                                       nullptr);
>+  if (NS_FAILED(rv)) return;
>+
>+  nsCOMPtr<nsIDOMMozPrintEvent> pevent(do_QueryInterface(event));
>+  rv = pevent->InitMozPrintEvent(aEventName, false, false, aIsPrintPreview);
>+  if (NS_FAILED(rv)) return;
>+
>+  rv = event->SetTarget(target);
>+  if (NS_FAILED(rv)) return;
>+  event->SetTrusted(true);
>+
>+  bool dummy;
>+  owner->DispatchEvent(event, &dummy);
>+}
>+
> void
>-nsDocumentViewer::DispatchEventToWindowTree(nsIDocument* aDoc,
>-                                              const nsAString& aEvent)
>+nsDocumentViewer::DispatchPrintEventToWindowTree(nsIDocument* aDoc,
>+                                                 uint32_t aEventType,
>+                                                 bool aIsPrintPreview)
> {
>+  MOZ_ASSERT(aEventType == NS_BEFOREPRINT || aEventType == NS_AFTERPRINT);
>+
>+  nsString eventName(aEventType == NS_BEFOREPRINT
>+                     ? NS_LITERAL_STRING("beforeprint")
>+                     : NS_LITERAL_STRING("afterprint"));
>+
>   nsCOMArray<nsIDocument> targets;
>   CollectDocuments(aDoc, &targets);
>   for (int32_t i = 0; i < targets.Count(); ++i) {
>     nsIDocument* d = targets[i];
>-    nsContentUtils::DispatchTrustedEvent(d, d->GetWindow(),
>-                                         aEvent, false, false, nullptr);
>+    DispatchSinglePrintEvent(d, eventName, aIsPrintPreview, false);
>+
>+    if (d == aDoc) {
>+      // Also send it to the outermost window's chrome event handler.
>+      // ??? May not be necessary.
Events propagate from content to chrome


>+      DispatchSinglePrintEvent(d, eventName, aIsPrintPreview, true);
>+    }
>   }
...but you end up dispatching wrong kind of before/afterprint events.
before/afterprint are plain old Events
>+  // The print settings can force-disable progress events for a particular
>+  // job, but they can't do the opposite.
>+  if (mShowPrintProgress)
>+    mPrt->mPrintSettings->GetShowPrintProgress(&mShowPrintProgress);
if (expr) {
  stmt;
}

>-    // Ask dialog to be Print Shown via the Plugable Printing Dialog Service
>-    // This service is for the Print Dialog and the Print Progress Dialog
>-    // If printing silently or you can't get the service continue on
>+    // The print settings can force a particular print job to be silent,
>+    // but they can't do the opposite.
>+    if (!printSilently)
>+      mPrt->mPrintSettings->GetPrintSilent(&printSilently);
ditto

>+    // NS_ERROR_ABORT here means the user cancelled printing from
>+    // inside the print dialog.  Do not spam stderr in that case.
>+    if (rv == NS_ERROR_ABORT)
>       return rv;
ditto
>+// We can't use nsAsyncDOMEvent for this because it doesn't support
>+// firing preconstructed events at the chrome event handler.
>+class AsyncChromeEvent : public nsRunnable {
{ goes to the next line, but you really should just change nsAsyncDOMEvent.
And I don't understand how this dispatching anything to chrome only.

>+nsPrintEngine::FirePrintErrorEvent(nsIDocument *aDocument,
>+                                   bool         aIsPrintPreview,
>+                                   nsresult     aErrorCode)
>+{
>+  nsIPresShell *shell = aDocument->GetShell();
>+  nsPresContext *ctxt = shell ? shell->GetPresContext() : nullptr;
>+  nsPIDOMWindow *win = aDocument->GetWindow();
* goes with type. nsIPresShell* shell etc.

Comment 45

[Zack Weinberg (:zwol)]

(In reply to Olli Pettay [:smaug] from comment #43)
>
> Don't add support for document.createEvent(). That is for legacy events only.

Oh, is it?  I was under the impression this was the preferred way to create events (and that various other mechanisms, e.g. the nsContentUtils::Dispatch*Event stuff, ended up in here too).

Can you comment on whether the constants added to widget/nsGUIEvent.h are necessary, and if so, for what?

> >+++ b/dom/tests/mochitest/general/test_interfaces.html
> >@@ -531,27 +531,37 @@ var interfaceNamesInGlobalScope =
> >     "RTCIceCandidate",
> >     "RTCPeerConnection",
> >     "LocalMediaStream",
> >     "CSSConditionRule",
> >     "CSSGroupingRule",
> >     "AsyncScrollEventDetail",
> >     "MozSmsSegmentInfo",
> >     "DOMCursor",
> >-    "BlobEvent"
> >+    "BlobEvent",
> >+    "MozPrintEvent",
> >+    "MozPrintErrorEvent",
> >+    "MozPrintProgressEvent",
> >+    "MozActivity",
> >+    "MozActivityHandlerDescription",
> >+    "MozActivityOptions",
> >+    "MozActivityRequestHandler",
> >+    "MozNavigatorActivities",
> >+    "NavigatorSystemMessages",
> >+    "SystemMessageCallback"
> >   ]
> Why you add so many interfaces here?

The test fails on my dev box if I don't also add the Activity and SystemMessages interfaces.  I don't know why it doesn't fail on the build farm; dom/activities and dom/messages appear to be built unconditionally.

> I think it is better to wait still few days so that event codegen can deal
> with webidl.
> Then we can implement new events without exposing them to web pages.

Sure, I'm not going to have time to revise this stuff for maybe a week or two anyway.  (And I'd prefer to finish writing the code that *uses* these events first.)  What's the bug for those changes, so I can see what will have to change here?

Comment 46

[Zack Weinberg (:zwol)]

(In reply to Olli Pettay [:smaug] from comment #44)
> >+    if (d == aDoc) {
> >+      // Also send it to the outermost window's chrome event handler.
> >+      // ??? May not be necessary.
> Events propagate from content to chrome

This could all stand to be better documented.  Is it possible for content to prevent chrome from seeing the event?

> >+      DispatchSinglePrintEvent(d, eventName, aIsPrintPreview, true);
> >+    }
> >   }
> ...but you end up dispatching wrong kind of before/afterprint events.
> before/afterprint are plain old Events

Intentional change which you already objected to (comment 42).  I think I have an alternative design which doesn't change that.

> >+  // The print settings can force-disable progress events for a particular
> >+  // job, but they can't do the opposite.
> >+  if (mShowPrintProgress)
> >+    mPrt->mPrintSettings->GetShowPrintProgress(&mShowPrintProgress);
> if (expr) {
>   stmt;
> }

Will change.

> >+    // NS_ERROR_ABORT here means the user cancelled printing from
> >+    // inside the print dialog.  Do not spam stderr in that case.
> >+    if (rv == NS_ERROR_ABORT)
> >       return rv;

Will not change; file style is no braces for early returns.

> ditto
> >+// We can't use nsAsyncDOMEvent for this because it doesn't support
> >+// firing preconstructed events at the chrome event handler.
> >+class AsyncChromeEvent : public nsRunnable {
> { goes to the next line, but you really should just change nsAsyncDOMEvent.
> And I don't understand how this dispatching anything to chrome only.

I suppose the name is a little misleading.  It dispatches to whatever it's given as the target, but it's always given a window's parent target by the users.  Anyway, happy to change nsAsyncDOMEvent, I just was trying to keep things local.

> >+nsPrintEngine::FirePrintErrorEvent(nsIDocument *aDocument,
> >+                                   bool         aIsPrintPreview,
> >+                                   nsresult     aErrorCode)
> >+{
> >+  nsIPresShell *shell = aDocument->GetShell();
> >+  nsPresContext *ctxt = shell ? shell->GetPresContext() : nullptr;
> >+  nsPIDOMWindow *win = aDocument->GetWindow();
> * goes with type. nsIPresShell* shell etc.

The file is not consistent on this one, and I feel *very strongly* that putting the asterisk with the type is *misleading*, since it actually binds to the right.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Zack Weinberg (:zwol) from comment #46)
> This could all stand to be better documented.  Is it possible for content to
> prevent chrome from seeing the event?
If chrome adds listener to capture phase, or system group, then the event is guaranteed to end up to chrome

Comment 48

[Zack Weinberg (:zwol)]

Philipp, Guillaume, I'd like to bring this bug to the fx team's attention.  This is currently *the* blocker for further progress on print subsystem modernization.  It's a big mess of poorly worked out DOM-level, chrome-level, and UI-level design decisions and I would love some help from people who understand the latter two better than I do (which is to say, better than hardly at all).

Comment 49

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

As this is too technical for me to really grasp, I put it up on the backlog for triage by people who have a better understanding of the matter.

Comment 50

[Benjamin Smedberg]

Resetting owner to default per Zack's request.