Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2470283002:
Convert profile import IPCs to Mojo (Closed)
|
Created:
4 years, 1 month ago by tibell Modified:
4 years ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, tfarina, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert profile import IPCs to Mojo
BUG=
Committed: https://crrev.com/1d83f16c4e8a49c614479bbd5e74f16aad7c8232
Cr-Commit-Position: refs/heads/master@{#435518}
Patch Set 1 #Patch Set 2 : Remove last reference to profile_import_process_messages.h #Patch Set 3 : Add out-of-line ImporterIE7PasswordInfo copy constructor #Patch Set 4 : Use enum instead of int in mojom where possible #
Total comments: 27
Patch Set 5 : Address review comments #
Total comments: 18
Patch Set 6 : Address review comments #
Total comments: 19
Patch Set 7 : Address dcheng's review comments #
Total comments: 2
Patch Set 8 : Merge #Patch Set 9 : Use correct int types #
Total comments: 6
Patch Set 10 : Clarify ownership of observer InterfacePtr #Patch Set 11 : Merge #Patch Set 12 : Fix incorrect merge #
Total comments: 11
Patch Set 13 : Use a ThreadSafeInterfacePtr instead #Patch Set 14 : Address final review comments #Patch Set 15 : Merge #Messages
Total messages: 119 (71 generated)
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Remove last reference to profile_import_process_messages.h
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
The CQ bit was unchecked by tibell@chromium.org
Add out-of-line ImporterIE7PasswordInfo copy constructor
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Use enum instead of int in mojom where possible
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tibell@chromium.org changed reviewers: + gab@chromium.org, sammc@chromium.org
Hi, I'm working on the Mojo team and we're currently working on converting Chrome to Mojo (see https://docs.google.com/spreadsheets/d/1pGWX_wxGdjAVtQOmlDDfhuIc3Pbwg9FtvFXAX... for progress).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/04 04:00:43, tibell wrote: > Hi, > > I'm working on the Mojo team and we're currently working on converting Chrome to > Mojo (see > https://docs.google.com/spreadsheets/d/1pGWX_wxGdjAVtQOmlDDfhuIc3Pbwg9FtvFXAX... > for progress). Awesome :). You added two reviewers, can you please explicitly mention what you expect from each one of us? Thanks, Gab
On 2016/11/04 14:38:35, gab wrote: > On 2016/11/04 04:00:43, tibell wrote: > > Hi, > > > > I'm working on the Mojo team and we're currently working on converting Chrome > to > > Mojo (see > > > https://docs.google.com/spreadsheets/d/1pGWX_wxGdjAVtQOmlDDfhuIc3Pbwg9FtvFXAX... > > for progress). > > Awesome :). You added two reviewers, can you please explicitly mention what you > expect from each one of us? > > Thanks, > Gab Gab, could you please review everything? :) Sam works on the Mojo team so I expect him to just have an additional look at the Mojo-specific issues.
https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... File chrome/browser/importer/external_process_importer_client.cc (right): https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:102: bridge_->NotifyItemStarted(static_cast<importer::ImportItem>(item_data)); Remove the cast. https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:286: void ExternalProcessImporterClient::SetupMojoOnUIThread( Have you considered doing this in Start() and passing a chrome::mojom::ProfileImportRequest to the IO thread? https://codereview.chromium.org/2470283002/diff/50001/chrome/common/importer/... File chrome/common/importer/importer_data_types.h (right): https://codereview.chromium.org/2470283002/diff/50001/chrome/common/importer/... chrome/common/importer/importer_data_types.h:65: ImporterIE7PasswordInfo(const ImporterIE7PasswordInfo& other); Add an assignment operator. https://codereview.chromium.org/2470283002/diff/50001/chrome/utility/importer... File chrome/utility/importer/external_process_importer_bridge.h (right): https://codereview.chromium.org/2470283002/diff/50001/chrome/utility/importer... chrome/utility/importer/external_process_importer_bridge.h:45: class ExternalProcessImporterBridge : public ImporterBridge { It feels like ImporterBridge, ProfileWriter or something in between should be the mojo interface. Please at least add a TODO to simplify these interfaces. https://codereview.chromium.org/2470283002/diff/50001/chrome/utility/profile_... File chrome/utility/profile_import_handler.cc (right): https://codereview.chromium.org/2470283002/diff/50001/chrome/utility/profile_... chrome/utility/profile_import_handler.cc:43: observer_->OnImportFinished(false, "Importer could not be created."); Consider splitting OnImportFinished into its own mojo interface and running |observer_| on |import_thread_|.
lg https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... File chrome/browser/importer/external_process_importer_client.cc (right): https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:77: return false; Why is this method still needed? https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:102: bridge_->NotifyItemStarted(static_cast<importer::ImportItem>(item_data)); On 2016/11/07 06:31:07, Sam McNally wrote: > Remove the cast. and also rename the parameter to |import_item| since it's no longer "raw" data https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:111: static_cast<importer::ImportItem>(item_data); no cast here too and rename param https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:281: BrowserThread::UI, FROM_HERE, Does it matter that this on the UI thread? Is this because it happens to be the owner today in practice? If so, can we avoid taking a dependency on owner thread and instead take a SequencedTaskRunner to be stored in a const scoped_refptr<SequencedTaskRunner> owning_sequence_; member which in practice will be the UI thread but could be any other sequence later? https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:287: chrome::mojom::ProfileImportPtrInfo profile_import_ptr_info) { DCHECK(owning_sequence_->RunsTasksOnCurrentThread()); https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:317: std::move(observer_ptr)); #include <utility> https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:320: void ExternalProcessImporterClient::CloseMojoHandles() { DCHECK runs on proper sequence/thread? https://codereview.chromium.org/2470283002/diff/50001/chrome/common/importer/... File chrome/common/importer/BUILD.gn (right): https://codereview.chromium.org/2470283002/diff/50001/chrome/common/importer/... chrome/common/importer/BUILD.gn:13: "//components/autofill/content/common:mojo_types", What's autofill for?
Address review comments
Thanks for reviewing! PTAL. https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... File chrome/browser/importer/external_process_importer_client.cc (right): https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:77: return false; On 2016/11/07 20:07:31, gab wrote: > Why is this method still needed? It's a (unfortunately) a pure virtual method in content::UtilityProcessHostClient, which we subclass to get e.g. OnProcessCrashed. Ideally it shouldn't be pure virtual in the super class. https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:102: bridge_->NotifyItemStarted(static_cast<importer::ImportItem>(item_data)); On 2016/11/07 06:31:07, Sam McNally wrote: > Remove the cast. Done. https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:102: bridge_->NotifyItemStarted(static_cast<importer::ImportItem>(item_data)); On 2016/11/07 20:07:31, gab wrote: > On 2016/11/07 06:31:07, Sam McNally wrote: > > Remove the cast. > > and also rename the parameter to |import_item| since it's no longer "raw" data Done. https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:111: static_cast<importer::ImportItem>(item_data); On 2016/11/07 20:07:32, gab wrote: > no cast here too and rename param Done. https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:281: BrowserThread::UI, FROM_HERE, On 2016/11/07 20:07:31, gab wrote: > Does it matter that this on the UI thread? Is this because it happens to be the > owner today in practice? If so, can we avoid taking a dependency on owner thread > and instead take a SequencedTaskRunner to be stored in a > const scoped_refptr<SequencedTaskRunner> owning_sequence_; > member which in practice will be the UI thread but could be any other sequence > later? I opted for Sam's suggestion below, which means we're not doing this post at all anymore (which means that we're no longer being explicit about what thread we're running on i.e. just as the code worked before). I'm punting on moving this class to SequencedTaskRunner, although it sounds like it could be a good independent refactoring. https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:286: void ExternalProcessImporterClient::SetupMojoOnUIThread( On 2016/11/07 06:31:07, Sam McNally wrote: > Have you considered doing this in Start() and passing a > chrome::mojom::ProfileImportRequest to the IO thread? Done. https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:287: chrome::mojom::ProfileImportPtrInfo profile_import_ptr_info) { On 2016/11/07 20:07:32, gab wrote: > DCHECK(owning_sequence_->RunsTasksOnCurrentThread()); Done. https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:317: std::move(observer_ptr)); On 2016/11/07 20:07:32, gab wrote: > #include <utility> Done. https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:320: void ExternalProcessImporterClient::CloseMojoHandles() { On 2016/11/07 20:07:32, gab wrote: > DCHECK runs on proper sequence/thread? Fortunately Mojo itself checks this (which is convenient for us, as we don't need to track which thread these things were created on.) https://codereview.chromium.org/2470283002/diff/50001/chrome/common/importer/... File chrome/common/importer/BUILD.gn (right): https://codereview.chromium.org/2470283002/diff/50001/chrome/common/importer/... chrome/common/importer/BUILD.gn:13: "//components/autofill/content/common:mojo_types", On 2016/11/07 20:07:32, gab wrote: > What's autofill for? It's for the use of autofill.mojom.PasswordForm in the mojom. https://codereview.chromium.org/2470283002/diff/50001/chrome/common/importer/... File chrome/common/importer/importer_data_types.h (right): https://codereview.chromium.org/2470283002/diff/50001/chrome/common/importer/... chrome/common/importer/importer_data_types.h:65: ImporterIE7PasswordInfo(const ImporterIE7PasswordInfo& other); On 2016/11/07 06:31:07, Sam McNally wrote: > Add an assignment operator. Done. https://codereview.chromium.org/2470283002/diff/50001/chrome/utility/importer... File chrome/utility/importer/external_process_importer_bridge.h (right): https://codereview.chromium.org/2470283002/diff/50001/chrome/utility/importer... chrome/utility/importer/external_process_importer_bridge.h:45: class ExternalProcessImporterBridge : public ImporterBridge { On 2016/11/07 06:31:07, Sam McNally wrote: > It feels like ImporterBridge, ProfileWriter or something in between should be > the mojo interface. Please at least add a TODO to simplify these interfaces. Done.
https://codereview.chromium.org/2470283002/diff/50001/chrome/utility/profile_... File chrome/utility/profile_import_handler.cc (right): https://codereview.chromium.org/2470283002/diff/50001/chrome/utility/profile_... chrome/utility/profile_import_handler.cc:43: observer_->OnImportFinished(false, "Importer could not be created."); On 2016/11/07 06:31:08, Sam McNally wrote: > Consider splitting OnImportFinished into its own mojo interface and running > |observer_| on |import_thread_|. Forgot to address this one. We discussed this offline. I will keep the current behavior for now (i.e. the code posts just as much as it did before). It would probably be a good independent improvement to rejig things such as it doesn't need to.
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % comments PS: I don't really know mojo syntax so I'm mostly assuming this is the right way to hook things up (and relying on sammc's review for that part). https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... File chrome/browser/importer/external_process_importer_client.cc (right): https://codereview.chromium.org/2470283002/diff/50001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:77: return false; On 2016/11/07 23:34:23, tibell wrote: > On 2016/11/07 20:07:31, gab wrote: > > Why is this method still needed? > > It's a (unfortunately) a pure virtual method in > content::UtilityProcessHostClient, which we subclass to get e.g. > OnProcessCrashed. Ideally it shouldn't be pure virtual in the super class. I see, does every mojo host need this to get OnProcessCrashed? Surprised the mojo messages don't implicitly include one for that as well? If it's common, it'd be nice to clean it up to at least not be pure virtual in a follow-up CL. https://codereview.chromium.org/2470283002/diff/70001/chrome/browser/importer... File chrome/browser/importer/external_process_importer_client.cc (right): https://codereview.chromium.org/2470283002/diff/70001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:55: thread_id, base::Passed(&request))); Using base::Passed() on a pointer to a local variable is weird. I understand that it's because |request| is moveable, as such I think it would be preferable to use the Passed(T&& scoper) constructor (i.e. base::Passed(std::move(request)). https://codereview.chromium.org/2470283002/diff/70001/chrome/browser/importer... File chrome/browser/importer/external_process_importer_client.h (left): https://codereview.chromium.org/2470283002/diff/70001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.h:95: #if defined(OS_WIN) Is this because mojom doesn't have the concept of ifdefs? https://codereview.chromium.org/2470283002/diff/70001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (left): https://codereview.chromium.org/2470283002/diff/70001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:9: #include "base/bind.h" rm https://codereview.chromium.org/2470283002/diff/70001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:26: #include "content/public/browser/browser_thread.h" rm https://codereview.chromium.org/2470283002/diff/70001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:72: using content::BrowserThread; rm https://codereview.chromium.org/2470283002/diff/70001/chrome/utility/importer... File chrome/utility/importer/external_process_importer_bridge.h (right): https://codereview.chromium.org/2470283002/diff/70001/chrome/utility/importer... chrome/utility/importer/external_process_importer_bridge.h:40: // TODO(tibell): Now when profile import is a Mojo service perhaps ImportBride, s/when/that/ ? https://codereview.chromium.org/2470283002/diff/70001/chrome/utility/importer... chrome/utility/importer/external_process_importer_bridge.h:51: // |observer| must outlive this object. |observer| and |task_runner| must outlive this object.
lgtm https://codereview.chromium.org/2470283002/diff/70001/chrome/utility/importer... File chrome/utility/importer/external_process_importer_bridge.h (right): https://codereview.chromium.org/2470283002/diff/70001/chrome/utility/importer... chrome/utility/importer/external_process_importer_bridge.h:40: // TODO(tibell): Now when profile import is a Mojo service perhaps ImportBride, ImportBride? https://codereview.chromium.org/2470283002/diff/70001/chrome/utility/importer... chrome/utility/importer/external_process_importer_bridge.h:51: // |observer| must outlive this object. On 2016/11/08 13:34:25, gab wrote: > |observer| and |task_runner| must outlive this object. |task_runner| ends up in a scoped_refptr field so its lifetime is not a concern. How about changing the parameter here to also be a scoped_refptr to make this clear?
Address review comments
Thanks for reviewing! https://codereview.chromium.org/2470283002/diff/70001/chrome/browser/importer... File chrome/browser/importer/external_process_importer_client.cc (right): https://codereview.chromium.org/2470283002/diff/70001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:55: thread_id, base::Passed(&request))); On 2016/11/08 13:34:24, gab wrote: > Using base::Passed() on a pointer to a local variable is weird. I understand > that it's because |request| is moveable, as such I think it would be preferable > to use the Passed(T&& scoper) constructor (i.e. > base::Passed(std::move(request)). Done. https://codereview.chromium.org/2470283002/diff/70001/chrome/browser/importer... File chrome/browser/importer/external_process_importer_client.h (left): https://codereview.chromium.org/2470283002/diff/70001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.h:95: #if defined(OS_WIN) On 2016/11/08 13:34:25, gab wrote: > Is this because mojom doesn't have the concept of ifdefs? Correct. This is by design. https://codereview.chromium.org/2470283002/diff/70001/chrome/browser/importer... File chrome/browser/importer/in_process_importer_bridge.cc (left): https://codereview.chromium.org/2470283002/diff/70001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:9: #include "base/bind.h" On 2016/11/08 13:34:25, gab wrote: > rm Done. https://codereview.chromium.org/2470283002/diff/70001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:26: #include "content/public/browser/browser_thread.h" On 2016/11/08 13:34:25, gab wrote: > rm Done. https://codereview.chromium.org/2470283002/diff/70001/chrome/browser/importer... chrome/browser/importer/in_process_importer_bridge.cc:72: using content::BrowserThread; On 2016/11/08 13:34:25, gab wrote: > rm Done. https://codereview.chromium.org/2470283002/diff/70001/chrome/utility/importer... File chrome/utility/importer/external_process_importer_bridge.h (right): https://codereview.chromium.org/2470283002/diff/70001/chrome/utility/importer... chrome/utility/importer/external_process_importer_bridge.h:40: // TODO(tibell): Now when profile import is a Mojo service perhaps ImportBride, On 2016/11/08 13:34:25, gab wrote: > s/when/that/ ? Done. https://codereview.chromium.org/2470283002/diff/70001/chrome/utility/importer... chrome/utility/importer/external_process_importer_bridge.h:40: // TODO(tibell): Now when profile import is a Mojo service perhaps ImportBride, On 2016/11/09 22:31:27, Sam McNally wrote: > ImportBride? Done. https://codereview.chromium.org/2470283002/diff/70001/chrome/utility/importer... chrome/utility/importer/external_process_importer_bridge.h:51: // |observer| must outlive this object. On 2016/11/09 22:31:27, Sam McNally wrote: > On 2016/11/08 13:34:25, gab wrote: > > |observer| and |task_runner| must outlive this object. > > |task_runner| ends up in a scoped_refptr field so its lifetime is not a concern. > How about changing the parameter here to also be a scoped_refptr to make this > clear? Done. https://codereview.chromium.org/2470283002/diff/70001/chrome/utility/importer... chrome/utility/importer/external_process_importer_bridge.h:51: // |observer| must outlive this object. On 2016/11/08 13:34:25, gab wrote: > |observer| and |task_runner| must outlive this object. I went with Sam's suggestion and instead changed the parameter type to scoped_refptr<base::TaskRunner>.
The CQ bit was checked by tibell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2470283002/#ps90001 (title: "Address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
tibell@chromium.org changed reviewers: + dcheng@chromium.org, sky@chromium.org
+sky for: chrome/utility/BUILD.gn chrome/utility/chrome_content_utility_client.cc chrome/utility/profile_import_handler.h chrome/utility/profile_import_handler.cc mojo/public/tools/bindings/chromium_bindings_configuration.gni +dcheng for: chrome/browser/chrome_content_utility_manifest_overlay.json chrome/common/all_messages.h chrome/common/importer/profile_import_process_messages.h
On 2016/11/09 23:28:03, tibell wrote: > +sky for: > chrome/utility/BUILD.gn > chrome/utility/chrome_content_utility_client.cc > chrome/utility/profile_import_handler.h > chrome/utility/profile_import_handler.cc > mojo/public/tools/bindings/chromium_bindings_configuration.gni > > +dcheng for: > chrome/browser/chrome_content_utility_manifest_overlay.json > chrome/common/all_messages.h > chrome/common/importer/profile_import_process_messages.h sky/dcheng: gab and sammc has already reviewed all the files, but I need sky as an OWNERS of some files and dcheng to do a security review.
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2470283002/diff/90001/chrome/browser/importer... File chrome/browser/importer/external_process_importer_client.cc (right): https://codereview.chromium.org/2470283002/diff/90001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:55: thread_id, base::Passed(std::move(request)))); Nit: just write base::Passed(&request) https://codereview.chromium.org/2470283002/diff/90001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:92: if (utility_process_host_.get()) Nit: no .get() https://codereview.chromium.org/2470283002/diff/90001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:149: int total_history_rows_count) { Nit: let's make these unsigned at least, then we don't have to think about what happens if it's -1 at least. https://codereview.chromium.org/2470283002/diff/90001/chrome/common/importer/... File chrome/common/importer/profile_import.mojom (right): https://codereview.chromium.org/2470283002/diff/90001/chrome/common/importer/... chrome/common/importer/profile_import.mojom:35: interface ProfileImportObserver { Please import some of the comments from the original IPC messages; while they're not particularly descriptive, they're better than nothing. https://codereview.chromium.org/2470283002/diff/90001/chrome/common/importer/... chrome/common/importer/profile_import.mojom:59: // Windows only: Sad. I saw your thread about this, and I really wish we had a better solution to this: this has come up in some GpuInfo structs as well, which want to send DirectX config information, etc. https://codereview.chromium.org/2470283002/diff/90001/chrome/utility/importer... File chrome/utility/importer/external_process_importer_bridge.cc (right): https://codereview.chromium.org/2470283002/diff/90001/chrome/utility/importer... chrome/utility/importer/external_process_importer_bridge.cc:39: : task_runner_(task_runner), observer_(observer) { Nit: std::move(task_runner) https://codereview.chromium.org/2470283002/diff/90001/chrome/utility/importer... File chrome/utility/importer/external_process_importer_bridge.h (right): https://codereview.chromium.org/2470283002/diff/90001/chrome/utility/importer... chrome/utility/importer/external_process_importer_bridge.h:99: void Send(Functor&& functor, Args&&... args); I'm not a fan of these sorts of helpers. Dumb question: why can't we just rebind the interface on the right thread so it Just Works? https://codereview.chromium.org/2470283002/diff/90001/chrome/utility/profile_... File chrome/utility/profile_import_handler.cc (right): https://codereview.chromium.org/2470283002/diff/90001/chrome/utility/profile_... chrome/utility/profile_import_handler.cc:39: localized_strings, base::ThreadTaskRunnerHandle::Get().get(), No .get(), this is just getting turned back into a scoped_refptr anyway
PTAL https://codereview.chromium.org/2470283002/diff/90001/chrome/browser/importer... File chrome/browser/importer/external_process_importer_client.cc (right): https://codereview.chromium.org/2470283002/diff/90001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:55: thread_id, base::Passed(std::move(request)))); On 2016/11/11 08:46:50, dcheng wrote: > Nit: just write base::Passed(&request) I just made the opposite change on gab@'s request. Unless the style guide prefers on over the other, I prefer to keep it this way. https://codereview.chromium.org/2470283002/diff/90001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:92: if (utility_process_host_.get()) On 2016/11/11 08:46:50, dcheng wrote: > Nit: no .get() Done. https://codereview.chromium.org/2470283002/diff/90001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:149: int total_history_rows_count) { On 2016/11/11 08:46:50, dcheng wrote: > Nit: let's make these unsigned at least, then we don't have to think about what > happens if it's -1 at least. Done. https://codereview.chromium.org/2470283002/diff/90001/chrome/common/importer/... File chrome/common/importer/profile_import.mojom (right): https://codereview.chromium.org/2470283002/diff/90001/chrome/common/importer/... chrome/common/importer/profile_import.mojom:35: interface ProfileImportObserver { On 2016/11/11 08:46:50, dcheng wrote: > Please import some of the comments from the original IPC messages; while they're > not particularly descriptive, they're better than nothing. Done. https://codereview.chromium.org/2470283002/diff/90001/chrome/common/importer/... chrome/common/importer/profile_import.mojom:59: // Windows only: On 2016/11/11 08:46:50, dcheng wrote: > Sad. I saw your thread about this, and I really wish we had a better solution to > this: this has come up in some GpuInfo structs as well, which want to send > DirectX config information, etc. Acknowledged. https://codereview.chromium.org/2470283002/diff/90001/chrome/utility/importer... File chrome/utility/importer/external_process_importer_bridge.cc (right): https://codereview.chromium.org/2470283002/diff/90001/chrome/utility/importer... chrome/utility/importer/external_process_importer_bridge.cc:39: : task_runner_(task_runner), observer_(observer) { On 2016/11/11 08:46:50, dcheng wrote: > Nit: std::move(task_runner) Done. https://codereview.chromium.org/2470283002/diff/90001/chrome/utility/importer... File chrome/utility/importer/external_process_importer_bridge.h (right): https://codereview.chromium.org/2470283002/diff/90001/chrome/utility/importer... chrome/utility/importer/external_process_importer_bridge.h:99: void Send(Functor&& functor, Args&&... args); On 2016/11/11 08:46:50, dcheng wrote: > I'm not a fan of these sorts of helpers. > > Dumb question: why can't we just rebind the interface on the right thread so it > Just Works? I discussed this with Sam earlier in the review. Messages are sent from two different threads so one of them has to post. Someone could rethink how this works, but that's better done by the owners of this code. https://codereview.chromium.org/2470283002/diff/90001/chrome/utility/profile_... File chrome/utility/profile_import_handler.cc (right): https://codereview.chromium.org/2470283002/diff/90001/chrome/utility/profile_... chrome/utility/profile_import_handler.cc:39: localized_strings, base::ThreadTaskRunnerHandle::Get().get(), On 2016/11/11 08:46:50, dcheng wrote: > No .get(), this is just getting turned back into a scoped_refptr anyway Done.
Address dcheng's review comments
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
Merge
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
Is there an appropriate BUG=# for this? (still lgtm) https://codereview.chromium.org/2470283002/diff/90001/chrome/browser/importer... File chrome/browser/importer/external_process_importer_client.cc (right): https://codereview.chromium.org/2470283002/diff/90001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:55: thread_id, base::Passed(std::move(request)))); On 2016/11/14 05:35:31, tibell wrote: > On 2016/11/11 08:46:50, dcheng wrote: > > Nit: just write base::Passed(&request) > > I just made the opposite change on gab@'s request. Unless the style guide > prefers on over the other, I prefer to keep it this way. @dcheng: my reasoning is that it's weird to hand raw pointers to things on the stack. In this case it's "fine" because the intent is really to move the value and as such I prefer an explicit std::move() highlighting that.
lgtm as for the base::Passed(...) thing, I'll start a cxx thread about this. I have some thoughts about simplifying this, since base::Passed() *always* implies std::move anyway. https://codereview.chromium.org/2470283002/diff/90001/chrome/browser/importer... File chrome/browser/importer/external_process_importer_client.cc (right): https://codereview.chromium.org/2470283002/diff/90001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:55: thread_id, base::Passed(std::move(request)))); On 2016/11/14 15:36:42, gab wrote: > On 2016/11/14 05:35:31, tibell wrote: > > On 2016/11/11 08:46:50, dcheng wrote: > > > Nit: just write base::Passed(&request) > > > > I just made the opposite change on gab@'s request. Unless the style guide > > prefers on over the other, I prefer to keep it this way. > > @dcheng: my reasoning is that it's weird to hand raw pointers to things on the > stack. In this case it's "fine" because the intent is really to move the value > and as such I prefer an explicit std::move() highlighting that. Well, the whole point of base::Passed(T*) is to avoid writing base::Passed(std::move(...)): https://cs.chromium.org/chromium/src/base/bind_helpers.h?rcl=0&l=433 https://codereview.chromium.org/2470283002/diff/110001/chrome/browser/importe... File chrome/browser/importer/external_process_importer_client.h (right): https://codereview.chromium.org/2470283002/diff/110001/chrome/browser/importe... chrome/browser/importer/external_process_importer_client.h:77: void OnHistoryImportStart(uint total_history_rows_count) override; uint isn't a standard type: let's write uint32_t to match the mojo definitions.
Use correct int types
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks For reviewing! https://codereview.chromium.org/2470283002/diff/90001/chrome/browser/importer... File chrome/browser/importer/external_process_importer_client.cc (right): https://codereview.chromium.org/2470283002/diff/90001/chrome/browser/importer... chrome/browser/importer/external_process_importer_client.cc:55: thread_id, base::Passed(std::move(request)))); On 2016/11/14 20:11:13, dcheng wrote: > On 2016/11/14 15:36:42, gab wrote: > > On 2016/11/14 05:35:31, tibell wrote: > > > On 2016/11/11 08:46:50, dcheng wrote: > > > > Nit: just write base::Passed(&request) > > > > > > I just made the opposite change on gab@'s request. Unless the style guide > > > prefers on over the other, I prefer to keep it this way. > > > > @dcheng: my reasoning is that it's weird to hand raw pointers to things on the > > stack. In this case it's "fine" because the intent is really to move the value > > and as such I prefer an explicit std::move() highlighting that. > > Well, the whole point of base::Passed(T*) is to avoid writing > base::Passed(std::move(...)): > https://cs.chromium.org/chromium/src/base/bind_helpers.h?rcl=0&l=433 Acknowledged. https://codereview.chromium.org/2470283002/diff/110001/chrome/browser/importe... File chrome/browser/importer/external_process_importer_client.h (right): https://codereview.chromium.org/2470283002/diff/110001/chrome/browser/importe... chrome/browser/importer/external_process_importer_client.h:77: void OnHistoryImportStart(uint total_history_rows_count) override; On 2016/11/14 20:11:13, dcheng wrote: > uint isn't a standard type: let's write uint32_t to match the mojo definitions. Done.
sky, I need your approval as well. Thanks!
https://codereview.chromium.org/2470283002/diff/150001/chrome/utility/profile... File chrome/utility/profile_import_handler.cc (right): https://codereview.chromium.org/2470283002/diff/150001/chrome/utility/profile... chrome/utility/profile_import_handler.cc:36: observer_ = std::move(observer); DCHECK(!observer_)? https://codereview.chromium.org/2470283002/diff/150001/chrome/utility/profile... chrome/utility/profile_import_handler.cc:38: bridge_ = new ExternalProcessImporterBridge( ExternalProcessImporterBridge is ref counted. This code passes observer_ to ExternalProcessImporterBridge. How do you know 'this' outlives ExternalProcessImporterBridge? https://codereview.chromium.org/2470283002/diff/150001/chrome/utility/profile... File chrome/utility/profile_import_handler.h (right): https://codereview.chromium.org/2470283002/diff/150001/chrome/utility/profile... chrome/utility/profile_import_handler.h:66: }; DISALLOW...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Clarify ownership of observer InterfacePtr
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Merge
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/29 04:28:46, tibell wrote: > Merge PTAL
PTAL https://codereview.chromium.org/2470283002/diff/150001/chrome/utility/profile... File chrome/utility/profile_import_handler.cc (right): https://codereview.chromium.org/2470283002/diff/150001/chrome/utility/profile... chrome/utility/profile_import_handler.cc:36: observer_ = std::move(observer); On 2016/11/14 23:12:18, sky wrote: > DCHECK(!observer_)? Done. https://codereview.chromium.org/2470283002/diff/150001/chrome/utility/profile... chrome/utility/profile_import_handler.cc:38: bridge_ = new ExternalProcessImporterBridge( On 2016/11/14 23:12:18, sky wrote: > ExternalProcessImporterBridge is ref counted. This code passes observer_ to > ExternalProcessImporterBridge. How do you know 'this' outlives > ExternalProcessImporterBridge? I've now changed so there's only one owner of the InterfacePtr. https://codereview.chromium.org/2470283002/diff/150001/chrome/utility/profile... File chrome/utility/profile_import_handler.h (right): https://codereview.chromium.org/2470283002/diff/150001/chrome/utility/profile... chrome/utility/profile_import_handler.h:66: }; On 2016/11/14 23:12:18, sky wrote: > DISALLOW... Done.
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_...)
Fix incorrect merge
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Convert profile import IPCs to Mojo BUG= ========== to ========== Convert profile import IPCs to Mojo BUG= ==========
sky@chromium.org changed reviewers: + yzshen@chromium.org
Yuzhu: I added you for question about Bind. See chrome/utility/profile_import_handler.cc where is supplies a different task runner. https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... File chrome/utility/profile_import_handler.cc (right): https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... chrome/utility/profile_import_handler.cc:10: #include "base/single_thread_task_runner.h" Remove any includes that are no longer applicable. https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... chrome/utility/profile_import_handler.cc:20: // static Declaration and definition order should match (see style guide). https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... chrome/utility/profile_import_handler.cc:56: observer.Bind(observer.PassInterface(), import_thread_->task_runner()); The description of Bind indicates the runner "must belong to the same thread". That doesn't seem to be the case here. I'm not familiar with InterfacePtr to know if this is problematic. +yzshen https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... File chrome/utility/profile_import_handler.h (right): https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... chrome/utility/profile_import_handler.h:36: static void Create( It seems like all creation should go through create. If that's right, can the constructor be made private? This would mean not using MakeUnique, but that's ok.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Use a ThreadSafeInterfacePtr instead
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... File chrome/utility/profile_import_handler.cc (right): https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... chrome/utility/profile_import_handler.cc:10: #include "base/single_thread_task_runner.h" On 2016/11/29 23:56:28, sky wrote: > Remove any includes that are no longer applicable. Still used below by the call to task_runner(). https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... chrome/utility/profile_import_handler.cc:20: // static On 2016/11/29 23:56:28, sky wrote: > Declaration and definition order should match (see style guide). Done. https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... chrome/utility/profile_import_handler.cc:56: observer.Bind(observer.PassInterface(), import_thread_->task_runner()); On 2016/11/29 23:56:28, sky wrote: > The description of Bind indicates the runner "must belong to the same thread". > That doesn't seem to be the case here. I'm not familiar with InterfacePtr to > know if this is problematic. +yzshen Talked this through with sammc@ and we decided that a ThreadSafeInterfacePtr is the right choice here (we could try to post tasks around manually to achieve e.g. deletion on the right thread, but that would basically just do what ThreadSafeInterfacePtr does.) https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... File chrome/utility/profile_import_handler.h (right): https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... chrome/utility/profile_import_handler.h:36: static void Create( On 2016/11/29 23:56:28, sky wrote: > It seems like all creation should go through create. If that's right, can the > constructor be made private? This would mean not using MakeUnique, but that's > ok. We use MakeUnique in every other use of MakeStrongBinding (that doesn't just move something), so I'd rather stick with the pattern.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by tibell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with the following addressed. https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... File chrome/utility/profile_import_handler.cc (right): https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... chrome/utility/profile_import_handler.cc:20: // static On 2016/11/30 03:36:47, tibell wrote: > On 2016/11/29 23:56:28, sky wrote: > > Declaration and definition order should match (see style guide). > > Done. Did you miss this one? This function should be after the destructor.
https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... File chrome/utility/profile_import_handler.cc (right): https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... chrome/utility/profile_import_handler.cc:20: // static On 2016/11/30 16:19:07, sky wrote: > On 2016/11/30 03:36:47, tibell wrote: > > On 2016/11/29 23:56:28, sky wrote: > > > Declaration and definition order should match (see style guide). > > > > Done. > > Did you miss this one? This function should be after the destructor. Done.
Address final review comments
The CQ bit was checked by tibell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, gab@chromium.org, dcheng@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2470283002/#ps250001 (title: "Address final review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Merge
The CQ bit was checked by tibell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, gab@chromium.org, sky@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2470283002/#ps270001 (title: "Merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 270001, "attempt_start_ts": 1480551884276040,
"parent_rev": "794c27339c92a89d0c686c0f680d06d30b042c7c", "commit_rev":
"0e2722e80faa5061a06a9a005566c55a144c5c3b"}
Message was sent while issue was closed.
Description was changed from ========== Convert profile import IPCs to Mojo BUG= ========== to ========== Convert profile import IPCs to Mojo BUG= ==========
Message was sent while issue was closed.
Committed patchset #15 (id:270001)
Message was sent while issue was closed.
Description was changed from ========== Convert profile import IPCs to Mojo BUG= ========== to ========== Convert profile import IPCs to Mojo BUG= Committed: https://crrev.com/1d83f16c4e8a49c614479bbd5e74f16aad7c8232 Cr-Commit-Position: refs/heads/master@{#435518} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/1d83f16c4e8a49c614479bbd5e74f16aad7c8232 Cr-Commit-Position: refs/heads/master@{#435518}
Message was sent while issue was closed.
https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... File chrome/utility/profile_import_handler.cc (right): https://codereview.chromium.org/2470283002/diff/210001/chrome/utility/profile... chrome/utility/profile_import_handler.cc:56: observer.Bind(observer.PassInterface(), import_thread_->task_runner()); On 2016/11/29 23:56:28, sky wrote: > The description of Bind indicates the runner "must belong to the same thread". > That doesn't seem to be the case here. I'm not familiar with InterfacePtr to > know if this is problematic. +yzshen Sorry for late reply. Just realized that I missed this. Correct. The task runner must be on the same thread as where Bind is called. It is used for task scheduling on the same thread. |
