Bug 194862

Summary: Web Inspector: hovering a node inside an object preview should highlight it
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=143206
Bug Depends on:    
Bug Blocks: 194997    
Attachments:
Description Flags
[Patch] WIP
none
[Patch] WIP
none
Archive of layout-test-results from ews102 for mac-highsierra
none
Patch none

Description Devin Rousso 2019-02-20 11:58:46 PST
As an example, logging `{a:document.body}` or `{a:{b:document.body}}` and hovering the <body> preview should highlight the node.
Comment 1 Radar WebKit Bug Importer 2019-02-20 11:59:10 PST
<rdar://problem/48246433>
Comment 2 Devin Rousso 2019-02-20 12:04:53 PST
Created attachment 362523 [details]
[Patch] WIP

Needs support for collection types (e.g. `Map`, `Set`, etc)
Comment 3 Joseph Pecoraro 2019-02-20 13:03:30 PST
Comment on attachment 362523 [details]
[Patch] WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=362523&action=review

> Source/WebInspectorUI/UserInterface/Views/ObjectPreviewView.js:115
> -            this._titleElement.appendChild(WI.FormattedValue.createElementForNodePreview(this._preview));
> +            this._titleElement.appendChild(WI.FormattedValue.createElementForNodePreview(this._preview, {object: this._object}));

createElementForNodePreview doesn't look for an `object` property, though perhaps it should. It only looks for `remoteObjectAccessor`.

> Source/WebInspectorUI/UserInterface/Views/ObjectPreviewView.js:245
> +                        this._object.getProperty(property.name, (error, remoteObject, wasThrown) => {
> +                            if (!error && remoteObject && !wasThrown)
> +                                callback(remoteObject);
> +                        });

Interesting, the resulting RemoteObject is not part of an objectGroup... so we would need to release it ourselves if we would want to prevent abandoned memory. Right now, as soon as you do this the node will never get garbage collected.

This is somewhat unfortunate, we'd want to release this as part of WI.ConsoleManager.Event.Cleared. Perhaps we should add this object to a set:

    WI.floatingRemoteObjectsToClearWithConsole = new Set;

Or call `remoteObject.release()` in the callbacks.

That said maybe this isn't too problematic...
Comment 4 Devin Rousso 2019-02-20 14:40:31 PST
Comment on attachment 362523 [details]
[Patch] WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=362523&action=review

>> Source/WebInspectorUI/UserInterface/Views/ObjectPreviewView.js:115
>> +            this._titleElement.appendChild(WI.FormattedValue.createElementForNodePreview(this._preview, {object: this._object}));
> 
> createElementForNodePreview doesn't look for an `object` property, though perhaps it should. It only looks for `remoteObjectAccessor`.

Oops.  Forgot to update this one.

>> Source/WebInspectorUI/UserInterface/Views/ObjectPreviewView.js:245
>> +                        });
> 
> Interesting, the resulting RemoteObject is not part of an objectGroup... so we would need to release it ourselves if we would want to prevent abandoned memory. Right now, as soon as you do this the node will never get garbage collected.
> 
> This is somewhat unfortunate, we'd want to release this as part of WI.ConsoleManager.Event.Cleared. Perhaps we should add this object to a set:
> 
>     WI.floatingRemoteObjectsToClearWithConsole = new Set;
> 
> Or call `remoteObject.release()` in the callbacks.
> 
> That said maybe this isn't too problematic...

I was banking on the idea that when we navigate the page, we'd clear these objects, since they're DOM nodes and that's related to the page's lifetime.  I like the idea of `WI.remoteObjectsToClearWithConsole` though :)
Comment 5 Devin Rousso 2019-02-20 14:47:56 PST
Created attachment 362545 [details]
[Patch] WIP

Needs support for collection types (e.g. `Map`, `Set`, etc)
Comment 6 EWS Watchlist 2019-02-20 15:30:12 PST Comment hidden (obsolete)
Comment 7 EWS Watchlist 2019-02-20 15:30:16 PST Comment hidden (obsolete)
Comment 8 Devin Rousso 2019-02-20 15:36:38 PST
Created attachment 362555 [details]
Patch

Needs support for collection types (e.g. `Map`, `Set`, etc)
Comment 9 Joseph Pecoraro 2019-02-20 15:43:48 PST
> Needs support for collection types (e.g. `Map`, `Set`, etc)

Should we just do those in a follow-up? They should be done, but are comparably rare.
Comment 10 Devin Rousso 2019-02-20 15:47:17 PST
(In reply to Joseph Pecoraro from comment #9)
> > Needs support for collection types (e.g. `Map`, `Set`, etc)
> Should we just do those in a follow-up? They should be done, but are comparably rare.
If I have time this week, I'll get to it then.  It shouldn't be too hard, as it would follow a similar pattern to an array/object.  If I run out of time or get bogged down by other things, I'll just make a separate bug.
Comment 11 Devin Rousso 2019-02-24 21:44:11 PST
(In reply to Devin Rousso from comment #10)
> (In reply to Joseph Pecoraro from comment #9)
> > > Needs support for collection types (e.g. `Map`, `Set`, etc)
> > Should we just do those in a follow-up? They should be done, but are comparably rare.
> If I have time this week, I'll get to it then.  It shouldn't be too hard, as it would follow a similar pattern to an array/object.  If I run out of time or get bogged down by other things, I'll just make a separate bug.
Created <https://webkit.org/b/194997> to track collections.  It's a bit more complicated in that case, as it's not just a simple `getProperty` call (e.g. for an `Iterator`, it may require some fancy `.next()` logic).  Considering that highlighting already works as expected for expanded values (see <https://webkit.org/b/194997> for more in depth description), I am less worried about that case being an issue.
Comment 12 Joseph Pecoraro 2019-02-25 23:07:38 PST
(In reply to Devin Rousso from comment #11)
> (In reply to Devin Rousso from comment #10)
> > (In reply to Joseph Pecoraro from comment #9)
> > > > Needs support for collection types (e.g. `Map`, `Set`, etc)
> > > Should we just do those in a follow-up? They should be done, but are comparably rare.
> > If I have time this week, I'll get to it then.  It shouldn't be too hard, as it would follow a similar pattern to an array/object.  If I run out of time or get bogged down by other things, I'll just make a separate bug.
> Created <https://webkit.org/b/194997> to track collections.  It's a bit more
> complicated in that case, as it's not just a simple `getProperty` call (e.g.
> for an `Iterator`, it may require some fancy `.next()` logic).  Considering
> that highlighting already works as expected for expanded values (see
> <https://webkit.org/b/194997> for more in depth description), I am less
> worried about that case being an issue.

Oh good point, yeah this could be tricky.
Comment 13 Joseph Pecoraro 2019-02-25 23:10:30 PST
Comment on attachment 362555 [details]
Patch

r=me
Comment 14 WebKit Commit Bot 2019-02-25 23:36:35 PST
Comment on attachment 362555 [details]
Patch

Clearing flags on attachment: 362555

Committed r242076: <https://trac.webkit.org/changeset/242076>
Comment 15 WebKit Commit Bot 2019-02-25 23:36:36 PST
All reviewed patches have been landed.  Closing bug.