Bug 194862

Summary:

Web Inspector: hovering a node inside an object preview should highlight it

Product:

WebKit

Reporter:

Devin Rousso <hi>

Component:

Web Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, rniwa, webkit-bug-importer

Priority:

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

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	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
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)

Comment on attachment 362545 [details] [Patch] WIP Attachment 362545 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11223909 New failing tests: inspector/debugger/tail-deleted-frames.html inspector/debugger/probe-manager-add-remove-actions.html inspector/debugger/tail-recursion.html inspector/debugger/reload-paused.html inspector/console/messagesCleared.html inspector/debugger/breakpoint-columns.html http/tests/inspector/network/set-resource-caching-disabled-memory-cache.html inspector/debugger/tail-deleted-frames-this-value.html http/tests/inspector/network/resource-response-source-memory-cache.html inspector/page/overrideUserAgent.html inspector/debugger/tail-deleted-frames-from-vm-entry.html inspector/console/webcore-logging.html inspector/debugger/search-scripts.html inspector/debugger/breakpoint-scope.html http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html inspector/runtime/saveResult.html http/tests/inspector/network/resource-response-source-memory-cache-revalidate-expired-only.html inspector/debugger/debugger-stack-overflow.html

EWS Watchlist

Comment 7 2019-02-20 15:30:16 PST Comment hidden (obsolete)

Created attachment 362553 [details] Archive of layout-test-results from ews102 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6

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.