Do not reinvent the wheel / use content APIs to find a frame by name.

chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <algorithm>
 #include <initializer_list>
 #include <memory>
 #include <string>
 #include <vector>
 
@@ skipping 684 matching lines @@
     frame = GetFrameByName(frame_name);
     DCHECK(frame);
 
     // Wait until frame contents have painted and are ready for hit testing.
     WaitForChildFrameSurfaceReady(frame);
 
     return true;
   }
 
   content::RenderFrameHost* GetFrameByName(const std::string& name_to_find) {
-    content::RenderFrameHost* result = nullptr;
-    for (content::RenderFrameHost* rfh : web_contents()->GetAllFrames()) {
-      if (rfh->GetFrameName() == name_to_find) {
-        if (result) {
-          ADD_FAILURE() << "More than one frame named "
-                        << "'" << name_to_find << "'";

[Łukasz Anforowicz, 2017/04/28 22:52:09]

This ADD_FAILURE will be replaced by a DCHECK from
content::FrameMatchingPredicate.  Such change seems fine as discussed with
nick@.

[sky, 2017/05/01 16:50:30]

Remember that a DCHECK in tests causes a crash. Wouldn't it be better for this
code to ADD_FAILURE or EXPECT_TRUE?

[Łukasz Anforowicz, 2017/05/01 18:11:29]

Done.  Thanks for raising this point again - I previously thought that maybe the
dependency on gtest is undesirable (from content::FrameMatchingPredicate). but I
see that other code in content/public/test/browser_test_utils.cc already uses
gtest assertsions.  I agree that gtest assertion is nicer for tests than a
DCHECK.  I see that //content/test:test_support is |testonly|, so enforcing
FrameMatchingPredicate contract via test assertions seems reasonable (i.e. I
think it is okay to assume that FrameMatchingPredicate is only called from
within a gtest test).

-          return nullptr;
-        }
-        result = rfh;
-      }
-    }
-
-    EXPECT_TRUE(result) << "Couldn't find a frame named "
-                        << "'" << name_to_find << "'";

[Łukasz Anforowicz, 2017/04/28 22:52:09]

This EXPECT_TRUE will be replaced by a DCHECK from
content::FrameMatchingPredicate.  Such change seems fine as discussed with
nick@.

-    return result;
+    return content::FrameMatchingPredicate(
+        web_contents(), base::Bind(&content::FrameMatchesName, name_to_find));
   }
 
   void AssertTestPageIsLoaded() {
     ASSERT_EQ(kTestPagePath, web_contents()->GetLastCommittedURL().path());
   }
 
   std::unique_ptr<DragAndDropSimulator> drag_simulator_;
 
   DISALLOW_COPY_AND_ASSIGN(DragAndDropBrowserTest);
 };
@@ skipping 539 matching lines @@
 // of a drag operation, and cross-site drags should be allowed across a
 // navigation.
 
 INSTANTIATE_TEST_CASE_P(
     SameSiteSubframe, DragAndDropBrowserTest, ::testing::Values(false));
 
 INSTANTIATE_TEST_CASE_P(
     CrossSiteSubframe, DragAndDropBrowserTest, ::testing::Values(true));
 
 }  // namespace chrome