Created attachment 188036 [details] Patch

Created attachment 188037 [details] Patch Quickly add "explicit" to one of the constructor before Gyuyoung notices :)

Comment on attachment 188036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188036&action=review > Source/WebKit2/ChangeLog:10 > + PageClient and EwkView. In the end, PageClient should only interact with when completed, Page... > Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:36 > + > +EwkView* ViewClientEfl::toEwkView(const void* clientInfo) As this is the default EFL implementation shoulnd this be moved to the API/efl directory?

Comment on attachment 188037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188037&action=review > Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:61 > + viewClient.clientInfo = this; it does not need "this" it needs EwkView, right? > Source/WebKit2/UIProcess/efl/ViewClientEfl.h:37 > +class ViewClientEfl { I am not convinced we need this class at all, could EwkView itself somehow implement client interface?

Comment on attachment 188036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188036&action=review >> Source/WebKit2/ChangeLog:10 >> + PageClient and EwkView. In the end, PageClient should only interact with > > when completed, Page... Hmm. Why not but I'm not convinced "In the end" is incorrect. >> Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:36 >> +EwkView* ViewClientEfl::toEwkView(const void* clientInfo) > > As this is the default EFL implementation shoulnd this be moved to the API/efl directory? All our EFL clients are currently in this folder, so I added ViewClientEfl here. We may decide to move all of them to API/efl at some point but at least it is consistent for now.

Comment on attachment 188037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188037&action=review >> Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:61 >> + viewClient.clientInfo = this; > > it does not need "this" it needs EwkView, right? For the current callbacks we have, yes. But when we start adding more callbacks, this may change. Having the ViewClientEfl is more flexible and more forward looking IMHO. >> Source/WebKit2/UIProcess/efl/ViewClientEfl.h:37 >> +class ViewClientEfl { > > I am not convinced we need this class at all, could EwkView itself somehow implement client interface? Well, so far we have split clients from Ewk classes to avoid cluttering their implementation. For example, look at EwkContext and DownloadManagerEfl / ContextHistoryClientEfl / NetworkInfoProvider ... I think it is nice to have a clear separation between the clients and the Ewk classes. This is what we have been doing so far and I don't see why this case is different.

Comment on attachment 188037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188037&action=review > Source/WebKit2/UIProcess/efl/ViewClientEfl.h:39 > + PassOwnPtr<ViewClientEfl> create(EwkView* view) mmm.. maybe I'm missing something, but who calls it? I cannot find it in the patch..

Comment on attachment 188037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188037&action=review >> Source/WebKit2/UIProcess/efl/ViewClientEfl.h:39 >> + PassOwnPtr<ViewClientEfl> create(EwkView* view) > > mmm.. maybe I'm missing something, but who calls it? I cannot find it in the patch.. Oops :-)

Created attachment 188058 [details] Patch Take feedback into consideration.

Comment on attachment 188058 [details] Patch looks OK

Comment on attachment 188058 [details] Patch LGTM

Comment on attachment 188058 [details] Patch It's a little weird to use a C API to do this since it seems like an implementation detail, but I'll let it slide since it's not cross platform code.

Comment on attachment 188058 [details] Patch Clearing flags on attachment: 188058 Committed r142750: <http://trac.webkit.org/changeset/142750>

All reviewed patches have been landed. Closing bug.