Bug 107364 – Improve PageVisibility API with enums

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Joseph Pecoraro]

The API should have enum values instead of taking in ints which it then casts. This if for WebKit1/mac and WebKit2.

[Joseph Pecoraro]

Created attachment 183858 [details]
[PATCH] Use enums in the PageVisibility WK1 and WK2 APIs

This is my first WK2 work, so be gentle if I'm doing something wrong or incorrect.

[David Kilzer (:ddkilzer)]

Comment on attachment 183858 [details]
[PATCH] Use enums in the PageVisibility WK1 and WK2 APIs

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

This looks good from an overall perspective, but I'd like a WebKit2 engineer to take a look to make sure some assumptions aren't being violated.

> Tools/TestWebKitAPI/Tests/WebKit2/PageVisibilityState.cpp:41
> +static void didRunJavaScript1(WKSerializedScriptValueRef, WKErrorRef, void*);
> +static void didRunJavaScript2(WKSerializedScriptValueRef, WKErrorRef, void*);
> +static void didRunJavaScript3(WKSerializedScriptValueRef, WKErrorRef, void*);
> +static void didRunJavaScript4(WKSerializedScriptValueRef, WKErrorRef, void*);

Can we come up with a better name for these methods that are more descriptive?  Maybe:

didRunStep1StateChangeVisibleToHidden
didRunStep2StateChangeHiddenToPrerender
didRunStep3StateChangePrerenderToPreview
didRunStep4InStatePreview

[Joseph Pecoraro]

(In reply to comment #2)
> (From update of attachment 183858 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183858&action=review
> 
> This looks good from an overall perspective, but I'd like a WebKit2 engineer to take a look to make sure some assumptions aren't being violated.
> 
> > Tools/TestWebKitAPI/Tests/WebKit2/PageVisibilityState.cpp:41
> > +static void didRunJavaScript1(WKSerializedScriptValueRef, WKErrorRef, void*);
> > +static void didRunJavaScript2(WKSerializedScriptValueRef, WKErrorRef, void*);
> > +static void didRunJavaScript3(WKSerializedScriptValueRef, WKErrorRef, void*);
> > +static void didRunJavaScript4(WKSerializedScriptValueRef, WKErrorRef, void*);
> 
> Can we come up with a better name for these methods that are more descriptive?  Maybe:
> 
> didRunStep1StateChangeVisibleToHidden
> didRunStep2StateChangeHiddenToPrerender
> didRunStep3StateChangePrerenderToPreview
> didRunStep4InStatePreview

Sounds good. I had originally considered 1 didRunJavaScript function and an array of input strings and expected values to reduce the clutter, but I never ended up doing that. I can make these changes.

[Benjamin Poulain]

Comment on attachment 183858 [details]
[PATCH] Use enums in the PageVisibility WK1 and WK2 APIs

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

Great overall, but you also need to look into updating the way WebKitTestRunner works.

Also some concern about the naming (about collapsing WKPage and PageVisibility together).

> Source/WebKit/mac/WebView/WebViewPrivate.h:118
> +// This needs to be in sync with WebCore::PageVisibilityState.

Since you cast between two type, a good way to enforce correctness is to have one COMPILE_ASSERT() per enum value in your .mm file.

> Source/WebKit2/Shared/API/c/WKPageLoadTypes.h:65
> +enum {
> +    kWKPageVisibilityStateVisible,
> +    kWKPageVisibilityStateHidden,
> +    kWKPageVisibilityStatePrerender,
> +    kWKPageVisibilityStatePreview
> +};
> +typedef uint32_t WKPageVisibilityState;
> +

I am unconvinced this is the right header for this. Maybe WKPage.h is better?

Why typedef it to uint32_t instead of:
typedef WKPageVisibilityState WKPageVisibilityState?

As weird as it sounds, I think WKPageDocumentVisibilityState or WKPagePageVisibilityState may be better names.
Rationale: WKPage is the C prefix for WKPage APIs. "PageVisibility" is the name of the spec, "DocumentVisibility" is the name of the API as exposed to JavaScript.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePrivate.h:101
> -WK_EXPORT void WKBundleSetPageVisibilityState(WKBundleRef bundle, WKBundlePageRef page, int state, bool isInitialState);
> +WK_EXPORT void WKBundleSetPageVisibilityState(WKBundleRef bundle, WKBundlePageRef page, WKPageVisibilityState visibilityState, bool isInitialState);

Now that you expose a public API for PageVisibility, WKBundleSetPageVisibilityState should go away.

The right way to do the testing is to got from TestRunnerBundle->TestRunner->Public API->Back to WebProcess.
WKBundleSetPageVisibilityState exists as a hack for testing.

(All of this is assuming the tests are well written and do not rely on the state updating instantaneously. Sometime we need to update the tests, or hacks are needed).

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:587
> -void InjectedBundle::setPageVisibilityState(WebPage* page, int state, bool isInitialState)
> +void InjectedBundle::setPageVisibilityState(WebPage* page, WebCore::PageVisibilityState visibilityState, bool isInitialState)
>  {
>  #if ENABLE(PAGE_VISIBILITY_API) || ENABLE(HIDDEN_PAGE_DOM_TIMER_THROTTLING)
> -    page->corePage()->setVisibilityState(static_cast<PageVisibilityState>(state), isInitialState);
> +    page->corePage()->setVisibilityState(visibilityState, isInitialState);
>  #endif
>  }

This should probably go away.

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:1068
>  				265AF55015D1E48A00B0CB4A /* WTFString.cpp in Sources */,
>  				C51AFB99169F49FF009CCF66 /* FindMatches.mm in Sources */,
> +				A51B650916ADF9B1007AA5D9 /* PageVisibilityState.cpp in Sources */,

Keep the build section sorted?

[Joseph Pecoraro]

(In reply to comment #4)
> (From update of attachment 183858 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183858&action=review
> 
> Great overall, but you also need to look into updating the way WebKitTestRunner works.
> 
> Also some concern about the naming (about collapsing WKPage and PageVisibility together).
> 
> > Source/WebKit/mac/WebView/WebViewPrivate.h:118
> > +// This needs to be in sync with WebCore::PageVisibilityState.
> 
> Since you cast between two type, a good way to enforce correctness is to have one COMPILE_ASSERT() per enum value in your .mm file.

Will fix. Good point.

> > Source/WebKit2/Shared/API/c/WKPageLoadTypes.h:65
> > +enum {
> > +    kWKPageVisibilityStateVisible,
> > +    kWKPageVisibilityStateHidden,
> > +    kWKPageVisibilityStatePrerender,
> > +    kWKPageVisibilityStatePreview
> > +};
> > +typedef uint32_t WKPageVisibilityState;
> > +
> 
> I am unconvinced this is the right header for this. Maybe WKPage.h is better?

I'll make a new file. I was just riding the wave, since WKPageLoadTypes.h had WKLayoutMilestones which also doesn't sound like it belongs there. But that is no excuse.

> Why typedef it to uint32_t instead of:
> typedef WKPageVisibilityState WKPageVisibilityState?

We discussed this on IRC. Sticking with uint, as that is the convention and might be better for bin compatibility.

> As weird as it sounds, I think WKPageDocumentVisibilityState or WKPagePageVisibilityState may be better names.
> Rationale: WKPage is the C prefix for WKPage APIs. "PageVisibility" is the name of the spec, "DocumentVisibility" is the name of the API as exposed to JavaScript.

I looked around before coming to this name, and other enums didn't follow that kind of naming:

    Source/WebKit2/Shared/API/c/WKPageLoadTypes.h
    typedef uint32_t WKFrameNavigationType;
    typedef uint32_t WKSameDocumentNavigationType;
    typedef uint32_t WKLayoutMilestones;

    Source/WebKit2/UIProcess/API/C/WKPage.h
    WK_EXPORT void WKPageListenForLayoutMilestones(WKPageRef page, WKLayoutMilestones milestones);

    Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:
    typedef uint32_t WKPaginationMode;
    WK_EXPORT void WKPageSetPaginationMode(WKPageRef page, WKPaginationMode paginationMode);

That being said, I don't mind going with WKPagePageVisibilityState.


> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePrivate.h:101
> > -WK_EXPORT void WKBundleSetPageVisibilityState(WKBundleRef bundle, WKBundlePageRef page, int state, bool isInitialState);
> > +WK_EXPORT void WKBundleSetPageVisibilityState(WKBundleRef bundle, WKBundlePageRef page, WKPageVisibilityState visibilityState, bool isInitialState);
> 
> Now that you expose a public API for PageVisibility, WKBundleSetPageVisibilityState should go away.
> 
> The right way to do the testing is to got from TestRunnerBundle->TestRunner->Public API->Back to WebProcess.
> WKBundleSetPageVisibilityState exists as a hack for testing.
> 
> (All of this is assuming the tests are well written and do not rely on the state updating instantaneously. Sometime we need to update the tests, or hacks are needed).

Sounds good, I'll look into this.

> > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:587
> > -void InjectedBundle::setPageVisibilityState(WebPage* page, int state, bool isInitialState)
> > +void InjectedBundle::setPageVisibilityState(WebPage* page, WebCore::PageVisibilityState visibilityState, bool isInitialState)
> >  {
> >  #if ENABLE(PAGE_VISIBILITY_API) || ENABLE(HIDDEN_PAGE_DOM_TIMER_THROTTLING)
> > -    page->corePage()->setVisibilityState(static_cast<PageVisibilityState>(state), isInitialState);
> > +    page->corePage()->setVisibilityState(visibilityState, isInitialState);
> >  #endif
> >  }
> 
> This should probably go away.

Will do.

> > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:1068
> >  				265AF55015D1E48A00B0CB4A /* WTFString.cpp in Sources */,
> >  				C51AFB99169F49FF009CCF66 /* FindMatches.mm in Sources */,
> > +				A51B650916ADF9B1007AA5D9 /* PageVisibilityState.cpp in Sources */,
> 
> Keep the build section sorted?

Will do.

[Joseph Pecoraro]

Created attachment 184635 [details]
[PATCH] Addressed Review Comments

[Joseph Pecoraro]

(In reply to comment #4)
> (From update of attachment 183858 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183858&action=review
> 
> > Source/WebKit/mac/WebView/WebViewPrivate.h:118
> > +// This needs to be in sync with WebCore::PageVisibilityState.
> 
> Since you cast between two type, a good way to enforce correctness is to have one COMPILE_ASSERT() per enum value in your .mm file.

Since I didn't see any other examples of WebKit API casting types to WebCore types, but I did find examples of core() and kit() functions converting between the two, I went with a core() approach.

[David Kilzer (:ddkilzer)]

Comment on attachment 184635 [details]
[PATCH] Addressed Review Comments

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

No COMPILE_ASSERT() statements?

> Source/WebKit/mac/WebView/WebView.mm:412
> +static PageVisibilityState corePageVisibilityState(WebPageVisibilityState visibilityState)

I think it would be best to name this "core(WebPageVisibilityState visibilityState)" since C++ will disambiguate based on the argument type.

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> (From update of attachment 184635 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184635&action=review
> 
> No COMPILE_ASSERT() statements?

Although one could argue that if you have switch statements for conversions in both directions, any change in the enum list would fail if a case statement were missing.

[Sam Weinig]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 184635 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=184635&action=review
> > 
> > No COMPILE_ASSERT() statements?
> 
> Although one could argue that if you have switch statements for conversions in both directions, any change in the enum list would fail if a case statement were missing.

In general, we have tried to no rely on WebCore's types where possible by using explicit conversion, so no static assertions should be needed or wanted.

[Sam Weinig]

Comment on attachment 184635 [details]
[PATCH] Addressed Review Comments

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

> Source/WebKit/mac/WebView/WebViewPrivate.h:118
> +// Mirroring WebCore::PageVisibilityState.

I don't think this comments adds anything useful.

[Joseph Pecoraro]

(In reply to comment #8)
> (From update of attachment 184635 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184635&action=review
> 
> No COMPILE_ASSERT() statements?

Went with the switch statement handing all cases.

> > Source/WebKit/mac/WebView/WebView.mm:412
> > +static PageVisibilityState corePageVisibilityState(WebPageVisibilityState visibilityState)
> 
> I think it would be best to name this "core(WebPageVisibilityState visibilityState)" since C++ will disambiguate based on the argument type.

Renamed to core.

[Joseph Pecoraro]

Landed r141010: <http://trac.webkit.org/changeset/141010>