Bug 89107 - Web Inspector: Extract InjectedScriptBase class from the InjectedScript
Summary: Web Inspector: Extract InjectedScriptBase class from the InjectedScript
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Adaikin
URL:
Keywords:
Depends on:
Blocks: 88973 89530
  Show dependency treegraph
 
Reported: 2012-06-14 09:17 PDT by Andrey Adaikin
Modified: 2012-06-19 16:46 PDT (History)
20 users (show)

See Also:


Attachments
Patch (38.93 KB, patch)
2012-06-14 09:22 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch to land (38.86 KB, patch)
2012-06-15 05:51 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (8.09 KB, patch)
2012-06-18 14:52 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (8.24 KB, patch)
2012-06-18 15:31 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (8.15 KB, patch)
2012-06-18 16:03 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
=Another API proposal (9.50 KB, patch)
2012-06-18 21:42 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Removed irrelevant files and rebased. (33.95 KB, patch)
2012-06-19 12:42 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Rebased patch to land. (34.47 KB, patch)
2012-06-19 14:09 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Rebased again. (34.47 KB, patch)
2012-06-19 15:56 PDT, Andrey Adaikin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Adaikin 2012-06-14 09:17:07 PDT
Extract methods from the InjectedScript that do not depend on the concrete InjectedScriptSource.js implementation.
Comment 1 Andrey Adaikin 2012-06-14 09:22:22 PDT
Created attachment 147599 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-14 09:26:39 PDT
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 3 Andrey Kosyakov 2012-06-15 05:17:59 PDT
Comment on attachment 147599 [details]
Patch

LGTM
Comment 4 Andrey Kosyakov 2012-06-15 05:22:17 PDT
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 5 Andrey Adaikin 2012-06-15 05:51:38 PDT
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.
Comment 6 Andrey Adaikin 2012-06-15 05:51:57 PDT
Created attachment 147795 [details]
Patch to land
Comment 7 WebKit Review Bot 2012-06-15 05:55:10 PDT
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 8 Pavel Feldman 2012-06-15 06:02:05 PDT
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?
Comment 9 Andrey Adaikin 2012-06-18 14:45:41 PDT
(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.
Comment 10 Andrey Adaikin 2012-06-18 14:52:59 PDT
Created attachment 148175 [details]
Patch
Comment 11 Andrey Kosyakov 2012-06-18 15:19:47 PDT
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 12 Andrey Adaikin 2012-06-18 15:29:52 PDT
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.
Comment 13 Andrey Adaikin 2012-06-18 15:31:40 PDT
Created attachment 148187 [details]
Patch
Comment 14 Andrey Kosyakov 2012-06-18 15:57:36 PDT
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 15 Andrey Adaikin 2012-06-18 16:03:21 PDT
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.
Comment 16 Andrey Adaikin 2012-06-18 16:03:57 PDT
Created attachment 148196 [details]
Patch
Comment 17 Andrey Kosyakov 2012-06-18 16:05:53 PDT
Comment on attachment 148196 [details]
Patch

LGTM. Pavel?
Comment 18 Andrey Adaikin 2012-06-18 21:42:50 PDT
Created attachment 148243 [details]
=Another API proposal
Comment 19 Build Bot 2012-06-18 22:09:23 PDT
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 20 Yury Semikhatsky 2012-06-19 01:58:19 PDT
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 21 Yury Semikhatsky 2012-06-19 01:59:28 PDT
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 22 Andrey Adaikin 2012-06-19 07:13:57 PDT
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.
Comment 23 Andrey Adaikin 2012-06-19 12:27:21 PDT
(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.
Comment 24 Andrey Adaikin 2012-06-19 12:42:15 PDT
Created attachment 148392 [details]
Removed irrelevant files and rebased.
Comment 25 WebKit Review Bot 2012-06-19 12:46:43 PDT
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 26 Build Bot 2012-06-19 13:13:01 PDT
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 27 Early Warning System Bot 2012-06-19 13:46:09 PDT
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 28 Gustavo Noronha (kov) 2012-06-19 13:46:25 PDT
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 29 Pavel Feldman 2012-06-19 14:04:50 PDT
Comment on attachment 148392 [details]
Removed irrelevant files and rebased.

Looks good, please make sure it is rebaselined.
Comment 30 Andrey Adaikin 2012-06-19 14:09:17 PDT
Created attachment 148417 [details]
Rebased patch to land.
Comment 31 WebKit Review Bot 2012-06-19 14:12:05 PDT
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.
Comment 32 Andrey Adaikin 2012-06-19 15:56:19 PDT
Created attachment 148448 [details]
Rebased again.
Comment 33 WebKit Review Bot 2012-06-19 16:01:18 PDT
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 34 Pavel Feldman 2012-06-19 16:21:23 PDT
Comment on attachment 148448 [details]
Rebased again.

Clearing flags on attachment: 148448

Committed r120769: <http://trac.webkit.org/changeset/120769>
Comment 35 Pavel Feldman 2012-06-19 16:21:30 PDT
All reviewed patches have been landed.  Closing bug.