Bug 110877 – [WK2][EFL] Encapsulate view states set-up within WebView

• Component: WebKit API
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

SSIA. Outcome:
1) Reduces usage of EwkView inside WebView class.
2) Reduces direct usage of WebPageProxy inside EwkView class.

[Mikhail Pozdnyakov]

Created attachment 190270 [details]
patch

[Kenneth Rohde Christiansen]

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

[Mikhail Pozdnyakov]

(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.

[Mikhail Pozdnyakov]

Created attachment 192920 [details]
patch v2

Changed initial values.

[Kenneth Rohde Christiansen]

Ping owner review/sign off.

[Benjamin Poulain]

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?

[Mikhail Pozdnyakov]

(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.

[Mikhail Pozdnyakov]

Created attachment 196406 [details]
patch v3

Changed API setter function names.

[WebKit Review Bot]

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

[Mikhail Pozdnyakov]

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