Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2849603005:
Do not reinvent the wheel / use content APIs to find a frame by name. (Closed)
|
Created:
3 years, 7 months ago by Łukasz Anforowicz Modified:
3 years, 7 months ago CC:
chromium-reviews, jam, subresource-filter-reviews_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, dcheng, mathp+autofillwatch_chromium.org, darin-cc_chromium.org, tfarina, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, Avi (use Gerrit) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not reinvent the wheel / use content APIs to find a frame by name.
This CL rewrites code that manually iterates over frames of WebContents
to find a frame with a matching RenderFrameHost::GetFrameName.
Tests above //content are rewritten to use
content::FrameMatchingPredicate from
content/public/test/browser_test_utils.h (instead of manually iterating
over the frames and manually looking at RenderFrameHost::GetFrameName).
//content code is rewritten to use content::FrameTree::FindByName.
This CL has no intended behavior change (except changing messages in
gtest assertions that verify that exactly 1 frame matches).
BUG=
Review-Url: https://codereview.chromium.org/2849603005
Cr-Commit-Position: refs/heads/master@{#468994}
Committed: https://chromium.googlesource.com/chromium/src/+/424c0450bb5edc552aa13da3b8e7a9176cba70fc
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fix null dereference in WebUIImpl::TargetFrame #
Total comments: 1
Patch Set 3 : Reverting changes in content_subresource_filter_driver_factory_unittest.cc #
Total comments: 4
Patch Set 4 : s/DCHECK_EQ/EXPECT_EQ/g #
Total comments: 4
Patch Set 5 : Making the return statement in FrameMatchingPredicate safe against null dereferencing. #
Messages
Total messages: 53 (31 generated)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Do not reinvent the wheel / use content APIs to find a frame by name. This CL rewrites code that manually iterates over frames of WebContents to find a frame with a matching RenderFrameHost::GetFrameName. Tests above //content are rewritten to use content::FrameMatchingPredicate from content/public/test/browser_test_utils.h (instead of manually iterating over the frames and manually looking at RenderFrameHost::GetFrameName). //content code is rewritten to use content::FrameTree::FindByName. ========== to ========== Do not reinvent the wheel / use content APIs to find a frame by name. This CL rewrites code that manually iterates over frames of WebContents to find a frame with a matching RenderFrameHost::GetFrameName. Tests above //content are rewritten to use content::FrameMatchingPredicate from content/public/test/browser_test_utils.h (instead of manually iterating over the frames and manually looking at RenderFrameHost::GetFrameName). //content code is rewritten to use content::FrameTree::FindByName. This CL has no intended behavior change (except changing in some cases a gtest assert into a DCHECK). BUG= ==========
lukasza@chromium.org changed reviewers: + nick@chromium.org
nick@, can you PTAL? https://codereview.chromium.org/2849603005/diff/1/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_interactive_uitest.cc (right): https://codereview.chromium.org/2849603005/diff/1/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_interactive_uitest.cc:219: web_contents, base::Bind(&content::FrameMatchesName, name)); Given how often the pattern above is used, I wonder if I shouldn't go one step further and add to content/public/test/browser_test_utils.h a FrameMatchingName(WebContents, const std::string&) function. This would allow ripping out the method reimplemented in the 3 (or 4 depending how you count) tests I am touching here.
+avi@, since he added FrameMatchingPredicate, FrameMatchesName, FrameIsChildOfMainFrame, FrameHasSourceUrl in content/public/test/browser_test_utils.h
Oh, and a clarification - this came up, because I need to find a frame by name in yet another //chrome-layer browser test - in one I am trying to add in hosted_app_browsertest.cc
https://codereview.chromium.org/2849603005/diff/1/chrome/browser/ui/views/dra... File chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc (right): https://codereview.chromium.org/2849603005/diff/1/chrome/browser/ui/views/dra... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:706: web_contents(), base::Bind(&content::FrameMatchesName, name_to_find)); You've removed some ADD_FAILURES here, but I don't see that this test is especially sensitive to duplicate names, so I think it's okay. https://codereview.chromium.org/2849603005/diff/1/content/browser/webui/web_u... File content/browser/webui/web_ui_impl.cc (right): https://codereview.chromium.org/2849603005/diff/1/content/browser/webui/web_u... content/browser/webui/web_ui_impl.cc:289: ->current_frame_host(); We need to handle a null return value of FindByName, and return nullptr. Looks like some of the callers of TargetFrame handle a null return value.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2849603005/diff/1/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_interactive_uitest.cc (right): https://codereview.chromium.org/2849603005/diff/1/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_interactive_uitest.cc:219: web_contents, base::Bind(&content::FrameMatchesName, name)); On 2017/04/27 21:36:32, Łukasz A. wrote: > Given how often the pattern above is used, I wonder if I shouldn't go one step > further and add to content/public/test/browser_test_utils.h a > FrameMatchingName(WebContents, const std::string&) function. This would allow > ripping out the method reimplemented in the 3 (or 4 depending how you count) > tests I am touching here. WDYT about the above? This would simplify the test code a bit further (at the cost of exposing everything and a kitchen sync from browser_test_utils.h - this doesn't seem that bad to me, as long as the new function has a reasonably obvious name) https://codereview.chromium.org/2849603005/diff/1/chrome/browser/ui/views/dra... File chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc (right): https://codereview.chromium.org/2849603005/diff/1/chrome/browser/ui/views/dra... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:706: web_contents(), base::Bind(&content::FrameMatchesName, name_to_find)); On 2017/04/27 22:03:42, ncarter wrote: > You've removed some ADD_FAILURES here, but I don't see that this test is > especially sensitive to duplicate names, so I think it's okay. Yes - I think replacing gtest asserts with a DCHECK (inside FrameMatchingPredicate) is fine. https://codereview.chromium.org/2849603005/diff/1/content/browser/webui/web_u... File content/browser/webui/web_ui_impl.cc (right): https://codereview.chromium.org/2849603005/diff/1/content/browser/webui/web_u... content/browser/webui/web_ui_impl.cc:289: ->current_frame_host(); On 2017/04/27 22:03:42, ncarter wrote: > We need to handle a null return value of FindByName, and return nullptr. Looks > like some of the callers of TargetFrame handle a null return value. Ooops. Done. (I first wrote returning of FindByName and mentally noted that it returning null matches what the old code does; I didn't recheck this when adding ->current_frame_host() later on... :-/ ; at least the browser tests caught this :-).
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
https://codereview.chromium.org/2849603005/diff/1/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_interactive_uitest.cc (right): https://codereview.chromium.org/2849603005/diff/1/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_interactive_uitest.cc:219: web_contents, base::Bind(&content::FrameMatchesName, name)); On 2017/04/27 21:36:32, Łukasz A. wrote: > Given how often the pattern above is used, I wonder if I shouldn't go one step > further and add to content/public/test/browser_test_utils.h a > FrameMatchingName(WebContents, const std::string&) function. This would allow > ripping out the method reimplemented in the 3 (or 4 depending how you count) > tests I am touching here. Should we instead expose a FindFrameByName to web_contents.h? There seems to be demand for it in real code.
On 2017/04/27 23:13:35, ncarter wrote: > https://codereview.chromium.org/2849603005/diff/1/chrome/browser/autofill/aut... > File chrome/browser/autofill/autofill_interactive_uitest.cc (right): > > https://codereview.chromium.org/2849603005/diff/1/chrome/browser/autofill/aut... > chrome/browser/autofill/autofill_interactive_uitest.cc:219: web_contents, > base::Bind(&content::FrameMatchesName, name)); > On 2017/04/27 21:36:32, Łukasz A. wrote: > > Given how often the pattern above is used, I wonder if I shouldn't go one step > > further and add to content/public/test/browser_test_utils.h a > > FrameMatchingName(WebContents, const std::string&) function. This would allow > > ripping out the method reimplemented in the 3 (or 4 depending how you count) > > tests I am touching here. > > Should we instead expose a FindFrameByName to web_contents.h? There seems to be > demand for it in real code. Sure - that might be even better. I've started by exposing it from browser_test_utils.h, because for some reason I thought that exposing this from //content's public API is undesirable, because most (all?) consumers outside of //content layers would be browser tests (as opposed to product code). If exposing WebContents::FindFrameByName sounds good to you as a //content owner, then I can go ahead and do it this way.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This lgtm. I'd approve adding this to web_contents.h too, but I agree that it's right on the border of meeting that cutoff, with only one use in production.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/04/28 20:26:17, ncarter wrote: > This lgtm. Ok, thanks. I still see a red linux_android_rel_ng, but since my CL isn't obviously android-related, I'll just try pushing the CL to CQ. > > I'd approve adding this to web_contents.h too, but I agree that it's right on > the border of meeting that cutoff, with only one use in production. I think there are zero uses in production (if you only look outside of //content). https://codereview.chromium.org/2849603005/diff/20001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2849603005/diff/20001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:273: base::Bind(&content::FrameMatchesName, kSubframeName)); I need to revert the change here - it turns out that the test needs to work (and pick the first frame?) when there is more than 1 frame named kSubframeName. Example: ContentSubresourceFilterDriverFactoryTest.InactiveMainFrame_SubframeNotFiltered: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux... Sigh... I should have run tryjobs before sending this CL out for review... I've assumed that code trying to search for frames by name would have made sure that the name is unique...
The CQ bit was checked by lukasza@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lukasza@chromium.org changed reviewers: + sky@chromium.org
sky@ can you PTAL? https://codereview.chromium.org/2849603005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc (left): https://codereview.chromium.org/2849603005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:710: << "'" << name_to_find << "'"; This ADD_FAILURE will be replaced by a DCHECK from content::FrameMatchingPredicate. Such change seems fine as discussed with nick@. https://codereview.chromium.org/2849603005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:718: << "'" << name_to_find << "'"; This EXPECT_TRUE will be replaced by a DCHECK from content::FrameMatchingPredicate. Such change seems fine as discussed with nick@.
https://codereview.chromium.org/2849603005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc (left): https://codereview.chromium.org/2849603005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:710: << "'" << name_to_find << "'"; On 2017/04/28 22:52:09, Łukasz A. wrote: > This ADD_FAILURE will be replaced by a DCHECK from > content::FrameMatchingPredicate. Such change seems fine as discussed with > nick@. Remember that a DCHECK in tests causes a crash. Wouldn't it be better for this code to ADD_FAILURE or EXPECT_TRUE?
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the feedback sky@ - can you take another look please? https://codereview.chromium.org/2849603005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc (left): https://codereview.chromium.org/2849603005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/drag_and_drop_interactive_uitest.cc:710: << "'" << name_to_find << "'"; On 2017/05/01 16:50:30, sky wrote: > On 2017/04/28 22:52:09, Łukasz A. wrote: > > This ADD_FAILURE will be replaced by a DCHECK from > > content::FrameMatchingPredicate. Such change seems fine as discussed with > > nick@. > > Remember that a DCHECK in tests causes a crash. Wouldn't it be better for this > code to ADD_FAILURE or EXPECT_TRUE? 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).
Description was changed from ========== Do not reinvent the wheel / use content APIs to find a frame by name. This CL rewrites code that manually iterates over frames of WebContents to find a frame with a matching RenderFrameHost::GetFrameName. Tests above //content are rewritten to use content::FrameMatchingPredicate from content/public/test/browser_test_utils.h (instead of manually iterating over the frames and manually looking at RenderFrameHost::GetFrameName). //content code is rewritten to use content::FrameTree::FindByName. This CL has no intended behavior change (except changing in some cases a gtest assert into a DCHECK). BUG= ========== to ========== Do not reinvent the wheel / use content APIs to find a frame by name. This CL rewrites code that manually iterates over frames of WebContents to find a frame with a matching RenderFrameHost::GetFrameName. Tests above //content are rewritten to use content::FrameMatchingPredicate from content/public/test/browser_test_utils.h (instead of manually iterating over the frames and manually looking at RenderFrameHost::GetFrameName). //content code is rewritten to use content::FrameTree::FindByName. This CL has no intended behavior change (except changing messages in gtest assertions that verify that exactly 1 frame matches). BUG= ==========
Description was changed from ========== Do not reinvent the wheel / use content APIs to find a frame by name. This CL rewrites code that manually iterates over frames of WebContents to find a frame with a matching RenderFrameHost::GetFrameName. Tests above //content are rewritten to use content::FrameMatchingPredicate from content/public/test/browser_test_utils.h (instead of manually iterating over the frames and manually looking at RenderFrameHost::GetFrameName). //content code is rewritten to use content::FrameTree::FindByName. This CL has no intended behavior change (except changing messages in gtest assertions that verify that exactly 1 frame matches). BUG= ========== to ========== Do not reinvent the wheel / use content APIs to find a frame by name. This CL rewrites code that manually iterates over frames of WebContents to find a frame with a matching RenderFrameHost::GetFrameName. Tests above //content are rewritten to use content::FrameMatchingPredicate from content/public/test/browser_test_utils.h (instead of manually iterating over the frames and manually looking at RenderFrameHost::GetFrameName). //content code is rewritten to use content::FrameTree::FindByName. This CL has no intended behavior change (except changing messages in gtest assertions that verify that exactly 1 frame matches). BUG= ==========
https://codereview.chromium.org/2849603005/diff/60001/content/public/test/bro... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/2849603005/diff/60001/content/public/test/bro... content/public/test/browser_test_utils.cc:938: EXPECT_EQ(1U, frame_set.size()); 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);
https://codereview.chromium.org/2849603005/diff/60001/content/public/test/bro... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/2849603005/diff/60001/content/public/test/bro... content/public/test/browser_test_utils.cc:938: EXPECT_EQ(1U, frame_set.size()); On 2017/05/01 21:14:09, sky wrote: > 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. Good catch - I should have been more careful about changing the DCHECK_EQ to EXPECT_EQ. > 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); 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]).
https://codereview.chromium.org/2849603005/diff/60001/content/public/test/bro... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/2849603005/diff/60001/content/public/test/bro... content/public/test/browser_test_utils.cc:938: EXPECT_EQ(1U, frame_set.size()); On 2017/05/01 21:42:31, Łukasz A. wrote: > On 2017/05/01 21:14:09, sky wrote: > > 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. > > Good catch - I should have been more careful about changing the DCHECK_EQ to > EXPECT_EQ. > > > 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); > > 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]). How about changing the return to: return frame_set.size() == 1 ? *frame_set.begin() : nullptr; ?
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2849603005/diff/60001/content/public/test/bro... File content/public/test/browser_test_utils.cc (right): https://codereview.chromium.org/2849603005/diff/60001/content/public/test/bro... content/public/test/browser_test_utils.cc:938: EXPECT_EQ(1U, frame_set.size()); On 2017/05/01 23:33:33, sky wrote: > On 2017/05/01 21:42:31, Łukasz A. wrote: > > On 2017/05/01 21:14:09, sky wrote: > > > 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. > > > > Good catch - I should have been more careful about changing the DCHECK_EQ to > > EXPECT_EQ. > > > > > 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); > > > > 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]). > > How about changing the return to: > return frame_set.size() == 1 ? *frame_set.begin() : nullptr; > ? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org Link to the patchset: https://codereview.chromium.org/2849603005/#ps80001 (title: "Making the return statement in FrameMatchingPredicate safe against null dereferencing.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/03 15:28:06, sky wrote: > LGTM Thanks for reviewing!
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493827328030490,
"parent_rev": "b966abcf92859d9600ceddf8d90b2caa832c7a6b", "commit_rev":
"424c0450bb5edc552aa13da3b8e7a9176cba70fc"}
Message was sent while issue was closed.
Description was changed from ========== Do not reinvent the wheel / use content APIs to find a frame by name. This CL rewrites code that manually iterates over frames of WebContents to find a frame with a matching RenderFrameHost::GetFrameName. Tests above //content are rewritten to use content::FrameMatchingPredicate from content/public/test/browser_test_utils.h (instead of manually iterating over the frames and manually looking at RenderFrameHost::GetFrameName). //content code is rewritten to use content::FrameTree::FindByName. This CL has no intended behavior change (except changing messages in gtest assertions that verify that exactly 1 frame matches). BUG= ========== to ========== Do not reinvent the wheel / use content APIs to find a frame by name. This CL rewrites code that manually iterates over frames of WebContents to find a frame with a matching RenderFrameHost::GetFrameName. Tests above //content are rewritten to use content::FrameMatchingPredicate from content/public/test/browser_test_utils.h (instead of manually iterating over the frames and manually looking at RenderFrameHost::GetFrameName). //content code is rewritten to use content::FrameTree::FindByName. This CL has no intended behavior change (except changing messages in gtest assertions that verify that exactly 1 frame matches). BUG= Review-Url: https://codereview.chromium.org/2849603005 Cr-Commit-Position: refs/heads/master@{#468994} Committed: https://chromium.googlesource.com/chromium/src/+/424c0450bb5edc552aa13da3b8e7... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/424c0450bb5edc552aa13da3b8e7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
