Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1891773002:
Implement the basic testing infrastructure for FindRequestManager. (Closed)
|
Created:
4 years, 8 months ago by paulmeyer Modified:
4 years, 8 months ago Reviewers:
nasko CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement the basic testing infrastructure for FindRequestManager.
This patch adds find_request_manager_browsertest.cc, along with a few find-in-page tests that can already be tested (though not yet with cross-process frames). There are also some new testing files and some small changes to FindRequestManager to facilitate testing.
Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zhodPxMOxa8Bg4Wg0/edit?usp=sharing
BUG=457440
Committed: https://crrev.com/be0de1325bc7493fa36e512a72eb1a9f710b3241
Cr-Commit-Position: refs/heads/master@{#388768}
Patch Set 1 #Patch Set 2 : Added comment to describe TestFindRequestManager. #
Total comments: 16
Patch Set 3 : Addressed comments by nasko@. #
Total comments: 19
Patch Set 4 : Addressed comments by nasko@. #
Total comments: 1
Patch Set 5 : Rebased and small fix. #
Messages
Total messages: 39 (17 generated)
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1891773002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1891773002/1
Description was changed from ========== Implement the basic testing infrastructure for FindRequestManager. This patch adds find_request_manager_browsertest.cc, along with a few find-in-page tests that can already be tested (though not yet with cross-process frames). There are also some new testing files and some small changes to FindRequestManager to facilitate testing. ========== to ========== Implement the basic testing infrastructure for FindRequestManager. This patch adds find_request_manager_browsertest.cc, along with a few find-in-page tests that can already be tested (though not yet with cross-process frames). There are also some new testing files and some small changes to FindRequestManager to facilitate testing. BUG=457440 ==========
Description was changed from ========== Implement the basic testing infrastructure for FindRequestManager. This patch adds find_request_manager_browsertest.cc, along with a few find-in-page tests that can already be tested (though not yet with cross-process frames). There are also some new testing files and some small changes to FindRequestManager to facilitate testing. BUG=457440 ========== to ========== Implement the basic testing infrastructure for FindRequestManager. This patch adds find_request_manager_browsertest.cc, along with a few find-in-page tests that can already be tested (though not yet with cross-process frames). There are also some new testing files and some small changes to FindRequestManager to facilitate testing. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
paulmeyer@chromium.org changed reviewers: + lfg@chromium.org
+lfg for initial review.
paulmeyer@chromium.org changed reviewers: + nasko@chromium.org - lfg@chromium.org
-lfg@ +nasko@
The overall comment we discussed is using WebContents::Find() as the entrypoint for testing the functionality and using WebContentsDelegate implementation to receive the events. Some more minor comments are also included. https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... File content/browser/find_request_manager_browsertest.cc (right): https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:27: protected: What is the goal of the protected section here? This class is not inherited as far as I can see. https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:29: WindowedNotificationObserver load_observer( Use TestNavigationObserver whenever possible. It does not rely on NotificationService, which is deprecated, and more closely monitors navigations. https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:41: void SetUpOnMainThread() override { No need for publicly inherited methods to be private. https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:52: command_line->AppendSwitch(switches::kSitePerProcess); IsolateAllSitesForTesting(command_line); which does the same thing, but it abstracts it out in a method we can easily affect with single change. https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:66: find_request_manager()->Find(1, base::UTF8ToUTF16("result"), options); I know this file is trying to target the FindRequestManager for testing, but I wonder if we will have any tests that would exercise find-in-page through the WebContents interface. Since all code will invoke the FRM through WebContents, should this test invoke searching the same way? https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:76: find_request_manager()->Find(i, base::UTF8ToUTF16("result"), options); Would using |L"result"| work instead of |base::UTF8ToUTF16("result")|? It might be a bit more readable. https://codereview.chromium.org/1891773002/diff/20001/content/test/test_find_... File content/test/test_find_request_manager.h (right): https://codereview.chromium.org/1891773002/diff/20001/content/test/test_find_... content/test/test_find_request_manager.h:19: class TestFindRequestManager : public FindRequestManager { Do we really need to subclass the FindRequestManager? It seems that it should be possible to use a class that implements WebContentsDelegate::FindReply. This is used in a couple of browser test files, so it might work.
PTAL https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... File content/browser/find_request_manager_browsertest.cc (right): https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:27: protected: On 2016/04/15 19:26:50, nasko (very slow Apr 8-18) wrote: > What is the goal of the protected section here? This class is not inherited as > far as I can see. All of the individual tests themselves inherit from this class, so methods in the "protected" section are accessible to the individual tests. Some other browser tests seem to do the same: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ne... https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:29: WindowedNotificationObserver load_observer( On 2016/04/15 19:26:50, nasko (very slow Apr 8-18) wrote: > Use TestNavigationObserver whenever possible. It does not rely on > NotificationService, which is deprecated, and more closely monitors navigations. Done. https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:41: void SetUpOnMainThread() override { On 2016/04/15 19:26:50, nasko (very slow Apr 8-18) wrote: > No need for publicly inherited methods to be private. I've always believed in giving the most limited access possible to class members. I get that in this case it doesn't really limit the access though, so I can move them to public. https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:52: command_line->AppendSwitch(switches::kSitePerProcess); On 2016/04/15 19:26:50, nasko (very slow Apr 8-18) wrote: > IsolateAllSitesForTesting(command_line); which does the same thing, but it > abstracts it out in a method we can easily affect with single change. Done. https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:66: find_request_manager()->Find(1, base::UTF8ToUTF16("result"), options); On 2016/04/15 19:26:50, nasko (very slow Apr 8-18) wrote: > I know this file is trying to target the FindRequestManager for testing, but I > wonder if we will have any tests that would exercise find-in-page through the > WebContents interface. Since all code will invoke the FRM through WebContents, > should this test invoke searching the same way? The tests now access WebContents::Find() and check results via WebContentsDelegate. https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:76: find_request_manager()->Find(i, base::UTF8ToUTF16("result"), options); On 2016/04/15 19:26:50, nasko (very slow Apr 8-18) wrote: > Would using |L"result"| work instead of |base::UTF8ToUTF16("result")|? It might > be a bit more readable. I tried it and it actually doesn't work. https://codereview.chromium.org/1891773002/diff/20001/content/test/test_find_... File content/test/test_find_request_manager.h (right): https://codereview.chromium.org/1891773002/diff/20001/content/test/test_find_... content/test/test_find_request_manager.h:19: class TestFindRequestManager : public FindRequestManager { On 2016/04/15 19:26:50, nasko (very slow Apr 8-18) wrote: > Do we really need to subclass the FindRequestManager? It seems that it should be > possible to use a class that implements WebContentsDelegate::FindReply. This is > used in a couple of browser test files, so it might work. Now using WebContentsDelegate instead.
Thanks for updating the test infrastructure. It looks much better this way! Few more comments, but it is getting there. https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... File content/browser/find_request_manager_browsertest.cc (right): https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:41: void SetUpOnMainThread() override { On 2016/04/18 19:11:22, Paul Meyer wrote: > On 2016/04/15 19:26:50, nasko (very slow Apr 8-18) wrote: > > No need for publicly inherited methods to be private. > > I've always believed in giving the most limited access possible to class > members. I get that in this case it doesn't really limit the access though, so I > can move them to public. However, you still can call those methods through a base class pointer, as the inheritance is public, so it doesn't really enforce access very well. https://codereview.chromium.org/1891773002/diff/20001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:66: find_request_manager()->Find(1, base::UTF8ToUTF16("result"), options); On 2016/04/18 19:11:22, Paul Meyer wrote: > On 2016/04/15 19:26:50, nasko (very slow Apr 8-18) wrote: > > I know this file is trying to target the FindRequestManager for testing, but I > > wonder if we will have any tests that would exercise find-in-page through the > > WebContents interface. Since all code will invoke the FRM through WebContents, > > should this test invoke searching the same way? > > The tests now access WebContents::Find() and check results via > WebContentsDelegate. Awesome! Thanks! https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager.cc:72: active_match_ordinal_ = active_match_ordinal; See my comment in the test file. Updating these conditionally for each variable can lead to storing values from different IPCs. https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... File content/browser/find_request_manager.h (right): https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager.h:33: virtual ~FindRequestManager(); We no longer need these extra virtuals, as there is no more inheritance for testing. FindRequestManager should remain unchanged in this CL, right? https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... File content/browser/find_request_manager_browsertest.cc (right): https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:65: current_results_.active_match_ordinal = active_match_ordinal; Shouldn't we update all the parts of the result together? Otherwise it is possible that these variables will store results from separate IPCs. Is this intentional? https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:77: // The lastest known results from the current find request. nit: latest? or last? https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:88: }; DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:90: using FindResults = TestWebContentsDelegate::FindResults; Instead of this, why not define the FindResults struct outside of the class? You can put it in an anonymous namespace if you are worried about name collisions. https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:152: EXPECT_EQ(19, results.number_of_matches); Why is the total number of matches only 19? There are strings "result XX" across both frames, where XX is 0-19, which is total of 20. https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:184: contents()->Find(1, base::UTF8ToUTF16("r"), default_options); minor nit: Instead of using hardcoded numbers, why not declare a local variable and increment it, then use the same to wait for reply?
Patchset #4 (id:60001) has been deleted
PTAL https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager.cc:72: active_match_ordinal_ = active_match_ordinal; On 2016/04/18 21:02:39, nasko (slow) wrote: > See my comment in the test file. Updating these conditionally for each variable > can lead to storing values from different IPCs. This was intentional, and is something that FindRequestManager will do eventually, but I'll leave it out until it's needed. Something similar will still be needed in the TestWebContentsDelegate though (see comment there). https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... File content/browser/find_request_manager.h (right): https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager.h:33: virtual ~FindRequestManager(); On 2016/04/18 21:02:39, nasko (slow) wrote: > We no longer need these extra virtuals, as there is no more inheritance for > testing. FindRequestManager should remain unchanged in this CL, right? That's right. Done. https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... File content/browser/find_request_manager_browsertest.cc (right): https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:65: current_results_.active_match_ordinal = active_match_ordinal; On 2016/04/18 21:02:39, nasko (slow) wrote: > Shouldn't we update all the parts of the result together? Otherwise it is > possible that these variables will store results from separate IPCs. Is this > intentional? This is intentional. Multiple find IPCs are used to update results incrementally. It is documented that -1 means "no update, same as before". https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:77: // The lastest known results from the current find request. On 2016/04/18 21:02:40, nasko (slow) wrote: > nit: latest? or last? Hahaha. "lastest", meaning "the most last". It was supposed to be "latest". https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:88: }; On 2016/04/18 21:02:40, nasko (slow) wrote: > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:90: using FindResults = TestWebContentsDelegate::FindResults; On 2016/04/18 21:02:40, nasko (slow) wrote: > Instead of this, why not define the FindResults struct outside of the class? You > can put it in an anonymous namespace if you are worried about name collisions. Done. https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:152: EXPECT_EQ(19, results.number_of_matches); On 2016/04/18 21:02:39, nasko (slow) wrote: > Why is the total number of matches only 19? There are strings "result XX" across > both frames, where XX is 0-19, which is total of 20. This is because "Result 19" does not actually match (and is not supposed to). There is a comment in find_in_page_frame.html about it, but I think it's a rather perplexing comment (it was already there in the file I took from the existing Blink find tests, so I just left it). I'll change it to hopefully be more clear. https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:184: contents()->Find(1, base::UTF8ToUTF16("r"), default_options); On 2016/04/18 21:02:40, nasko (slow) wrote: > minor nit: Instead of using hardcoded numbers, why not declare a local variable > and increment it, then use the same to wait for reply? Okay, I've done something like that. I'd prefer to use a member on the parent class to automate the incrementation.
LGTM https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... File content/browser/find_request_manager_browsertest.cc (right): https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:65: current_results_.active_match_ordinal = active_match_ordinal; On 2016/04/19 15:15:00, paulmeyer wrote: > On 2016/04/18 21:02:39, nasko (slow) wrote: > > Shouldn't we update all the parts of the result together? Otherwise it is > > possible that these variables will store results from separate IPCs. Is this > > intentional? > > This is intentional. Multiple find IPCs are used to update results > incrementally. It is documented that -1 means "no update, same as before". Acknowledged. https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:152: EXPECT_EQ(19, results.number_of_matches); On 2016/04/19 15:15:01, paulmeyer wrote: > On 2016/04/18 21:02:39, nasko (slow) wrote: > > Why is the total number of matches only 19? There are strings "result XX" > across > > both frames, where XX is 0-19, which is total of 20. > > This is because "Result 19" does not actually match (and is not supposed to). > There is a comment in find_in_page_frame.html about it, but I think it's a > rather perplexing comment (it was already there in the file I took from the > existing Blink find tests, so I just left it). I'll change it to hopefully be > more clear. Acknowledged. https://codereview.chromium.org/1891773002/diff/40001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:184: contents()->Find(1, base::UTF8ToUTF16("r"), default_options); On 2016/04/19 15:15:01, paulmeyer wrote: > On 2016/04/18 21:02:40, nasko (slow) wrote: > > minor nit: Instead of using hardcoded numbers, why not declare a local > variable > > and increment it, then use the same to wait for reply? > > Okay, I've done something like that. I'd prefer to use a member on the parent > class to automate the incrementation. Thanks! This looks much cleaner. https://codereview.chromium.org/1891773002/diff/80001/content/browser/find_re... File content/browser/find_request_manager_browsertest.cc (right): https://codereview.chromium.org/1891773002/diff/80001/content/browser/find_re... content/browser/find_request_manager_browsertest.cc:235: } It will be nice to add a test for wrap-around, but that can be added in later CL if you want.
The CQ bit was checked by paulmeyer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1891773002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1891773002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1891773002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1891773002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by paulmeyer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1891773002/#ps100001 (title: "Rebased and small fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1891773002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1891773002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by paulmeyer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1891773002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1891773002/100001
Message was sent while issue was closed.
Description was changed from ========== Implement the basic testing infrastructure for FindRequestManager. This patch adds find_request_manager_browsertest.cc, along with a few find-in-page tests that can already be tested (though not yet with cross-process frames). There are also some new testing files and some small changes to FindRequestManager to facilitate testing. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 ========== to ========== Implement the basic testing infrastructure for FindRequestManager. This patch adds find_request_manager_browsertest.cc, along with a few find-in-page tests that can already be tested (though not yet with cross-process frames). There are also some new testing files and some small changes to FindRequestManager to facilitate testing. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:100001) has been created in https://codereview.chromium.org/1910073002/ by jbudorick@chromium.org. The reason for reverting is: All three of the FindRequestManagerTests failed on Android: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/2....
Message was sent while issue was closed.
On 2016/04/21 18:17:57, jbudorick wrote: > A revert of this CL (patchset #5 id:100001) has been created in > https://codereview.chromium.org/1910073002/ by mailto:jbudorick@chromium.org. > > The reason for reverting is: All three of the FindRequestManagerTests failed on > Android: > https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/2....
Message was sent while issue was closed.
On 2016/04/21 18:18:17, jbudorick wrote: > On 2016/04/21 18:17:57, jbudorick wrote: > > A revert of this CL (patchset #5 id:100001) has been created in > > https://codereview.chromium.org/1910073002/ by mailto:jbudorick@chromium.org. > > > > The reason for reverting is: All three of the FindRequestManagerTests failed > on > > Android: > > > https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/2.... (It's not immediately clear to me why the tests passed on the CQ but failed on the main waterfall...)
Message was sent while issue was closed.
On 2016/04/21 18:19:04, jbudorick wrote: > On 2016/04/21 18:18:17, jbudorick wrote: > > On 2016/04/21 18:17:57, jbudorick wrote: > > > A revert of this CL (patchset #5 id:100001) has been created in > > > https://codereview.chromium.org/1910073002/ by > mailto:jbudorick@chromium.org. > > > > > > The reason for reverting is: All three of the FindRequestManagerTests failed > > on > > > Android: > > > > > > https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/2.... > > (It's not immediately clear to me why the tests passed on the CQ but failed on > the main waterfall...) Yeah, it's very weird. I also cannot figure out why they would fail only on Android. I don't see anything obvious that should be different for Android. I did see this failure before though when I did a dry run on Patch Set 1. I wasn't able to repro locally in over 100 tries though, and then it passed fine when I tried again on the trybots as well, so I thought it was a fluke.
Message was sent while issue was closed.
Description was changed from ========== Implement the basic testing infrastructure for FindRequestManager. This patch adds find_request_manager_browsertest.cc, along with a few find-in-page tests that can already be tested (though not yet with cross-process frames). There are also some new testing files and some small changes to FindRequestManager to facilitate testing. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 ========== to ========== Implement the basic testing infrastructure for FindRequestManager. This patch adds find_request_manager_browsertest.cc, along with a few find-in-page tests that can already be tested (though not yet with cross-process frames). There are also some new testing files and some small changes to FindRequestManager to facilitate testing. Design doc for multi-process find-in-page: https://docs.google.com/a/google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zh... BUG=457440 Committed: https://crrev.com/be0de1325bc7493fa36e512a72eb1a9f710b3241 Cr-Commit-Position: refs/heads/master@{#388768} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/be0de1325bc7493fa36e512a72eb1a9f710b3241 Cr-Commit-Position: refs/heads/master@{#388768} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
