Bug 94906 – [EFL][WK2] Fix PageClientImpl layer violation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kangil Han]

Compared to other ports, i.e. mac and gtk+, ours has WebContext reference in PageClientImpl.
It seems not necessary because WebContext is singleton and PageClientImpl could be implemented with view only.
Having different implementation to gtk+ isn't be our policy too.

[Kangil Han]

Created attachment 160393 [details]
patch

[Kangil Han]

Created attachment 160398 [details]
patch

rebase

[Kangil Han]

Created attachment 160406 [details]
patch

rebase again.

[Kangil Han]

ryuan: review please?

[Chris Dumez]

Comment on attachment 160406 [details]
patch

LGTM. Does it stop complaining about the WebContext being leaked now?

[Kangil Han]

(In reply to comment #5)
> (From update of attachment 160406 [details])
> LGTM. Does it stop complaining about the WebContext being leaked now?

Unfortunately, nope. :-(
WebProcessProxy has the key for resolving WebContext leak but seems WebProcess didn't do anything for it. To fix it I should discuss more with apple people.

[Ryuan Choi]

Comment on attachment 160406 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160406&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:191
> +    priv->pageProxy.get()->viewStateDidChange(WebPageProxy::ViewIsFocused | WebPageProxy::ViewWindowIsActive);

I think that .get() is unnecessary when you call a method.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 160406 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160406&action=review

> Source/WebKit2/ChangeLog:10
> +        Given WK2 hierarchy, current PageClientImpl has violated API layer by having WebPageProxy.
> +        Subsequently, it has been given WebContext, static singleton object, in its argument unnecessarily.
> +        Therefore, this patch fixed addressed issues and let view has WebPageProxy as other ports, i.e. mac and gtk+, does currently.

It'd be good to explain how those arguments that have been removed from PageClientImpl's constructor are supposed to be retrieved now, as well as add a short indication of what has been done to the methods below when there's been more than a search-and-replace change.

[Gyuyoung Kim]

Comment on attachment 160406 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160406&action=review

> Source/WebKit2/UIProcess/API/efl/PageClientImpl.h:46
> +    PageClientImpl(Evas_Object*);

Missing *explicit* keyword.

[Kangil Han]

Created attachment 160900 [details]
patch

Done. Thanks!

[Ryuan Choi]

(In reply to comment #10)
> Created an attachment (id=160900) [details]
> patch
> 
> Done. Thanks!

Looks good to me.

[Gyuyoung Kim]

Comment on attachment 160900 [details]
patch

LGTM.

[WebKit Review Bot]

Comment on attachment 160900 [details]
patch

Clearing flags on attachment: 160900

Committed r126844: <http://trac.webkit.org/changeset/126844>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.