Bug 79027 - Output a message on the console when using showModalDialog discouraging its use
Summary: Output a message on the console when using showModalDialog discouraging its use
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-20 04:39 PST by jochen
Modified: 2014-12-17 16:36 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.21 KB, patch)
2012-02-20 04:39 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (4.14 KB, patch)
2012-02-21 03:46 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (4.17 KB, patch)
2012-02-24 01:48 PST, jochen
no flags Details | Formatted Diff | Diff
Patch for landing (5.77 KB, patch)
2012-02-24 12:29 PST, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2012-02-20 04:39:25 PST
Output a message on the console when using showModalDialog discouraging its use
Comment 1 jochen 2012-02-20 04:39:56 PST
Created attachment 127807 [details]
Patch
Comment 2 jochen 2012-02-20 04:40:45 PST
Hey Sam, what do you think about this change?
Comment 3 WebKit Review Bot 2012-02-20 06:38:18 PST
Comment on attachment 127807 [details]
Patch

Attachment 127807 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11552166

New failing tests:
inspector/console/console-long-eval-crash.html
Comment 4 Adam Barth 2012-02-20 19:53:27 PST
Comment on attachment 127807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127807&action=review

> Source/WebCore/page/DOMWindow.cpp:1903
> +    DEFINE_STATIC_LOCAL(String, consoleMessage, ("consider using window.open instead of window.showModalDialog"));

Can we say something stronger about how showModalDialog might be removed in a future version?  (Also, please capitalize the "c" in Consider.)
Comment 5 jochen 2012-02-21 03:46:06 PST
Created attachment 127951 [details]
Patch
Comment 6 Adam Barth 2012-02-22 15:22:15 PST
Comment on attachment 127951 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127951&action=review

> Source/WebCore/page/DOMWindow.cpp:1903
> +    DEFINE_STATIC_LOCAL(String, consoleMessage, ("window.showModalDialog is broken and deprecated in WebKit. It will be removed from the engine in the future."));

Apple isn't going to like commenting on future plans.  Perhaps "It will likely be removed ..." ?
Comment 7 WebKit Commit Bot 2012-02-23 19:28:54 PST
Attachment 127951 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 jochen 2012-02-24 01:48:02 PST
Created attachment 128688 [details]
Patch
Comment 9 jochen 2012-02-24 12:29:33 PST
Created attachment 128782 [details]
Patch for landing
Comment 10 Alexey Proskuryakov 2012-02-24 14:22:42 PST
Comment on attachment 128782 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=128782&action=review

As you already mentioned on IRC, the value of these messages is questionable at best.

I don't think that users of enterprise software that likes showModalDialog would be a grateful audience for warnings they can't fix (OTOH, end users don't open Web Inspector anyway).

Another reason why showModalDialog would be hard to remove in foreseeable future is that Adobe installer on OS X uses it (not on the web, but from a WebView in a native application).

> Source/WebCore/ChangeLog:6
> +        Reviewed by Jochen Eisinger.

Adam reviewed this.

> Source/WebCore/page/DOMWindow.cpp:1835
> +    DEFINE_STATIC_LOCAL(String, consoleMessage, ("window.showModalDialog is broken and deprecated in WebKit. It will likely be removed from the engine in the future."));

I don't think that WebKit should ever officially claim that something "is broken". Better wording is needed.
Comment 11 jochen 2012-02-24 15:00:01 PST
(In reply to comment #10)
> (From update of attachment 128782 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128782&action=review
> 
> As you already mentioned on IRC, the value of these messages is questionable at best.

Oh, I wouldn't put it like that. I think it can help to slow down the addition of new websites using this.

> 
> I don't think that users of enterprise software that likes showModalDialog would be a grateful audience for warnings they can't fix (OTOH, end users don't open Web Inspector anyway).
> 
> Another reason why showModalDialog would be hard to remove in foreseeable future is that Adobe installer on OS X uses it (not on the web, but from a WebView in a native application).
> 
> > Source/WebCore/ChangeLog:6
> > +        Reviewed by Jochen Eisinger.
> 
> Adam reviewed this.

ouch, good catch

> 
> > Source/WebCore/page/DOMWindow.cpp:1835
> > +    DEFINE_STATIC_LOCAL(String, consoleMessage, ("window.showModalDialog is broken and deprecated in WebKit. It will likely be removed from the engine in the future."));
> 
> I don't think that WebKit should ever officially claim that something "is broken". Better wording is needed.


Do you have a suggestion? I took this text from UIEvent.cpp
Comment 12 Brian Burg 2014-12-17 16:36:19 PST
I don't think there is interest in pushing this feature request further.