Bug 179273 – Web Inspector: Two little Layers tab fixes

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

[Ross Kirsling]

Web Inspector: Two little Layers tab fixes

[Ross Kirsling]

Created attachment 325986 [details]
Patch

[Ross Kirsling]

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.

[Ross Kirsling]

Created attachment 325991 [details]
Patch

[Devin Rousso]

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?

[Ross Kirsling]

(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

[WebKit Commit Bot]

Comment on attachment 325991 [details]
Patch

Clearing flags on attachment: 325991

Committed r224520: <https://trac.webkit.org/changeset/224520>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

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

[Ross Kirsling]

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

[Joseph Pecoraro]

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

[Ross Kirsling]

(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

[Ross Kirsling]

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

[Joseph Pecoraro]

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

[Radar WebKit Bug Importer]

<rdar://problem/35567171>