Bug 126038 - Web Inspector: Extract CommandLineAPI into its own InjectedScriptModule
Summary: Web Inspector: Extract CommandLineAPI into its own InjectedScriptModule
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-12-19 18:48 PST by Joseph Pecoraro
Modified: 2013-12-20 14:01 PST (History)
14 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>