Bug 237046 - Use of showModalDialog should appear as a warning in WI console
Summary: Use of showModalDialog should appear as a warning in WI console
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Nguyen (:ntim)
URL:
Keywords: InRadar
Depends on: 237047
Blocks:
  Show dependency treegraph
 
Reported: 2022-02-22 10:53 PST by Tim Nguyen (:ntim)
Modified: 2022-02-23 22:17 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.50 KB, patch)
2022-02-22 11:20 PST, Tim Nguyen (:ntim)
hi: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
[fast-cq] Patch (6.56 KB, patch)
2022-02-22 22:54 PST, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (11.87 KB, patch)
2022-02-23 21:25 PST, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Nguyen (:ntim) 2022-02-22 10:53:09 PST
rdar://89261960
Comment 1 Tim Nguyen (:ntim) 2022-02-22 10:53:19 PST
<rdar://problem/89261960>
Comment 2 Tim Nguyen (:ntim) 2022-02-22 10:53:31 PST
<rdar://89261960>
Comment 3 Tim Nguyen (:ntim) 2022-02-22 11:20:41 PST
Created attachment 452886 [details]
Patch
Comment 4 Devin Rousso 2022-02-22 11:25:49 PST
Comment on attachment 452886 [details]
Patch

rs=me
Comment 5 Darin Adler 2022-02-22 11:28:05 PST
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.
Comment 6 Tim Nguyen (:ntim) 2022-02-22 13:46:00 PST
(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.
Comment 7 Tim Nguyen (:ntim) 2022-02-22 22:54:17 PST
Created attachment 452939 [details]
[fast-cq] Patch
Comment 8 EWS 2022-02-22 23:24:29 PST
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].
Comment 9 Radar WebKit Bug Importer 2022-02-22 23:25:19 PST Comment hidden (duplicate, obsolete)
Comment 10 Robert Jenner 2022-02-23 17:12:41 PST
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
Comment 11 Robert Jenner 2022-02-23 17:31:40 PST
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>
Comment 12 Robert Jenner 2022-02-23 17:31:42 PST
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>
Comment 13 Robert Jenner 2022-02-23 17:41:50 PST
(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.
Comment 14 Tim Nguyen (:ntim) 2022-02-23 21:25:46 PST
Created attachment 453071 [details]
Patch
Comment 15 EWS 2022-02-23 22:17:40 PST
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].