[Webkit-unassigned] [Bug 107364] Improve PageVisibility API with enums

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=107364


Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #183858|review?                     |review-
               Flag|                            |




--- Comment #4 from Benjamin Poulain <benjamin at webkit.org>  2013-01-23 17:41:30 PST ---
(From update of attachment 183858)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.