Output a message on the console when using showModalDialog discouraging its use

Output a message on the console when using showModalDialog discouraging its use

Created attachment 127807 [details]
Patch

Hey Sam, what do you think about this change?

[WebKit Review Bot]

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

[Adam Barth]

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.)

Created attachment 127951 [details]
Patch

[Adam Barth]

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 ..." ?

[WebKit Commit Bot]

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.

Created attachment 128688 [details]
Patch

Created attachment 128782 [details]
Patch for landing

[Alexey Proskuryakov]

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.

(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

[Brian Burg]

I don't think there is interest in pushing this feature request further.