[EFL][WK2] Minibrowser crashes on right mouse click

[Jinwoo Song]

Make eary return if the PageClient does not create a ContextMenuProxy.

[Jinwoo Song]

Created attachment 162674 [details]
patch

[Gyuyoung Kim]

Comment on attachment 162674 [details]
patch

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

> Source/WebKit2/ChangeLog:8
> +        Make eary return if the PageClient does not create a ContextMenuProxy.

eary -> early ?

[Gyuyoung Kim]

Comment on attachment 162674 [details]
patch

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

Basically, I think EFL port should support createContextMenuProxy() as well.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2965
>  

Unneeded line.

[Jinwoo Song]

Created attachment 162692 [details]
patch

[Jinwoo Song]

(In reply to comment #3)
> (From update of attachment 162674 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162674&action=review
> 
> Basically, I think EFL port should support createContextMenuProxy() as well.
> 
I agree with you. :) 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:2965
> >  
> 
> Unneeded line.
Remove empty line.

[Jinwoo Song]

(In reply to comment #2)
> (From update of attachment 162674 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162674&action=review
> 
> > Source/WebKit2/ChangeLog:8
> > +        Make eary return if the PageClient does not create a ContextMenuProxy.
> 
> eary -> early ?
fixed.

[Jinwoo Song]

Created attachment 162695 [details]
patch.

wrong file was attached.

[Jinwoo Song]

Created attachment 162743 [details]
patch

[Raphael Kubo da Costa (:rakuco)]

Shouldn`t a context menu actually get created on right click?

[Jinwoo Song]

(In reply to comment #9)
> Shouldn`t a context menu actually get created on right click?
Yes, a context menu should be created on right click, but as the EFL has no implementation yet and it caused the crash.

PassRefPtr<WebContextMenuProxy> PageClientImpl::createContextMenuProxy(WebPageProxy*)
{
    notImplemented();
    return 0;
}
AFAIK, Gyuyoung will implement this. 

Did the answer fit to your question?

[WebKit Review Bot]

Comment on attachment 162743 [details]
patch

Clearing flags on attachment: 162743

Committed r127968: <http://trac.webkit.org/changeset/127968>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

Well, since the context menu implementation has not been written yet, this looks like a fix in the wrong place -- why bother trying to create a context menu in the first place? Now the check might end up being forgotten and lose its need once Gyuyoung adds his context menu implementation.

[Jinwoo Song]

(In reply to comment #13)
> Well, since the context menu implementation has not been written yet, this looks like a fix in the wrong place -- why bother trying to create a context menu in the first place? Now the check might end up being forgotten and lose its need once Gyuyoung adds his context menu implementation.

IMO, In the case that a port did not implement the ContextMenuProxy, this defensive code is worth well enough. Surely if EFL implement the ContextMenuProxy, it is no more necessary but it may care the other ports as well. 
The similar code have been landed recently for the PopupMenuProxy. 
http://trac.webkit.org/changeset/127745