Bug 107883 – Web Inspector: Add support for handling modal dialogs

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Ken Kania]

We'd like to add support for handling modal dialogs opened by javascript, including onbeforeonload. I propose:

Page.modalDialogOpening [event]
frameId: <frameId>
message: String

Page.handleModalDialog [cmd]
accept: Boolean
promptText: String [optional]


Page.modalDialogOpening will be sent before asking the client to run the javascript dialog in Chrome.cpp. Page.handleModalDialog will need to be delegated to the inspector client and be asynchronous.

[Ken Kania]

Created attachment 185380 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Build Bot]

Comment on attachment 185380 [details]
Patch

Attachment 185380 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16198423

[Build Bot]

Comment on attachment 185380 [details]
Patch

Attachment 185380 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16187477

[Ken Kania]

Created attachment 185385 [details]
Patch

[Build Bot]

Comment on attachment 185385 [details]
Patch

Attachment 185385 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16202492

New failing tests:
inspector/audits/audits-panel-functional.html
inspector-protocol/page/javascriptDialogEvents.html

[Pavel Feldman]

Comment on attachment 185385 [details]
Patch

New test fails on Mac EWS. You should file a bug requesting that mac port implements client hook and disable the test on non-chromium ports. Implementation-wise, it looks good.

[Ken Kania]

Created attachment 185588 [details]
Patch

[Ken Kania]

(In reply to comment #7)
> (From update of attachment 185385 [details])
> New test fails on Mac EWS. You should file a bug requesting that mac port implements client hook and disable the test on non-chromium ports. Implementation-wise, it looks good.

The new test failed on mac-wk2 because the printf for onbeforeunload occurs in the UIProcess (TestController.cpp), while the printfs for alert/prompt/confirm occur in the WebProcess (InjectedBundlePage.cpp) and aren't printed to the test log until the end, which causes the 'CONFIRM NAVIGATION' to be at the beginning. I think the easiest fix, which my 3rd patch does, is just test the onbeforeunload case at the beginning.

[Build Bot]

Comment on attachment 185588 [details]
Patch

Attachment 185588 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16258095

[Yury Semikhatsky]

Comment on attachment 185588 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:560
> +                    { "name": "message", "type": "string", "description": "Message that will be displayed by the dialog." }

Since we cannot pass the message yet then the parameter doesn't make much sense.

> Source/WebCore/inspector/InspectorInstrumentation.h:423
> +    static void javascriptDialogOpeningImpl(InstrumentingAgents*, const String& message);

In case of pairs willFoo/didFoo methods we usually return InspectorInstrumentationCookie from the first method and pass it to the second (see http://trac.webkit.org/browser/trunk/Source/WebCore/inspector/InspectorInstrumentation.h#L220 for example). I'd use the same approach here for consistency.

> Source/WebKit/chromium/public/WebDevToolsAgent.h:56
> +        BrowserDataHintAcceptJavaScriptDialog = 2,

We don't have to explicitly specify enum values here. We could only leave "BrowserDataHintNone = 0," for clarity but since it matches default behavior I'd omit that as well.

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:589
> +    // Operation is going to be performed on the browser level.

I'd rephrase "on the browser level" -> "in the browser process"

[Ken Kania]

Created attachment 185791 [details]
Patch

[Ken Kania]

(In reply to comment #11)
> (From update of attachment 185588 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185588&action=review
> 
> > Source/WebCore/inspector/Inspector.json:560
> > +                    { "name": "message", "type": "string", "description": "Message that will be displayed by the dialog." }
> 
> Since we cannot pass the message yet then the parameter doesn't make much sense.
> 
> > Source/WebCore/inspector/InspectorInstrumentation.h:423
> > +    static void javascriptDialogOpeningImpl(InstrumentingAgents*, const String& message);
> 
> In case of pairs willFoo/didFoo methods we usually return InspectorInstrumentationCookie from the first method and pass it to the second (see http://trac.webkit.org/browser/trunk/Source/WebCore/inspector/InspectorInstrumentation.h#L220 for example). I'd use the same approach here for consistency.
> 
> > Source/WebKit/chromium/public/WebDevToolsAgent.h:56
> > +        BrowserDataHintAcceptJavaScriptDialog = 2,
> 
> We don't have to explicitly specify enum values here. We could only leave "BrowserDataHintNone = 0," for clarity but since it matches default behavior I'd omit that as well.
> 
> > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:589
> > +    // Operation is going to be performed on the browser level.
> 
> I'd rephrase "on the browser level" -> "in the browser process"

Thanks for the review. I've made all your proposed changes.

[Pavel Feldman]

Comment on attachment 185791 [details]
Patch

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

A couple of renames and it is good to go.

> Source/WebCore/inspector/Inspector.json:556
> +                "name": "willRunJavaScriptDialog",

Protocol naming that you had earlier was correct. Yury's comment was only about the InspectorInstrumentation.

[Ken Kania]

Created attachment 185803 [details]
Patch

[WebKit Review Bot]

Comment on attachment 185803 [details]
Patch

Clearing flags on attachment: 185803

Committed r141555: <http://trac.webkit.org/changeset/141555>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.