Bug 65218 - Web Inspector: Add Inspector menu items to Mac MiniBrowser
Summary: Web Inspector: Add Inspector menu items to Mac MiniBrowser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.9
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-07-26 16:08 PDT by Genisim
Modified: 2017-10-31 18:27 PDT (History)
12 users (show)

See Also:


Attachments
Add Web Inspector support for MAC MiniBrowser (29.38 KB, patch)
2011-07-26 17:34 PDT, Genisim
mrowe: review-
Details | Formatted Diff | Diff
Patch fixed (29.06 KB, patch)
2011-07-26 18:47 PDT, Genisim
no flags Details | Formatted Diff | Diff
Patch (5.79 KB, patch)
2017-10-31 14:12 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (6.79 KB, patch)
2017-10-31 16:23 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Genisim 2011-07-26 16:08:49 PDT
Modify MiniBrowser (MAC platform, MAC OS X 10.6), enable Web Inspector
Comment 1 Genisim 2011-07-26 17:34:31 PDT
Created attachment 102082 [details]
Add Web Inspector support for MAC MiniBrowser
Comment 2 Simon Fraser (smfr) 2011-07-26 17:38:48 PDT
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.
Comment 3 Mark Rowe (bdash) 2011-07-26 17:41:37 PDT
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.
Comment 4 Brian Weinstein 2011-07-26 17:43:47 PDT
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.
Comment 5 Genisim 2011-07-26 18:47:30 PDT
Created attachment 102088 [details]
Patch fixed

Thanks for fast responses. Updated patch fixed according team comments.
Comment 6 Eric Seidel (no email) 2012-02-16 14:26:49 PST
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...
Comment 7 Vsevolod Vlasov 2012-06-26 05:27:54 PDT
Comment on attachment 102088 [details]
Patch fixed

Clearign r? per Eric's comment.
Patch does not apply anymore anyway.
Comment 8 Brian Burg 2013-05-03 10:35:30 PDT
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.
Comment 9 Brian Burg 2013-05-03 10:57:09 PDT
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.
Comment 10 Radar WebKit Bug Importer 2014-07-21 10:13:07 PDT
<rdar://problem/17748459>
Comment 11 Ross Kirsling 2017-10-31 14:12:22 PDT
Created attachment 325490 [details]
Patch
Comment 12 Ross Kirsling 2017-10-31 14:21:00 PDT
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...?
Comment 13 Joseph Pecoraro 2017-10-31 14:47:08 PDT
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.
Comment 14 Ross Kirsling 2017-10-31 16:18:30 PDT
(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.
Comment 15 Ross Kirsling 2017-10-31 16:23:22 PDT
Created attachment 325517 [details]
Patch
Comment 16 WebKit Commit Bot 2017-10-31 18:27:43 PDT
Comment on attachment 325517 [details]
Patch

Clearing flags on attachment: 325517

Committed r224268: <https://trac.webkit.org/changeset/224268>
Comment 17 WebKit Commit Bot 2017-10-31 18:27:44 PDT
All reviewed patches have been landed.  Closing bug.