Dialogs-enabled bits in nsDocumentViewer are broken

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment, 1 obsolete attachment)

They check whether dialogs are enabled on one window, but change the dialogs-enabled state on a different one (its scriptable top).  See bug 1190641 comment 28 for the resulting issues.

But nsDocumentViewer has no real way to know about this scriptable top weirdness in nsGlobalWindow.  We should move this stuff to an RAII helper defined in nsGlobalWindow instead.
Created attachment 8750608 [details] [diff] [review]
Add a sane way of temporarily disabling dialogs on a window

What we have right now is not very usable, because you can only enable/disable
on the scriptable top of the window, but it's not at all obvious that this is
the case when using the API.
Attachment #8750608 - Flags: review?(bugs)

Comment 2

2 years ago
Comment on attachment 8750608 [details] [diff] [review]
Add a sane way of temporarily disabling dialogs on a window


>+nsGlobalWindow::TemporarilyDisableDialogs::~TemporarilyDisableDialogs()
>+{
>+  if (mTopWindow) {
>+    mTopWindow->mAreDialogsEnabled = mSavedDialogsEnabled;
>+  }
So mSavedDialogsEnabled is always uninitialized here.

>+  class MOZ_RAII TemporarilyDisableDialogs {
Nit, { goes to its own line.

>-    nsGlobalWindow *globalWindow =
>-      static_cast<nsGlobalWindow*>(reinterpret_cast<nsPIDOMWindow<nsISupports>*>(window));
>+    nsGlobalWindow *globalWindow = nsGlobalWindow::Cast(window);
Nit, * goes with the type



r- because of mSavedDialogsEnabled usage.
Attachment #8750608 - Flags: review?(bugs) → review-
> So mSavedDialogsEnabled is always uninitialized here.

Er, right you are.  Fixing.
Created attachment 8750790 [details] [diff] [review]
Add a sane way of temporarily disabling dialogs on a window

What we have right now is not very usable, because you can only enable/disable
on the scriptable top of the window, but it's not at all obvious that this is
the case when using the API.
Attachment #8750790 - Flags: review?(bugs)
Attachment #8750608 - Attachment is obsolete: true

Comment 5

2 years ago
Comment on attachment 8750790 [details] [diff] [review]
Add a sane way of temporarily disabling dialogs on a window

Not sure why mSavedDialogsEnabled needs such a long comment about not using Maybe, but fine.
Attachment #8750790 - Flags: review?(bugs) → review+
The comment is really about why it's not using AutoRestore.  It'd have to be a Maybe because getting the right window is so complicated.

I'll drop the Maybe from the comment to make that clearer.
Whiteboard: btpp-active

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/184bd90a2f1e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.