Summary: | viewStateDidChange should always fully update ViewState | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gavin Barraclough <barraclough> | ||||||
Component: | WebKit2 | Assignee: | Gavin Barraclough <barraclough> | ||||||
Status: | ASSIGNED --- | ||||||||
Severity: | Normal | CC: | berto, bshafiei, bunhere, cgarcia, cmarcelo, commit-queue, gustavo, luiz, mrobinson, noam, sam, sergio, webkit-bug-importer, zeno | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Gavin Barraclough
2014-05-21 15:34:04 PDT
Created attachment 232099 [details]
Fix
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Look good to me, but I would like Anders to review it as well. Created attachment 232100 [details]
gtk/efl fix
Comment on attachment 232100 [details] gtk/efl fix View in context: https://bugs.webkit.org/attachment.cgi?id=232100&action=review Thanks for fixing the GTK+ port too. > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:110 > + if (webkitWebViewBaseIsFocused(WEBKIT_WEB_VIEW_BASE(m_viewWidget))) > + viewState |= ViewState::IsFocused; > + if (webkitWebViewBaseIsInWindowActive(WEBKIT_WEB_VIEW_BASE(m_viewWidget))) > + viewState |= ViewState::WindowIsActive; > + if (webkitWebViewBaseIsInWindow(WEBKIT_WEB_VIEW_BASE(m_viewWidget))) > + viewState |= ViewState::IsInWindow; > + if (webkitWebViewBaseIsVisible(WEBKIT_WEB_VIEW_BASE(m_viewWidget))) Now that all these are in the same function, I would use a local variable to avoid casting all the time. WebKitWebViewBase* webView = WEBKIT_WEB_VIEW_BASE(m_viewWidget); if (webkitWebViewBaseIsFocused(webView)) .... I think it also improves a bit the readability. > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.h:70 > + bool isViewVisible(); Why re-adding this as non-virtual? Comment on attachment 232100 [details]
gtk/efl fix
This looks really great, but please address the comments from Carlos.
(In reply to comment #5) > (From update of attachment 232100 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=232100&action=review > > Thanks for fixing the GTK+ port too. > > > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:110 > > + if (webkitWebViewBaseIsFocused(WEBKIT_WEB_VIEW_BASE(m_viewWidget))) > > + viewState |= ViewState::IsFocused; > > + if (webkitWebViewBaseIsInWindowActive(WEBKIT_WEB_VIEW_BASE(m_viewWidget))) > > + viewState |= ViewState::WindowIsActive; > > + if (webkitWebViewBaseIsInWindow(WEBKIT_WEB_VIEW_BASE(m_viewWidget))) > > + viewState |= ViewState::IsInWindow; > > + if (webkitWebViewBaseIsVisible(WEBKIT_WEB_VIEW_BASE(m_viewWidget))) > > Now that all these are in the same function, I would use a local variable to avoid casting all the time. > WebKitWebViewBase* webView = WEBKIT_WEB_VIEW_BASE(m_viewWidget); > > if (webkitWebViewBaseIsFocused(webView)) > .... > > I think it also improves a bit the readability. > Ah – yes, good point. Done. > > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.h:70 > > + bool isViewVisible(); > > Why re-adding this as non-virtual? Ooops, remnants of a partial refactor – that's gone now, removed. Transmitting file data ................. Committed revision 169439. Roll out on trunk in https://trac.webkit.org/r169465 |