Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2007473004:
[Autofill] Migrate ContentAutofillDriver<-->AutofillAgent IPCs to mojo. (Closed)
|
Created:
4 years, 7 months ago by leonhsl(Using Gerrit) Modified:
4 years, 4 months ago Reviewers:
Ken Rockot(use gerrit already), jochen (gone - plz use gerrit), sky, Mathieu, dcheng, yzshen1, vabr (Chromium) CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, bondd+autofillwatch_chromium.org, browser-components-watch_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, estade+watch_chromium.org, jam, jdonnelly+autofillwatch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, qsr+mojo_chromium.org, rouslan+autofill_chromium.org, vabr+watchlistautofill_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Autofill] Migrate ContentAutofillDriver<-->AutofillAgent IPCs to mojo.
This CL
-- Migrates all ContentAutofillDriver<-->AutofillAgent
IPC messages into mojo interfaces.
-- Revises related unit tests and browser tests.
TEST=components_unittests, browser_tests
BUG=582391
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/90fd63b511fa3c6612d3ad032911025226293df8
Cr-Commit-Position: refs/heads/master@{#407673}
Patch Set 1 #
Total comments: 14
Patch Set 2 : migration done, but no gyp and no test codes #Patch Set 3 : gn, gyp, test codes all done #
Total comments: 12
Patch Set 4 : Address comments from Vaclav #Patch Set 5 : Fix unit tests -- separate to a new CL 2098743002 #Patch Set 6 : Rebase only #Patch Set 7 : Fix unit tests -- separate to a new CL 2098743002 #Patch Set 8 : Rebase only #
Total comments: 25
Patch Set 9 : Address comments from Yuzhu and Ken #Patch Set 10 : Rebase only #
Total comments: 2
Patch Set 11 : Name getter/setter functions properly #
Total comments: 4
Patch Set 12 : GetMojoAutofill{Driver,Agent} ==> GetAutofill{Driver,Agent} #Patch Set 13 : Rebase only #Patch Set 14 : Rebase only #
Total comments: 32
Patch Set 15 : Address Daniel comments #
Total comments: 4
Patch Set 16 : Address Daniel's comments #Patch Set 17 : Rebase only #Patch Set 18 : Fix trybots #Patch Set 19 : Initialize MockRenderProcessHost::remote_interfaces_ #Patch Set 20 : Initialize remote_interfaces_ in Init() #
Total comments: 3
Messages
Total messages: 140 (65 generated)
leon.han@intel.com changed reviewers: + mathp@chromium.org, vabr@chromium.org
Hi, Vaclav, Mathieu, This CL intends to migrate all ContentAutofillDriver<-->AutofillAgent IPCs into mojo interfaces, but ps#1 just migrates AutofillMsg_FillForm/AutofillHostMsg_DidFillAutofillFormData as a start ;-) Would you PTAL firstly to see whether there is any big miss so I can correct at early time. Then if OK I'll continue updating this CL for all other IPCs. #Note: This CL depends on https://codereview.chromium.org/1999623002/(waiting for landing now). But, locally combined this CL with the above dependency CL then confirmed OK for browser_tests and components_unittests.
leon.han@intel.com changed reviewers: + yzshen@chromium.org
Hi, Yuzhu, I think your feedback would also be very helpful for me, around the Mojo part, including the typemap usage etc. Would you PTAL at ps#1? Thanks~
Hello Han Leon. Thanks for your further work on this. The approach here looks good to me, given that before landing the CL you intend to convert all IPCs in AutofillAgent (otherwise I would be worried about the interleaving of the old and the Mojo messages). Cheers, Vaclav https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... File components/autofill/content/browser/content_autofill_driver_unittest.cc (right): https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... components/autofill/content/browser/content_autofill_driver_unittest.cc:84: public mojom::AutofillAgent { Please aggregate a test version of an mojom::AutofillAgent into ContentAutofillDriverTest as a data member instead of inheriting from it. You need the fixture to provide a mojom::AutofillAgent, not to be one. Splitting the AutofillAgent implementation into a separate class (in the anonymous namespace) will make the fixture smaller and easier to understand. It will also make it easier to locate the mojom::AutofillAgent-specific bits. https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... components/autofill/content/browser/content_autofill_driver_unittest.cc:93: id_sent_ = -1; Please initialise this in the constructor or via a non-static class member initializer in the class definition. This will ensure that the data member has a sane value through the whole existence of the text fixture. https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... components/autofill/content/browser/content_autofill_driver_unittest.cc:94: form_sent_.reset(); No need for this, it is null since construction, and you are already relying on SetUp being called before anything else touching |form_sent_|. https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... components/autofill/content/browser/content_autofill_driver_unittest.cc:236: int32_t id_sent_; nit: Please comment about what |id_sent_| and |form_sent_| are used for.
https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... File components/autofill/content/browser/content_autofill_driver_unittest.cc (right): https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... components/autofill/content/browser/content_autofill_driver_unittest.cc:50: FROM_HERE, base::MessageLoop::QuitWhenIdleClosure()); QuitWhenIdleClosure is deprecated according to the comments in message_loop.h. You might want to use corresponding RunLoop method. https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... components/autofill/content/browser/content_autofill_driver_unittest.cc:272: driver_->SendFormDataToRenderer( I am not familiar with auto fill code. Does this call somehow make a mojo call of mojom::AutofillAgent::FillForm() which will be handled by line 120? It is potentially flaky to use RunAllPendingTasks() below. There is no guarantee that the message loop stays busy between you make a mojo call of FillFrom() and receive this call, even if they are on the same thread. The safer way is to signal run loop to exit from mojom::AutofillAgent::FillForm() implementation. https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... File components/autofill/content/public/interfaces/autofill_agent.mojom (right): https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... components/autofill/content/public/interfaces/autofill_agent.mojom:16: FillForm(int32 id, FormData form) => (FormData filled, int64 timestamp); what is the exact meaning of the timestamp?
Thanks a lot for so helpful comments! Answered one inline question and I'll address all other comments later together with migration of other IPC messages. https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... File components/autofill/content/public/interfaces/autofill_agent.mojom (right): https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... components/autofill/content/public/interfaces/autofill_agent.mojom:16: FillForm(int32 id, FormData form) => (FormData filled, int64 timestamp); On 2016/05/24 16:12:10, yzshen1 wrote: > what is the exact meaning of the timestamp? This is the timestamp when AutofillAgent received FillForm call and filled the formdata. Originally it is carried in the response message as bellow: IPC_MESSAGE_ROUTED2(AutofillHostMsg_DidFillAutofillFormData, autofill::FormData /* the form */, base::TimeTicks /* timestamp */)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007473004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
There are still red trybots because of components_unittests failure, I'm investigating the root cause and I've left an inline question bellow, hoping Yuzhu would have some insights about this. But I think ps#3 is ready for the first round review, would you PTAL? Thanks~ Note: 1, All legacy IPCs handled by AutofillAgent have been turned into the mojo interface still implemented by AutofillAgent. And let AutofillAgent only bind on ONE message pipe, means if more than one client want to talk with it, they should share the same AutofillAgentPtr instance there, this way all incoming mojo calls will keep the same order with before(legacy IPCs). For example, instead of creating a new connection to AutofillAgent, ContentPasswordManagerDriver will use the connection established by ContentAutofillDriver. 2, All the same to ContentAutofillDriver as above. https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... File components/autofill/content/browser/content_autofill_driver_unittest.cc (right): https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... components/autofill/content/browser/content_autofill_driver_unittest.cc:50: FROM_HERE, base::MessageLoop::QuitWhenIdleClosure()); On 2016/05/24 16:12:10, yzshen1 wrote: > QuitWhenIdleClosure is deprecated according to the comments in message_loop.h. > > You might want to use corresponding RunLoop method. Done. https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... components/autofill/content/browser/content_autofill_driver_unittest.cc:84: public mojom::AutofillAgent { On 2016/05/24 08:08:20, vabr (Chromium) wrote: > Please aggregate a test version of an mojom::AutofillAgent into > ContentAutofillDriverTest as a data member instead of inheriting from it. You > need the fixture to provide a mojom::AutofillAgent, not to be one. Splitting the > AutofillAgent implementation into a separate class (in the anonymous namespace) > will make the fixture smaller and easier to understand. It will also make it > easier to locate the mojom::AutofillAgent-specific bits. Done. https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... components/autofill/content/browser/content_autofill_driver_unittest.cc:93: id_sent_ = -1; On 2016/05/24 08:08:20, vabr (Chromium) wrote: > Please initialise this in the constructor or via a non-static class member > initializer in the class definition. This will ensure that the data member has a > sane value through the whole existence of the text fixture. Done. https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... components/autofill/content/browser/content_autofill_driver_unittest.cc:94: form_sent_.reset(); On 2016/05/24 08:08:20, vabr (Chromium) wrote: > No need for this, it is null since construction, and you are already relying on > SetUp being called before anything else touching |form_sent_|. Done. https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... components/autofill/content/browser/content_autofill_driver_unittest.cc:236: int32_t id_sent_; On 2016/05/24 08:08:20, vabr (Chromium) wrote: > nit: Please comment about what |id_sent_| and |form_sent_| are used for. Done. https://codereview.chromium.org/2007473004/diff/1/components/autofill/content... components/autofill/content/browser/content_autofill_driver_unittest.cc:272: driver_->SendFormDataToRenderer( On 2016/05/24 16:12:10, yzshen1 wrote: > I am not familiar with auto fill code. Does this call somehow make a mojo call > of mojom::AutofillAgent::FillForm() which will be handled by line 120? > > It is potentially flaky to use RunAllPendingTasks() below. There is no guarantee > that the message loop stays busy between you make a mojo call of FillFrom() and > receive this call, even if they are on the same thread. > > The safer way is to signal run loop to exit from > mojom::AutofillAgent::FillForm() implementation. Done. https://codereview.chromium.org/2007473004/diff/40001/components/autofill/con... File components/autofill/content/browser/content_autofill_driver_unittest.cc (right): https://codereview.chromium.org/2007473004/diff/40001/components/autofill/con... components/autofill/content/browser/content_autofill_driver_unittest.cc:268: web_contents()->GetMainFrame()->GetRemoteInterfaces(); Hi, Yuzhu, I find that the unit tests always crash because |remote_interfaces| here seems always be nullptr, I investigated around but could not get any idea. Any hints about this? Thanks~
Thank you, Han Leon. This LGTM with a few nits. I was looking at the remaining IPCs, for the communication with PasswordAutofillAgent and PasswordGenerationAgent, to check if there are any issues from mixing the IPC and Mojo. If I understand correctly, the problem would be if one side sent message A via, say, IPC, and then message B via Mojo, and the other side expected to receive A before B just because of the sending order, right? In that case I think we are good to land this, I don't think there is a moment where the code sends two messages one way, relying on keeping the order. We mostly do a "ping-pong" of replies going in opposite directions. Thanks a lot for your progress on this. Converting the AutofillAgent-related code is a big step forward! Cheers, Vaclav https://codereview.chromium.org/2007473004/diff/40001/chrome/renderer/autofil... File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right): https://codereview.chromium.org/2007473004/diff/40001/chrome/renderer/autofil... chrome/renderer/autofill/autofill_renderer_browsertest.cc:65: void FirstUserGestureObserved() override {} nit: Please separate methods with a blank line. https://codereview.chromium.org/2007473004/diff/40001/chrome/renderer/autofil... chrome/renderer/autofill/autofill_renderer_browsertest.cc:113: // We only use the fake driver for main frame Just out of curiosity, could the added code go to the constructor instead? Putting stuff into the constructor is preferred, but of course, if it depends on something from ChromeRenderViewTest::SetUp, it is fine to leave it here. More info: https://github.com/google/googletest/blob/master/googletest/docs/V1_7_FAQ.md#... and https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/J2K-2Dmupb8... https://codereview.chromium.org/2007473004/diff/40001/chrome/renderer/autofil... chrome/renderer/autofill/autofill_renderer_browsertest.cc:151: ASSERT_NE(nullptr, fake_driver_.forms_); nit: This is the usual way to write the pointer test: ASSERT_TRUE(fake_driver_.forms_); Also on other places. This is not super-important, though, just a matter of consistency and a more succinct error message. https://codereview.chromium.org/2007473004/diff/40001/chrome/renderer/autofil... File chrome/renderer/autofill/form_autocomplete_browsertest.cc (right): https://codereview.chromium.org/2007473004/diff/40001/chrome/renderer/autofil... chrome/renderer/autofill/form_autocomplete_browsertest.cc:92: ASSERT_NE(nullptr, fake_driver.form_willsubmit_); ASSERT_TRUE(fake_driver.form_willsubmit_); (Also below.) https://codereview.chromium.org/2007473004/diff/40001/chrome/renderer/autofil... chrome/renderer/autofill/form_autocomplete_browsertest.cc:173: // We only use the fake driver for main frame If possible, please move the SetUp code to the constructor (see my earlier comment for details).
https://codereview.chromium.org/2007473004/diff/40001/components/autofill/con... File components/autofill/content/browser/content_autofill_driver_unittest.cc (right): https://codereview.chromium.org/2007473004/diff/40001/components/autofill/con... components/autofill/content/browser/content_autofill_driver_unittest.cc:268: web_contents()->GetMainFrame()->GetRemoteInterfaces(); On 2016/06/23 10:00:12, leonhsl wrote: > Hi, Yuzhu, I find that the unit tests always crash because |remote_interfaces| > here seems always be nullptr, I investigated around but could not get any idea. > Any hints about this? Thanks~ Sorry I am not familiar with this code. My guess is that content::RenderViewHostTestHarness doesn't properly setup this. But I have no idea whether content::RenderViewHostTestHarness is the right thing to use or how to fix it properly. You might be able to get help from relevant authors (such as the author of GetRemoteInterfaces).
Description was changed from ========== [Autofill] Migrate ContentAutofillDriver<-->AutofillAgent IPCs to mojo. This CL -- Migrates all ContentAutofillDriver<-->AutofillAgent IPC messages into mojo interfaces. -- Revises related unit tests and browser tests. TEST=components_unittests, browser_tests BUG=582391 ========== to ========== [Autofill] Migrate ContentAutofillDriver<-->AutofillAgent IPCs to mojo. This CL -- Migrates all ContentAutofillDriver<-->AutofillAgent IPC messages into mojo interfaces. -- Revises related unit tests and browser tests. TEST=components_unittests, browser_tests BUG=582391 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007473004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by leon.han@intel.com 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...
Hi, Vaclav, Thanks a lot for review. Uploaded ps#4 to address comments, ps#5 to solve previous unit tests crash problem, and ps#6 to rebase to make all trybots happy, PTAnL, Thanks~ And about bellow comments, I think you're exactly correct. Thanks for confirmation! " If I understand correctly, the problem would be if one side sent message A via, say, IPC, and then message B via Mojo, and the other side expected to receive A before B just because of the sending order, right? In that case I think we are good to land this, I don't think there is a moment where the code sends two messages one way, relying on keeping the order. We mostly do a "ping-pong" of replies going in opposite directions. " https://codereview.chromium.org/2007473004/diff/40001/chrome/renderer/autofil... File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right): https://codereview.chromium.org/2007473004/diff/40001/chrome/renderer/autofil... chrome/renderer/autofill/autofill_renderer_browsertest.cc:65: void FirstUserGestureObserved() override {} On 2016/06/23 14:21:21, vabr (OOO until 27 June) wrote: > nit: Please separate methods with a blank line. Done. https://codereview.chromium.org/2007473004/diff/40001/chrome/renderer/autofil... chrome/renderer/autofill/autofill_renderer_browsertest.cc:113: // We only use the fake driver for main frame On 2016/06/23 14:21:21, vabr (OOO until 27 June) wrote: > Just out of curiosity, could the added code go to the constructor instead? > Putting stuff into the constructor is preferred, but of course, if it depends on > something from ChromeRenderViewTest::SetUp, it is fine to leave it here. > > More info: > https://github.com/google/googletest/blob/master/googletest/docs/V1_7_FAQ.md#... > and > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/J2K-2Dmupb8... > OK now I understand the rule, thanks for the sharing! Because |view_| needed by our codes is created in ChromeRenderViewTest::SetUp, we need to add our codes here instead of constructor. https://codereview.chromium.org/2007473004/diff/40001/chrome/renderer/autofil... chrome/renderer/autofill/autofill_renderer_browsertest.cc:151: ASSERT_NE(nullptr, fake_driver_.forms_); On 2016/06/23 14:21:21, vabr (OOO until 27 June) wrote: > nit: This is the usual way to write the pointer test: > > ASSERT_TRUE(fake_driver_.forms_); > > Also on other places. This is not super-important, though, just a matter of > consistency and a more succinct error message. Done and Thanks! https://codereview.chromium.org/2007473004/diff/40001/chrome/renderer/autofil... File chrome/renderer/autofill/form_autocomplete_browsertest.cc (right): https://codereview.chromium.org/2007473004/diff/40001/chrome/renderer/autofil... chrome/renderer/autofill/form_autocomplete_browsertest.cc:92: ASSERT_NE(nullptr, fake_driver.form_willsubmit_); On 2016/06/23 14:21:21, vabr (OOO until 27 June) wrote: > ASSERT_TRUE(fake_driver.form_willsubmit_); > > (Also below.) Done. https://codereview.chromium.org/2007473004/diff/40001/chrome/renderer/autofil... chrome/renderer/autofill/form_autocomplete_browsertest.cc:173: // We only use the fake driver for main frame On 2016/06/23 14:21:22, vabr (OOO until 27 June) wrote: > If possible, please move the SetUp code to the constructor (see my earlier > comment for details). As our codes depend on ChromeRenderViewTest::SetUp, we'll leave code here instead of constructor.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
leon.han@intel.com changed reviewers: + dcheng@chromium.org
+dcheng@ for security review of IPC messages and mojom. Thanks~
The CQ bit was checked by leon.han@intel.com 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.
ps#5 and ps#7 have been separated to a new CL https://codereview.chromium.org/2098743002/, once it get landed, I'll remove those changes from this CL.
Thanks, Han Leon, Your changes in p.s. 7 (https://codereview.chromium.org/2007473004/#ps120001) LGTM! Cheers, Vaclav
On 2016/06/27 09:00:39, vabr (Chromium) wrote: > Thanks, Han Leon, > Your changes in p.s. 7 (https://codereview.chromium.org/2007473004/#ps120001) > LGTM! > > Cheers, > Vaclav Thanks Vaclav a lot for kindly review~ And, would you please also take a look? Thanks. Yuzhu: Review for mojo related overall as mojo expert. Daniel: Review for autofill IPC message and mojom files.
The CQ bit was checked by leon.han@intel.com 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.
yzshen@chromium.org changed reviewers: + rockot@chromium.org
I don't know much about this feature. I assume the owner of the feature has done extensive review. +CC Ken Would you please take a look at the changes in render_frame_host_impl.cc? (Just a few lines.) You probably have better idea whether this makes sense or not. Thanks! https://codereview.chromium.org/2007473004/diff/140001/chrome/renderer/autofi... File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right): https://codereview.chromium.org/2007473004/diff/140001/chrome/renderer/autofi... chrome/renderer/autofill/autofill_renderer_browsertest.cc:59: bool called_fieldchange_; nit: Please use setter/getter instead of making the fields public. We usually only do that for simple structs. (Here and other files in this CL) https://codereview.chromium.org/2007473004/diff/140001/chrome/renderer/autofi... chrome/renderer/autofill/autofill_renderer_browsertest.cc:162: run_loop.RunUntilIdle(); Please note that we don't guarantee that the message loop stays busy. The more reliable way is to let the fake_driver_ quit the run loop. (here and elsewhere) https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... File components/autofill/content/browser/content_autofill_driver_unittest.cc (right): https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... components/autofill/content/browser/content_autofill_driver_unittest.cc:327: run_loop.RunUntilIdle(); Shouldn't we use run_loop.Run() here (and below)? |fake_agent_| has got a quit closure which it can use to quit the run loop, right? https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... File components/autofill/content/public/interfaces/autofill_agent.mojom (right): https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... components/autofill/content/public/interfaces/autofill_agent.mojom:20: // Send the heuristic and server field type predictions to the renderer. nit: Send*s* https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... File components/autofill/content/public/interfaces/autofill_driver.mojom (right): https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... components/autofill/content/public/interfaces/autofill_driver.mojom:32: int32 id, FormData form, FormFieldData field, gfx.mojom.RectF bounding_box); 80 columns https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... components/autofill/content/public/interfaces/autofill_driver.mojom:49: // Send when a text field is done editing. Sent? https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... components/autofill/content/public/interfaces/autofill_driver.mojom:52: // Inform browser of data list values for the current field. Inform*s* https://codereview.chromium.org/2007473004/diff/140001/services/shell/public/... File services/shell/public/cpp/lib/interface_provider.cc (right): https://codereview.chromium.org/2007473004/diff/140001/services/shell/public/... services/shell/public/cpp/lib/interface_provider.cc:22: if (interface_provider_) Why this check is needed? It seems this is never null. https://codereview.chromium.org/2007473004/diff/140001/services/shell/public/... services/shell/public/cpp/lib/interface_provider.cc:39: if (interface_provider_) ditto.
render_frame_host_impl.cc lgtm % nits. I didn't review anything else, but please let me know if that's needed. https://codereview.chromium.org/2007473004/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2007473004/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2398: // Even RenderProcessHost has no remote interfaces, such as nit: "The RenderProcessHost may not have remote interfaces in some test environments. In that case we create an empty InterfaceProvider so tests can register mock implementations." https://codereview.chromium.org/2007473004/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2401: remote_interfaces_.reset(new shell::InterfaceProvider); nit: Move this into the if condition just below
Thanks Yuzhu for review. Added inline comments as bellow and will address other comments later. https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... File components/autofill/content/browser/content_autofill_driver_unittest.cc (right): https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... components/autofill/content/browser/content_autofill_driver_unittest.cc:327: run_loop.RunUntilIdle(); On 2016/06/29 22:25:44, yzshen1 wrote: > Shouldn't we use run_loop.Run() here (and below)? |fake_agent_| has got a quit > closure which it can use to quit the run loop, right? > Inside driver->SendFormDataToRenderer() and some other driver functions, under some conditions, the mojo call won't be triggered, so the quit closure will pend there forever.. Should we set a timer here to wait? Or some better solutions? https://codereview.chromium.org/2007473004/diff/140001/services/shell/public/... File services/shell/public/cpp/lib/interface_provider.cc (right): https://codereview.chromium.org/2007473004/diff/140001/services/shell/public/... services/shell/public/cpp/lib/interface_provider.cc:22: if (interface_provider_) On 2016/06/29 22:25:44, yzshen1 wrote: > Why this check is needed? It seems this is never null. With the render_frame_host_impl.cc changes, InterfaceProvider could be constructed but with no binding, so added the check here.
On 2016/06/29 22:33:21, Ken Rockot wrote: > render_frame_host_impl.cc lgtm % nits. I didn't review anything else, but please > let me know if that's needed. > > https://codereview.chromium.org/2007473004/diff/140001/content/browser/frame_... > File content/browser/frame_host/render_frame_host_impl.cc (right): > > https://codereview.chromium.org/2007473004/diff/140001/content/browser/frame_... > content/browser/frame_host/render_frame_host_impl.cc:2398: // Even > RenderProcessHost has no remote interfaces, such as > nit: "The RenderProcessHost may not have remote interfaces in some test > environments. In that case we create an empty InterfaceProvider so tests can > register mock implementations." > > https://codereview.chromium.org/2007473004/diff/140001/content/browser/frame_... > content/browser/frame_host/render_frame_host_impl.cc:2401: > remote_interfaces_.reset(new shell::InterfaceProvider); > nit: Move this into the if condition just below Hi, Ken, would you PTAL also at the overall when you have time? Thanks for help ;-)
https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... File components/autofill/content/browser/content_autofill_driver_unittest.cc (right): https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... components/autofill/content/browser/content_autofill_driver_unittest.cc:327: run_loop.RunUntilIdle(); On 2016/06/29 23:28:34, leonhsl wrote: > On 2016/06/29 22:25:44, yzshen1 wrote: > > Shouldn't we use run_loop.Run() here (and below)? |fake_agent_| has got a quit > > closure which it can use to quit the run loop, right? > > > > Inside driver->SendFormDataToRenderer() and some other driver functions, under > some conditions, the mojo call won't be triggered, so the quit closure will pend > there forever.. Should we set a timer here to wait? Or some better solutions? Hmm.. This is a good question. We don't have a way to expect that some mojo call doesn't happen (yet). I don't have a better solution at this moment. +rockot@: I usually suggest people to not rely on RunUntilIdle() because the fact that the message loop stays busy is not explicitly guaranteed by our APIs. Before we figure out a way to support "expect_not_called", I guess I need to allow RunUntilIdle()? WDYT? Thanks! https://codereview.chromium.org/2007473004/diff/140001/services/shell/public/... File services/shell/public/cpp/lib/interface_provider.cc (right): https://codereview.chromium.org/2007473004/diff/140001/services/shell/public/... services/shell/public/cpp/lib/interface_provider.cc:22: if (interface_provider_) On 2016/06/29 23:28:35, leonhsl wrote: > On 2016/06/29 22:25:44, yzshen1 wrote: > > Why this check is needed? It seems this is never null. > > With the render_frame_host_impl.cc changes, InterfaceProvider could be > constructed but with no binding, so added the check here. But the GetProxy() at line 10 guarantees that it is not null. (The other end is not bound, but that is okay, method calls are cached.) Have I missed anything?
https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... File components/autofill/content/browser/content_autofill_driver_unittest.cc (right): https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... components/autofill/content/browser/content_autofill_driver_unittest.cc:327: run_loop.RunUntilIdle(); On 2016/06/29 23:28:34, leonhsl wrote: > On 2016/06/29 22:25:44, yzshen1 wrote: > > Shouldn't we use run_loop.Run() here (and below)? |fake_agent_| has got a quit > > closure which it can use to quit the run loop, right? > > > > Inside driver->SendFormDataToRenderer() and some other driver functions, under > some conditions, the mojo call won't be triggered, so the quit closure will pend > there forever.. Should we set a timer here to wait? Or some better solutions? Hmm.. This is a good question. We don't have a way to expect that some mojo call doesn't happen (yet). I don't have a better solution at this moment. +rockot@: I usually suggest people to not rely on RunUntilIdle() because the fact that the message loop stays busy is not explicitly guaranteed by our APIs. Before we figure out a way to support "expect_not_called", I guess I need to allow RunUntilIdle()? WDYT? Thanks! https://codereview.chromium.org/2007473004/diff/140001/services/shell/public/... File services/shell/public/cpp/lib/interface_provider.cc (right): https://codereview.chromium.org/2007473004/diff/140001/services/shell/public/... services/shell/public/cpp/lib/interface_provider.cc:22: if (interface_provider_) On 2016/06/29 23:28:35, leonhsl wrote: > On 2016/06/29 22:25:44, yzshen1 wrote: > > Why this check is needed? It seems this is never null. > > With the render_frame_host_impl.cc changes, InterfaceProvider could be > constructed but with no binding, so added the check here. But the GetProxy() at line 10 guarantees that it is not null. (The other end is not bound, but that is okay, method calls are cached.) Have I missed anything?
https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... File components/autofill/content/browser/content_autofill_driver_unittest.cc (right): https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... components/autofill/content/browser/content_autofill_driver_unittest.cc:327: run_loop.RunUntilIdle(); On 2016/06/30 at 16:58:17, yzshen1 wrote: > On 2016/06/29 23:28:34, leonhsl wrote: > > On 2016/06/29 22:25:44, yzshen1 wrote: > > > Shouldn't we use run_loop.Run() here (and below)? |fake_agent_| has got a quit > > > closure which it can use to quit the run loop, right? > > > > > > > Inside driver->SendFormDataToRenderer() and some other driver functions, under > > some conditions, the mojo call won't be triggered, so the quit closure will pend > > there forever.. Should we set a timer here to wait? Or some better solutions? > > Hmm.. This is a good question. We don't have a way to expect that some mojo call doesn't happen (yet). > I don't have a better solution at this moment. > > +rockot@: I usually suggest people to not rely on RunUntilIdle() because the fact that the message loop stays busy is not explicitly guaranteed by our APIs. Before we figure out a way to support "expect_not_called", I guess I need to allow RunUntilIdle()? WDYT? Thanks! I agree that RunUntilIdle() is generally bad, but I think it would be OK to make an exception until we have a solution for expecting "not-called" scenarios. It is dependent upon hidden implementation details, but is safe to assume that if the fake impl is going to respond immediately (if at all), the response task must be queued by the time RunUntilIdle is invoked.
Patchset #9 (id:160001) has been deleted
The CQ bit was checked by leon.han@intel.com 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: Try jobs failed on following builders: mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gyp_...)
The CQ bit was checked by leon.han@intel.com 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.
Uploaded ps#9 to address comments and ps#10 to rebase, PTAnL, Thanks! https://codereview.chromium.org/2007473004/diff/140001/chrome/renderer/autofi... File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right): https://codereview.chromium.org/2007473004/diff/140001/chrome/renderer/autofi... chrome/renderer/autofill/autofill_renderer_browsertest.cc:59: bool called_fieldchange_; On 2016/06/29 22:25:44, yzshen1 wrote: > nit: Please use setter/getter instead of making the fields public. > We usually only do that for simple structs. > > (Here and other files in this CL) Done for all files in this CL. https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... File components/autofill/content/public/interfaces/autofill_agent.mojom (right): https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... components/autofill/content/public/interfaces/autofill_agent.mojom:20: // Send the heuristic and server field type predictions to the renderer. On 2016/06/29 22:25:44, yzshen1 wrote: > nit: Send*s* Done. https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... File components/autofill/content/public/interfaces/autofill_driver.mojom (right): https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... components/autofill/content/public/interfaces/autofill_driver.mojom:32: int32 id, FormData form, FormFieldData field, gfx.mojom.RectF bounding_box); On 2016/06/29 22:25:44, yzshen1 wrote: > 80 columns Done. https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... components/autofill/content/public/interfaces/autofill_driver.mojom:49: // Send when a text field is done editing. On 2016/06/29 22:25:44, yzshen1 wrote: > Sent? Done. https://codereview.chromium.org/2007473004/diff/140001/components/autofill/co... components/autofill/content/public/interfaces/autofill_driver.mojom:52: // Inform browser of data list values for the current field. On 2016/06/29 22:25:44, yzshen1 wrote: > Inform*s* Done. https://codereview.chromium.org/2007473004/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2007473004/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2398: // Even RenderProcessHost has no remote interfaces, such as On 2016/06/29 22:33:21, Ken Rockot wrote: > nit: "The RenderProcessHost may not have remote interfaces in some test > environments. In that case we create an empty InterfaceProvider so tests can > register mock implementations." Done. https://codereview.chromium.org/2007473004/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2401: remote_interfaces_.reset(new shell::InterfaceProvider); On 2016/06/29 22:33:21, Ken Rockot wrote: > nit: Move this into the if condition just below Done. https://codereview.chromium.org/2007473004/diff/140001/services/shell/public/... File services/shell/public/cpp/lib/interface_provider.cc (right): https://codereview.chromium.org/2007473004/diff/140001/services/shell/public/... services/shell/public/cpp/lib/interface_provider.cc:22: if (interface_provider_) On 2016/06/30 16:58:17, yzshen1 wrote: > On 2016/06/29 23:28:35, leonhsl wrote: > > On 2016/06/29 22:25:44, yzshen1 wrote: > > > Why this check is needed? It seems this is never null. > > > > With the render_frame_host_impl.cc changes, InterfaceProvider could be > > constructed but with no binding, so added the check here. > > But the GetProxy() at line 10 guarantees that it is not null. (The other end is > not bound, but that is okay, method calls are cached.) > > Have I missed anything? Yeah you are right.. As pending_request_ is used now, this check has become unnecessary. Thanks! Done. https://codereview.chromium.org/2007473004/diff/140001/services/shell/public/... services/shell/public/cpp/lib/interface_provider.cc:39: if (interface_provider_) On 2016/06/29 22:25:44, yzshen1 wrote: > ditto. Done.
Mostly looks good, with one style nit: https://codereview.chromium.org/2007473004/diff/200001/chrome/renderer/autofi... File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right): https://codereview.chromium.org/2007473004/diff/200001/chrome/renderer/autofi... chrome/renderer/autofill/autofill_renderer_browsertest.cc:58: bool GetCalledFieldChange() const { return called_fieldchange_; } C++ coding style says: Getter's name should be called_fieldchange(), i.e., the same as the field name but without the trailing '_'. Correspondingly, if you have a setter you should name it set_called_fieldchange(). Reference: https://google.github.io/styleguide/cppguide.html#Class_Format
Thanks Yuzhu a lot for the review. Uploaded ps#11 to address comments, PTAnL, Thanks. https://codereview.chromium.org/2007473004/diff/200001/chrome/renderer/autofi... File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right): https://codereview.chromium.org/2007473004/diff/200001/chrome/renderer/autofi... chrome/renderer/autofill/autofill_renderer_browsertest.cc:58: bool GetCalledFieldChange() const { return called_fieldchange_; } On 2016/07/01 15:53:52, yzshen1 wrote: > C++ coding style says: > > Getter's name should be called_fieldchange(), i.e., the same as the field name > but without the trailing '_'. > Correspondingly, if you have a setter you should name it > set_called_fieldchange(). > > Reference: https://google.github.io/styleguide/cppguide.html#Class_Format Done. Thanks for knowledge sharing.
lgtm
Ping Ken and Daniel, Thanks~
leon.han@intel.com changed reviewers: + jochen@chromium.org
Hi, Jochen, would you PTAL for OWNER review of: chrome/* except autofill components/* except autofill and password_manager content/browser/frame_host/render_frame_host_impl.cc Thanks~
lgtm https://codereview.chromium.org/2007473004/diff/220001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/2007473004/diff/220001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.h:231: const mojom::AutofillDriverPtr& GetMojoAutofillDriver(); nit: Please avoid adding "Mojo" to symbol names. GetAutofillDriver? https://codereview.chromium.org/2007473004/diff/220001/components/password_ma... File components/password_manager/content/browser/content_password_manager_driver.h (right): https://codereview.chromium.org/2007473004/diff/220001/components/password_ma... components/password_manager/content/browser/content_password_manager_driver.h:105: const autofill::mojom::AutofillAgentPtr& GetMojoAutofillAgent(); nit: same comment - GetAutofillAgent?
lgtm
The CQ bit was checked by leon.han@intel.com 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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
Uploaded ps#12 to address Ken's comments. Thanks. https://codereview.chromium.org/2007473004/diff/220001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/2007473004/diff/220001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.h:231: const mojom::AutofillDriverPtr& GetMojoAutofillDriver(); On 2016/07/07 04:52:32, Ken Rockot wrote: > nit: Please avoid adding "Mojo" to symbol names. GetAutofillDriver? Done. https://codereview.chromium.org/2007473004/diff/220001/components/password_ma... File components/password_manager/content/browser/content_password_manager_driver.h (right): https://codereview.chromium.org/2007473004/diff/220001/components/password_ma... components/password_manager/content/browser/content_password_manager_driver.h:105: const autofill::mojom::AutofillAgentPtr& GetMojoAutofillAgent(); On 2016/07/07 04:52:32, Ken Rockot wrote: > nit: same comment - GetAutofillAgent? Done.
The CQ bit was checked by leon.han@intel.com 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.
The CQ bit was checked by leon.han@intel.com 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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by leon.han@intel.com 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...
Ping Daniel, Thanks~
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping Daniel, for the left files OWNER review: autofill_messages.h autofill_agent.mojom autofill_driver.mojom
https://codereview.chromium.org/2007473004/diff/280001/chrome/renderer/autofi... File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right): https://codereview.chromium.org/2007473004/diff/280001/chrome/renderer/autofi... chrome/renderer/autofill/autofill_renderer_browsertest.cc:109: // Records whether TextFieldDidChange() get called. Nit: called_field_change_. https://codereview.chromium.org/2007473004/diff/280001/chrome/renderer/autofi... File chrome/renderer/autofill/form_autocomplete_browsertest.cc (right): https://codereview.chromium.org/2007473004/diff/280001/chrome/renderer/autofi... chrome/renderer/autofill/form_autocomplete_browsertest.cc:92: bool called_nofocus_; Ditto: called_no_focus_ (I would suggest wording this as did_unfocus_form_ to increase clarity). Similar comments apply to the other fields that don't separate words with underscores. https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... File components/autofill/content/browser/content_autofill_driver.cc (right): https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/browser/content_autofill_driver.cc:262: mojo::GetProxy(&mojo_autofill_agent_)); I guess we do this lazy binding to avoid connecting to this interface if it's not used? Maybe add a comment about that? Also, is an interface ptr that was bound that experienced an error still considered as bound? I'm guessing not, as the message pipe would be gone but it's not clear from the comments. ... but if the message pipe was closed to a connection error (e.g. failing validation), do we really want to try to reconnect? https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... File components/autofill/content/browser/content_autofill_driver_unittest.cc (right): https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/browser/content_autofill_driver_unittest.cc:212: std::unique_ptr<FormData> fillform_form_; Nit: I suspect using base::Optional<T> for these fields might lead to slightly easier to read code, since it lets you write code like this instead: field_preview_value_ = value.To<base::string16>(); https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... File components/autofill/content/public/interfaces/autofill_agent.mojom (right): https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/public/interfaces/autofill_agent.mojom:9: interface AutofillAgent { Please comment where this interface lives (in the renderers, presumably) https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/public/interfaces/autofill_agent.mojom:15: FillForm(int32 id, FormData form); Nit: document what |id| is (I guess it's a general autofill concept?) https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/public/interfaces/autofill_agent.mojom:48: ShowInitialPasswordAccountSuggestions(int32 key, PasswordFormFillData form_data); Ditto: please comment what |key| is. https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... File components/autofill/content/public/interfaces/autofill_driver.mojom (right): https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/public/interfaces/autofill_driver.mojom:11: interface AutofillDriver { Ditto: please describe where this interface lives and its general duties (something like "lives in the browser process and manages the autofill agent for one web frame") https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/public/interfaces/autofill_driver.mojom:31: QueryFormFieldAutofill(int32 id, Hmm... I guess |id| here is the same type of ID as in autofill agent, and it wouldn't be ideal to duplicate the comments. Is there a good place to put shared documentation for this stuff? Maybe in a markdown in here instead? https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/renderer/autofill_agent.cc:518: for (size_t i = 0; i < forms.size(); ++i) { Nit: I think ranged-based for loops work with mojo::Array (and will require less changes in the future if mojo starts passing vectors as STL vectors directly). https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/renderer/autofill_agent.cc:811: mojo_autofill_driver_.encountered_error()) { How comes this check isn't simply !mojo_autofill_driver_? What's the difference? ALso, a similar question to earlier applies: why do we want to late bind, and why do we want to reconnect on connection error? On a connection error, I'd imagine we'd have consistency problems. https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.h:264: AutofillAgent* autofill_agent_; // Weak reference. What's the lifetime relationship? Does AutofillAgent own this class? Or does some other class own both and make sure they are created / destroyed together? https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2400: // can register mock implementations. Can we fix the test harness (ChromeRenderViewTest, I assume?) to just set this up correctly? I'd rather limit the test-specific code to tests.
(Also, many apologies for the long latency.)
On 2016/07/12 14:23:03, dcheng wrote: > (Also, many apologies for the long latency.) Oops,, this must have taken you much time, thanks for the detailed review. Well I'm considering maybe I should have invited you as the first reviewer ;-)
The CQ bit was checked by leon.han@intel.com 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/2007473004/diff/280001/chrome/renderer/autofi... File chrome/renderer/autofill/autofill_renderer_browsertest.cc (right): https://codereview.chromium.org/2007473004/diff/280001/chrome/renderer/autofi... chrome/renderer/autofill/autofill_renderer_browsertest.cc:109: // Records whether TextFieldDidChange() get called. On 2016/07/12 14:22:12, dcheng wrote: > Nit: called_field_change_. Done. https://codereview.chromium.org/2007473004/diff/280001/chrome/renderer/autofi... File chrome/renderer/autofill/form_autocomplete_browsertest.cc (right): https://codereview.chromium.org/2007473004/diff/280001/chrome/renderer/autofi... chrome/renderer/autofill/form_autocomplete_browsertest.cc:92: bool called_nofocus_; On 2016/07/12 14:22:12, dcheng wrote: > Ditto: called_no_focus_ (I would suggest wording this as did_unfocus_form_ to > increase clarity). Similar comments apply to the other fields that don't > separate words with underscores. Done. https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... File components/autofill/content/browser/content_autofill_driver.cc (right): https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/browser/content_autofill_driver.cc:262: mojo::GetProxy(&mojo_autofill_agent_)); On 2016/07/12 14:22:12, dcheng wrote: > I guess we do this lazy binding to avoid connecting to this interface if it's > not used? Maybe add a comment about that? > Done. > Also, is an interface ptr that was bound that experienced an error still > considered as bound? I'm guessing not, as the message pipe would be gone but > it's not clear from the comments. > The interface ptr is still considered as bound. > ... but if the message pipe was closed to a connection error (e.g. failing > validation), do we really want to try to reconnect? No, we will not reconnect once connection error. https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... File components/autofill/content/browser/content_autofill_driver_unittest.cc (right): https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/browser/content_autofill_driver_unittest.cc:212: std::unique_ptr<FormData> fillform_form_; On 2016/07/12 14:22:12, dcheng wrote: > Nit: I suspect using base::Optional<T> for these fields might lead to slightly > easier to read code, since it lets you write code like this instead: > field_preview_value_ = value.To<base::string16>(); Done and thanks! https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... File components/autofill/content/public/interfaces/autofill_agent.mojom (right): https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/public/interfaces/autofill_agent.mojom:9: interface AutofillAgent { On 2016/07/12 14:22:12, dcheng wrote: > Please comment where this interface lives (in the renderers, presumably) Done. https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/public/interfaces/autofill_agent.mojom:15: FillForm(int32 id, FormData form); On 2016/07/12 14:22:12, dcheng wrote: > Nit: document what |id| is (I guess it's a general autofill concept?) Done. Yes it's a autofill concept. https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/public/interfaces/autofill_agent.mojom:48: ShowInitialPasswordAccountSuggestions(int32 key, PasswordFormFillData form_data); On 2016/07/12 14:22:12, dcheng wrote: > Ditto: please comment what |key| is. Done. https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... File components/autofill/content/public/interfaces/autofill_driver.mojom (right): https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/public/interfaces/autofill_driver.mojom:11: interface AutofillDriver { On 2016/07/12 14:22:12, dcheng wrote: > Ditto: please describe where this interface lives and its general duties > (something like "lives in the browser process and manages the autofill agent for > one web frame") Done. https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/public/interfaces/autofill_driver.mojom:31: QueryFormFieldAutofill(int32 id, On 2016/07/12 14:22:12, dcheng wrote: > Hmm... I guess |id| here is the same type of ID as in autofill agent, and it > wouldn't be ideal to duplicate the comments. Is there a good place to put shared > documentation for this stuff? Maybe in a markdown in here instead? Done. https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/renderer/autofill_agent.cc:518: for (size_t i = 0; i < forms.size(); ++i) { On 2016/07/12 14:22:12, dcheng wrote: > Nit: I think ranged-based for loops work with mojo::Array (and will require less > changes in the future if mojo starts passing vectors as STL vectors directly). Done. https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/renderer/autofill_agent.cc:811: mojo_autofill_driver_.encountered_error()) { On 2016/07/12 14:22:12, dcheng wrote: > How comes this check isn't simply !mojo_autofill_driver_? What's the difference? > > ALso, a similar question to earlier applies: why do we want to late bind, and > why do we want to reconnect on connection error? On a connection error, I'd > imagine we'd have consistency problems. En,, seems reconnecting after a connection error is a problem. Let me consider more about this.. https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.h:264: AutofillAgent* autofill_agent_; // Weak reference. On 2016/07/12 14:22:12, dcheng wrote: > What's the lifetime relationship? Does AutofillAgent own this class? Or does > some other class own both and make sure they are created / destroyed together? Both of them are observers of the same render frame, they don't own each other but are created/destroyed together. https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2400: // can register mock implementations. On 2016/07/12 14:22:13, dcheng wrote: > Can we fix the test harness (ChromeRenderViewTest, I assume?) to just set this > up correctly? I'd rather limit the test-specific code to tests. I think even we change ChromeRenderViewTest and some other test harness, we still can't avoid adding some logic into RenderProcessHostImpl to access the |remote_interfaces_|. And, seems Ben and Ken are working for some big changes for content layer {InterfaceProvider,InterfaceRegistry}, so later maybe the problem will disappear or we have to go a different way here. So I don't think current solution is unacceptable.
Uploaded ps#15 to address Daniel's comments, PTAnL, thanks.
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/renderer/autofill_agent.cc:811: mojo_autofill_driver_.encountered_error()) { On 2016/07/13 03:48:30, leonhsl wrote: > On 2016/07/12 14:22:12, dcheng wrote: > > How comes this check isn't simply !mojo_autofill_driver_? What's the > difference? > > > > ALso, a similar question to earlier applies: why do we want to late bind, and > > why do we want to reconnect on connection error? On a connection error, I'd > > imagine we'd have consistency problems. > > En,, seems reconnecting after a connection error is a problem. Let me consider > more about this.. Shall we remove the reconnecting part? https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2400: // can register mock implementations. On 2016/07/13 03:48:31, leonhsl wrote: > On 2016/07/12 14:22:13, dcheng wrote: > > Can we fix the test harness (ChromeRenderViewTest, I assume?) to just set this > > up correctly? I'd rather limit the test-specific code to tests. > > I think even we change ChromeRenderViewTest and some other test harness, we > still can't avoid adding some logic into RenderProcessHostImpl to access the > |remote_interfaces_|. And, seems Ben and Ken are working for some big changes > for content layer {InterfaceProvider,InterfaceRegistry}, so later maybe the > problem will disappear or we have to go a different way here. So I don't think > current solution is unacceptable. To help me understand better, which tests were failing without this? https://codereview.chromium.org/2007473004/diff/300001/components/autofill/co... File components/autofill/content/public/interfaces/autofill_agent.mojom (right): https://codereview.chromium.org/2007473004/diff/300001/components/autofill/co... components/autofill/content/public/interfaces/autofill_agent.mojom:10: // is responsible to communicate with corresponding autofill driver. Nit: There is one instance of this interface per render frame in the render process. https://codereview.chromium.org/2007473004/diff/300001/components/autofill/co... File components/autofill/content/public/interfaces/autofill_driver.mojom (right): https://codereview.chromium.org/2007473004/diff/300001/components/autofill/co... components/autofill/content/public/interfaces/autofill_driver.mojom:12: // is responsible to communicate with corresponding autofill agent. Nit: There is one instance of this interface per render frame host in the browser process. I think it's OK not to mention the corresponding class here.
The CQ bit was checked by leon.han@intel.com 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...
Uploaded ps#16 to address comments from Daniel, PTAnL, Thanks. https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/2007473004/diff/280001/components/autofill/co... components/autofill/content/renderer/autofill_agent.cc:811: mojo_autofill_driver_.encountered_error()) { On 2016/07/14 04:31:44, dcheng wrote: > On 2016/07/13 03:48:30, leonhsl wrote: > > On 2016/07/12 14:22:12, dcheng wrote: > > > How comes this check isn't simply !mojo_autofill_driver_? What's the > > difference? > > > > > > ALso, a similar question to earlier applies: why do we want to late bind, > and > > > why do we want to reconnect on connection error? On a connection error, I'd > > > imagine we'd have consistency problems. > > > > En,, seems reconnecting after a connection error is a problem. Let me consider > > more about this.. > > Shall we remove the reconnecting part? Yeah,, I think I've finally figured out that we do not need reconnect here. The normal scenario of binding remote AutofillDriver would not close the pipe unexpectedly except when AutofillDriver destroyed, otherwise if connection error happened it just means something really went wrong and even reconnecting would not make anything better. Thanks for raising the discussion thread in chromium-mojo@ and getting things clearer! Done. https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2400: // can register mock implementations. On 2016/07/14 04:31:44, dcheng wrote: > On 2016/07/13 03:48:31, leonhsl wrote: > > On 2016/07/12 14:22:13, dcheng wrote: > > > Can we fix the test harness (ChromeRenderViewTest, I assume?) to just set > this > > > up correctly? I'd rather limit the test-specific code to tests. > > > > I think even we change ChromeRenderViewTest and some other test harness, we > > still can't avoid adding some logic into RenderProcessHostImpl to access the > > |remote_interfaces_|. And, seems Ben and Ken are working for some big changes > > for content layer {InterfaceProvider,InterfaceRegistry}, so later maybe the > > problem will disappear or we have to go a different way here. So I don't think > > current solution is unacceptable. > > To help me understand better, which tests were failing without this? I encountered the problem in https://codereview.chromium.org/2007473004/diff/300001/components/autofill/co... Line267, where I found the |remote_interfaces_| of main render frame host is always nullptr. https://codereview.chromium.org/2007473004/diff/300001/components/autofill/co... File components/autofill/content/public/interfaces/autofill_agent.mojom (right): https://codereview.chromium.org/2007473004/diff/300001/components/autofill/co... components/autofill/content/public/interfaces/autofill_agent.mojom:10: // is responsible to communicate with corresponding autofill driver. On 2016/07/14 04:31:44, dcheng wrote: > Nit: There is one instance of this interface per render frame in the render > process. Done and Thanks. https://codereview.chromium.org/2007473004/diff/300001/components/autofill/co... File components/autofill/content/public/interfaces/autofill_driver.mojom (right): https://codereview.chromium.org/2007473004/diff/300001/components/autofill/co... components/autofill/content/public/interfaces/autofill_driver.mojom:12: // is responsible to communicate with corresponding autofill agent. On 2016/07/14 04:31:44, dcheng wrote: > Nit: There is one instance of this interface per render frame host in the > browser process. > > I think it's OK not to mention the corresponding class here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping Daniel, Thanks
On 2016/07/18 11:54:31, leonhsl wrote: > Ping Daniel, Thanks Ping!
https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2400: // can register mock implementations. On 2016/07/14 09:57:31, leonhsl wrote: > On 2016/07/14 04:31:44, dcheng wrote: > > On 2016/07/13 03:48:31, leonhsl wrote: > > > On 2016/07/12 14:22:13, dcheng wrote: > > > > Can we fix the test harness (ChromeRenderViewTest, I assume?) to just set > > this > > > > up correctly? I'd rather limit the test-specific code to tests. > > > > > > I think even we change ChromeRenderViewTest and some other test harness, we > > > still can't avoid adding some logic into RenderProcessHostImpl to access the > > > |remote_interfaces_|. And, seems Ben and Ken are working for some big > changes > > > for content layer {InterfaceProvider,InterfaceRegistry}, so later maybe the > > > problem will disappear or we have to go a different way here. So I don't > think > > > current solution is unacceptable. > > > > To help me understand better, which tests were failing without this? > > I encountered the problem in > https://codereview.chromium.org/2007473004/diff/300001/components/autofill/co... > Line267, where I found the |remote_interfaces_| of main render frame host is > always nullptr. I think we just need to set an InterfaceProvider for the MockRenderProcesHost's interface registry to make this work, right? See https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_...
https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2400: // can register mock implementations. On 2016/07/19 14:34:11, dcheng wrote: > On 2016/07/14 09:57:31, leonhsl wrote: > > On 2016/07/14 04:31:44, dcheng wrote: > > > On 2016/07/13 03:48:31, leonhsl wrote: > > > > On 2016/07/12 14:22:13, dcheng wrote: > > > > > Can we fix the test harness (ChromeRenderViewTest, I assume?) to just > set > > > this > > > > > up correctly? I'd rather limit the test-specific code to tests. > > > > > > > > I think even we change ChromeRenderViewTest and some other test harness, > we > > > > still can't avoid adding some logic into RenderProcessHostImpl to access > the > > > > |remote_interfaces_|. And, seems Ben and Ken are working for some big > > changes > > > > for content layer {InterfaceProvider,InterfaceRegistry}, so later maybe > the > > > > problem will disappear or we have to go a different way here. So I don't > > think > > > > current solution is unacceptable. > > > > > > To help me understand better, which tests were failing without this? > > > > I encountered the problem in > > > https://codereview.chromium.org/2007473004/diff/300001/components/autofill/co... > > Line267, where I found the |remote_interfaces_| of main render frame host is > > always nullptr. > > I think we just need to set an InterfaceProvider for the MockRenderProcesHost's > interface registry to make this work, right? See > https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_... Which on do you mean? 1, Initialize MockRenderProcesHost class with an empty InterfaceProvider; 2, Set an empty InterfaceProvider for MockRenderProcesHost only for this test case. Actually I don't understand why we must avoid initializing an empty InterfaceProvider for RFHI, this behavior aligns with before(When we were using ServiceRegistry).
On 2016/07/20 03:01:07, leonhsl wrote: > https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... > File content/browser/frame_host/render_frame_host_impl.cc (right): > > https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... > content/browser/frame_host/render_frame_host_impl.cc:2400: // can register mock > implementations. > On 2016/07/19 14:34:11, dcheng wrote: > > On 2016/07/14 09:57:31, leonhsl wrote: > > > On 2016/07/14 04:31:44, dcheng wrote: > > > > On 2016/07/13 03:48:31, leonhsl wrote: > > > > > On 2016/07/12 14:22:13, dcheng wrote: > > > > > > Can we fix the test harness (ChromeRenderViewTest, I assume?) to just > > set > > > > this > > > > > > up correctly? I'd rather limit the test-specific code to tests. > > > > > > > > > > I think even we change ChromeRenderViewTest and some other test harness, > > we > > > > > still can't avoid adding some logic into RenderProcessHostImpl to access > > the > > > > > |remote_interfaces_|. And, seems Ben and Ken are working for some big > > > changes > > > > > for content layer {InterfaceProvider,InterfaceRegistry}, so later maybe > > the > > > > > problem will disappear or we have to go a different way here. So I don't > > > think > > > > > current solution is unacceptable. > > > > > > > > To help me understand better, which tests were failing without this? > > > > > > I encountered the problem in > > > > > > https://codereview.chromium.org/2007473004/diff/300001/components/autofill/co... > > > Line267, where I found the |remote_interfaces_| of main render frame host is > > > always nullptr. > > > > I think we just need to set an InterfaceProvider for the > MockRenderProcesHost's > > interface registry to make this work, right? See > > > https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_... > > Which on do you mean? > 1, Initialize MockRenderProcesHost class with an empty InterfaceProvider; > 2, Set an empty InterfaceProvider for MockRenderProcesHost only for this test > case. > > Actually I don't understand why we must avoid initializing an empty > InterfaceProvider for RFHI, this behavior aligns with before(When we were using > ServiceRegistry). Well, on the other hand, someone wrote it intentionally this way (perhaps this is unintentionally unset if we aren't initializing mojo), so if we change it, we should have a good reason (and that reason shouldn't necessarily just be "this makes the tests pass").
On 2016/07/20 03:01:07, leonhsl wrote: > https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... > File content/browser/frame_host/render_frame_host_impl.cc (right): > > https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... > content/browser/frame_host/render_frame_host_impl.cc:2400: // can register mock > implementations. > On 2016/07/19 14:34:11, dcheng wrote: > > On 2016/07/14 09:57:31, leonhsl wrote: > > > On 2016/07/14 04:31:44, dcheng wrote: > > > > On 2016/07/13 03:48:31, leonhsl wrote: > > > > > On 2016/07/12 14:22:13, dcheng wrote: > > > > > > Can we fix the test harness (ChromeRenderViewTest, I assume?) to just > > set > > > > this > > > > > > up correctly? I'd rather limit the test-specific code to tests. > > > > > > > > > > I think even we change ChromeRenderViewTest and some other test harness, > > we > > > > > still can't avoid adding some logic into RenderProcessHostImpl to access > > the > > > > > |remote_interfaces_|. And, seems Ben and Ken are working for some big > > > changes > > > > > for content layer {InterfaceProvider,InterfaceRegistry}, so later maybe > > the > > > > > problem will disappear or we have to go a different way here. So I don't > > > think > > > > > current solution is unacceptable. > > > > > > > > To help me understand better, which tests were failing without this? > > > > > > I encountered the problem in > > > > > > https://codereview.chromium.org/2007473004/diff/300001/components/autofill/co... > > > Line267, where I found the |remote_interfaces_| of main render frame host is > > > always nullptr. > > > > I think we just need to set an InterfaceProvider for the > MockRenderProcesHost's > > interface registry to make this work, right? See > > > https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_... > > Which on do you mean? > 1, Initialize MockRenderProcesHost class with an empty InterfaceProvider; > 2, Set an empty InterfaceProvider for MockRenderProcesHost only for this test > case. > > Actually I don't understand why we must avoid initializing an empty > InterfaceProvider for RFHI, this behavior aligns with before(When we were using > ServiceRegistry). Either 1 or 2 works for me. I suppose 1 is probably more generally useful, but I'm not sure if that's desirable behavior.
On 2016/07/20 10:33:20, dcheng wrote: > On 2016/07/20 03:01:07, leonhsl wrote: > > > https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... > > File content/browser/frame_host/render_frame_host_impl.cc (right): > > > > > https://codereview.chromium.org/2007473004/diff/280001/content/browser/frame_... > > content/browser/frame_host/render_frame_host_impl.cc:2400: // can register > mock > > implementations. > > On 2016/07/19 14:34:11, dcheng wrote: > > > On 2016/07/14 09:57:31, leonhsl wrote: > > > > On 2016/07/14 04:31:44, dcheng wrote: > > > > > On 2016/07/13 03:48:31, leonhsl wrote: > > > > > > On 2016/07/12 14:22:13, dcheng wrote: > > > > > > > Can we fix the test harness (ChromeRenderViewTest, I assume?) to > just > > > set > > > > > this > > > > > > > up correctly? I'd rather limit the test-specific code to tests. > > > > > > > > > > > > I think even we change ChromeRenderViewTest and some other test > harness, > > > we > > > > > > still can't avoid adding some logic into RenderProcessHostImpl to > access > > > the > > > > > > |remote_interfaces_|. And, seems Ben and Ken are working for some big > > > > changes > > > > > > for content layer {InterfaceProvider,InterfaceRegistry}, so later > maybe > > > the > > > > > > problem will disappear or we have to go a different way here. So I > don't > > > > think > > > > > > current solution is unacceptable. > > > > > > > > > > To help me understand better, which tests were failing without this? > > > > > > > > I encountered the problem in > > > > > > > > > > https://codereview.chromium.org/2007473004/diff/300001/components/autofill/co... > > > > Line267, where I found the |remote_interfaces_| of main render frame host > is > > > > always nullptr. > > > > > > I think we just need to set an InterfaceProvider for the > > MockRenderProcesHost's > > > interface registry to make this work, right? See > > > > > > https://cs.chromium.org/chromium/src/content/public/test/mock_render_process_... > > > > Which on do you mean? > > 1, Initialize MockRenderProcesHost class with an empty InterfaceProvider; > > 2, Set an empty InterfaceProvider for MockRenderProcesHost only for this > test > > case. > > > > Actually I don't understand why we must avoid initializing an empty > > InterfaceProvider for RFHI, this behavior aligns with before(When we were > using > > ServiceRegistry). > > Either 1 or 2 works for me. I suppose 1 is probably more generally useful, but > I'm not sure if that's desirable behavior. Thanks for clarification and suggestion. OK I'll dig down to find a way and update soon.
The CQ bit was checked by leon.han@intel.com 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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_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 leon.han@intel.com 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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Description was changed from ========== [Autofill] Migrate ContentAutofillDriver<-->AutofillAgent IPCs to mojo. This CL -- Migrates all ContentAutofillDriver<-->AutofillAgent IPC messages into mojo interfaces. -- Revises related unit tests and browser tests. TEST=components_unittests, browser_tests BUG=582391 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== [Autofill] Migrate ContentAutofillDriver<-->AutofillAgent IPCs to mojo. This CL -- Migrates all ContentAutofillDriver<-->AutofillAgent IPC messages into mojo interfaces. -- Revises related unit tests and browser tests. TEST=components_unittests, browser_tests BUG=582391 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by leon.han@intel.com 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: 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...)
The CQ bit was checked by leon.han@intel.com 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.
leon.han@intel.com changed reviewers: + sky@chromium.org
+sky for OWNER review of content/public/test/mock_render_process_host.cc. The context: I encountered a problem at Line267 of https://codereview.chromium.org/2007473004/diff/400001/components/autofill/co..., where I found the |remote_interfaces_| of main render frame host is always nullptr. This is because RenderFrameHostImpl::SetUpMojoIfNeeded() did not create |remote_interfaces_| in case of MockRenderProcessHost. To solve this, we need to initialize MockRenderProcessHost with an empty InterfaceProvider instead of an null one.
Daniel, would you PTAnL at ps#20 to see whether it's OK for you? thanks.
LGTM
https://codereview.chromium.org/2007473004/diff/400001/content/public/test/mo... File content/public/test/mock_render_process_host.cc (right): https://codereview.chromium.org/2007473004/diff/400001/content/public/test/mo... content/public/test/mock_render_process_host.cc:107: remote_interfaces_.reset(new shell::InterfaceProvider); This seems reasonable, but we this class also defines SetRemoteInterfaces. Do we need both?
https://codereview.chromium.org/2007473004/diff/400001/content/public/test/mo... File content/public/test/mock_render_process_host.cc (right): https://codereview.chromium.org/2007473004/diff/400001/content/public/test/mo... content/public/test/mock_render_process_host.cc:107: remote_interfaces_.reset(new shell::InterfaceProvider); On 2016/07/22 16:47:48, sky wrote: > This seems reasonable, but we this class also defines SetRemoteInterfaces. Do we > need both? Good catch~~ I think we can remove SetRemoteInterfaces now, currently its only user is EmbeddedWorkerTestHelper, which can instead use this empty InterfaceProvider we initialized here. And I don't think the initial InterfaceProvider instance needs to be replaced with new one in normal scenarios. So let's remove SetRemoteInterfaces. WDYT? Thanks.
The CQ bit was checked by leon.han@intel.com 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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #21 (id:420001) has been deleted
https://codereview.chromium.org/2007473004/diff/400001/content/public/test/mo... File content/public/test/mock_render_process_host.cc (right): https://codereview.chromium.org/2007473004/diff/400001/content/public/test/mo... content/public/test/mock_render_process_host.cc:107: remote_interfaces_.reset(new shell::InterfaceProvider); On 2016/07/23 00:41:45, leonhsl wrote: > On 2016/07/22 16:47:48, sky wrote: > > This seems reasonable, but we this class also defines SetRemoteInterfaces. Do > we > > need both? > > Good catch~~ I think we can remove SetRemoteInterfaces now, currently its only > user is EmbeddedWorkerTestHelper, which can instead use this empty > InterfaceProvider we initialized here. And I don't think the initial > InterfaceProvider instance needs to be replaced with new one in normal > scenarios. So let's remove SetRemoteInterfaces. > WDYT? Thanks. Sorry I had some misunderstanding about MockRenderProcessHost's usage. "I don't think the initial InterfaceProvider instance needs to be replaced with new one in normal scenarios." is not true. In case of EmbeddedWorkerTestHelper, it does not call MockRenderProcessHost::Init() at all, so no empty InterfaceProvider gets created there, EmbeddedWorkerTestHelper just creates its own InterfaceProvider and calls SetRemoteInterfaces() to set into MRPH. This means that SetRemoteInterfaces() has its own value and we should keep it. And what we added here is to provide a default value for |remote_interfaces_|.
Ok, LGTM
The CQ bit was checked by leon.han@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, rockot@chromium.org, jochen@chromium.org, yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/2007473004/#ps400001 (title: "Initialize remote_interfaces_ in Init()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #20 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== [Autofill] Migrate ContentAutofillDriver<-->AutofillAgent IPCs to mojo. This CL -- Migrates all ContentAutofillDriver<-->AutofillAgent IPC messages into mojo interfaces. -- Revises related unit tests and browser tests. TEST=components_unittests, browser_tests BUG=582391 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== [Autofill] Migrate ContentAutofillDriver<-->AutofillAgent IPCs to mojo. This CL -- Migrates all ContentAutofillDriver<-->AutofillAgent IPC messages into mojo interfaces. -- Revises related unit tests and browser tests. TEST=components_unittests, browser_tests BUG=582391 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/90fd63b511fa3c6612d3ad032911025226293df8 Cr-Commit-Position: refs/heads/master@{#407673} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/90fd63b511fa3c6612d3ad032911025226293df8 Cr-Commit-Position: refs/heads/master@{#407673} |
