Bug 65218 – Web Inspector: Add Inspector menu items to Mac MiniBrowser

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

[Genisim]

Modify MiniBrowser (MAC platform, MAC OS X 10.6), enable Web Inspector

[Genisim]

Created attachment 102082 [details]
Add Web Inspector support for MAC MiniBrowser

[Simon Fraser (smfr)]

Comment on attachment 102082 [details]
Add Web Inspector support for MAC MiniBrowser

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

> Source/WebKit2/ChangeLog:3
> +        Update MAC MiniBrowser, enable WEB Inspector 

Neither Mac nor Web are acronyms, so should not be in ALL CAPS.

[Mark Rowe (bdash)]

Comment on attachment 102082 [details]
Add Web Inspector support for MAC MiniBrowser

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

> Tools/MiniBrowser/MiniBrowser.xcodeproj/project.pbxproj:390
> +				GCC_PREPROCESSOR_DEFINITIONS = "ENABLE_INSPECTOR=1";

Please do not set configuration settings in the Xcode project file.  They live in the .xcconfig file.

> Tools/MiniBrowser/mac/BrowserWindowController.m:255
> +    {

This brace is in the wrong place.

[Brian Weinstein]

Comment on attachment 102082 [details]
Add Web Inspector support for MAC MiniBrowser

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

> Tools/MiniBrowser/mac/BrowserWindowController.m:33
> +#import <WebKit2/WKPreferencesPrivate.h>

These imports should be in alphabetical order.

[Genisim]

Created attachment 102088 [details]
Patch fixed

Thanks for fast responses. Updated patch fixed according team comments.

[Eric Seidel (no email)]

Comment on attachment 102088 [details]
Patch fixed

What is MacMiniBrowser?  I'm happy to rubber-stamp this, but I'm not even sure why we have this in our tree...

[Vsevolod Vlasov]

Comment on attachment 102088 [details]
Patch fixed

Clearign r? per Eric's comment.
Patch does not apply anymore anyway.

[Brian Burg]

AFAICT, Web Inspector support is still missing for Mac MiniBrowser (both WK1 and WK2). 

I'm going to recreate this patch so there's an easy way to test WK1 versions of InspectorClient.

[Brian Burg]

Actually, Web Inspector is available in Mac MiniBrowser (supplied by WebInspector.framework), but you need to set the pref for WebKitDeveloperExtras for "Inspect Element" to appear. So, this patch would only add menu items, which I don't care enough about to implement.

[Radar WebKit Bug Importer]

<rdar://problem/17748459>

[Ross Kirsling]

Created attachment 325490 [details]
Patch

[Ross Kirsling]

Reviving this bug from the dead, because I want Opt-Cmd-I in Mac MiniBrowser.

This patch works with two disclaimers:

1. The shortcut & menu item only apply when the browser window is focused -- not sure if this can be easily addressed since MiniBrowser currently seems to have no awareness of the Inspector window(?), but this limitation is better than not having the feature at all.

2. I'm not sure whether I'm getting the WKPageRef in an appropriate fashion or not. Everything pageRef-related was removed a few years ago in https://webkit.org/b/130061; Alex recently re-exposed this in https://webkit.org/b/177022 but the field name is awfully specific so it makes me a bit worried...?

[Joseph Pecoraro]

Comment on attachment 325490 [details]
Patch

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

r=me, but it should be easy to implement the WebKit1 portion, so I'd strongly suggest adding that before landing.

> Tools/MiniBrowser/mac/BrowserWindowController.h:73
>  - (IBAction)dumpSourceToConsole:(id)sender;

Does anyone actually implement this? It doesn't seem to do anything, unless I'm missing it. We should remove this in a follow-up patch.

> Tools/MiniBrowser/mac/WK1BrowserWindowController.m:257
> +- (IBAction)showHideWebInspector:(id)sender
> +{
> +}

Hmm, I think you can do:

    WebInspector *inspector = _webView.inspector;
    if (inspector.isOpen)
        [inspector close:sender];
    else
        [inspector show:sender];

As long as you import:

    <WebKit/WebViewPrivate.h>
    <WebKit/WebInspector.h>

You probably then need to fix up -validateMenuItem for this.

[Ross Kirsling]

(In reply to Joseph Pecoraro from comment #13)
> > Tools/MiniBrowser/mac/BrowserWindowController.h:73
> >  - (IBAction)dumpSourceToConsole:(id)sender;
> 
> Does anyone actually implement this? It doesn't seem to do anything, unless
> I'm missing it. We should remove this in a follow-up patch.

This functionality was removed in bug 130061 and replaced with the comment "Disabled until missing WK2 functionality is exposed via API/SPI." (WK2BrowserWindowController.m:172)

Getting a page ref seems to be exactly the functionality they mean...

> > Tools/MiniBrowser/mac/WK1BrowserWindowController.m:257
> > +- (IBAction)showHideWebInspector:(id)sender
> > +{
> > +}
> 
> Hmm, I think you can do:
> 
>     WebInspector *inspector = _webView.inspector;
>     if (inspector.isOpen)
>         [inspector close:sender];
>     else
>         [inspector show:sender];
> 
> As long as you import:
> 
>     <WebKit/WebViewPrivate.h>
>     <WebKit/WebInspector.h>
> 
> You probably then need to fix up -validateMenuItem for this.

Thanks a bunch for the code snippet! I will add this before landing.

[Ross Kirsling]

Created attachment 325517 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 325517 [details]
Patch

Clearing flags on attachment: 325517

Committed r224268: <https://trac.webkit.org/changeset/224268>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.