[webkit-reviews] review denied: [Bug 107364] Improve PageVisibility API with enums : [Attachment 183858] [PATCH] Use enums in the PageVisibility WK1 and WK2 APIs

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 107364: Improve PageVisibility API with enums
https://bugs.webkit.org/show_bug.cgi?id=107364

Attachment 183858: [PATCH] Use enums in the PageVisibility WK1 and WK2 APIs
https://bugs.webkit.org/attachment.cgi?id=183858&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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?