Bug 165507

Summary: Web Inspector: Remove unused and mostly untested Page domain commands and events
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, joepeck, keith_miller, mark.lam, mattbaker, msaboff, nvasilyev, rniwa, saam, timothy, webkit-bug-importer
Priority: P2 Keywords: DoNotImportToRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] For Landing
rniwa: review+
[PATCH] Proposed Fix none

Joseph Pecoraro
Reported 2016-12-06 19:30:23 PST
* 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).
Attachments
[PATCH] For Landing (41.25 KB, patch)
2016-12-06 19:35 PST, Joseph Pecoraro 		rniwa: review+
DetailsFormatted DiffDiff
[PATCH] Proposed Fix (44.40 KB, patch)
2016-12-06 19:43 PST, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Joseph Pecoraro
Comment 1 2016-12-06 19:30:38 PST
*** Bug 110186 has been marked as a duplicate of this bug. ***
Joseph Pecoraro
Comment 2 2016-12-06 19:35:51 PST
Created attachment 296366 [details] [PATCH] For Landing
Joseph Pecoraro
Comment 3 2016-12-06 19:37:29 PST
Comment on attachment 296366 [details] [PATCH] For Landing Hmm, I can also remove Page.setDocumentContent.
Ryosuke Niwa
Comment 4 2016-12-06 19:38:48 PST
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
Comment 5 2016-12-06 19:43:03 PST
Created attachment 296368 [details] [PATCH] Proposed Fix
Joseph Pecoraro
Comment 6 2016-12-06 19:43:59 PST
(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 7 2016-12-06 19:58:06 PST
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
Comment 8 2016-12-06 20:00:20 PST
(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
Comment 9 2016-12-07 11:08:10 PST
(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
Comment 10 2016-12-07 11:22:02 PST
(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 11 2016-12-07 11:31:35 PST
Comment on attachment 296368 [details] [PATCH] Proposed Fix Clearing flags on attachment: 296368 Committed r209465: <http://trac.webkit.org/changeset/209465>
WebKit Commit Bot
Comment 12 2016-12-07 11:31:40 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.