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

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from Joseph Pecoraro <joepeck at webkit.org>  2013-01-23 18:26:33 PST ---
(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.

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