RESOLVED FIXED Bug 194862
Web Inspector: hovering a node inside an object preview should highlight it 
https://bugs.webkit.org/show_bug.cgi?id=194862
Summary Web Inspector: hovering a node inside an object preview should highlight it
Devin Rousso
Reported 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.
Attachments
[Patch] WIP (10.79 KB, patch)
2019-02-20 12:04 PST, Devin Rousso 		no flags
DetailsFormatted DiffDiff
[Patch] WIP (12.77 KB, patch)
2019-02-20 14:47 PST, Devin Rousso 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2019-02-20 11:59:10 PST
<rdar://problem/48246433>
Devin Rousso
Comment 2 2019-02-20 12:04:53 PST
Created attachment 362523 [details] [Patch] WIP Needs support for collection types (e.g. `Map`, `Set`, etc)
Joseph Pecoraro
Comment 3 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...
Devin Rousso
Comment 4 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 :)
Devin Rousso
Comment 5 2019-02-20 14:47:56 PST
Created attachment 362545 [details] [Patch] WIP Needs support for collection types (e.g. `Map`, `Set`, etc)
EWS Watchlist
Comment 6 2019-02-20 15:30:12 PST Comment hidden (obsolete)
EWS Watchlist
Comment 7 2019-02-20 15:30:16 PST Comment hidden (obsolete)
Devin Rousso
Comment 8 2019-02-20 15:36:38 PST
Created attachment 362555 [details] Patch Needs support for collection types (e.g. `Map`, `Set`, etc)
Joseph Pecoraro
Comment 9 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.
Devin Rousso
Comment 10 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.
Devin Rousso
Comment 11 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.
Joseph Pecoraro
Comment 12 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.
Joseph Pecoraro
Comment 13 2019-02-25 23:10:30 PST
Comment on attachment 362555 [details] Patch r=me
WebKit Commit Bot
Comment 14 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>
WebKit Commit Bot
Comment 15 2019-02-25 23:36:36 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.