Closed Bug 933040 Opened 7 years ago Closed 7 years ago

Warn for showModalDialog uses

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: sicking, Assigned: vendo)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [good first bug][mentor=jdm][lang=c++])

Attachments

(1 file, 2 obsolete files)

showModalDialog is a terrible API, both to implement and to use. And it's relatively little used on the web.

We should try to remove the last remaining usage by warning in the developer console whenever it is used. Chrome just did the same so we're not going alone.
This should be trivial, we just need to call ReportUseOfDeprecatedMethod() in nsGlobalWindow::ShowModalDialog() here, and set the message up for localization etc.
Whiteboard: good first bug
Whiteboard: good first bug → [good first bug][mentor=jdm][lang=c++]
We also have WarnOnceAbout() which appear to have the same purpose as ReportUseOfDeprecatedMethod(). I filed bug 933563 on consolidating them :)
I would like to work on it.
Flags: needinfo?(nobody)
Assignee: nobody → riteshnpatel1994
Flags: needinfo?(nobody)
Attached patch bug933040 (obsolete) — Splinter Review
patch
ups I am sorry Ritesh. I have done it meanwhile.
Attachment #826031 - Flags: review?(josh)
Assignee: riteshnpatel1994 → vendo.ruzicka
Attachment #826031 - Flags: review?(josh) → review?(jonas)
Comment on attachment 826031 [details] [diff] [review]
bug933040

Stealing review, and thanks vendo! r=jst
Attachment #826031 - Flags: review?(jonas) → review+
Comment on attachment 826031 [details] [diff] [review]
bug933040

>+# LOCALIZATION NOTE: Do not translate "Window.showModalDialog()"
>+ShowModalDialogWarning=Use of Window.showModalDialog() is deprecated. There is no short answer what to use instead, basically all JavaScript frameworks provide an alternative. For more help https://developer.mozilla.org/en-US/docs/Web/API/Window.showModalDialog

"Window" should be lower cased (except inside the URL).
Attached patch bug933040 (obsolete) — Splinter Review
lower case applied
Attachment #826031 - Attachment is obsolete: true
Attachment #826265 - Flags: review?(jst)
Attachment #826265 - Flags: review?(jst) → review+
Comment on attachment 826031 [details] [diff] [review]
bug933040

I'd say simply refer to window.open instead. showModalDialog() doesn't provide much value over what window.open does, other than that it's synchronous, and the synchronous part is not something that any frameworks can help with.
Status: NEW → ASSIGNED
Attached patch bug933040Splinter Review
ok, patch updated
Attachment #826265 - Attachment is obsolete: true
Attachment #826925 - Flags: review?(jonas)
Attachment #826925 - Flags: review?(jonas) → review+
FWIW, I also filed bug 933042 on making WarnOnceAbout collect telemetry so that we know if/when it's safe to remove the implementation.
I guess this patch is ready to be checked in then.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e495c0d3c240
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.