Extract methods from the InjectedScript that do not depend on the concrete InjectedScriptSource.js implementation.
Created attachment 147599 [details] Patch
Attachment 147599 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/inspector/InjectedScriptBase.h:63: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScriptBase.h:64: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScriptBase.cpp:90: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScriptBase.cpp:109: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 4 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 147599 [details] Patch LGTM
Comment on attachment 147599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147599&action=review > Source/WebCore/inspector/InjectedScript.cpp:53 > + : InjectedScriptBase() This looks redundant.
Comment on attachment 147599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147599&action=review >> Source/WebCore/inspector/InjectedScript.cpp:53 >> + : InjectedScriptBase() > > This looks redundant. Removed.
Created attachment 147795 [details] Patch to land
Attachment 147795 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/inspector/InjectedScriptBase.h:63: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScriptBase.h:64: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScriptBase.cpp:90: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScriptBase.cpp:109: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 4 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 147795 [details] Patch to land It is not clear where this is going. Do you have a complete proof of concept implementation of what you are targeting?
(In reply to comment #8) > (From update of attachment 147795 [details]) > It is not clear where this is going. Do you have a complete proof of concept implementation of what you are targeting? So, we will have one InjectedScript coupled with the InjectedScriptManager and InjectedScriptSource.js, but support injecting different modules (such as WebGL injected script) into it. This way we will hide module implementation details from those classes.
Created attachment 148175 [details] Patch
Comment on attachment 148175 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148175&action=review > Source/WebCore/inspector/InjectedScript.cpp:260 > + ScriptValue resultValue = callFunctionWithEvalEnabled(function, hadException); > + ASSERT(!hadException); > + > + if (hadException || resultValue.hasNoValue() || !resultValue.isObject()) > + return ScriptObject(); So we ASSERT() if we had exception, but do not assert if we return invalid/unexpected result. Is that right? > Source/WebCore/inspector/InjectedScriptSource.js:423 > + return (this._modules[name] = eval("(" + source + ")")); please split that into assignment and return, e.g. var module = eval(...); this._modules[name] = module; return module;
Comment on attachment 148175 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148175&action=review >> Source/WebCore/inspector/InjectedScript.cpp:260 >> + return ScriptObject(); > > So we ASSERT() if we had exception, but do not assert if we return invalid/unexpected result. Is that right? Done. Added those to the ASSERT >> Source/WebCore/inspector/InjectedScriptSource.js:423 >> + return (this._modules[name] = eval("(" + source + ")")); > > please split that into assignment and return, e.g. > var module = eval(...); > this._modules[name] = module; > return module; Done.
Created attachment 148187 [details] Patch
Comment on attachment 148187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148187&action=review > Source/WebCore/inspector/InjectedScript.cpp:276 > + ASSERT(!hadException && !resultValue.hasNoValue() && resultValue.isObject()); > + > + if (hadException || resultValue.hasNoValue() || !resultValue.isObject()) The relationship between the assert and the conditional below is obscured by the inverted condition. This would better go as if (hadException || resultValue.hasNoValue() || !resultValue.isObject()) { ASSERT_NOT_REACHED(); return ScriptObject(); }
Comment on attachment 148187 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148187&action=review >> Source/WebCore/inspector/InjectedScript.cpp:276 >> + if (hadException || resultValue.hasNoValue() || !resultValue.isObject()) > > The relationship between the assert and the conditional below is obscured by the inverted condition. This would better go as > if (hadException || resultValue.hasNoValue() || !resultValue.isObject()) { > ASSERT_NOT_REACHED(); > return ScriptObject(); > } Done.
Created attachment 148196 [details] Patch
Comment on attachment 148196 [details] Patch LGTM. Pavel?
Created attachment 148243 [details] =Another API proposal
Comment on attachment 148243 [details] =Another API proposal Attachment 148243 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12979359
Comment on attachment 148243 [details] =Another API proposal View in context: https://bugs.webkit.org/attachment.cgi?id=148243&action=review > Source/WebCore/inspector/InjectedScript.h:107 > + virtual void visitArguments(ScriptCallArgumentHandler&) = 0; This API looks awkward to me, why not have just callModuleFunction(moduleName, functionName, arguments) method on InjectedScript? Can you describe what kind of modules do you need? Are they going to have some fixed API like InjectedScript or they may some random objects with a set of methods? Will they need to access injected script structures?
Comment on attachment 148196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148196&action=review > Source/WebCore/inspector/InjectedScript.h:103 > + ScriptObject getModule(const String& name); moduleForName ?
Comment on attachment 148243 [details] =Another API proposal View in context: https://bugs.webkit.org/attachment.cgi?id=148243&action=review >> Source/WebCore/inspector/InjectedScript.h:107 >> + virtual void visitArguments(ScriptCallArgumentHandler&) = 0; > > This API looks awkward to me, why not have just callModuleFunction(moduleName, functionName, arguments) method on InjectedScript? Can you describe what kind of modules do you need? Are they going to have some fixed API like InjectedScript or they may some random objects with a set of methods? Will they need to access injected script structures? What type would arguments have in the callModuleFunction(moduleName, functionName, arguments)? Module is an abstract thing with a random set of methods that would accept a random set of arguments. Probably some modules will need an access to InjectedScript.
(In reply to comment #8) > (From update of attachment 147795 [details]) > It is not clear where this is going. Do you have a complete proof of concept implementation of what you are targeting? So, we discussed the API with Pavel and came back to this patch. The idea is to add a new InjectedModule abstract class that will inherit InjectedScriptBase to reuse implementation, and which javascript code will be injected via InjectedScript's addModule. This is to be implemented next.
Created attachment 148392 [details] Removed irrelevant files and rebased.
Attachment 148392 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/inspector/InjectedScriptBase.h:63: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScriptBase.h:64: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScriptBase.cpp:90: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScriptBase.cpp:109: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 4 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 148392 [details] Removed irrelevant files and rebased. Attachment 148392 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12992021
Comment on attachment 148392 [details] Removed irrelevant files and rebased. Attachment 148392 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12981516
Comment on attachment 148392 [details] Removed irrelevant files and rebased. Attachment 148392 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12974822
Comment on attachment 148392 [details] Removed irrelevant files and rebased. Looks good, please make sure it is rebaselined.
Created attachment 148417 [details] Rebased patch to land.
Attachment 148417 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/inspector/InjectedScriptBase.h:63: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScriptBase.h:64: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScriptBase.cpp:90: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScriptBase.cpp:109: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 4 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 148448 [details] Rebased again.
Attachment 148448 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/inspector/InjectedScriptBase.h:63: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScriptBase.h:64: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScriptBase.cpp:90: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScriptBase.cpp:109: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 4 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 148448 [details] Rebased again. Clearing flags on attachment: 148448 Committed r120769: <http://trac.webkit.org/changeset/120769>
All reviewed patches have been landed. Closing bug.