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: | 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
Devin Rousso
2019-02-20 11:58:46 PST
Created attachment 362523 [details]
[Patch] WIP
Needs support for collection types (e.g. `Map`, `Set`, etc)
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 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 :) Created attachment 362545 [details]
[Patch] WIP
Needs support for collection types (e.g. `Map`, `Set`, etc)
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 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
Created attachment 362555 [details]
Patch
Needs support for collection types (e.g. `Map`, `Set`, etc)
> 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.
(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. (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. (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 on attachment 362555 [details]
Patch
r=me
Comment on attachment 362555 [details] Patch Clearing flags on attachment: 362555 Committed r242076: <https://trac.webkit.org/changeset/242076> All reviewed patches have been landed. Closing bug. |