Bug 126038

Summary:

Web Inspector: Extract CommandLineAPI into its own InjectedScriptModule

Product:

WebKit

Reporter:

Joseph Pecoraro <joepeck>

Component:

Web Inspector

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, eflews.bot, graouts, gyuyoung.kim, joepeck, philn, rakuco, rego+ews, rniwa, timothy, webkit-bug-importer, xan.lopez, zan

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
[PATCH] Proposed Fix	joepeck: commit-queue-
[PATCH] Part 1 - Extract CommandLineAPIModule	timothy: review+, joepeck: commit-queue-
[PATCH] Part 2 - Inject CommandLineAPIModule once, instead checking over and over again	timothy: review+
[PATCH] For Bots 1	eflews.bot: commit-queue-
[PATCH] For Bots 2	none

Joseph Pecoraro

Reported 2013-12-19 18:48:32 PST

The CommandLineAPI that currently lives inside of InjectedScriptSource.js is infested with knowledge and functions dealing with the DOM. "getEventListeners", "$n", "$$", etc. These APIs cannot be used when inspecting a JSContext. However the rest of InjectedScriptSource is generic. Split the CommandLineAPI code out into is own module.

Attachments
[PATCH] Proposed Fix (56.88 KB, patch) 2013-12-19 19:01 PST, Joseph Pecoraro	joepeck: commit-queue-	Details Formatted Diff Diff
[PATCH] Part 1 - Extract CommandLineAPIModule (56.87 KB, patch) 2013-12-19 19:19 PST, Joseph Pecoraro	timothy: review+ joepeck: commit-queue-	Details Formatted Diff Diff
[PATCH] Part 2 - Inject CommandLineAPIModule once, instead checking over and over again (19.31 KB, patch) 2013-12-20 11:23 PST, Joseph Pecoraro	timothy: review+	Details Formatted Diff Diff
[PATCH] For Bots 1 (70.81 KB, patch) 2013-12-20 11:24 PST, Joseph Pecoraro	eflews.bot: commit-queue-	Details Formatted Diff Diff
[PATCH] For Bots 2 (71.20 KB, patch) 2013-12-20 11:32 PST, Joseph Pecoraro	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2013-12-19 18:50:47 PST

<rdar://problem/15705590>

Joseph Pecoraro

Comment 2 2013-12-19 19:01:02 PST

Created attachment 219713 [details] [PATCH] Proposed Fix

Joseph Pecoraro

Comment 3 2013-12-19 19:03:07 PST

Comment on attachment 219713 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=219713&action=review > Source/WebCore/inspector/PageRuntimeAgent.cpp:143 > JSC::ExecState* scriptState = mainWorldExecState(&m_inspectedPage->mainFrame()); > InjectedScript result = injectedScriptManager()->injectedScriptFor(scriptState); > if (result.hasNoValue()) > - *errorString = "Internal error: main world execution context not found."; > + *errorString = ASCIILiteral("Internal error: main world execution context not found."); > + CommandLineAPIModule::injectIfNeeded(injectedScriptManager(), result); > return result; > } > + > InjectedScript injectedScript = injectedScriptManager()->injectedScriptForId(*executionContextId); > if (injectedScript.hasNoValue()) > - *errorString = "Execution context with given id not found."; > + *errorString = ASCIILiteral("Execution context with given id not found."); > + CommandLineAPIModule::injectIfNeeded(injectedScriptManager(), injectedScript); > return injectedScript; > } Although this works, I don't think it is ideal. This means every time you evaluate something in the console (e.g. 1+1) we first evaluate and check the result of 'InjectedScript.module("CommandLineAPI")'. I think a better approach will be to subclass InjectedScriptManager, and provide a one evaluate at the same time we create InjectedScript. So we only have to inject the CommandLineAPI once, instead of keep checking it. I'll make this change tomorrow.

WebKit Commit Bot

Comment 4 2013-12-19 19:03:37 PST

Attachment 219713 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/GNUmakefile.am', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/inspector/CommandLineAPIModule.cpp', u'Source/WebCore/inspector/CommandLineAPIModule.h', u'Source/WebCore/inspector/CommandLineAPIModuleSource.js', u'Source/WebCore/inspector/InjectedScriptCanvasModule.h', u'Source/WebCore/inspector/InjectedScriptModule.cpp', u'Source/WebCore/inspector/InjectedScriptModule.h', u'Source/WebCore/inspector/InjectedScriptSource.js', u'Source/WebCore/inspector/InspectorAllInOne.cpp', u'Source/WebCore/inspector/PageRuntimeAgent.cpp', '--commit-queue']" exit_code: 1 ERROR: Source/WebCore/inspector/PageRuntimeAgent.cpp:133: Declaration has space between type name and * in *errorString [whitespace/declaration] [3] ERROR: Source/WebCore/inspector/PageRuntimeAgent.cpp:140: Declaration has space between type name and * in *errorString [whitespace/declaration] [3] Total errors found: 2 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

EFL EWS Bot

Comment 5 2013-12-19 19:05:54 PST

Comment on attachment 219713 [details] [PATCH] Proposed Fix Attachment 219713 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/49338152

Joseph Pecoraro

Comment 6 2013-12-19 19:19:52 PST

Created attachment 219717 [details] [PATCH] Part 1 - Extract CommandLineAPIModule Same comment from Part 1 still applies here.

WebKit Commit Bot

Comment 7 2013-12-19 19:22:16 PST

Attachment 219717 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/GNUmakefile.am', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/inspector/CommandLineAPIModule.cpp', u'Source/WebCore/inspector/CommandLineAPIModule.h', u'Source/WebCore/inspector/CommandLineAPIModuleSource.js', u'Source/WebCore/inspector/InjectedScriptCanvasModule.h', u'Source/WebCore/inspector/InjectedScriptModule.cpp', u'Source/WebCore/inspector/InjectedScriptModule.h', u'Source/WebCore/inspector/InjectedScriptSource.js', u'Source/WebCore/inspector/InspectorAllInOne.cpp', u'Source/WebCore/inspector/PageRuntimeAgent.cpp', '--commit-queue']" exit_code: 1 ERROR: Source/WebCore/inspector/PageRuntimeAgent.cpp:133: Declaration has space between type name and * in *errorString [whitespace/declaration] [3] ERROR: Source/WebCore/inspector/PageRuntimeAgent.cpp:140: Declaration has space between type name and * in *errorString [whitespace/declaration] [3] Total errors found: 2 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Joseph Pecoraro

Comment 8 2013-12-20 11:23:38 PST

Created attachment 219771 [details] [PATCH] Part 2 - Inject CommandLineAPIModule once, instead checking over and over again

Joseph Pecoraro

Comment 9 2013-12-20 11:24:58 PST

Created attachment 219773 [details] [PATCH] For Bots 1

WebKit Commit Bot

Comment 10 2013-12-20 11:26:04 PST

Attachment 219773 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/GNUmakefile.am', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/inspector/CommandLineAPIModule.cpp', u'Source/WebCore/inspector/CommandLineAPIModule.h', u'Source/WebCore/inspector/CommandLineAPIModuleSource.js', u'Source/WebCore/inspector/InjectedScriptCanvasModule.h', u'Source/WebCore/inspector/InjectedScriptManager.cpp', u'Source/WebCore/inspector/InjectedScriptManager.h', u'Source/WebCore/inspector/InjectedScriptModule.cpp', u'Source/WebCore/inspector/InjectedScriptModule.h', u'Source/WebCore/inspector/InjectedScriptSource.js', u'Source/WebCore/inspector/InspectorAllInOne.cpp', u'Source/WebCore/inspector/PageInjectedScriptManager.cpp', u'Source/WebCore/inspector/PageInjectedScriptManager.h', u'Source/WebCore/inspector/PageRuntimeAgent.cpp', '--commit-queue']" exit_code: 1 ERROR: Source/WebCore/inspector/PageInjectedScriptManager.h:35: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.

EFL EWS Bot

Comment 11 2013-12-20 11:28:56 PST

Comment on attachment 219773 [details] [PATCH] For Bots 1 Attachment 219773 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/47608249

Build Bot

Comment 12 2013-12-20 11:29:59 PST

Comment on attachment 219773 [details] [PATCH] For Bots 1 Attachment 219773 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/46678233

Joseph Pecoraro

Comment 13 2013-12-20 11:32:24 PST

Created attachment 219775 [details] [PATCH] For Bots 2

WebKit Commit Bot

Comment 14 2013-12-20 11:34:38 PST

Attachment 219775 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/GNUmakefile.am', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/inspector/CommandLineAPIModule.cpp', u'Source/WebCore/inspector/CommandLineAPIModule.h', u'Source/WebCore/inspector/CommandLineAPIModuleSource.js', u'Source/WebCore/inspector/InjectedScriptCanvasModule.h', u'Source/WebCore/inspector/InjectedScriptManager.cpp', u'Source/WebCore/inspector/InjectedScriptManager.h', u'Source/WebCore/inspector/InjectedScriptModule.cpp', u'Source/WebCore/inspector/InjectedScriptModule.h', u'Source/WebCore/inspector/InjectedScriptSource.js', u'Source/WebCore/inspector/InspectorAllInOne.cpp', u'Source/WebCore/inspector/PageDebuggerAgent.cpp', u'Source/WebCore/inspector/PageInjectedScriptManager.cpp', u'Source/WebCore/inspector/PageInjectedScriptManager.h', u'Source/WebCore/inspector/PageRuntimeAgent.cpp', '--commit-queue']" exit_code: 1 ERROR: Source/WebCore/inspector/PageInjectedScriptManager.h:35: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Joseph Pecoraro

Comment 15 2013-12-20 14:01:46 PST

Committed r160924 <http://trac.webkit.org/changeset/160924> Committed r160925 <http://trac.webkit.org/changeset/160925>

Note You need to log in before you can comment on or make changes to this bug.