Bug 165507 – Web Inspector: Remove unused and mostly untested Page domain commands and events

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* Summary:
Remove unused and mostly untested Page domain commands and events

Commands:

    Page.getScriptExecutionStatus
    Page.setScriptExecutionDisabled
    Page.handleJavaScriptDialog

Events:

    Page.javascriptDialogOpening
    Page.javascriptDialogClosed
    Page.scriptsEnabled

These were added a long time ago and have never been used in the frontend that is now in trunk. Some haven't had any port implementation (handleJavaScriptDialog).

[Joseph Pecoraro]

*** Bug 110186 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

Created attachment 296366 [details]
[PATCH] For Landing

[Joseph Pecoraro]

Comment on attachment 296366 [details]
[PATCH] For Landing

Hmm, I can also remove Page.setDocumentContent.

[Ryosuke Niwa]

Comment on attachment 296366 [details]
[PATCH] For Landing

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

> Source/WebInspectorUI/Versions/Inspector-iOS-7.0.json:-275
> -                "name": "handleJavaScriptDialog",

Do we really want to remove from the older iOS versions’ API definitions?

[Joseph Pecoraro]

Created attachment 296368 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

(In reply to comment #4)
> Comment on attachment 296366 [details]
> [PATCH] For Landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296366&action=review
> 
> > Source/WebInspectorUI/Versions/Inspector-iOS-7.0.json:-275
> > -                "name": "handleJavaScriptDialog",
> 
> Do we really want to remove from the older iOS versions’ API definitions?

We do when they aren't really supported. It gives us (when reading it) a better idea of how that version should really be used instead of everything that it exposed which might not work correctly. We only edit in rare cases, like this, when removing something that didn't work for it.

[Blaze Burg]

Comment on attachment 296368 [details]
[PATCH] Proposed Fix

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

r=me, but wait for EWS

> Source/WebCore/page/Chrome.cpp:-289
> -    InspectorInstrumentation::didRunJavaScriptDialog(cookie);

Oh gross, this would not even distinguish different kinds of alerts.

[Blaze Burg]

(In reply to comment #4)
> Comment on attachment 296366 [details]
> [PATCH] For Landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296366&action=review
> 
> > Source/WebInspectorUI/Versions/Inspector-iOS-7.0.json:-275
> > -                "name": "handleJavaScriptDialog",
> 
> Do we really want to remove from the older iOS versions’ API definitions?

These JSON files are ingested at runtime to build up Agent (i.e., DebuggerAgent) classes in WebInspectorUI. They aren't used for anything else, and we have already removed several other unused methods. If someone wants to know what backend commands shipped at that time, they'll need to go look at repository history.

[Joseph Pecoraro]

(In reply to comment #8)
> (In reply to comment #4)
> > Comment on attachment 296366 [details]
> > [PATCH] For Landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=296366&action=review
> > 
> > > Source/WebInspectorUI/Versions/Inspector-iOS-7.0.json:-275
> > > -                "name": "handleJavaScriptDialog",
> > 
> > Do we really want to remove from the older iOS versions’ API definitions?
> 
> These JSON files are ingested at runtime

Not at runtime. They compile into the UserInterface/Protocol/Legacy/7.0/InspectorBackendCommands.js with Scripts/update-LegacyInspectorBackendCommands.rb, which I run manually whenever I modify the legacy iOS protocol jsons.

[Blaze Burg]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #4)
> > > Comment on attachment 296366 [details]
> > > [PATCH] For Landing
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=296366&action=review
> > > 
> > > > Source/WebInspectorUI/Versions/Inspector-iOS-7.0.json:-275
> > > > -                "name": "handleJavaScriptDialog",
> > > 
> > > Do we really want to remove from the older iOS versions’ API definitions?
> > 
> > These JSON files are ingested at runtime
> 
> Not at runtime. They compile into the
> UserInterface/Protocol/Legacy/7.0/InspectorBackendCommands.js with
> Scripts/update-LegacyInspectorBackendCommands.rb, which I run manually
> whenever I modify the legacy iOS protocol jsons.

Oops, yes. The *JS files* are created from the JSON files and dynamically loaded by the Web Inspector.

For WebDriver, the Automation.json file is parsed directly on the web server side, and is also used for code generation in WebKit2.

[WebKit Commit Bot]

Comment on attachment 296368 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 296368

Committed r209465: <http://trac.webkit.org/changeset/209465>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.