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

content/public/test/browser_test_utils.cc

 // Copyright (c) 2012 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 "content/public/test/browser_test_utils.h"
 
 #include <stddef.h>
 #include <tuple>
 #include <utility>
 
@@ skipping 917 matching lines @@
     frame_set->insert(host);
 }
 }
 
 RenderFrameHost* FrameMatchingPredicate(
     WebContents* web_contents,
     const base::Callback<bool(RenderFrameHost*)>& predicate) {
   std::set<RenderFrameHost*> frame_set;
   web_contents->ForEachFrame(
       base::Bind(&AddToSetIfFrameMatchesPredicate, &frame_set, predicate));
-  DCHECK_EQ(1U, frame_set.size());
+  EXPECT_EQ(1U, frame_set.size());

[sky, 2017/05/01 21:14:09]

One comment here. This should really be an ASSERT. The reason for that is with
an EXPECT, even if the predicate is false execution continues. So, when a
failure happens 939 is executed. If frame_set is empty, then 939 crashes. I
recommend the following, remove EXPECT and use a conditional, e.g.:

return !frame_size.size().empty() ? *frame_set.begin() : nullptr;

And then change callers to have the assert,
e.g
frame_host = FrameMatchingPredicate(...);
ASSERT_TRUE(frame_host);

[Łukasz Anforowicz, 2017/05/01 21:42:31]

Good catch - I should have been more careful about changing the DCHECK_EQ to
EXPECT_EQ.
This would change the test behavior - the tests would no longer catch the case
when a test case unexpectedly sees 2 frames with a matching predicate.

Having the callers ASSERT won't work for non-void functions.  Maybe we should
just go back to a DCHECK inside FrameMatchesPredicate?  WDYT?

PS. We could also modify FrameMatchingPredicate to take an extra parameter
|expected_number_of_matching_frames| that defaults to 1 (so preserving existing
behavior) and 1) keep EXPECT_EQ and 2) have null-dereference change you
proposed.  Doing things this way seems unnecessarily complicated and doesn't
really make FrameMatchingPredicate more versatile (for example the caller in
content_subresource_filter_driver_factory_unittest.cc just wants the first
maching frame [and doesn't care if there is 1, 2 or 6 matching frames]).

[sky, 2017/05/01 23:33:33]

How about changing the return to:
return frame_set.size() == 1 ? *frame_set.begin() : nullptr;
?

[Łukasz Anforowicz, 2017/05/02 19:55:48]

Done.

   return *frame_set.begin();
 }
 
 bool FrameMatchesName(const std::string& name, RenderFrameHost* frame) {
   return frame->GetFrameName() == name;
 }
 
 bool FrameIsChildOfMainFrame(RenderFrameHost* frame) {
   return frame->GetParent() && !frame->GetParent()->GetParent();
 }
@@ skipping 1082 matching lines @@
                                  bool user_gesture,
                                  bool last_unlocked_by_target,
                                  bool privileged) {
   IPC::IpcSecurityTestUtil::PwnMessageReceived(
       process->GetChannel(),
       ViewHostMsg_LockMouse(routing_id, user_gesture, last_unlocked_by_target,
                             privileged));
 }
 
 }  // namespace content