Cleanup WebFrame class.

Fixes for comments on crrev.com/c/1192424 after merge.

Bug: 851636
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: Id742b36e6f46a647b510f81307f371b51bac7b3f
Reviewed-on: https://chromium-review.googlesource.com/1208463
Commit-Queue: Mike Dougherty <michaeldo@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589433}

ios/web/public/web_state/web_frame.h

@@ -9,10 +9,10 @@
 
 #include "base/macros.h"
 #include "base/supports_user_data.h"
+#include "base/time/time.h"
 #include "url/gurl.h"
 
 namespace base {
-class TimeDelta;
 class Value;
 }
 

ios/web/web_state/web_frame_impl.h

@@ -36,17 +36,12 @@ class WebFrameImpl : public WebFrame, public web::WebStateObserver {
 
   // The associated web state.
   WebState* GetWebState();
-  // Detaches the receiver from the associated  WebState.
-  void DetachFromWebState();
 
   // WebFrame implementation
   std::string GetFrameId() const override;
   bool IsMainFrame() const override;
   GURL GetSecurityOrigin() const override;
 
-  // WebStateObserver implementation
-  void WebStateDestroyed(web::WebState* web_state) override;
-
   bool CallJavaScriptFunction(
       const std::string& name,
       const std::vector<base::Value>& parameters) override;
@@ -56,6 +51,9 @@ class WebFrameImpl : public WebFrame, public web::WebStateObserver {
       base::OnceCallback<void(const base::Value*)> callback,
       base::TimeDelta timeout) override;
 
+  // WebStateObserver implementation
+  void WebStateDestroyed(web::WebState* web_state) override;
+
  private:
   // Calls the JavaScript function |name| in the frame context in the same
   // manner as the inherited CallJavaScriptFunction functions. If
@@ -65,6 +63,11 @@ class WebFrameImpl : public WebFrame, public web::WebStateObserver {
                               const std::vector<base::Value>& parameters,
                               bool reply_with_result);
 
+  // Detaches the receiver from the associated  WebState.
+  void DetachFromWebState();
+  // Returns the script command name to use for this WebFrame.
+  const std::string GetScriptCommandPrefix();
+
   // A structure to store the callbacks associated with the
   // |CallJavaScriptFunction| requests.
   typedef base::CancelableOnceCallback<void(void)> TimeoutCallback;

ios/web/web_state/web_frame_impl.mm

@@ -43,11 +43,10 @@ WebFrameImpl::WebFrameImpl(const std::string& frame_id,
   DCHECK(web_state);
   web_state->AddObserver(this);
 
-  const std::string command_prefix = kJavaScriptReplyCommandPrefix + frame_id;
   web_state->AddScriptCommandCallback(
       base::BindRepeating(&WebFrameImpl::OnJavaScriptReply,
                           base::Unretained(this), base::Unretained(web_state)),
-      command_prefix);
+      GetScriptCommandPrefix());
 }
 
 WebFrameImpl::~WebFrameImpl() {
@@ -55,16 +54,6 @@ WebFrameImpl::~WebFrameImpl() {
   DetachFromWebState();
 }
 
-void WebFrameImpl::DetachFromWebState() {
-  if (web_state_) {
-    const std::string command_prefix =
-        kJavaScriptReplyCommandPrefix + frame_id_;
-    web_state_->RemoveScriptCommandCallback(command_prefix);
-    web_state_->RemoveObserver(this);
-    web_state_ = nullptr;
-  }
-}
-
 WebState* WebFrameImpl::GetWebState() {
   return web_state_;
 }
@@ -126,7 +115,7 @@ bool WebFrameImpl::CallJavaScriptFunction(
 bool WebFrameImpl::CallJavaScriptFunction(
     const std::string& name,
     const std::vector<base::Value>& parameters) {
-  return CallJavaScriptFunction(name, parameters, /*replyWithResult=*/false);
+  return CallJavaScriptFunction(name, parameters, /*reply_with_result=*/false);
 }
 
 bool WebFrameImpl::CallJavaScriptFunction(
@@ -145,7 +134,7 @@ bool WebFrameImpl::CallJavaScriptFunction(
 
   base::PostDelayedTaskWithTraits(FROM_HERE, {web::WebThread::UI},
                                   timeout_callback_ptr->callback(), timeout);
-  return CallJavaScriptFunction(name, parameters, /*replyWithResult=*/true);
+  return CallJavaScriptFunction(name, parameters, /*reply_with_result=*/true);
 }
 
 void WebFrameImpl::CancelRequest(int message_id) {
@@ -182,8 +171,7 @@ bool WebFrameImpl::OnJavaScriptReply(web::WebState* web_state,
   }
 
   const std::string command_string = command->GetString();
-  if (command_string !=
-      (std::string(kJavaScriptReplyCommandPrefix) + frame_id_ + ".reply")) {
+  if (command_string != (GetScriptCommandPrefix() + ".reply")) {
     NOTREACHED();
     return false;
   }
@@ -211,6 +199,18 @@ bool WebFrameImpl::OnJavaScriptReply(web::WebState* web_state,
   return true;
 }
 
+void WebFrameImpl::DetachFromWebState() {
+  if (web_state_) {
+    web_state_->RemoveScriptCommandCallback(GetScriptCommandPrefix());
+    web_state_->RemoveObserver(this);
+    web_state_ = nullptr;
+  }
+}
+
+const std::string WebFrameImpl::GetScriptCommandPrefix() {
+  return kJavaScriptReplyCommandPrefix + frame_id_;
+}
+
 void WebFrameImpl::WebStateDestroyed(web::WebState* web_state) {
   CancelPendingRequests();
   DetachFromWebState();
@@ -221,6 +221,6 @@ WebFrameImpl::RequestCallbacks::RequestCallbacks(
     std::unique_ptr<TimeoutCallback> timeout)
     : completion(std::move(completion)), timeout_callback(std::move(timeout)) {}
 
-WebFrameImpl::RequestCallbacks::~RequestCallbacks(){};
+WebFrameImpl::RequestCallbacks::~RequestCallbacks() {}
 
 }  // namespace web

ios/web/web_state/web_frame_impl_inttest.mm

@@ -165,7 +165,7 @@ TEST_F(WebFrameImplIntTest, PreventMessageReplay) {
     return;
   }
 
-  ASSERT_TRUE(LoadHtml("<p>>"));
+  ASSERT_TRUE(LoadHtml("<p>"));
 
   WebFrame* main_frame = GetMainWebFrameForWebState(web_state());
   ASSERT_TRUE(main_frame);
@@ -220,13 +220,13 @@ TEST_F(WebFrameImplIntTest, PreventMessageReplay) {
     return called;
   }));
 
-  EXPECT_EQ(1, [ExecuteJavaScript(@"sensitiveValue") intValue]);
+  EXPECT_NSEQ(@1, ExecuteJavaScript(@"sensitiveValue"));
 
   ExecuteJavaScript(@"replayInterceptedMessage()");
 
   // Value should not increase because replaying message should not re-execute
   // the called function.
-  EXPECT_EQ(1, [ExecuteJavaScript(@"sensitiveValue") intValue]);
+  EXPECT_NSEQ(@1, ExecuteJavaScript(@"sensitiveValue"));
 }
 
 }  // namespace web