Closed Bug 1271521 Opened 8 years ago Closed 8 years ago

Dialogs-enabled bits in nsDocumentViewer are broken

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1190641
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 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.
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 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
https://hg.mozilla.org/mozilla-central/rev/184bd90a2f1e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: