Bug 237046 – Use of showModalDialog should appear as a warning in WI console

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Tim Nguyen (:ntim)]

rdar://89261960

[Tim Nguyen (:ntim)]

<rdar://problem/89261960>

[Tim Nguyen (:ntim)]

<rdar://89261960>

[Tim Nguyen (:ntim)]

Created attachment 452886 [details]
Patch

[Devin Rousso]

Comment on attachment 452886 [details]
Patch

rs=me

[Darin Adler]

Comment on attachment 452886 [details]
Patch

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

> Source/WebCore/page/DOMWindow.cpp:2673
> +    if (RefPtr document = this->document())
> +        document->addConsoleMessage(MessageSource::JS, MessageLevel::Warning, "The showModalDialog() API is deprecated, please use the <dialog> element instead."_s);

We should use an idiom more like what is used in DOMWindow::printErrorMessage. Maybe add a printWarningMessage function alongside that and use it? There’s no need to go through the document to get to the console.

Ideally, would like this wording to match what we use elsewhere. Not sure we should use "()" after the function name, nor the term "API", not sure we should say "deprecated" either. Error messages in this file also say "window." in them.

[Tim Nguyen (:ntim)]

(In reply to Darin Adler from comment #5)
> Comment on attachment 452886 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452886&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:2673
> > +    if (RefPtr document = this->document())
> > +        document->addConsoleMessage(MessageSource::JS, MessageLevel::Warning, "The showModalDialog() API is deprecated, please use the <dialog> element instead."_s);
> 
> We should use an idiom more like what is used in
> DOMWindow::printErrorMessage. Maybe add a printWarningMessage function
> alongside that and use it? There’s no need to go through the document to get
> to the console.

I'm doing this in bug 237047

> Ideally, would like this wording to match what we use elsewhere. Not sure we
> should use "()" after the function name, nor the term "API", not sure we
> should say "deprecated" either. Error messages in this file also say
> "window." in them.

I'll take care of this along with gardening tomorrow.

[Tim Nguyen (:ntim)]

Created attachment 452939 [details]
[fast-cq] Patch

[EWS]

Committed r290351 (247669@main): <https://commits.webkit.org/247669@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452939 [details].

[Radar WebKit Bug Importer]

<rdar://problem/89339650>

[Robert Jenner]

It appears that the following three tests also need a rebase to take into account the new console warning:

fast/events/beforeunload-showModalDialog.html 
fast/events/unload-showModalDialog.html 
fast/events/pagehide-showModalDialog.html


HISTORY:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&test=fast%2Fevents%2Fbeforeunload-showModalDialog.html&test=fast%2Fevents%2Fpagehide-showModalDialog.html&test=fast%2Fevents%2Funload-showModalDialog.html

Basically, these all started failing on Mac wk1 and Windows wk1 at r290351. So I have re-based them here: 

https://trac.webkit.org/changeset/290404/webkit

[Robert Jenner]

Reverted r290351 and r290404 for reason:

Broke the build after revert of initial commit (r290348) Rebases no longer needed due to revert

Committed r290406 (?): <https://commits.webkit.org/r290406>

[Robert Jenner]

Reverted r290351 and r290404 for reason:

Broke the build after revert of initial commit (r290348) Rebases no longer needed due to revert

Committed r290406 (?): <https://commits.webkit.org/r290406>

[Robert Jenner]

(In reply to Robert Jenner from comment #12)
> Reverted r290351 and r290404 for reason:
> 
> Broke the build after revert of initial commit (r290348) Rebases no longer
> needed due to revert
> 
> Committed r290406 (?): <https://commits.webkit.org/r290406>

r290351 needed to be reverted because it referenced 'printWarningMessage' changes that were made at r290348. That was reverted at r290399, but because r290351 referenced that, we had to revert that as well because it caused build failures. Also reverted was r290404 which were rebases for wk1 Mac and wk1 Win that needed to happen due to the new console warnings.

[Tim Nguyen (:ntim)]

Created attachment 453071 [details]
Patch

[EWS]

Committed r290412 (247720@main): <https://commits.webkit.org/247720@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453071 [details].