bindings,devtools: Shows DOM attributes' values in DevTools.

bindings,devtools: Shows DOM attributes' values in DevTools.

A new utility function V8DOMWrapper::isWrapper provides a better way to
tell whether it's a DOM wrapper or not.  Using this function, improves
the inspector to show DOM attributes' values and not to show DOM attributes
which have observable side effect on getter.

BUG=450623, 43394
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191617

[Yuki, 2015-03-05 12:19:53]

yukishiino@chromium.org changed reviewers:
+ haraken@chromium.org, pfeldman@chromium.org

[Yuki, 2015-03-05 12:19:54]

Could you review this CL?

V8DOMWrapper::isWrapper has been introduced and it safely tells whether it's a
DOM wrapper or not.  So we change the approach again from whitelisting to
blacklisting.

This time, we don't use an extended attribute.

[haraken, 2015-03-05 15:14:26]

https://codereview.chromium.org/978233002/diff/1/Source/core/inspector/Inject...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/978233002/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptSource.js:558: if ("get" in descriptor &&
"set" in descriptor && name != "__proto__" &&
InjectedScriptHost.isDOMAttributeWithNoSideEffect(object, name)) {

If pfeldman@ is fine, IMHO, we should do the blacklisting in JS instead of C++,
because JS-side blacklisting would be much simpler and thus safer (this CL
introduces a bunch of complicated logic to C++). For example, we can create a
helper function like:

  function isDOMAttributeWithNoSideEffect(object, name) {
    if (object instanceof Request && name == "body")
      return false;
    if (object instanceof Response && name == "body")
      return false;
    return true;
  }

If it is not acceptable, I'm OK with the current approach.

[pfeldman, 2015-03-05 17:42:55]

https://codereview.chromium.org/978233002/diff/1/Source/core/inspector/Inject...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/978233002/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptSource.js:558: if ("get" in descriptor &&
"set" in descriptor && name != "__proto__" &&
InjectedScriptHost.isDOMAttributeWithNoSideEffect(object, name)) {
I'm fine with it as long as the instanceof Request / Response is implemented in
the bindings.

[haraken, 2015-03-06 02:29:18]

haraken@chromium.org changed reviewers:
+ jl@opera.com

[haraken, 2015-03-06 02:29:19]

https://codereview.chromium.org/978233002/diff/1/Source/core/inspector/Inject...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/978233002/diff/1/Source/core/inspector/Inject...
Source/core/inspector/InjectedScriptSource.js:558: if ("get" in descriptor &&
"set" in descriptor && name != "__proto__" &&
InjectedScriptHost.isDOMAttributeWithNoSideEffect(object, name)) {
On 2015/03/05 17:42:54, pfeldman wrote:
> I'm fine with it as long as the instanceof Request / Response is implemented
in
> the bindings.

If we want to implement the instanceof Request/Response in bindings/, we need to
implement it in C++, resulting in this CL...

One way to avoid the complexity would be to implement
InjectedScriptHost.isDOMAttributeWithNoSideEffect in Blink-in-JS. Then we can
put it in bindings/ but can write the logic in JS. This might be worth doing,
because we can move a lot of complicated existing C++ in
V8InjectedScriptHostCustom.cpp to the JS as well.

Jens?

[pfeldman, 2015-03-06 05:03:15]

> If we want to implement the instanceof Request/Response in bindings/, we need
to
> implement it in C++, resulting in this CL...
> 

I was thinking of a more specialized binding that would return canonical type
array per given instance ["Object", "Blah", "Response"].

> One way to avoid the complexity would be to implement
> InjectedScriptHost.isDOMAttributeWithNoSideEffect in Blink-in-JS. Then we can
> put it in bindings/ but can write the logic in JS. This might be worth doing,
> because we can move a lot of complicated existing C++ in
> V8InjectedScriptHostCustom.cpp to the JS as well.

Could you give me a better idea on what from V8InjectedScriptHostCustom.cpp can
go into blink-in-js? Note that we are generally unhappy about the code in
InjectedScriptSource.js and are willing to do more outside of page context. Both
native and blink-in-js would do.

> 
> Jens?

[pfeldman, 2015-03-06 05:03:24]

pfeldman@chromium.org changed reviewers:
+ yurys@chromium.org

[haraken, 2015-03-06 05:34:42]

On 2015/03/06 05:03:15, pfeldman wrote:
> > If we want to implement the instanceof Request/Response in bindings/, we
need
> to
> > implement it in C++, resulting in this CL...
> > 
> 
> I was thinking of a more specialized binding that would return canonical type
> array per given instance ["Object", "Blah", "Response"].

ah, this sounds like a good approach.

> > One way to avoid the complexity would be to implement
> > InjectedScriptHost.isDOMAttributeWithNoSideEffect in Blink-in-JS. Then we
can
> > put it in bindings/ but can write the logic in JS. This might be worth
doing,
> > because we can move a lot of complicated existing C++ in
> > V8InjectedScriptHostCustom.cpp to the JS as well.
> 
> Could you give me a better idea on what from V8InjectedScriptHostCustom.cpp
can
> go into blink-in-js? Note that we are generally unhappy about the code in
> InjectedScriptSource.js and are willing to do more outside of page context.
Both
> native and blink-in-js would do.

If you add an [ImplementedInPrivateScript] IDL attribute on
InjectedScriptHost.idl, then the DOM attribute/method is automatically
redirected to JavaScript (say, bindings/core/v8/custom/InjectedScriptHost.js).
The binding code that connects the DOM attribute/method and the JavaScript is
auto-generated. Blink-in-JS is a way to implement DOM attribute/method (in this
case, InjectedScriptHost.isDOMAttributeWithNoSideEffect) in JS.

[pfeldman, 2015-03-06 05:36:28]

> If you add an [ImplementedInPrivateScript] IDL attribute on
> InjectedScriptHost.idl, then the DOM attribute/method is automatically
> redirected to JavaScript (say, bindings/core/v8/custom/InjectedScriptHost.js).
> The binding code that connects the DOM attribute/method and the JavaScript is
> auto-generated. Blink-in-JS is a way to implement DOM attribute/method (in
this
> case, InjectedScriptHost.isDOMAttributeWithNoSideEffect) in JS.

Right, but in most cases we start off JS with InjectedScriptSource and then go
into native bindings for things that we could not do in JS. My point was that
there is little in V8InjectedScriptHostCustom.cpp that could be done in JS.

[haraken, 2015-03-06 05:40:30]

On 2015/03/06 05:36:28, pfeldman wrote:
> > If you add an [ImplementedInPrivateScript] IDL attribute on
> > InjectedScriptHost.idl, then the DOM attribute/method is automatically
> > redirected to JavaScript (say,
bindings/core/v8/custom/InjectedScriptHost.js).
> > The binding code that connects the DOM attribute/method and the JavaScript
is
> > auto-generated. Blink-in-JS is a way to implement DOM attribute/method (in
> this
> > case, InjectedScriptHost.isDOMAttributeWithNoSideEffect) in JS.
> 
> Right, but in most cases we start off JS with InjectedScriptSource and then go
> into native bindings for things that we could not do in JS. My point was that
> there is little in V8InjectedScriptHostCustom.cpp that could be done in JS.

I thought there would be many (e.g., isHTMLAllCollectionMethodCustom,
internalConstructorNameMethodCustom etc), but I haven't looked at it in details.

Either way I agree that your proposal to create a binding method that returns
["Response", "Request"] would be simpler to solve the current problem.

[yurys, 2015-03-06 09:55:40]

https://codereview.chromium.org/978233002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/978233002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InjectedScriptSource.js:242: if (window[interfaceName] &&
object instanceof window[interfaceName]) {
Note that window[interfaceName] may well be overwritten by user code and this
code alone can have unwanted side effects.

[Yuki, 2015-03-06 14:15:57]

Could you guys take another look?

https://codereview.chromium.org/978233002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/978233002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InjectedScriptSource.js:242: if (window[interfaceName] &&
object instanceof window[interfaceName]) {
On 2015/03/06 09:55:39, yurys wrote:
> Note that window[interfaceName] may well be overwritten by user code and this
> code alone can have unwanted side effects. 

I've enclosed this part with InjectedScriptHost.suppressWarningsAndCallFunction.

[haraken, 2015-03-06 14:21:55]

bindings/ LGTM.

[Yuki, 2015-03-09 10:19:05]

pfeldman@, could you take another look?

[pfeldman, 2015-03-09 11:41:41]

https://codereview.chromium.org/978233002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/978233002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:566: typedef
Vector<std::pair<String, bool>> AttributeSet;
I don't see why this is being created here, it is way simpler to do this in the
InjectedScriptSource.js

https://codereview.chromium.org/978233002/diff/40001/Source/core/inspector/In...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/978233002/diff/40001/Source/core/inspector/In...
Source/core/inspector/InjectedScriptSource.js:232: var
domAttributesWithObservableSideEffectOnGet =
InjectedScriptHost.getDOMAttributesWithObservableSideEffectOnGet();
var domAttributesWithObservableSideEffectOnGet = nullifyObjectProto({});
domAttributesWithObservableSideEffectOnGet["Request"] = nullifyObjectProto({});
domAttributesWithObservableSideEffectOnGet["Request"]["body"] = true;

https://codereview.chromium.org/978233002/diff/40001/Source/core/inspector/In...
Source/core/inspector/InjectedScriptSource.js:243: return typeof
window[interfaceName] === "function" && object instanceof window[interfaceName];
This is no better than what it used to be as long as we are fetching the
window[interfaceName]. Page can easily override it. Our options are:

1) not care about it at all, which might lead to a new devtools detection
technique
2) fix it as I was suggesting via returning the canonical types from the
bindings:

var types = InjectedScriptHost.canonicalTypes(object); // returns ["Request",
"Object"]
for (var i = 0; i < types.length; ++i) {
    if (domAttrsWithSideEffects[types[i]] &&
domAttrsWithSideEffects[types[i]][attribute])
        return true;
}
return false;

[Yuki, 2015-03-09 12:48:13]

https://codereview.chromium.org/978233002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/978233002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:566: typedef
Vector<std::pair<String, bool>> AttributeSet;
On 2015/03/09 11:41:41, pfeldman wrote:
> I don't see why this is being created here, it is way simpler to do this in
the
> InjectedScriptSource.js

I misunderstood that you disliked to have this code in InjectedScriptSource.js. 
I'm happier with having this code in *.js.

https://codereview.chromium.org/978233002/diff/40001/Source/core/inspector/In...
File Source/core/inspector/InjectedScriptSource.js (right):

https://codereview.chromium.org/978233002/diff/40001/Source/core/inspector/In...
Source/core/inspector/InjectedScriptSource.js:232: var
domAttributesWithObservableSideEffectOnGet =
InjectedScriptHost.getDOMAttributesWithObservableSideEffectOnGet();
On 2015/03/09 11:41:41, pfeldman wrote:
> var domAttributesWithObservableSideEffectOnGet = nullifyObjectProto({});
> domAttributesWithObservableSideEffectOnGet["Request"] =
nullifyObjectProto({});
> domAttributesWithObservableSideEffectOnGet["Request"]["body"] = true;

Done.

https://codereview.chromium.org/978233002/diff/40001/Source/core/inspector/In...
Source/core/inspector/InjectedScriptSource.js:243: return typeof
window[interfaceName] === "function" && object instanceof window[interfaceName];
On 2015/03/09 11:41:41, pfeldman wrote:
> This is no better than what it used to be as long as we are fetching the
> window[interfaceName]. Page can easily override it. Our options are:
> 
> 1) not care about it at all, which might lead to a new devtools detection
> technique
> 2) fix it as I was suggesting via returning the canonical types from the
> bindings:
> 
> var types = InjectedScriptHost.canonicalTypes(object); // returns ["Request",
> "Object"]
> for (var i = 0; i < types.length; ++i) {
>     if (domAttrsWithSideEffects[types[i]] &&
> domAttrsWithSideEffects[types[i]][attribute])
>         return true;
> }
> return false;

I prefer option 1.  I don't think it's worth creating a new mechanism to
retrieve class inheritance info as string, which we don't have for now.

We already check that the given object is a DOM wrapper.  I think it's enough.

[pfeldman, 2015-03-10 10:18:05]

lgtm

[Yuki, 2015-03-10 10:19:09]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2015-03-10 10:19:10]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/978233002/#ps80001 (title:
"Addressed review comments.")

[commit-bot: I haz the power, 2015-03-10 10:25:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/978233002/80001

[commit-bot: I haz the power, 2015-03-10 11:59:04]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191617

[yurys, 2015-03-10 13:43:28]

This CL introduced new JS compiler errors:

$ Source/devtools/scripts/compile_frontend.py 
Java executable: /usr/local/buildtools/java/jdk7/bin/java
Checking duplicate files across modules...
Compiling frontend...
Compiling InjectedScriptSource.js and InjectedScriptCanvasModuleSource.js...
Verifying JSDoc comments...
/sources/chromium/src/third_party/WebKit/Source/core/inspector/InjectedScriptSourceTmp.js:240:
ERROR - Non-object type explicitly marked with "!" (non-nullable), which is the
default and should be omitted
 * @param {!string} attribute
           ^

[Yuki, 2015-03-10 13:44:28]

On 2015/03/10 13:43:28, yurys wrote:
> This CL introduced new JS compiler errors:
> 
> $ Source/devtools/scripts/compile_frontend.py 
> Java executable: /usr/local/buildtools/java/jdk7/bin/java
> Checking duplicate files across modules...
> Compiling frontend...
> Compiling InjectedScriptSource.js and InjectedScriptCanvasModuleSource.js...
> Verifying JSDoc comments...
>
/sources/chromium/src/third_party/WebKit/Source/core/inspector/InjectedScriptSourceTmp.js:240:
> ERROR - Non-object type explicitly marked with "!" (non-nullable), which is
the
> default and should be omitted
>  * @param {!string} attribute
>            ^

Thanks for letting me know.
Will fix very soon.