Web Inspector: Two little Layers tab fixes
Created attachment 325986 [details] Patch
As explained in the ChangeLog: 1. Although we want to avoid refiring a selected event when updating the data grid programmatically, we still do want a popover to appear. 2. It seems that when WI is opened with the Layers tab showing first, the initial fetch of layer data doesn't occur right away. We could simply have _layersChangedWhileHidden start out true (and rename it to _layersAreStale perhaps), but this is a good opportunity to ditch that hack anyway. Now that we're properly tracking layer mutations, it's not such a big deal to hard update on every shown(), and detach the event listener in between.
Created attachment 325991 [details] Patch
Comment on attachment 325991 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325991&action=review r=me > Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:75 > + } else if (this._dataGrid.selectedNode) > this._dataGrid.selectedNode.deselect(suppressEvent); Should you dismiss the popover in the case that it is being shown and we have no new node to select?
(In reply to Devin Rousso from comment #4) > > Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:75 > > + } else if (this._dataGrid.selectedNode) > > this._dataGrid.selectedNode.deselect(suppressEvent); > > Should you dismiss the popover in the case that it is being shown and we > have no new node to select? Popover dismissal happens automatically by clicking anywhere. :D
Comment on attachment 325991 [details] Patch Clearing flags on attachment: 325991 Committed r224520: <https://trac.webkit.org/changeset/224520>
All reviewed patches have been landed. Closing bug.
(In reply to Ross Kirsling from comment #0) > Web Inspector: Two little Layers tab fixes This is a poor bug title. I'm sure a more descriptive title would be possible.
(In reply to Joseph Pecoraro from comment #8) > (In reply to Ross Kirsling from comment #0) > > Web Inspector: Two little Layers tab fixes > > This is a poor bug title. I'm sure a more descriptive title would be > possible. I debated splitting it into two (I'm sure being Friday EOD influenced my decision), but given that I didn't do so, I think it's about as coherent as the title can get? The two changes are in separate view classes and have no impact on each other, but they are both small fixes for the Layers tab.
(In reply to Ross Kirsling from comment #9) > (In reply to Joseph Pecoraro from comment #8) > > (In reply to Ross Kirsling from comment #0) > > > Web Inspector: Two little Layers tab fixes > > > > This is a poor bug title. I'm sure a more descriptive title would be > > possible. > > I debated splitting it into two (I'm sure being Friday EOD influenced my > decision), but given that I didn't do so, I think it's about as coherent as > the title can get? The two changes are in separate view classes and have no > impact on each other, but they are both small fixes for the Layers tab. It is always better to describe the user impact. Even if that ends up being a performance/memory improvement without any visible change.
(In reply to Joseph Pecoraro from comment #10) > It is always better to describe the user impact. Even if that ends up being > a performance/memory improvement without any visible change. Absolutely, and these are both user-facing fixes. But each one is rather long to describe in itself, and as they have no overlap, there's no way to combine them without a conjunction. "Web Inspector: [Layers] Show popover even when data grid selected event is suppressed and ensure initial fetch of layer data is never skipped" is a very long title. When I was considering splitting up the mouse UX patch into less-controversial and more-controversial parts, I had asked in channel whether a "three fixes"-type patch is something acceptable to do, and I received a positive answer. I didn't end up doing that then, but I did do so here, as a sort of cleanup task. I'd be happy to keep unrelated changes split up in the future, but I don't see how I could've possibly named this one better. :P
(In reply to Ross Kirsling from comment #11) > I'd be happy to keep unrelated changes split up in the future, but I don't > see how I could've possibly named this one better. :P Having slept on it, I suppose "Web Inspector: [Layers] Fixes for popover and data retrieval" would've worked. Bugzilla makes life tricky, haha.
(In reply to Ross Kirsling from comment #12) > (In reply to Ross Kirsling from comment #11) > > I'd be happy to keep unrelated changes split up in the future, but I don't > > see how I could've possibly named this one better. :P > > Having slept on it, I suppose "Web Inspector: [Layers] Fixes for popover and > data retrieval" would've worked. Bugzilla makes life tricky, haha. That is much better. But it sounds like you got bad advice from the channel. If you are indeed fixing two distinct and separate issues, it would be better to file two bugs and put up two patches. This way, if one of the changes regresses something and needs to be rolled out, it can be rolled out without affecting the other change. Likewise bisecting revisions for regressions can be more fine grained. Reviewing can be easier. There are many benefits to small patches.
<rdar://problem/35567171>