Disambiguate ShowContextMenu implementation.

In the regular chromium flow, there is a hook invoked when
a long press gesture happens. The terminology used in the
messages and methods involved is "show context menu".

However, upon a long press gesture, chromium-efl does not
necessarily show a context menu right way. It might, for instance,
enter selection mode if the long press gesture happens on a
text (non anchor) content, and only tries to show an actual
context menu when user lifts up his finger.
Alternatively, it might show a context menu right away
in case the gesture happens on an <image> or a link.

For both cases, the same chromium hook is called by chromium,
whereas the former performs a second pass on the chromium hook,
which is confusing. Our current implementation ends up like:

EWebView::ShowContextMenu() {
if (called by chromium)
do this;
else // called by EWK on a second pass
do that;
}

Problem is that this is a constant source of issues because
it requires control of state variables to handle each passes
on the method.

Patch makes that separation clear by disambiguate this logic:
- EWebView::HandleLongPressEvent calls SelectionController
- SelectionController calls out to EWebView::ShowContextMenu.

Bug: http://107.108.218.239/bugzilla/show_bug.cgi?id=14304

Reviewed by: a.renevier, djmix.kim, j.majnert, sns.park

Change-Id: I2bf2a1b24e20d7f0f50e6881144cb3cf8cc62925
Signed-off-by: Antonio Gomes <a1.gomes@samsung.com>

diff --git a/tizen_src/chromium_impl/content/browser/selection/selection_controller_efl.cc b/tizen_src/chromium_impl/content/browser/selection/selection_controller_efl.cc
index 0bb7059..dd2d275 100644 (file)
--- a/tizen_src/chromium_impl/content/browser/selection/selection_controller_efl.cc
+++ b/tizen_src/chromium_impl/content/browser/selection/selection_controller_efl.cc
@@ -317,19 +317,16 @@ bool SelectionControllerEfl::IsAnyHandleOnScreen() const {
 }
 
 void SelectionControllerEfl::ShowContextMenu() {
-  WebContentsImpl* wci = static_cast<WebContentsImpl*>(&web_contents_);
-  RenderWidgetHostViewEfl* rwhv =
-      static_cast<RenderWidgetHostViewEfl*>(web_contents_.GetRenderWidgetHostView());
-  RenderViewHost* rvh = static_cast<RenderViewHost*>(wci->GetRenderViewHost());
-
-  content::ContextMenuParams convertedParams = *(selection_data_->GetContextMenuParams());
-
   // If there is no handles on screen, do not show context
   // menu, but also do not leave selection mode.
   if (!IsAnyHandleOnScreen())
     return;
 
+  content::ContextMenuParams convertedParams = *(selection_data_->GetContextMenuParams());
+
   int blinkX, blinkY;
+  RenderWidgetHostViewEfl* rwhv =
+      static_cast<RenderWidgetHostViewEfl*>(web_contents_.GetRenderWidgetHostView());
   if (rwhv)
     rwhv->EvasToBlinkCords(convertedParams.x, convertedParams.y, &blinkX, &blinkY);
   convertedParams.x = blinkX;
@@ -338,8 +335,9 @@ void SelectionControllerEfl::ShowContextMenu() {
   // TODO(a1.gomes): In case of EWK apps, the call below end up calling
   // EWebView::ShowContextMenu. We have to make sure parameters
   // are correct.
+  WebContentsImpl* wci = static_cast<WebContentsImpl*>(&web_contents_);
   WebContentsViewEfl* wcve = static_cast<WebContentsViewEfl*>(wci->GetView());
-  wcve->ShowContextMenu(rvh->GetMainFrame(), convertedParams);
+  wcve->ShowContextMenu(convertedParams);
 }
 
 void SelectionControllerEfl::CancelContextMenu(int request_id) {

diff --git a/tizen_src/chromium_impl/content/browser/web_contents/web_contents_view_efl.cc b/tizen_src/chromium_impl/content/browser/web_contents/web_contents_view_efl.cc
index 7ff68d5..a22c28d 100644 (file)
--- a/tizen_src/chromium_impl/content/browser/web_contents/web_contents_view_efl.cc
+++ b/tizen_src/chromium_impl/content/browser/web_contents/web_contents_view_efl.cc
@@ -231,6 +231,18 @@ void WebContentsViewEfl::ShowContextMenu(RenderFrameHost* render_frame_host,
     delegate_->ShowContextMenu(render_frame_host, params);
 }
 
+// Note: This is a EFL specific extension to WebContentsView responsible
+// for actually displaying a context menu UI.
+// On the other side, its overload method (above) is called by chromium
+// in response to a long press gesture. It is up to the embedder to define
+// how this long press gesture will be
+// handled, e.g. showing context menu right away or afte user lifts up his
+// finger.
+void WebContentsViewEfl::ShowContextMenu(const ContextMenuParams& params) {
+  if (efl_delegate_)
+    efl_delegate_->ShowContextMenu(params);
+}
+
 void WebContentsViewEfl::QuerySelectionStyle() {
   if (efl_delegate_)
     efl_delegate_->QuerySelectionStyle();

diff --git a/tizen_src/chromium_impl/content/browser/web_contents/web_contents_view_efl.h b/tizen_src/chromium_impl/content/browser/web_contents/web_contents_view_efl.h
index b0c191d..72b5f15 100644 (file)
--- a/tizen_src/chromium_impl/content/browser/web_contents/web_contents_view_efl.h
+++ b/tizen_src/chromium_impl/content/browser/web_contents/web_contents_view_efl.h
@@ -57,6 +57,7 @@ class CONTENT_EXPORT WebContentsViewEfl
   // content::RenderViewHostDelegateView implementation.
   void ShowContextMenu(RenderFrameHost* render_frame_host,
                        const ContextMenuParams& params) override;
+  void ShowContextMenu(const ContextMenuParams& params);
   void ShowPopupMenu(RenderFrameHost* render_frame_host,
                      const gfx::Rect& bounds,
                      int item_height,

diff --git a/tizen_src/chromium_impl/content/public/browser/web_contents_view_efl_delegate.h b/tizen_src/chromium_impl/content/public/browser/web_contents_view_efl_delegate.h
index f46a98c..75702b5 100644 (file)
--- a/tizen_src/chromium_impl/content/public/browser/web_contents_view_efl_delegate.h
+++ b/tizen_src/chromium_impl/content/public/browser/web_contents_view_efl_delegate.h
@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "content/common/content_export.h"
+#include "content/public/common/context_menu_params.h"
 #include "content/public/common/menu_item.h"
 #include "third_party/WebKit/public/web/WebInputEvent.h"
 #include "ui/gfx/geometry/rect.h"
@@ -39,6 +40,8 @@ class WebContentsViewEflDelegate {
   virtual void HandleZoomGesture(blink::WebGestureEvent& event) = 0;
 
   virtual bool UseKeyPadWithoutUserAction() = 0;
+
+  virtual void ShowContextMenu(const ContextMenuParams& params) = 0;
 };
 
 } // namespace content

diff --git a/tizen_src/ewk/efl_integration/eweb_view.cc b/tizen_src/ewk/efl_integration/eweb_view.cc
index 317f126..222f57f 100644 (file)
--- a/tizen_src/ewk/efl_integration/eweb_view.cc
+++ b/tizen_src/ewk/efl_integration/eweb_view.cc
@@ -1106,7 +1106,7 @@ Eina_Bool EWebView::PopupMenuClose() {
   return true;
 }
 
-void EWebView::ShowContextMenu(const content::ContextMenuParams& params) {
+void EWebView::HandleLongPressGesture(const content::ContextMenuParams& params) {
   // This menu is created in renderer process and it does not now anything about
   // view scaling factor and it has another calling sequence, so coordinates is not updated.
   content::ContextMenuParams convertedParams = params;
@@ -1114,7 +1114,26 @@ void EWebView::ShowContextMenu(const content::ContextMenuParams& params) {
   convertedParams.x = convertedPoint.x();
   convertedParams.y = convertedPoint.y();
 
-  context_menu_.reset(new content::ContextMenuControllerEfl(this, *web_contents_.get()));
+  Evas_Coord x, y;
+  evas_object_geometry_get(evas_object(), &x, &y, 0, 0);
+  convertedParams.x += x;
+  convertedParams.y += y;
+
+  if (GetSelectionController() && GetSelectionController()->GetLongPressed()) {
+    bool show_context_menu_now =
+        !GetSelectionController()->HandleLongPressEvent(convertedPoint, convertedParams);
+    if (show_context_menu_now)
+      ShowContextMenuInternal(convertedParams);
+  }
+}
+
+void EWebView::ShowContextMenu(const content::ContextMenuParams& params) {
+  // This menu is created in renderer process and it does not now anything about
+  // view scaling factor and it has another calling sequence, so coordinates is not updated.
+  content::ContextMenuParams convertedParams = params;
+  gfx::Point convertedPoint = rwhv()->ConvertPointInViewPix(gfx::Point(params.x, params.y));
+  convertedParams.x = convertedPoint.x();
+  convertedParams.y = convertedPoint.y();
 
   Evas_Coord x, y;
   evas_object_geometry_get(evas_object(), &x, &y, 0, 0);
@@ -1123,17 +1142,14 @@ void EWebView::ShowContextMenu(const content::ContextMenuParams& params) {
 
   context_menu_position_ = gfx::Point(convertedParams.x, convertedParams.y);
 
-  if (GetSelectionController()) {
-    bool handled = false;
-    if (GetSelectionController()->GetLongPressed())
-      handled = GetSelectionController()->HandleLongPressEvent(convertedPoint, convertedParams);
-
-    if (!handled) {
-      if (!context_menu_->PopulateAndShowContextMenu(convertedParams)) {
-        context_menu_.reset();
-        GetSelectionController()->HideHandles();
-      }
-    }
+  ShowContextMenuInternal(convertedParams);
+}
+
+void EWebView::ShowContextMenuInternal(const content::ContextMenuParams& params) {
+  context_menu_.reset(new content::ContextMenuControllerEfl(this, *web_contents_.get()));
+  if (!context_menu_->PopulateAndShowContextMenu(params)) {
+    context_menu_.reset();
+    GetSelectionController()->HideHandles();
   }
 }
 

diff --git a/tizen_src/ewk/efl_integration/eweb_view.h b/tizen_src/ewk/efl_integration/eweb_view.h
index c230d55..0cb6a5b 100644 (file)
--- a/tizen_src/ewk/efl_integration/eweb_view.h
+++ b/tizen_src/ewk/efl_integration/eweb_view.h
@@ -288,6 +288,7 @@ class EWebView {
   Eina_Bool DidSelectPopupMenuItem(int selectedIndex);
   Eina_Bool DidMultipleSelectPopupMenuItem(std::vector<int>& selectedIndices);
   Eina_Bool PopupMenuClose();
+  void HandleLongPressGesture(const content::ContextMenuParams&);
   void ShowContextMenu(const content::ContextMenuParams&);
   void CancelContextMenu(int request_id);
   void SetScale(double scale_factor, int x, int y);
@@ -475,6 +476,8 @@ class EWebView {
 
   void ReleasePopupMenuList();
 
+  void ShowContextMenuInternal(const content::ContextMenuParams&);
+
   scoped_refptr<WebViewEvasEventHandler> evas_event_handler_;
   scoped_refptr<Ewk_Context> context_;
   scoped_refptr<Ewk_Context> old_context_;

diff --git a/tizen_src/ewk/efl_integration/web_contents_view_delegate_ewk.cc b/tizen_src/ewk/efl_integration/web_contents_view_delegate_ewk.cc
index fd4daf7..b69f4b3 100644 (file)
--- a/tizen_src/ewk/efl_integration/web_contents_view_delegate_ewk.cc
+++ b/tizen_src/ewk/efl_integration/web_contents_view_delegate_ewk.cc
@@ -10,8 +10,12 @@ WebContentsViewDelegateEwk::WebContentsViewDelegateEwk(EWebView* wv)
     : web_view_(wv) {
 }
 
+// Note: WebContentsViewDelegate::ShowContextMenu is the hook called
+// by chromium in response to a long press gesture.
+// However, in EWK apps, browser enters in selection mode in response
+// to it.
 void WebContentsViewDelegateEwk::ShowContextMenu(
     content::RenderFrameHost* render_frame_host,
     const content::ContextMenuParams& params) {
-  web_view_->ShowContextMenu(params);
+  web_view_->HandleLongPressGesture(params);
 }

diff --git a/tizen_src/ewk/efl_integration/web_contents_view_efl_delegate_ewk.cc b/tizen_src/ewk/efl_integration/web_contents_view_efl_delegate_ewk.cc
index fa916c7..6881c3b 100644 (file)
--- a/tizen_src/ewk/efl_integration/web_contents_view_efl_delegate_ewk.cc
+++ b/tizen_src/ewk/efl_integration/web_contents_view_efl_delegate_ewk.cc
@@ -46,3 +46,8 @@ void WebContentsViewEflDelegateEwk::HandleZoomGesture(blink::WebGestureEvent& ev
 bool WebContentsViewEflDelegateEwk::UseKeyPadWithoutUserAction() {
   return web_view_->GetSettings()->useKeyPadWithoutUserAction();
 }
+
+void WebContentsViewEflDelegateEwk::ShowContextMenu(
+    const content::ContextMenuParams& params) {
+  web_view_->ShowContextMenu(params);
+}

diff --git a/tizen_src/ewk/efl_integration/web_contents_view_efl_delegate_ewk.h b/tizen_src/ewk/efl_integration/web_contents_view_efl_delegate_ewk.h
index 193b348..d80fb5a 100644 (file)
--- a/tizen_src/ewk/efl_integration/web_contents_view_efl_delegate_ewk.h
+++ b/tizen_src/ewk/efl_integration/web_contents_view_efl_delegate_ewk.h
@@ -41,6 +41,8 @@ class WebContentsViewEflDelegateEwk
 
   bool UseKeyPadWithoutUserAction() override;
 
+  void ShowContextMenu(const content::ContextMenuParams& params) override;
+
  private:
   EWebView* web_view_;
 };