Summary: | Web Inspector: Remove unused DataGrid and TreeOutline code | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||||
Component: | Web Inspector | Assignee: | Matt Baker <mattbaker> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, webkit-bug-importer | ||||||||
Priority: | P3 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Matt Baker
2018-11-13 20:33:41 PST
Created attachment 354761 [details]
Patch
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 Created attachment 354857 [details]
Patch
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 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". Created attachment 355894 [details]
Patch for landing
> > 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 on attachment 355894 [details] Patch for landing Clearing flags on attachment: 355894 Committed r238626: <https://trac.webkit.org/changeset/238626> All reviewed patches have been landed. Closing bug. |