Bug 126038

Summary: Web Inspector: Extract CommandLineAPI into its own InjectedScriptModule
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: 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: P2 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

Description Joseph Pecoraro 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.
Comment 1 Radar WebKit Bug Importer 2013-12-19 18:50:47 PST 
<rdar://problem/15705590>
Comment 2 Joseph Pecoraro 2013-12-19 19:01:02 PST 
Created attachment 219713 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 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.
Comment 4 WebKit Commit Bot 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.
Comment 5 EFL EWS Bot 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
Comment 6 Joseph Pecoraro 2013-12-19 19:19:52 PST 
Created attachment 219717 [details]
[PATCH] Part 1 - Extract CommandLineAPIModule

Same comment from Part 1 still applies here.
Comment 7 WebKit Commit Bot 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.
Comment 8 Joseph Pecoraro 2013-12-20 11:23:38 PST 
Created attachment 219771 [details]
[PATCH] Part 2 - Inject CommandLineAPIModule once, instead checking over and over again
Comment 9 Joseph Pecoraro 2013-12-20 11:24:58 PST 
Created attachment 219773 [details]
[PATCH] For Bots 1
Comment 10 WebKit Commit Bot 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.
Comment 11 EFL EWS Bot 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
Comment 12 Build Bot 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
Comment 13 Joseph Pecoraro 2013-12-20 11:32:24 PST 
Created attachment 219775 [details]
[PATCH] For Bots 2
Comment 14 WebKit Commit Bot 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.
Comment 15 Joseph Pecoraro 2013-12-20 14:01:46 PST 
Committed r160924 <http://trac.webkit.org/changeset/160924>
Committed r160925 <http://trac.webkit.org/changeset/160925>