Bug 110877

Created attachment 190270 [details] patch

Comment on attachment 190270 [details] patch LGTM, but are you sure the initial state is correct? any way to test it? Focus issues are hard to debug, so we better be certain

(In reply to comment #2) > (From update of attachment 190270 [details]) > LGTM, but are you sure the initial state is correct? any way to test it? Focus issues are hard to debug, so we better be certain Initial states are set by evas callbacks while evas object creation, so the values will be certain anyway. However think, both isFocused and isVisible are better to be set to 'false' initially indeed.

Created attachment 192920 [details] patch v2 Changed initial values.

Ping owner review/sign off.

Comment on attachment 192920 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=192920&action=review > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:66 > +void WKViewSetFocused(WKViewRef viewRef, bool flag) Not a fan of "flag" for the argument name. > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:76 > +void WKViewSetVisible(WKViewRef viewRef, bool flag) Ditto. SetIsVisible maybe? > Source/WebKit2/UIProcess/efl/WebView.cpp:95 > +void WebView::setFocused(bool focused) > +{ > + if (m_focused == focused) > + return; > + > + m_focused = focused; > + m_page->viewStateDidChange(WebPageProxy::ViewIsFocused | WebPageProxy::ViewWindowIsActive); > +} setFocused(false) call viewStateDidChange(WebPageProxy::ViewIsFocused)??? > Source/WebKit2/UIProcess/efl/WebView.cpp:104 > +void WebView::setVisible(bool visible) > +{ > + if (m_visible == visible) > + return; > + > + m_visible = visible; > + m_page->viewStateDidChange(WebPageProxy::ViewIsVisible); > +} You duplicate states between WebPageProxy and WebView. This is a really bad idea. > Source/WebKit2/UIProcess/efl/WebView.cpp:305 > bool WebView::isViewFocused() > { > - return m_ewkView->isFocused(); > + return isFocused(); > } > > bool WebView::isViewVisible() > { > - return m_ewkView->isVisible(); > + return isVisible(); > } Why don't you get rid of those APIs?

(In reply to comment #6) > (From update of attachment 192920 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=192920&action=review > > > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:66 > > +void WKViewSetFocused(WKViewRef viewRef, bool flag) > > Not a fan of "flag" for the argument name. > > > Source/WebKit2/UIProcess/API/C/efl/WKView.cpp:76 > > +void WKViewSetVisible(WKViewRef viewRef, bool flag) > > Ditto. > > SetIsVisible maybe? agree, SetIsVisible is better. > > > Source/WebKit2/UIProcess/efl/WebView.cpp:95 > > +void WebView::setFocused(bool focused) > > +{ > > + if (m_focused == focused) > > + return; > > + > > + m_focused = focused; > > + m_page->viewStateDidChange(WebPageProxy::ViewIsFocused | WebPageProxy::ViewWindowIsActive); > > +} > > setFocused(false) call viewStateDidChange(WebPageProxy::ViewIsFocused)??? right, this code is in page proxy is invoked: if (flags & ViewIsFocused) m_process->send(Messages::WebPage::SetFocused(m_pageClient->isViewFocused()), m_pageID); so it just makes page proxy to query state from page client (web view itself). > > > Source/WebKit2/UIProcess/efl/WebView.cpp:104 > > +void WebView::setVisible(bool visible) > > +{ > > + if (m_visible == visible) > > + return; > > + > > + m_visible = visible; > > + m_page->viewStateDidChange(WebPageProxy::ViewIsVisible); > > +} > > You duplicate states between WebPageProxy and WebView. This is a really bad idea. there is no duplication, as page client is supposed to hold the state. > > > Source/WebKit2/UIProcess/efl/WebView.cpp:305 > > bool WebView::isViewFocused() > > { > > - return m_ewkView->isFocused(); > > + return isFocused(); > > } > > > > bool WebView::isViewVisible() > > { > > - return m_ewkView->isVisible(); > > + return isVisible(); > > } > > Why don't you get rid of those APIs? mm.. isViewVisible and isViewFocused are mandatory things to have for page client.

Created attachment 196406 [details] patch v3 Changed API setter function names.

Comment on attachment 196406 [details] patch v3 Rejecting attachment 196406 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 196406, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: ebView.cpp Hunk #2 succeeded at 104 with fuzz 1 (offset 7 lines). Hunk #3 succeeded at 321 (offset 13 lines). patching file Source/WebKit2/UIProcess/efl/WebView.h Hunk #1 succeeded at 60 with fuzz 2 (offset 3 lines). Hunk #2 FAILED at 186. 1 out of 2 hunks FAILED -- saving rejects to file Source/WebKit2/UIProcess/efl/WebView.h.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Benjamin Poulain']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://webkit-commit-queue.appspot.com/results/17460137

Committed r147723: <http://trac.webkit.org/changeset/147723>