Hide unused public functions of WebFramesManagerImpl

Some public functions WebFramesManagerImpl are only used for unittests.
These functions should be converted to private to improve encapsulation
and unittests of WebFramesManagerImpl.

Bug: 956516,956511
Change-Id: I9d5b85f9014ffe7cf60a77331cb308a27fb6b1be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1718408
Commit-Queue: Yi Su <mrsuyi@chromium.org>
Reviewed-by: Mike Dougherty <michaeldo@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682633}

ios/web/js_messaging/web_frames_manager_impl.h

@@ -36,27 +36,13 @@ class WebFramesManagerImpl : public WebFramesManager {
   explicit WebFramesManagerImpl(WebFramesManagerDelegate& delegate);
   ~WebFramesManagerImpl() override;
 
-  // Adds |frame| to the list of web frames associated with WebState.
-  // The frame must not be already in the frame manager (the frame manager must
-  // not have a frame with the same frame ID). If |frame| is a main frame, the
-  // frame manager must not have a main frame already.
-  void AddFrame(std::unique_ptr<WebFrame> frame);
-  // Removes the web frame with |frame_id|, if one exists, from the list of
-  // associated web frames.
-  // If the frame manager does not contain a frame with this ID, operation is a
-  // no-op.
-  void RemoveFrameWithId(const std::string& frame_id);
   // Removes all web frames from the list of associated web frames.
   void RemoveAllWebFrames();
+
   // Broadcasts a (not encrypted) JavaScript message to get the identifiers
   // and keys of existing frames.
   void RegisterExistingFrames();
 
-  // WebFramesManager overrides
-  std::set<WebFrame*> GetAllWebFrames() override;
-  WebFrame* GetMainWebFrame() override;
-  WebFrame* GetFrameWithId(const std::string& frame_id) override;
-
   // Use |message_router| to unregister JS message handlers for |old_web_view|
   // and register handlers for |new_web_view|. Owner of this class should call
   // this method whenever associated WKWebView changes.
@@ -64,7 +50,24 @@ class WebFramesManagerImpl : public WebFramesManager {
                         WKWebView* new_web_view,
                         CRWWKScriptMessageRouter* message_router);
 
+  // WebFramesManager overrides.
+  std::set<WebFrame*> GetAllWebFrames() override;
+  WebFrame* GetMainWebFrame() override;
+  WebFrame* GetFrameWithId(const std::string& frame_id) override;
+
  private:
+  // Adds |frame| to the list of web frames associated with WebState and invoke
+  // |delegate_|.OnWebFrameAvailable with |frame|. The frame must not be already
+  // in the frame manager (the frame manager must not have a frame with the same
+  // frame ID). If |frame| is a main frame, the frame manager must not have a
+  // main frame already.
+  void AddFrame(std::unique_ptr<WebFrame> frame);
+  // Removes the web frame with |frame_id|, if one exists, from the list of
+  // associated web frames, and invoke |delegate_|.OnWebFrameUnavailable with
+  // the web frame. If the frame manager does not contain a frame with
+  // |frame_id|, operation is a no-op.
+  void RemoveFrameWithId(const std::string& frame_id);
+
   // Handles FrameBecameAvailable JS message and creates new WebFrame based on
   // frame info from the message (e.g. ID, encryption key, message counter,
   // etc.).

ios/web/js_messaging/web_frames_manager_impl.mm

@@ -33,64 +33,12 @@ WebFramesManagerImpl::~WebFramesManagerImpl() {
   RemoveAllWebFrames();
 }
 
-void WebFramesManagerImpl::AddFrame(std::unique_ptr<WebFrame> frame) {
-  DCHECK(frame);
-  DCHECK(!frame->GetFrameId().empty());
-  if (frame->IsMainFrame()) {
-    DCHECK(!main_web_frame_);
-    main_web_frame_ = frame.get();
-  }
-  DCHECK(web_frames_.count(frame->GetFrameId()) == 0);
-  std::string frame_id = frame->GetFrameId();
-  web_frames_[frame_id] = std::move(frame);
-}
-
-void WebFramesManagerImpl::RemoveFrameWithId(const std::string& frame_id) {
-  DCHECK(!frame_id.empty());
-  // If the removed frame is a main frame, it should be the current one.
-  DCHECK(web_frames_.count(frame_id) == 0 ||
-         !web_frames_[frame_id]->IsMainFrame() ||
-         main_web_frame_ == web_frames_[frame_id].get());
-  if (web_frames_.count(frame_id) == 0) {
-    return;
-  }
-  if (main_web_frame_ && main_web_frame_->GetFrameId() == frame_id) {
-    main_web_frame_ = nullptr;
-  }
-  // The web::WebFrame destructor can call some callbacks that will try to
-  // access the frame via GetFrameWithId. This can lead to a reentrancy issue
-  // on |web_frames_|.
-  // To avoid this issue, keep the frame alive during the map operation and
-  // destroy it after.
-  auto keep_frame_alive = std::move(web_frames_[frame_id]);
-  web_frames_.erase(frame_id);
-}
-
 void WebFramesManagerImpl::RemoveAllWebFrames() {
   while (web_frames_.size()) {
     RemoveFrameWithId(web_frames_.begin()->first);
   }
 }
 
-WebFrame* WebFramesManagerImpl::GetFrameWithId(const std::string& frame_id) {
-  DCHECK(!frame_id.empty());
-  auto web_frames_it = web_frames_.find(frame_id);
-  return web_frames_it == web_frames_.end() ? nullptr
-                                            : web_frames_it->second.get();
-}
-
-std::set<WebFrame*> WebFramesManagerImpl::GetAllWebFrames() {
-  std::set<WebFrame*> frames;
-  for (const auto& it : web_frames_) {
-    frames.insert(it.second.get());
-  }
-  return frames;
-}
-
-WebFrame* WebFramesManagerImpl::GetMainWebFrame() {
-  return main_web_frame_;
-}
-
 void WebFramesManagerImpl::RegisterExistingFrames() {
   delegate_.GetWebState()->ExecuteJavaScript(
       base::UTF8ToUTF16("__gCrWeb.message.getExistingFrames();"));
@@ -102,6 +50,7 @@ void WebFramesManagerImpl::OnWebViewUpdated(
     CRWWKScriptMessageRouter* message_router) {
   DCHECK(old_web_view != new_web_view);
   if (old_web_view) {
+    RemoveAllWebFrames();
     [message_router
         removeScriptMessageHandlerForName:kFrameBecameAvailableMessageName
                                   webView:old_web_view];
@@ -132,6 +81,65 @@ void WebFramesManagerImpl::OnWebViewUpdated(
   }
 }
 
+#pragma mark - WebFramesManager
+
+std::set<WebFrame*> WebFramesManagerImpl::GetAllWebFrames() {
+  std::set<WebFrame*> frames;
+  for (const auto& it : web_frames_) {
+    frames.insert(it.second.get());
+  }
+  return frames;
+}
+
+WebFrame* WebFramesManagerImpl::GetMainWebFrame() {
+  return main_web_frame_;
+}
+
+WebFrame* WebFramesManagerImpl::GetFrameWithId(const std::string& frame_id) {
+  DCHECK(!frame_id.empty());
+  auto web_frames_it = web_frames_.find(frame_id);
+  return web_frames_it == web_frames_.end() ? nullptr
+                                            : web_frames_it->second.get();
+}
+
+#pragma mark - Private
+
+void WebFramesManagerImpl::AddFrame(std::unique_ptr<WebFrame> frame) {
+  DCHECK(frame);
+  DCHECK(!frame->GetFrameId().empty());
+  if (frame->IsMainFrame()) {
+    DCHECK(!main_web_frame_);
+    main_web_frame_ = frame.get();
+  }
+  DCHECK(web_frames_.count(frame->GetFrameId()) == 0);
+  std::string frame_id = frame->GetFrameId();
+  web_frames_[frame_id] = std::move(frame);
+
+  delegate_.OnWebFrameAvailable(GetFrameWithId(frame_id));
+}
+
+void WebFramesManagerImpl::RemoveFrameWithId(const std::string& frame_id) {
+  DCHECK(!frame_id.empty());
+  // If the removed frame is a main frame, it should be the current one.
+  DCHECK(web_frames_.count(frame_id) == 0 ||
+         !web_frames_[frame_id]->IsMainFrame() ||
+         main_web_frame_ == web_frames_[frame_id].get());
+  if (web_frames_.count(frame_id) == 0) {
+    return;
+  }
+  delegate_.OnWebFrameUnavailable(web_frames_[frame_id].get());
+  if (main_web_frame_ && main_web_frame_->GetFrameId() == frame_id) {
+    main_web_frame_ = nullptr;
+  }
+  // The web::WebFrame destructor can call some callbacks that will try to
+  // access the frame via GetFrameWithId. This can lead to a reentrancy issue
+  // on |web_frames_|.
+  // To avoid this issue, keep the frame alive during the map operation and
+  // destroy it after.
+  auto keep_frame_alive = std::move(web_frames_[frame_id]);
+  web_frames_.erase(frame_id);
+}
+
 void WebFramesManagerImpl::OnFrameBecameAvailable(WKScriptMessage* message) {
   DCHECK(!delegate_.GetWebState()->IsBeingDestroyed());
   // Validate all expected message components because any frame could falsify
@@ -172,7 +180,6 @@ void WebFramesManagerImpl::OnFrameBecameAvailable(WKScriptMessage* message) {
     }
 
     AddFrame(std::move(new_frame));
-    delegate_.OnWebFrameAvailable(GetFrameWithId(frame_id));
   }
 }
 
@@ -183,11 +190,7 @@ void WebFramesManagerImpl::OnFrameBecameUnavailable(WKScriptMessage* message) {
     return;
   }
   std::string frame_id = base::SysNSStringToUTF8(message.body);
-  WebFrame* frame = GetFrameWithId(frame_id);
-  if (frame) {
-    delegate_.OnWebFrameUnavailable(frame);
-    RemoveFrameWithId(frame_id);
-  }
+  RemoveFrameWithId(frame_id);
 }
 
 }  // namespace

ios/web/navigation/crw_wk_navigation_handler.mm

@@ -694,7 +694,7 @@ void ReportOutOfSyncURLInDidStartProvisionalNavigation(
     }
   }
 
-  [self removeAllWebFrames];
+  self.webStateImpl->GetWebFramesManagerImpl().RemoveAllWebFrames();
   // This must be reset at the end, since code above may need information about
   // the pending load.
   self.pendingNavigationInfo = nil;
@@ -817,7 +817,7 @@ void ReportOutOfSyncURLInDidStartProvisionalNavigation(
 
   [self commitPendingNavigationInfoInWebView:webView];
 
-  [self removeAllWebFrames];
+  self.webStateImpl->GetWebFramesManagerImpl().RemoveAllWebFrames();
 
   // This point should closely approximate the document object change, so reset
   // the list of injected scripts to those that are automatically injected.
@@ -1064,7 +1064,7 @@ void ReportOutOfSyncURLInDidStartProvisionalNavigation(
           forNavigation:navigation
                 webView:webView
         provisionalLoad:NO];
-  [self removeAllWebFrames];
+  self.webStateImpl->GetWebFramesManagerImpl().RemoveAllWebFrames();
   _certVerificationErrors->Clear();
   [self forgetNullWKNavigation:navigation];
 }
@@ -1117,7 +1117,7 @@ void ReportOutOfSyncURLInDidStartProvisionalNavigation(
 
   _certVerificationErrors->Clear();
   self.webProcessCrashed = YES;
-  [self removeAllWebFrames];
+  self.webStateImpl->GetWebFramesManagerImpl().RemoveAllWebFrames();
 
   [self.delegate navigationHandlerWebProcessDidCrash:self];
 }
@@ -1970,16 +1970,6 @@ void ReportOutOfSyncURLInDidStartProvisionalNavigation(
   self.webStateImpl->OnPageLoaded(failingURL, NO);
 }
 
-// Clears the frames list.
-- (void)removeAllWebFrames {
-  web::WebFramesManagerImpl& framesManager =
-      self.webStateImpl->GetWebFramesManagerImpl();
-  for (auto* frame : framesManager.GetAllWebFrames()) {
-    self.webStateImpl->OnWebFrameUnavailable(frame);
-  }
-  framesManager.RemoveAllWebFrames();
-}
-
 // Resets any state that is associated with a specific document object (e.g.,
 // page interaction tracking).
 - (void)resetDocumentSpecificState {