Bug 113588 – Web Inspector: find dialog does not populate search string from system find pasteboard

• Component: Web Inspector
• Status: RESOLVED FIXED

[Brad]

In almost all Cocoa applications on the Mac where you can search for something in the document, the find command is system wide. That is, you can highlight a word, press Command-E to make it the search term, then switch to another Cocoa application and press Command-G to find again. The second application will search for the term from the first application. And it goes both ways (search for a term in the second app, find it in the first app). This generally works great in Safari and Chrome, and I often use it as a developer to highlight something on a page, press command-e and then find it in BBEdit (and vice versa). Or I highlight something in an e-mail and then go find it in a Web page.

However, the developer tools don't work that way. My deeply-ingrained habit of using this very useful standard Mac keyboard shortcut don't work in the developer tools, because they don't access the systemwide Find pasteboard (or whatever its called). I end up having to copy and paste the term into the Find field for "Sources" and "Resources" especially (it is what I search for things in the most), and also for "Elements". Please bring system-wide "Find" to the Inspector and Developer Tools.

PS. I have "Use Webkit Inspector" checked in the "Develop" menu all the time, because I find the UI on the newer version of the inspector tools to be pretty much unusable, and a huge step backwards. I hate the new version.

[Brian Burg]

More details:

On Mac, doing Ctrl+E on selected content within the inspector will:

 * put it into the system find pasteboard (NSFindPboard?)
 * populate the main content view's find dialog with that text (open or closed)

doing Ctrl+E on inspected page content, or any other application, will put it into the find pasteboard but not populate any find dialog in the inspector.

[Radar WebKit Bug Importer]

<rdar://problem/19281466>

[Devin Rousso]

Created attachment 397432 [details]
Patch

this has bothered me for too long T.T

[Blaze Burg]

Comment on attachment 397432 [details]
Patch

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

There are some lifetime issues to address in the ObjC parts, but other than that this patch looks god and is a welcome improvement!

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm:240
> +    if (!!_delegate && [_delegate respondsToSelector:@selector(inspectorViewControllerDidBecomeActive:)])

Nit: respondsToSelector check will return NO if _delegate is nil, so the null check is not necessary.

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorWKWebView.mm:62
> +        [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(_handleWindowDidBecomeKey:) name:NSWindowDidBecomeKeyNotification object:nil];

This needs to be unregistered at some point or `self` will be retained forever by NSNotificationCenter.

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorWKWebView.mm:88
> +- (BOOL)becomeFirstResponder

Does this correctly handle the case when Inspector is attached?

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorWKWebView.mm:95
> +- (void)_handleWindowDidBecomeKey:(NSNotification *)notification

You don't check which window is key, so this would probably fire if Web Inspector window is not key.

[Blaze Burg]

good* 😆

[Devin Rousso]

Comment on attachment 397432 [details]
Patch

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

>> Source/WebKit/UIProcess/Inspector/mac/WKInspectorWKWebView.mm:62
>> +        [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(_handleWindowDidBecomeKey:) name:NSWindowDidBecomeKeyNotification object:nil];
> 
> This needs to be unregistered at some point or `self` will be retained forever by NSNotificationCenter.

Ah, oops.

>> Source/WebKit/UIProcess/Inspector/mac/WKInspectorWKWebView.mm:88
>> +- (BOOL)becomeFirstResponder
> 
> Does this correctly handle the case when Inspector is attached?

I believe so, but only if the Safari window was already focused (e.g. clicking on Web Inspector when the inspected page was previously focused).  I also have the `NSWindowDidBecomeKeyNotification` to handle the case that Web Inspector is focused (as well as if it's its own window).

[Devin Rousso]

Created attachment 397693 [details]
Patch

[Devin Rousso]

Created attachment 397804 [details]
Patch

[Blaze Burg]

Comment on attachment 397804 [details]
Patch

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

r=me!

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorWKWebView.mm:98
> +    [self.inspectorWKWebViewDelegate inspectorWKWebViewDidBecomeActive:self];

Nit: usually prefer to use the ivar in this situation, _inspectorWKWebViewDelegate.

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorWKWebView.mm:104
> +    if ([notification object] == [self window])

nit: notification.object, self.window

[Devin Rousso]

Comment on attachment 397804 [details]
Patch

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

>> Source/WebKit/UIProcess/Inspector/mac/WKInspectorWKWebView.mm:98
>> +    [self.inspectorWKWebViewDelegate inspectorWKWebViewDidBecomeActive:self];
> 
> Nit: usually prefer to use the ivar in this situation, _inspectorWKWebViewDelegate.

I'd prefer to follow the convention of what is done in this file.  Not to mention, the ivar is a `WeakObjCPtr<id <WKInspectorWKWebViewDelegate>>` so I think I'd have to do some extra casting, which `-[WKInspectorWKWebView inspectorWKWebViewDelegate]` does for me.

>> Source/WebKit/UIProcess/Inspector/mac/WKInspectorWKWebView.mm:104
>> +    if ([notification object] == [self window])
> 
> nit: notification.object, self.window

lol oops :P

[Devin Rousso]

Created attachment 397875 [details]
Patch

[EWS]

Committed r260847: <https://trac.webkit.org/changeset/260847>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397875 [details].