[webkit-reviews] review granted: [Bug 220773] [Cocoa] Web Inspector: Console search box is broken, advancing to next result instead pastes from system find pasteboard : [Attachment 418400] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Razvan Caliman
<rcaliman at apple.com>'s request for review:
Bug 220773: [Cocoa] Web Inspector: Console search box is broken, advancing to
next result instead pastes from system find pasteboard
https://bugs.webkit.org/show_bug.cgi?id=220773

Attachment 418400: Patch

https://bugs.webkit.org/attachment.cgi?id=418400&action=review




--- Comment #9 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 418400
  --> https://bugs.webkit.org/attachment.cgi?id=418400
Patch

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

r=me :)

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:153
> +	   this.element.classList.toggle(WI.FindBanner.ShowingStyleClassName,
this._alwaysShowing);

NIT: it's possible/likely that we'd remove and then re-add
`WI.FindBanner.ShowingStyleClassName` inside a single call to `set
targetElement`.  I think it may be simpler to guard the
`this.element.classList.remove(WI.FindBanner.ShowingStyleClassName);` above
inside an `if (!this._alwaysShowing)` (like you have in `hide()`) and then add
`this.element.classList.add(WI.FindBanner.ShowingStyleClassName);` inside the
constructor in the `if (this._alwaysShowing)`.	I'd originally suggested `set
targetElement` because I'd incorrectly thought that we were adding/removing
this class from `this._targetElement`, so my apologies if I lead you astray ��