Bug 28983 – WebInspector: Wrap primitive values (as objects) in InspectorController::wrap

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

[Pavel Feldman]

This way all the evaluate results will be object proxies having proper .type and .description values. One would also be able to feed evaluate results to the ObjectPropertySections.

[Pavel Feldman]

My intention was to ease the way evaluation results are rendered (for watch expressions functionality). I started doing it, but then realized that evaluating into some primitive value is a good capability and it should stay. For example we use it when calculating completions.

So instead, I would recommend reusing Console's _format capabilities for rendering watches. WebInspector.ConsoleCommandResult should render any evaluation result properly. One should not need to get into InspectorController.getProperties, Object.describe and other specifics. Just do the eval and wrap render it using this class.

[Patrick Mueller]

wrt watches: I still need to get the properties of the result of the watch expression evaluation.  That's how it currently works anyway - eval the expression, and then the result appears in the tree just like a normal property inspector.  The result's properties appear in the tree as children of the expression.  I don't believe the current ObjectProperty* code is factored well enough for me to do this easily, so I did that part "by hand".

I can imagine other uses within the debugger for the ability to get the properties of an arbitrary object, so I think this needs to be a 1st class capability.  For instance, I've been thinking of writing some kind of object explorer where you could see a graphical layout of an object, it's properties, their properties, etc.  Such a tool would also need a way to get a list of properties of an arbitrary object.

[Pavel Feldman]

(In reply to comment #2)
> wrt watches: I still need to get the properties of the result of the watch
> expression evaluation.  That's how it currently works anyway - eval the
> expression, and then the result appears in the tree just like a normal property
> inspector.  The result's properties appear in the tree as children of the
> expression.  I don't believe the current ObjectProperty* code is factored well
> enough for me to do this easily, so I did that part "by hand".
> 

Imagine that you have objectProxy that is a result of your evaluation. Try typing the following:

element.appendChild(new WebInspector.ObjectPropertiesSection(objectProxy, watchedExpression, null, true).element);

That will render what you need in one line. What doesn't work here for you? As I mentioned in this bug, rendering primitive values could be trickier, but you can check for the typeof and render it manually in one line.

> I can imagine other uses within the debugger for the ability to get the
> properties of an arbitrary object, so I think this needs to be a 1st class
> capability.  For instance, I've been thinking of writing some kind of object
> explorer where you could see a graphical layout of an object, it's properties,
> their properties, etc.  Such a tool would also need a way to get a list of
> properties of an arbitrary object.

No, it would not. There are generic capabilities that would be reused for that. Like dumping object structure in console (by means of dir(object)), allows getting and rendering arbitrary objects. You don't need to write an object explorer - it is already in place.

[Patrick Mueller]

(In reply to comment #3)
> > I can imagine other uses within the debugger for the ability to get the
> > properties of an arbitrary object, so I think this needs to be a 1st class
> > capability.  For instance, I've been thinking of writing some kind of object
> > explorer where you could see a graphical layout of an object, it's properties,
> > their properties, etc.  Such a tool would also need a way to get a list of
> > properties of an arbitrary object.
> 
> No, it would not. There are generic capabilities that would be reused for that.
> Like dumping object structure in console (by means of dir(object)), allows
> getting and rendering arbitrary objects. You don't need to write an object
> explorer - it is already in place.

I'm suggesting I might want to write an alternative UI to explore objects and their properties.  I think you're suggesting that no one will ever need this.  Which is also saying that the presentation can't be separated from the model, in terms of getting the properties of an object - there IS only one way to ever do this and render the UI.

I'm guessing that with the new proxy stuff here, there are memory- and performance-related impacts to making calls like InspectorController.getProperties(); namely, every property that comes back over as a proxy will be, for the life of the session, tracked in some kind of proxy-handle/object map.  Is that the concern with exposing a capability of getting the properties from an object?  It seems like a pretty basic API that you want in your toolkit when building debugging tools.  Do we just need to be careful when using these APIs?  Is there some cleanup we can perform?  Do we need to add weak support to the JS engines so we can automagically clean up the mapping tables?

[Pavel Feldman]

(In reply to comment #4)
> (In reply to comment #3)
> > > I can imagine other uses within the debugger for the ability to get the
> > > properties of an arbitrary object, so I think this needs to be a 1st class
> > > capability.  For instance, I've been thinking of writing some kind of object
> > > explorer where you could see a graphical layout of an object, it's properties,
> > > their properties, etc.  Such a tool would also need a way to get a list of
> > > properties of an arbitrary object.
> > 
> > No, it would not. There are generic capabilities that would be reused for that.
> > Like dumping object structure in console (by means of dir(object)), allows
> > getting and rendering arbitrary objects. You don't need to write an object
> > explorer - it is already in place.
> 
> I'm suggesting I might want to write an alternative UI to explore objects and
> their properties.  I think you're suggesting that no one will ever need this. 
> Which is also saying that the presentation can't be separated from the model,
> in terms of getting the properties of an object - there IS only one way to ever
> do this and render the UI.
> 
> I'm guessing that with the new proxy stuff here, there are memory- and
> performance-related impacts to making calls like
> InspectorController.getProperties(); namely, every property that comes back
> over as a proxy will be, for the life of the session, tracked in some kind of
> proxy-handle/object map.  Is that the concern with exposing a capability of
> getting the properties from an object?  It seems like a pretty basic API that
> you want in your toolkit when building debugging tools.  Do we just need to be
> careful when using these APIs?  Is there some cleanup we can perform?  Do we
> need to add weak support to the JS engines so we can automagically clean up the
> mapping tables?

Getting properties of an object is not a big deal given that you have a valid ObjectProxy for it. You are right, depending on the object id type, it has different lifetime. Like evaluation results are cached on the injected script side and that cache is being cleaned up when console is cleared. So you should be a bit careful when managing these object proxies.

But that is not the main reason I did not want you to use this API. I think there should be a limited number of UI primitives that we render page-side-objects with such as ObjectPropertySection. I think we should stick to them and make sure they are flexible enough. It was clear to me that ObjectPropertySection was solving your problem and I sent you a snippet with the code. If your requirements have changed since then, let us revisit it.

[Patrick Mueller]

(In reply to comment #5)

> Getting properties of an object is not a big deal given that you have a valid
> ObjectProxy for it. You are right, depending on the object id type, it has
> different lifetime. Like evaluation results are cached on the injected script
> side and that cache is being cleaned up when console is cleared. So you should
> be a bit careful when managing these object proxies.

It would probably be nice to have some documentation on this stuff, if it's not already documented somewhere.

> But that is not the main reason I did not want you to use this API. I think
> there should be a limited number of UI primitives that we render
> page-side-objects with such as ObjectPropertySection. I think we should stick
> to them and make sure they are flexible enough. It was clear to me that
> ObjectPropertySection was solving your problem and I sent you a snippet with
> the code. If your requirements have changed since then, let us revisit it.

My concerns aren't related to the Watch Expressions support - it's more of a generic concern regarding not having an API to get the properties of an object in the target window.  I don't have a need for such a an API at the moment (assuming I can get the ObjectPropertiesSection flavored version of Watch Expressions to work), just thinking generically.

[Yury Semikhatsky]

Created attachment 39428 [details]
patch

[Pavel Feldman]

Patch looks good to me.

[Patrick Mueller]

(In reply to comment #7)
> Created an attachment (id=39428) [details]

I'm glad to see that catching the case of null being typeof "object", as I got hung up on this yesterday.

Do you have test cases?  I'd like to ensure we are testing for all the oddball cases like NaN, Infinity, etc as well.  I think I noticed some of the eval()-y bits were returning results looking like null for NaN's or Infinity.  For instance, evaluating

- 1/0
- Infinity
- NaN

in the console all yield null right now, which isn't right.

[Yury Semikhatsky]

> Do you have test cases? 
Not really. We definitely need to write some unit tests for the inspector.


> I'd like to ensure we are testing for all the oddball
> cases like NaN, Infinity, etc as well.  I think I noticed some of the eval()-y
> bits were returning results looking like null for NaN's or Infinity.  For
> instance, evaluating
> 
> - 1/0
With the patch applied that yields Infinity.

> - Infinity
> - NaN
> 
Evaluating 1-a gives me NaN in the console.

> in the console all yield null right now, which isn't right.
Can you confirm that with this patch you also have null results?

[Patrick Mueller]

(In reply to comment #10)
> > Do you have test cases? 
> Not really. We definitely need to write some unit tests for the inspector.

I've been creating manual test cases in WebCore/manual-tests/inspector, for example: named-evals.html

Better than nothing.  
 
> > in the console all yield null right now, which isn't right.
> Can you confirm that with this patch you also have null results?

Wishing I could use Git, but I can't.  Will try to test after getting my current code ship-shape, was hoping to post the patch today.  I don't think is a blocker for me, but obviously needs to get fixed eventually, and guessing at about 90% certainly this will fix me as well.

[Timothy Hatcher]

Comment on attachment 39428 [details]
patch


> +    try {
> +      var expressionResult = InjectedScript._evaluateOn(InjectedScript._window().eval, InjectedScript._window(), expression);
> +      for (var prop in expressionResult)
> +          props[prop] = true;
> +      if (includeInspectorCommandLineAPI)
> +          for (var prop in InjectedScript._window()._inspectorCommandLineAPI)
> +              if (prop.charAt(0) !== '_')
> +                  props[prop] = true;
> +    } catch(e) {
> +    }
> +    return props;

Looks fine, but you use 2 space indentation in the try block.

[Yury Semikhatsky]

Created attachment 39536 [details]
patch

[Yury Semikhatsky]

(In reply to comment #12)
> (From update of attachment 39428 [details])
> 
> > +    try {
> > +      var expressionResult = InjectedScript._evaluateOn(InjectedScript._window().eval, InjectedScript._window(), expression);
> > +      for (var prop in expressionResult)
> > +          props[prop] = true;
> > +      if (includeInspectorCommandLineAPI)
> > +          for (var prop in InjectedScript._window()._inspectorCommandLineAPI)
> > +              if (prop.charAt(0) !== '_')
> > +                  props[prop] = true;
> > +    } catch(e) {
> > +    }
> > +    return props;
> 
> Looks fine, but you use 2 space indentation in the try block.

Fixed.

[Timothy Hatcher]

Comment on attachment 39536 [details]
patch

> diff --git a/WebCore/inspector/InspectorController.cpp b/WebCore/inspector/InspectorController.cpp

> +    try {
> +      var expressionResult = InjectedScript._evaluateOn(InjectedScript._window().eval, InjectedScript._window(), expression);
> +      for (var prop in expressionResult)
> +          props[prop] = true;
> +      if (includeInspectorCommandLineAPI)
> +          for (var prop in InjectedScript._window()._inspectorCommandLineAPI)
> +              if (prop.charAt(0) !== '_')
> +                  props[prop] = true;
> +    } catch(e) {
> +    }
> +    return props;

I still see 2 space indents (only on one level.)

[Yury Semikhatsky]

Created attachment 39551 [details]
patch

[Yury Semikhatsky]

Created attachment 39688 [details]
previous patch with changelog entry

[WebKit Commit Bot]

Comment on attachment 39688 [details]
previous patch with changelog entry

Rejecting patch 39688 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Committing to http://svn.webkit.org/repository/webkit/trunk ...
Merge conflict during commit: Your file or directory 'WebCore/ChangeLog' is probably out-of-date: resource out of date; try updating at /usr/local/libexec/git-core//git-svn line 469

[Eric Seidel (no email)]

Comment on attachment 39688 [details]
previous patch with changelog entry

Race during landing.  We were bit by bug 28316 which I will be fixing this week.  Sorry!

[WebKit Commit Bot]

Comment on attachment 39688 [details]
previous patch with changelog entry

Clearing flags on attachment: 39688

Committed r48491: <http://trac.webkit.org/changeset/48491>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.