Bug 191622 - Web Inspector: Remove unused DataGrid and TreeOutline code
Summary: Web Inspector: Remove unused DataGrid and TreeOutline code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P3 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-13 20:33 PST by Matt Baker
Modified: 2018-11-28 11:47 PST (History)
4 users (show)

See Also:


Attachments
Patch (7.01 KB, patch)
2018-11-13 20:35 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (14.75 KB, patch)
2018-11-14 15:07 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Patch for landing (14.72 KB, patch)
2018-11-28 11:18 PST, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2018-11-13 20:33:41 PST
- DOMTreeOutline property `_selectEnabled` duplicates `_selectable` in the base class
- TreeOutline methods `removeChildrenRecursive` and `reattachIfIndexChanged` are no longer used
Comment 1 Radar WebKit Bug Importer 2018-11-13 20:33:56 PST
<rdar://problem/46052014>
Comment 2 Matt Baker 2018-11-13 20:35:27 PST
Created attachment 354761 [details]
Patch
Comment 3 Devin Rousso 2018-11-14 09:46:39 PST
Comment on attachment 354761 [details]
Patch

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

Looks good.  I'd like to test this on my computer to make sure we aren't missing any edge-cases where `selectable` is `false` while `selectEnabled` is true (it looks like this may be the case when the "Show All Nodes" button is added).

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:35
> +        super(null, selectable);

I just looked around, and it seems that `WI.ErrorObjectView` is the only user of `WI.TreeOutline` that specifies an `element`.  Perhaps you could refactor that view and `WI.TreeOutline` as well to remove that parameter.

> Source/WebInspectorUI/UserInterface/Views/TreeElement.js:-65
> -    removeChildrenRecursive() { return WI.TreeOutline.prototype.removeChildrenRecursive.apply(this, arguments); }

It looks like `WI.DataGrid` has a similar function that isn't used :P
Comment 4 Matt Baker 2018-11-14 15:07:25 PST
Created attachment 354857 [details]
Patch
Comment 5 Devin Rousso 2018-11-26 10:39:53 PST
Comment on attachment 354857 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.css:61
> +    -webkit-padding-start: 16px

Did you test this in RTL?  Is this the correct behavior?

> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:2
> + * Copyright (C) 2015-2018 Apple Inc. All rights reserved.

I'm not sure what our "style" is on this, but I've heard both that "it should only be the most recent year" and "it should be every year it was significantly modified".  Maybe confirm which is preferred?
Comment 6 Matt Baker 2018-11-28 11:18:03 PST
Comment on attachment 354857 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ErrorObjectView.js:2
>> + * Copyright (C) 2015-2018 Apple Inc. All rights reserved.
> 
> I'm not sure what our "style" is on this, but I've heard both that "it should only be the most recent year" and "it should be every year it was significantly modified".  Maybe confirm which is preferred?

I'll use the more conservative "2015, 2018".
Comment 7 Matt Baker 2018-11-28 11:18:32 PST
Created attachment 355894 [details]
Patch for landing
Comment 8 Joseph Pecoraro 2018-11-28 11:23:07 PST
> > I'm not sure what our "style" is on this, but I've heard both that "it should only be the most recent year" and "it should be every year it was significantly modified".  Maybe confirm which is preferred?
> 
> I'll use the more conservative "2015, 2018".

I have seen us moving to ranges more and more: "2015-2018".

The source repository has complete history of the edits of the file so exact years is cumbersome and ultimately unnecessary.
Comment 9 WebKit Commit Bot 2018-11-28 11:47:09 PST
Comment on attachment 355894 [details]
Patch for landing

Clearing flags on attachment: 355894

Committed r238626: <https://trac.webkit.org/changeset/238626>
Comment 10 WebKit Commit Bot 2018-11-28 11:47:10 PST
All reviewed patches have been landed.  Closing bug.