Closed
Bug 1271521
Opened 8 years ago
Closed 8 years ago
Dialogs-enabled bits in nsDocumentViewer are broken
Categories
(Core :: DOM: Navigation, defect)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Whiteboard: btpp-active)
Attachments
(1 file, 1 obsolete file)
5.61 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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•8 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-
Assignee | ||
Comment 3•8 years ago
|
||
> So mSavedDialogsEnabled is always uninitialized here.
Er, right you are. Fixing.
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8750608 -
Attachment is obsolete: true
Comment 5•8 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+
Assignee | ||
Comment 6•8 years ago
|
||
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.
Updated•8 years ago
|
Whiteboard: btpp-active
Comment 8•8 years ago
|
||
bugherder |
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.
Description
•