Refactor implementation of ewk_geolocation_permission* API
authorWojciech Bielawski <w.bielawski@samsung.com>
Mon, 26 Jan 2015 08:48:56 +0000 (09:48 +0100)
committerYoungsoo Choi <kenshin.choi@samsung.com>
Tue, 10 Jul 2018 06:57:09 +0000 (06:57 +0000)
Corrected behaviour of
GeolocationPermissionContextEfl::RequestPermissionOnUIThread. Previously it
wrongly called SendGeolocationPermissionResponse callback even when the API
embeder made a decision in his callback.
Geolocation private implementation refactored to use common
Ewk_Suspendable_Object implementation.
In addition unit test implementation refactor.

Bug: http://107.108.218.239/bugzilla/show_bug.cgi?id=9524
Reviewed by: Piotr Tworek, arno renevier

Change-Id: Ib1674d06f3c2f48ddea009c842e30643cdf1c64c
Signed-off-by: Wojciech Bielawski <w.bielawski@samsung.com>
tizen_src/ewk/efl_integration/public/ewk_geolocation.cc
tizen_src/ewk/unittest/utc_blink_ewk_geolocation_permission_request_origin_get_func.cpp
tizen_src/ewk/unittest/utc_blink_ewk_geolocation_permission_request_set_func.cpp
tizen_src/impl/API/ewk_geolocation_private.cc
tizen_src/impl/API/ewk_geolocation_private.h
tizen_src/impl/API/ewk_suspendable_object.cc
tizen_src/impl/browser/geolocation/geolocation_permission_context_efl.cc

index f4996cd..06cfd4f 100644 (file)
@@ -19,18 +19,18 @@ const Ewk_Security_Origin* ewk_geolocation_permission_request_origin_get(const E
 Eina_Bool ewk_geolocation_permission_request_set(Ewk_Geolocation_Permission_Request* permissionRequest, Eina_Bool allow)
 {
     EINA_SAFETY_ON_NULL_RETURN_VAL(permissionRequest, EINA_FALSE);
-    return permissionRequest->SetDecision(allow);
+    return permissionRequest->setDecision(allow == EINA_TRUE);
 }
 
 Eina_Bool ewk_geolocation_permission_reply(Ewk_Geolocation_Permission_Request* permissionRequest, Eina_Bool allow)
 {
     EINA_SAFETY_ON_NULL_RETURN_VAL(permissionRequest, EINA_FALSE);
-    return permissionRequest->SetDecision(allow); // the same as ewk_geolocation_permission_request_set
+    return permissionRequest->setDecision(allow == EINA_TRUE); // the same as ewk_geolocation_permission_request_set
 }
 
 void ewk_geolocation_permission_request_suspend(Ewk_Geolocation_Permission_Request* permissionRequest)
 {
     EINA_SAFETY_ON_NULL_RETURN(permissionRequest);
-    permissionRequest->Suspend();
+    permissionRequest->suspend();
 }
 
index 58dbdf9..592c9f2 100755 (executable)
@@ -6,8 +6,6 @@
 
 #include "utc_blink_ewk_base.h"
 
-#define SAMPLE_HTML_FILE                 ("/common/sample_js_geolocation.html")
-
 class utc_blink_ewk_geolocation_permission_request_origin_get : public utc_blink_ewk_base {
 
 protected:
@@ -22,53 +20,42 @@ protected:
     evas_object_smart_callback_del(GetEwkWebView(),"geolocation,permission,request", request_geolocation_permisson);
   }
 
-  void LoadFinished(Evas_Object* webview) {
-
-    EventLoopStop(utc_blink_ewk_base::Failure);
+  void LoadFinished(Evas_Object* webview)
+  {
+    EventLoopStop(Failure);
   }
 
   static void request_geolocation_permisson(void* data, Evas_Object* webview, void* event_info)
   {
-    utc_message("[request_geolocation_permisson] :: \n");
+    utc_message("[request_geolocation_permisson]");
     utc_blink_ewk_geolocation_permission_request_origin_get *owner = static_cast<utc_blink_ewk_geolocation_permission_request_origin_get*>(data);
 
     Ewk_Geolocation_Permission_Request* permission_request = (Ewk_Geolocation_Permission_Request*)event_info;
-    if (permission_request && ewk_geolocation_permission_request_origin_get(permission_request)) {
+    ASSERT_NE(nullptr, permission_request);
 
-        owner->EventLoopStop(utc_blink_ewk_base::Success);
-    }
+    const Ewk_Security_Origin* security_origin = ewk_geolocation_permission_request_origin_get(permission_request);
+    ASSERT_NE(nullptr, security_origin);
+
+    // Just to be sure returned struct is a valid Ewk_Security_Origin object
+    ASSERT_NE(nullptr, ewk_security_origin_host_get(security_origin));
+    ASSERT_NE(nullptr, ewk_security_origin_protocol_get(security_origin));
+    owner->EventLoopStop(Success);
   }
 };
 
 /**
- * @brief Positive test whether Ewk_Geolocation_Permission_Request has the Ewk_Security_Origin value when web page requests geolocation permission for frame.
+ * @brief Positive test whether ewk_geolocation_permission_request_origin_get returns valid object
  */
 TEST_F(utc_blink_ewk_geolocation_permission_request_origin_get, POS_TEST)
 {
-  Eina_Bool result = ewk_view_url_set(GetEwkWebView(), GetResourceUrl(SAMPLE_HTML_FILE).c_str());
-  if (!result)
-    FAIL();
-
-  utc_blink_ewk_base::MainLoopResult main_result = EventLoopStart();
-  if (main_result != utc_blink_ewk_base::Success)
-    FAIL();
-
-//TODO: this code should use ewk_geolocation_permission_request_origin_get and check its behaviour.
-//Results should be reported using one of utc_ macros
+  ASSERT_EQ(EINA_TRUE, ewk_view_url_set(GetEwkWebView(), GetResourceUrl("/common/sample_js_geolocation.html").c_str()));
+  ASSERT_EQ(Success, EventLoopStart());
 }
 
 /**
- * @brief Negative test whether Ewk_Geolocation_Permission_Data has the Ewk_Geolocation_Permission_Request value when web page requests geolocation permission for frame.
+ * @brief Negative test whether API behaves properly with invalid argument.
  */
 TEST_F(utc_blink_ewk_geolocation_permission_request_origin_get, NEG_TEST)
 {
-  Eina_Bool test_result = EINA_FALSE;
-  if( ewk_geolocation_permission_request_origin_get(NULL) ) {
-
-    test_result = EINA_TRUE;
-  }
-  utc_check_ne(test_result, EINA_TRUE);
-
-//TODO: this code should use ewk_geolocation_permission_request_origin_get and check its behaviour.
-//Results should be reported using one of utc_ macros
+  ASSERT_EQ(nullptr, ewk_geolocation_permission_request_origin_get(nullptr));
 }
index 41e53d0..70f2ac1 100755 (executable)
@@ -6,68 +6,72 @@
 
 #include "utc_blink_ewk_base.h"
 
-#define SAMPLE_HTML_FILE                 ("/common/sample_js_geolocation.html")
-
 class utc_blink_ewk_geolocation_permission_request_set : public utc_blink_ewk_base {
 
 protected:
 
-  void PostSetUp()
+  virtual void PostSetUp() override
   {
     evas_object_smart_callback_add(GetEwkWebView(),"geolocation,permission,request", request_geolocation_permisson, this);
   }
 
-  void PreTearDown()
+  virtual void PreTearDown() override
   {
     evas_object_smart_callback_del(GetEwkWebView(),"geolocation,permission,request", request_geolocation_permisson);
   }
 
-  void LoadFinished(Evas_Object* webview) {
+  virtual void ConsoleMessage(Evas_Object* webview, const Ewk_Console_Message* msg) override
+  {
+    utc_blink_ewk_base::ConsoleMessage(webview, msg);
 
-    EventLoopStop(utc_blink_ewk_base::Failure);
+    const char* message = ewk_console_message_text_get(msg);
+    consoleMessage = message ? message : "";
+    EventLoopStop(Failure);
   }
 
   static void request_geolocation_permisson(void* data, Evas_Object* webview, void* event_info)
   {
-    utc_message("[request_geolocation_permisson] :: \n");
+    utc_message("[request_geolocation_permisson]");
     utc_blink_ewk_geolocation_permission_request_set *owner = static_cast<utc_blink_ewk_geolocation_permission_request_set*>(data);
 
     Ewk_Geolocation_Permission_Request* permission_request = (Ewk_Geolocation_Permission_Request*)event_info;
-    if (permission_request && ewk_geolocation_permission_request_set(permission_request, EINA_TRUE)) {
-
-       owner->EventLoopStop(utc_blink_ewk_base::Success);
-    }
+    owner->EventLoopStop(permission_request
+                         && ewk_geolocation_permission_request_set(permission_request, owner->permissionDecision) ?
+                             Success : Failure);
   }
+
+protected:
+  std::string consoleMessage;
+  bool permissionDecision = false;
 };
 
 /**
- * @brief Positive test whether request of permission is allowed when web page requests geolocation permission for frame.
+ * @brief Positive test whether ewk_geolocation_permission_request_set works.
  */
 TEST_F(utc_blink_ewk_geolocation_permission_request_set, POS_TEST)
 {
-  Eina_Bool result = ewk_view_url_set(GetEwkWebView(), GetResourceUrl(SAMPLE_HTML_FILE).c_str());
-  if (!result)
-    FAIL();
+  ASSERT_EQ(EINA_TRUE, ewk_view_url_set(GetEwkWebView(), GetResourceUrl("/common/sample_js_geolocation.html").c_str()));
 
-  utc_blink_ewk_base::MainLoopResult main_result = EventLoopStart();
-  if (main_result != utc_blink_ewk_base::Success)
-    FAIL();
+  permissionDecision = true;
+  ASSERT_EQ(Success, EventLoopStart());
+  ASSERT_EQ(Failure, EventLoopStart(5));
+  if (!("EWK Geolocation SUCCESS" == consoleMessage
+        || "EWK Geolocation POSITION_UNAVAILABLE"))
+    FAIL() << "Geolocation permission request set to allow failed";
 
-//TODO: this code should use ewk_geolocation_permission_request_set and check its behaviour.
-//Results should be reported using one of utc_ macros */
+  ASSERT_EQ(EINA_TRUE, ewk_view_url_set(GetEwkWebView(), GetResourceUrl("/common/sample_js_geolocation.html").c_str()));
+
+  permissionDecision = false;
+  ASSERT_EQ(Success, EventLoopStart());
+  ASSERT_EQ(Failure, EventLoopStart(5));
+  ASSERT_STREQ("EWK Geolocation PERMISSION_DENIED", consoleMessage.c_str());
 }
 
 /**
- * @brief Negative test whether request of permission is allowed when web page requests geolocation permission for frame.
+ * @brief Negative test whether ewk_geolocation_permission_request_set behaves correctly for invalid arguments.
  */
 TEST_F(utc_blink_ewk_geolocation_permission_request_set, NEG_TEST)
 {
-  Eina_Bool test_result = EINA_FALSE;
-  if( ewk_geolocation_permission_request_set(NULL, EINA_TRUE) ) {
-
-    test_result = EINA_TRUE;
-  }
-
-//TODO: this code should use ewk_geolocation_permission_request_set and check its behaviour.
-//Results should be reported using one of utc_ macros */
+  ASSERT_EQ(EINA_FALSE, ewk_geolocation_permission_request_set(nullptr, EINA_TRUE));
+  ASSERT_EQ(EINA_FALSE, ewk_geolocation_permission_request_set(nullptr, EINA_FALSE));
 }
index b8202d8..c210b72 100644 (file)
@@ -11,40 +11,12 @@ using content::BrowserThread;
 
 _Ewk_Geolocation_Permission_Request::_Ewk_Geolocation_Permission_Request(
     const char* host, const char* protocol, uint16_t port,
-    base::Callback<void(bool)> inCallback)
-  : origin_(new tizen_webview::Security_Origin(host, protocol, port))
-  , isDecided_(false)
-  , isSuspended_(false)
-  , callback_(inCallback) {
+    base::Callback<void(bool)> callback)
+  : Ewk_Suspendable_Object(callback)
+  , origin_(new tizen_webview::Security_Origin(host, protocol, port)) {
 }
 
 _Ewk_Geolocation_Permission_Request::~_Ewk_Geolocation_Permission_Request() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   delete origin_;
 }
-
-bool _Ewk_Geolocation_Permission_Request::SetDecision(Eina_Bool allow) {
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  // Decision may be done only once.
-  if (!isDecided_) {
-    isDecided_ = true;
-    callback_.Run(allow == EINA_TRUE);
-
-    if (isSuspended_) {
-      // If decision was suspended, then it was not deleted by the creator
-      // Deletion of this object must be done after decision was made, as
-      // this object will no longer be valid. But if decision was not suspended
-      // it will be deleted right after permission callbacks are executed.
-      BrowserThread::DeleteSoon(BrowserThread::UI, FROM_HERE, this);
-    }
-    return true;
-  }
-  return false;
-}
-
-void _Ewk_Geolocation_Permission_Request::Suspend() {
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (!isDecided_) {
-    isSuspended_ = true;
-  }
-}
index da738f5..54fd995 100644 (file)
@@ -5,29 +5,24 @@
 #ifndef ewk_geolocation_private_h
 #define ewk_geolocation_private_h
 
-#include <base/callback.h>
+#include "base/callback.h"
+#include "ewk_suspendable_object.h"
 #include "tizen_webview/public/tw_security_origin.h"
 
 // This holds the geolocation permission request data.
 // The callback present is the direct engine callback which need
 // to be called once the permission is determined by app.
-class _Ewk_Geolocation_Permission_Request {
+class _Ewk_Geolocation_Permission_Request : public Ewk_Suspendable_Object{
  public:
-  _Ewk_Geolocation_Permission_Request(const char* host, const char* protocol, uint16_t port, base::Callback<void(bool)> inCallback);
+  _Ewk_Geolocation_Permission_Request(const char* host, const char* protocol,
+                                      uint16_t port,
+                                      base::Callback<void(bool)> callback);
   ~_Ewk_Geolocation_Permission_Request();
 
-  bool isDecided() const { return isDecided_; }
-  bool isSuspended() const { return isSuspended_; }
-  void Suspend();
-  bool SetDecision(Eina_Bool allow);
   tizen_webview::Security_Origin* GetOrigin() const { return origin_; }
 
  private:
   tizen_webview::Security_Origin* origin_;
-  bool isDecided_;
-  bool isSuspended_;
-  base::Callback<void(bool)> callback_;
-
 };
 
 #endif // ewk_geolocation_private_h
index 61e4eef..f2b0ea8 100644 (file)
@@ -32,6 +32,11 @@ void Ewk_Suspendable_Object::ignore() {
 }
 
 bool Ewk_Suspendable_Object::suspend() {
-  isSuspended_ = true;
-  return true;
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (!isDecided_) {
+    isSuspended_ = true;
+    return true;
+  }
+  return false;
 }
index 07f54eb..5cf434f 100644 (file)
@@ -69,14 +69,11 @@ void GeolocationPermissionContextEfl::RequestPermissionOnUIThread(
           request.get());
     }
 
-    if (request->isSuspended()) {
+    if (request->isSuspended())
       request.release();
-      return;
-    }
+    else if (!request->isDecided())  // Reject permission if request is not suspended and not decided
+      callback.Run(false);
   }
-
-  // Otherwise reject permission.
-  callback.Run(false);
 }
 
 void GeolocationPermissionContextEfl::RequestPermission(