WebKit2: Status bar, toolbar, and menu bar checks should be in the injected bundle

• Component: WebKit2
• Status: RESOLVED FIXED

[Brian Weinstein]

The status bar visibility check should be in the injected bundle, it shouldn't require a synchronous message from the WebProcess -> UIProcess.

<rdar://problem/9468337>

[Brian Weinstein]

Created attachment 94871 [details]
[PATCH] Fix

[Sam Weinig]

Comment on attachment 94871 [details]
[PATCH] Fix

This will never send the GetStatusBarIsVisible message anymore.  The client function you added needs to support passthrough like the bundle policy client functions.  We also should be doing this for all the similar functions.

[Brian Weinstein]

(In reply to comment #2)
> (From update of attachment 94871 [details])
> This will never send the GetStatusBarIsVisible message anymore.  The client function you added needs to support passthrough like the bundle policy client functions.  

I didn't know I needed to send the GetStatusBarIsVisibile message. What do you mean by supporting passthrough? Are there any examples of this?

> We also should be doing this for all the similar functions.

Which similar functions did you mean? I see toolbarsVisible and menubarVisible, is that what you mean?

[Brian Weinstein]

Created attachment 94885 [details]
[PATCH] Fix v2 (no other methods)

This patch doesn't do the other methods, just making sure it is on the right track.

[Brian Weinstein]

Created attachment 94900 [details]
[PATCH] Fix v3

[Adam Roben (:aroben)]

Comment on attachment 94900 [details]
[PATCH] Fix v3

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

Did you make sure you don't need to modify MiniBrowser or You need to modify MiniBrowser or qwkpage.cpp?

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:179
> +enum {
> +    WKBundlePageUIBooleanPassThrough,
> +    WKBundlePageUIBooleanTrue,
> +    WKBundlePageUIBooleanFalse
> +};
> +typedef uint32_t WKBundlePageUIBoolean;

Calling this a "boolean" feels a little weird. Booleans usually only have two states! Maybe something a little less generic would work better, like:

enum {
    WKBundlePageUIElementVisibilityUnknown,
    WKBundlePageUIElementVisible,
    WKBundlePageUIElementHidden,
};
typedef uint32_t WKBundlePageUIElementVisibility;

[Brian Weinstein]

(In reply to comment #6)
> (From update of attachment 94900 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94900&action=review
> 
> Did you make sure you don't need to modify MiniBrowser or You need to modify MiniBrowser or qwkpage.cpp?

Minibrowser built on my machine, and qwkpage.cpp doesn't have anything about the injected bundle page UI client (Qt EWS was fine as well).

> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:179
> > +enum {
> > +    WKBundlePageUIBooleanPassThrough,
> > +    WKBundlePageUIBooleanTrue,
> > +    WKBundlePageUIBooleanFalse
> > +};
> > +typedef uint32_t WKBundlePageUIBoolean;
> 
> Calling this a "boolean" feels a little weird. Booleans usually only have two states! Maybe something a little less generic would work better, like:
> 
> enum {
>     WKBundlePageUIElementVisibilityUnknown,
>     WKBundlePageUIElementVisible,
>     WKBundlePageUIElementHidden,
> };
> typedef uint32_t WKBundlePageUIElementVisibility;

I do like this better. I will switch over to using this.

[Brian Weinstein]

Created attachment 95021 [details]
[PATCH] Fix v4

[Adam Roben (:aroben)]

Comment on attachment 95021 [details]
[PATCH] Fix v4

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:201
> +    WKBundlePageUIElementVisibility toolbarsVisible = m_page->injectedBundleUIClient().toolbarsAreVisible(m_page);

I'd call the variable toolbarsVisibility, instead of toolbarsVisible. (Ditto for the other local variables you added in this patch.)

[Brian Weinstein]

Landed in r87421.