[webkit-reviews] review granted: [Bug 176433] Web Inspector: ⌘E and ⌘G do not work in main content area when quick console drawer is open : [Attachment 319963] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Joseph Pecoraro <joepeck at webkit.org>'s
request for review:
Bug 176433: Web Inspector: ⌘E and ⌘G do not work in main content area when
quick console drawer is open
https://bugs.webkit.org/show_bug.cgi?id=176433

Attachment 319963: [PATCH] Proposed Fix

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




--- Comment #4 from Brian Burg <bburg at apple.com> ---
Comment on attachment 319963
  --> https://bugs.webkit.org/attachment.cgi?id=319963
[PATCH] Proposed Fix

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

r=me, Thank you Joe!

> Source/WebInspectorUI/ChangeLog:45
> +	   are ContentBrowsers 

Nit: missing end of sentence

> Source/WebInspectorUI/UserInterface/Base/Main.js:291
> +    // FIXME: FindBanner search queries should be shared among search fields
and be common with the system.

Nit: you are looking for bugs 151310 and 113588.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:452
> +	   let selection = window.getSelection();

I guess this is better than nothing. We need to do a better job overriding this
in more ContentView subclasses.

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:-227
> -    {

I didn't realize we were doing this, its kinda gross. If we really were going
to do this in a transparent way that each content view handles it by itself,
then we'd need some notion of a first responder and responder chain. I think
the solution in this patch is fine, though.