Bug 191622 – Web Inspector: Remove unused DataGrid and TreeOutline code

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

- DOMTreeOutline property `_selectEnabled` duplicates `_selectable` in the base class
- TreeOutline methods `removeChildrenRecursive` and `reattachIfIndexChanged` are no longer used

[Radar WebKit Bug Importer]

<rdar://problem/46052014>

[Matt Baker]

Created attachment 354761 [details]
Patch

[Devin Rousso]

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

[Matt Baker]

Created attachment 354857 [details]
Patch

[Devin Rousso]

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?

[Matt Baker]

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".

[Matt Baker]

Created attachment 355894 [details]
Patch for landing

[Joseph Pecoraro]

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

[WebKit Commit Bot]

Comment on attachment 355894 [details]
Patch for landing

Clearing flags on attachment: 355894

Committed r238626: <https://trac.webkit.org/changeset/238626>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.