Bug 191770 - Web Inspector: Remove parameters from TreeOutline SelectionDidChange event
Summary: Web Inspector: Remove parameters from TreeOutline SelectionDidChange event
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: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks: 191483
  Show dependency tree / graph
 
Reported: 2018-11-16 13:57 PST by Matt Baker
Modified: 2018-11-25 13:44 PST (History)
5 users (show)

See Also:

Attachments
Patch (13.07 KB, patch)
2018-11-16 14:01 PST, Matt Baker 		no flags Details | Formatted Diff | Diff
Patch (14.98 KB, patch)
2018-11-16 14:52 PST, Matt Baker 		no flags Details | Formatted Diff | Diff
Patch for landing (15.10 KB, patch)
2018-11-21 17:17 PST, Matt Baker 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.53 MB, application/zip)
2018-11-21 20:17 PST, EWS Watchlist 		no flags Details
Patch for landing (15.10 KB, patch)
2018-11-25 13:05 PST, Matt Baker 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2018-11-16 13:57:05 PST 
Summary:
Remove parameters from TreeOutline SelectionDidChange event.

Current the event data includes {selectedElement, deselectedElement}. In a world where TreeOutline can have multiple selected items, these properties don't make sense. Let's remove them and let the handler decide what to do.

`selectedElement` is redundant; the handler can just ask for `event.target.selectedTreeElement`. `deselectedElement` is only used in one place, and can be avoided easily.
Comment 1 Matt Baker 2018-11-16 14:01:08 PST 
Created attachment 355116 [details]
Patch
Comment 2 Matt Baker 2018-11-16 14:52:53 PST 
Created attachment 355131 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2018-11-16 14:53:18 PST 
<rdar://problem/46140601>
Comment 4 Devin Rousso 2018-11-20 21:23:42 PST 
Comment on attachment 355131 [details]
Patch

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

r=me, nice cleanup :)

Rather than use `event.target.selectedTreeElement`, it seems like most of these cases could refer to the actual member `WI.TreeOutline` object (e.g. CallFrameTreeController.js could use `this._treeOutline.selectedTreeElement` instead, or most of the `WI.NavigationSidebarPanel` subclasses could use `this.contentTreeOutline` instead)

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:845
> +        let selectedTreeElement = event.target.selectedTreeElement;

NIT: I'd move this to be right above the `if` that checks for it's existence (down one line)
Comment 5 Matt Baker 2018-11-21 17:17:55 PST 
Created attachment 355447 [details]
Patch for landing
Comment 6 EWS Watchlist 2018-11-21 20:17:05 PST 
Comment on attachment 355447 [details]
Patch for landing

Attachment 355447 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10105201

New failing tests:
media/no-fullscreen-when-hidden.html
Comment 7 EWS Watchlist 2018-11-21 20:17:07 PST 
Created attachment 355460 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 8 Matt Baker 2018-11-25 13:05:11 PST 
Created attachment 355602 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2018-11-25 13:44:53 PST 
Comment on attachment 355602 [details]
Patch for landing

Clearing flags on attachment: 355602

Committed r238483: <https://trac.webkit.org/changeset/238483>
Comment 10 WebKit Commit Bot 2018-11-25 13:44:54 PST 
All reviewed patches have been landed.  Closing bug.