[Webkit-unassigned] [Bug 109422] [Qt] Add Page Visibility API support

[bugzilla-daemon at webkit.org]

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





--- Comment #61 from Benjamin Dupont <bdupont at nds.com>  2013-08-05 09:28:58 PST ---
(In reply to comment #59)
> (From update of attachment 208125 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208125&action=review
> 
> > Source/WebKit/qt/ChangeLog:3
> > +        [Qt] Add Page Visibility API support
> 
> Ideally this patch should also enable PAGE_VISIBILITY_API by default in the build, but this can also be done in a separate patch as long as you do it soon.
I think PAGE_VISIBILITY_API is already activate by default (in Tools/qmake/mkspecs/features/features.pri ENABLE_PAGE_VISIBILITY_API=1). Is there any other places to enable it?

> 
> > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:159
> > +        result = WebCore::PageVisibilityStatePrerender;
> > +        break;
> 
> you can replace the temporary result variable by "return WebCore::PageVisibilityStatePrerender;" directly and also remove the breaks and the final return.
I totally agree with you, I wanted to be inspired by dragOpToDropAction function but it seems it was a bad idea :)

> 
> > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:247
> > +    page->setVisibilityState(webPageVisibilityStateToWebCoreVisibilityState(m_visibilityState), true);
> 
> Page::m_visibilityState is already initialized to PageVisibilityStateVisible, I'm not sure I see a use for this.
Yes with the visible default value is no more needed.

> 
> > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:339
> > +    if (!page)
> > +        return;
> 
> By looking at the rest of the code I'd say that you can expect page to be non-null here, am I right?
Yes, but as I saw "return page ? page->viewportArguments() : WebCore::ViewportArguments();", I'd preferred to add this check.

> 
> > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp:349
> > +    return m_visibilityState;
> 
> Why not use WebCore::Page::visibilityState() directly instead?
Yes I agree. (getter/setter habit mistake...)
I'll remove the m_visibilityState attribute because it's never used...

> 
> > Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.h:136
> > +    // Must match with values of PageVisibilityState enum from PageVisibilityState.h.
> 
> It's not true anymore, it must however match QWebPage::PageVisibilityState.
> 
Erf I didn't update this comment. Yes, if there is a difference between these values and the ones defined in WebCore it'll generate a compile error into the switch (like for this patch).

> > Source/WebKit/qt/WidgetApi/qwebpage.cpp:3153
> > +    It should be called by Qt applications who want to notify Javascript application
> 
> grammar: "JavaScript applications" or "the JavaScript application"
> 
Oops, thanks.


> > Source/WebKit/qt/WidgetApi/qwebpage.h:220
> > +    enum PageVisibilityState {
> 
> VisibilityState would be fine and match with the property name. "Page" isn't helpful here.
Ok, is the same for QWebPageAdapter.h?

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