Bug 89107 – Web Inspector: Extract InjectedScriptBase class from the InjectedScript

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Adaikin]

Extract methods from the InjectedScript that do not depend on the concrete InjectedScriptSource.js implementation.

[Andrey Adaikin]

Created attachment 147599 [details]
Patch

[WebKit Review Bot]

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.

[Andrey Kosyakov]

Comment on attachment 147599 [details]
Patch

LGTM

[Andrey Kosyakov]

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.

[Andrey Adaikin]

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.

[Andrey Adaikin]

Created attachment 147795 [details]
Patch to land

[WebKit Review Bot]

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.

[Pavel Feldman]

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?

[Andrey Adaikin]

(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.

[Andrey Adaikin]

Created attachment 148175 [details]
Patch

[Andrey Kosyakov]

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;

[Andrey Adaikin]

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.

[Andrey Adaikin]

Created attachment 148187 [details]
Patch

[Andrey Kosyakov]

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();
}

[Andrey Adaikin]

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.

[Andrey Adaikin]

Created attachment 148196 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 148196 [details]
Patch

LGTM. Pavel?

[Andrey Adaikin]

Created attachment 148243 [details]
=Another API proposal

[Build Bot]

Comment on attachment 148243 [details]
=Another API proposal

Attachment 148243 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12979359

[Yury Semikhatsky]

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?

[Yury Semikhatsky]

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 ?

[Andrey Adaikin]

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.

[Andrey Adaikin]

(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.

[Andrey Adaikin]

Created attachment 148392 [details]
Removed irrelevant files and rebased.

[WebKit Review Bot]

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.

[Build Bot]

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

[Early Warning System Bot]

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

[Gustavo Noronha (kov)]

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

[Pavel Feldman]

Comment on attachment 148392 [details]
Removed irrelevant files and rebased.

Looks good, please make sure it is rebaselined.

[Andrey Adaikin]

Created attachment 148417 [details]
Rebased patch to land.

[WebKit Review Bot]

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.

[Andrey Adaikin]

Created attachment 148448 [details]
Rebased again.

[WebKit Review Bot]

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.

[Pavel Feldman]

Comment on attachment 148448 [details]
Rebased again.

Clearing flags on attachment: 148448

Committed r120769: <http://trac.webkit.org/changeset/120769>

[Pavel Feldman]

All reviewed patches have been landed.  Closing bug.