Bug 61474 - WebKit2: Status bar, toolbar, and menu bar checks should be in the injected bundle
Summary: WebKit2: Status bar, toolbar, and menu bar checks should be in the injected b...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brian Weinstein
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-05-25 15:14 PDT by Brian Weinstein
Modified: 2011-05-26 12:54 PDT (History)
2 users (show)

See Also:


Attachments
[PATCH] Fix (6.12 KB, patch)
2011-05-25 15:31 PDT, Brian Weinstein
sam: review-
Details | Formatted Diff | Diff
[PATCH] Fix v2 (no other methods) (6.86 KB, patch)
2011-05-25 17:24 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
[PATCH] Fix v3 (9.23 KB, patch)
2011-05-25 18:49 PDT, Brian Weinstein
aroben: review-
Details | Formatted Diff | Diff
[PATCH] Fix v4 (9.41 KB, patch)
2011-05-26 12:29 PDT, Brian Weinstein
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Weinstein 2011-05-25 15:14:36 PDT
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>
Comment 1 Brian Weinstein 2011-05-25 15:31:08 PDT
Created attachment 94871 [details]
[PATCH] Fix
Comment 2 Sam Weinig 2011-05-25 16:27:01 PDT
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.
Comment 3 Brian Weinstein 2011-05-25 16:38:21 PDT
(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?
Comment 4 Brian Weinstein 2011-05-25 17:24:58 PDT
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.
Comment 5 Brian Weinstein 2011-05-25 18:49:41 PDT
Created attachment 94900 [details]
[PATCH] Fix v3
Comment 6 Adam Roben (:aroben) 2011-05-26 11:29:59 PDT
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;
Comment 7 Brian Weinstein 2011-05-26 12:04:58 PDT
(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.
Comment 8 Brian Weinstein 2011-05-26 12:29:48 PDT
Created attachment 95021 [details]
[PATCH] Fix v4
Comment 9 Adam Roben (:aroben) 2011-05-26 12:31:12 PDT
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.)
Comment 10 Brian Weinstein 2011-05-26 12:54:10 PDT
Landed in r87421.