Bug 194862 - Web Inspector: hovering a node inside an object preview should highlight it
Summary: Web Inspector: hovering a node inside an object preview should highlight it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 194997
  Show dependency treegraph
 
Reported: 2019-02-20 11:58 PST by Devin Rousso
Modified: 2019-02-25 23:36 PST (History)
7 users (show)

See Also:


Attachments
[Patch] WIP (10.79 KB, patch)
2019-02-20 12:04 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Patch] WIP (12.77 KB, patch)
2019-02-20 14:47 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (2.81 MB, application/zip)
2019-02-20 15:30 PST, EWS Watchlist
no flags Details
Patch (12.84 KB, patch)
2019-02-20 15:36 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.