Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2049303002:
Add the UtilityProcessMojoClient class and convert SafeJsonParser (Closed)
|
Created:
4 years, 6 months ago by Patrick Monette Modified:
4 years, 6 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, kinuko+watch, ben+mojo_chromium.org, jsbell+serviceworker_chromium.org, tzik, jam, abarth-chromium, darin-cc_chromium.org, blink-worker-reviews_chromium.org, mlamouri+watch-content_chromium.org, Peter Beverloo, nhiroki, mkwst+moarreviews-shell_chromium.org, jochen+watch_chromium.org, michaeln, serviceworker-reviews, Aaron Boodman, kinuko+serviceworker, horo+watch_chromium.org, darin (slow to review), grt (UTC plus 2) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd the UtilityProcessMojoClient class and convert SafeJsonParser
This class takes care of the mundane job of managing the utility process
lifetime and connecting to the remote Mojo service in an effort to make
it very easy to add new Mojo services that run on a utility process.
As a first example, SafeJsonParser is converted to use this class.
BUG=618459
Committed: https://crrev.com/e6f1ea1a8b2145019c3e7a7248ab1fc1d880b069
Cr-Commit-Position: refs/heads/master@{#400522}
Patch Set 1 : #
Total comments: 43
Patch Set 2 : Responding to all comments #
Total comments: 4
Patch Set 3 : comments from grt@ #
Total comments: 2
Patch Set 4 : bauerb@ comment #
Total comments: 10
Patch Set 5 : Rebase + comments #Patch Set 6 : Merged back to one templated class #Patch Set 7 : Comment about utility_host_ #Patch Set 8 : Disabled test and fixed headers #Patch Set 9 : Fix test compilation on non-Windows platforms #Patch Set 10 : Fixed compilation 2 #
Total comments: 2
Patch Set 11 : s/mojo/Mojo #Patch Set 12 : Fixed tests with --no-sandbox #Patch Set 13 : Fixed android sandbox browser tests #Messages
Total messages: 47 (20 generated)
Description was changed from ========== Adding the UtilityProcessMojoClient class This class takes care of the mundane job of managing the utility process lifetime and connecting to the remote Mojo service in an effort to make it very easy to add new Mojo services that run on a utility process. BUG=618459 ========== to ========== Add the UtilityProcessMojoClient class and convert SafeJsonParser This class takes care of the mundane job of managing the utility process lifetime and connecting to the remote Mojo service in an effort to make it very easy to add new Mojo services that run on a utility process. As a first example, SafeJsonParser is converted to use this class. BUG=618459 ==========
Patchset #1 (id:1) has been deleted
pmonette@chromium.org changed reviewers: + amistry@chromium.org, bauerb@chromium.org
For context, lots of discussion here: https://codereview.chromium.org/1951983002/ amistry@ (Mojo expert) PTAL for all Mojo related stuff bauerb@ (Owner for safe_json) Can you take a look? jam@ (Owner of content) Do you think you're a good reviewer for this?
pmonette@chromium.org changed reviewers: + jam@chromium.org
+jam@
grt@chromium.org changed reviewers: + grt@chromium.org
looks good. a few comments below. https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... File components/safe_json/safe_json_parser_impl.cc (right): https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.cc:38: DCHECK(io_thread_checker_.CalledOnValidThread()); what's the advantage of doing these two things? why not remove io_thread_checker_ and use CURRENTLY_ON in the callback functions below? https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.cc:48: unsafe_json_, base::Bind(&SafeJsonParserImpl::OnParseDone, this)); does this class support calling |Start| multiple times? if not, consider std::moving unsafe_json_ into the Parse call so that the memory for the source can be released once it has been sent along. might be worth it if the input could be large. https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.cc:88: void SafeJsonParserImpl::ReportResults(std::unique_ptr<base::Value> parsed_json, apologies if this is a silly question; i haven't looked at the details too closely: is this method needed? can the spots above that post tasks to run this instead directly post the proper callback and args? https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... File components/safe_json/safe_json_parser_impl.h (right): https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.h:42: void StartOnIoThread(); nit: OnIOThread is far more prevalent in the codebase than OnIoThread. https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.h:43: nit: either remove ths blank lines separating these, or add doc comments to these two methods https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... File content/public/browser/utility_process_mojo_client.h (right): https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... content/public/browser/utility_process_mojo_client.h:29: void set_error_handler(const base::Closure& on_error_callback) { specifying an error handler seems like it will be the common case. how about making on_error_callback a 2nd param to the ctor? callers can pass in an empty closure if they really don't care. https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... content/public/browser/utility_process_mojo_client.h:65: base::ThreadChecker thread_checker_; how about getting rid of this and instead move the one in the Impl class up into that class's "protected:" block so it can be used here, too?
https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... File components/safe_json/safe_json_parser_impl.h (right): https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.h:27: : public base::RefCountedThreadSafe<SafeJsonParserImpl>, Ooh, if this class now doesn't inherit from a refcounted base class, can we get rid of the refcounting completely and just have it delete itself when it's done? https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.h:62: // Used instead of DCHECK_CURRENTLY_ON(BrowserThread::UI) because it's BrowserThread::IO?
https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... File components/safe_json/safe_json_parser_impl.h (left): https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.h:14: #include "base/threading/thread_checker.h" thread_checker.h is still used. https://codereview.chromium.org/2049303002/diff/20001/content/browser/utility... File content/browser/utility_process_mojo_client_browsertest.cc (right): https://codereview.chromium.org/2049303002/diff/20001/content/browser/utility... content/browser/utility_process_mojo_client_browsertest.cc:38: void StartTestOnIOThread() { Part of the point of the helper is that it doesn't need to run on the IO thread. The thread just has to have a message loop. I'd argue this would be a better test if it didn't run on the IO thread. https://codereview.chromium.org/2049303002/diff/20001/content/browser/utility... File content/browser/utility_process_mojo_client_impl.cc (right): https://codereview.chromium.org/2049303002/diff/20001/content/browser/utility... content/browser/utility_process_mojo_client_impl.cc:57: utility_host_->Start(); Ordering doesn't matter, but I think it's more logical to start the process and then connect to the service. But again, it doesn't matter. https://codereview.chromium.org/2049303002/diff/20001/content/browser/utility... content/browser/utility_process_mojo_client_impl.cc:70: base::ThreadChecker thread_checker_; This isn't particularly useful. The thread is only checked once. https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... File content/public/browser/utility_process_mojo_client.h (right): https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... content/public/browser/utility_process_mojo_client.h:8: #include "base/callback.h" #include "base/logging.h" for DCHECK https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... content/public/browser/utility_process_mojo_client.h:23: class UtilityProcessMojoClient : public UtilityProcessMojoClientImpl { It's kinda weird to derive from the impl. Since there are no virtual functions and the only public function in impl is disable_sandbox() (which can be wrapped trivially), I think the impl can be a private member instead of the base class. https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... content/public/browser/utility_process_mojo_client.h:29: void set_error_handler(const base::Closure& on_error_callback) { On 2016/06/09 14:48:58, grt (slow) wrote: > specifying an error handler seems like it will be the common case. how about > making on_error_callback a 2nd param to the ctor? callers can pass in an empty > closure if they really don't care. +1. I think we want to ensure a valid callback is always specified. The callback is how a process failure (launch or crash) is propagated and I think we want to ensure that case is always handled. Several of the non-mojo utility process things I've looked at haven't handled failure correctly (including both safe json and image decoder, now mojo), and I think it would be good to ensure this is fixed going forward. https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... content/public/browser/utility_process_mojo_client.h:34: DCHECK(thread_checker_.CalledOnValidThread()); DCHECK(!start_called_); https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... content/public/browser/utility_process_mojo_client.h:54: void OnConnectionError() { on_error_callback_.Run(); } If you don't make the error handler mandatory like I suggest above, you'll need to check to see if this is null before running. Running a null base::Closure will segfault. https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... content/public/browser/utility_process_mojo_client.h:65: base::ThreadChecker thread_checker_; On 2016/06/09 14:48:58, grt (slow) wrote: > how about getting rid of this and instead move the one in the Impl class up into > that class's "protected:" block so it can be used here, too? Or get rid of the impl's thread checker. I don't think it's needed there. https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... File content/public/browser/utility_process_mojo_client_impl.h (right): https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... content/public/browser/utility_process_mojo_client_impl.h:35: class Holder; I'm not a fan of the name Holder... but I don't have a better suggestion.
Thanks for the comments everyone. I uploaded a new patchset. PTAnL https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... File components/safe_json/safe_json_parser_impl.cc (right): https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.cc:38: DCHECK(io_thread_checker_.CalledOnValidThread()); On 2016/06/09 14:48:58, grt (slow) wrote: > what's the advantage of doing these two things? why not remove > io_thread_checker_ and use CURRENTLY_ON in the callback functions below? There's a comment explaining this with the declaration of io_thread_checker_ in the .h file. Should I move the comment to the .cc file? https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.cc:48: unsafe_json_, base::Bind(&SafeJsonParserImpl::OnParseDone, this)); On 2016/06/09 14:48:58, grt (slow) wrote: > does this class support calling |Start| multiple times? if not, consider > std::moving unsafe_json_ into the Parse call so that the memory for the source > can be released once it has been sent along. might be worth it if the input > could be large. Done. https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.cc:88: void SafeJsonParserImpl::ReportResults(std::unique_ptr<base::Value> parsed_json, On 2016/06/09 14:48:58, grt (slow) wrote: > apologies if this is a silly question; i haven't looked at the details too > closely: is this method needed? can the spots above that post tasks to run this > instead directly post the proper callback and args? It was an artifact of when this class was refcounted and needed to be destroyed on the caller_task_runner. In the last patch set, it is a unique exit point where the clean-up is done. I think it's useful. https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... File components/safe_json/safe_json_parser_impl.h (left): https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.h:14: #include "base/threading/thread_checker.h" On 2016/06/10 11:13:54, Anand Mistry wrote: > thread_checker.h is still used. Done. https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... File components/safe_json/safe_json_parser_impl.h (right): https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.h:27: : public base::RefCountedThreadSafe<SafeJsonParserImpl>, On 2016/06/09 15:16:50, Bernhard Bauer wrote: > Ooh, if this class now doesn't inherit from a refcounted base class, can we get > rid of the refcounting completely and just have it delete itself when it's done? Done. https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.h:42: void StartOnIoThread(); On 2016/06/09 14:48:58, grt (slow) wrote: > nit: OnIOThread is far more prevalent in the codebase than OnIoThread. Done. https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.h:43: On 2016/06/09 14:48:58, grt (slow) wrote: > nit: either remove ths blank lines separating these, or add doc comments to > these two methods Done. https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.h:62: // Used instead of DCHECK_CURRENTLY_ON(BrowserThread::UI) because it's On 2016/06/09 15:16:50, Bernhard Bauer wrote: > BrowserThread::IO? Right!Fixed. https://codereview.chromium.org/2049303002/diff/20001/content/browser/utility... File content/browser/utility_process_mojo_client_browsertest.cc (right): https://codereview.chromium.org/2049303002/diff/20001/content/browser/utility... content/browser/utility_process_mojo_client_browsertest.cc:38: void StartTestOnIOThread() { On 2016/06/10 11:13:54, Anand Mistry wrote: > Part of the point of the helper is that it doesn't need to run on the IO thread. > The thread just has to have a message loop. I'd argue this would be a better > test if it didn't run on the IO thread. Done. It even simplified the test. https://codereview.chromium.org/2049303002/diff/20001/content/browser/utility... File content/browser/utility_process_mojo_client_impl.cc (right): https://codereview.chromium.org/2049303002/diff/20001/content/browser/utility... content/browser/utility_process_mojo_client_impl.cc:57: utility_host_->Start(); On 2016/06/10 11:13:54, Anand Mistry wrote: > Ordering doesn't matter, but I think it's more logical to start the process and > then connect to the service. But again, it doesn't matter. Done. https://codereview.chromium.org/2049303002/diff/20001/content/browser/utility... content/browser/utility_process_mojo_client_impl.cc:70: base::ThreadChecker thread_checker_; On 2016/06/10 11:13:54, Anand Mistry wrote: > This isn't particularly useful. The thread is only checked once. Removed. https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... File content/public/browser/utility_process_mojo_client.h (right): https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... content/public/browser/utility_process_mojo_client.h:8: #include "base/callback.h" On 2016/06/10 11:13:55, Anand Mistry wrote: > #include "base/logging.h" for DCHECK Done. https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... content/public/browser/utility_process_mojo_client.h:23: class UtilityProcessMojoClient : public UtilityProcessMojoClientImpl { On 2016/06/10 11:13:54, Anand Mistry wrote: > It's kinda weird to derive from the impl. Since there are no virtual functions > and the only public function in impl is disable_sandbox() (which can be wrapped > trivially), I think the impl can be a private member instead of the base class. Done. https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... content/public/browser/utility_process_mojo_client.h:29: void set_error_handler(const base::Closure& on_error_callback) { On 2016/06/10 11:13:55, Anand Mistry wrote: > On 2016/06/09 14:48:58, grt (slow) wrote: > > specifying an error handler seems like it will be the common case. how about > > making on_error_callback a 2nd param to the ctor? callers can pass in an empty > > closure if they really don't care. > > +1. I think we want to ensure a valid callback is always specified. The callback > is how a process failure (launch or crash) is propagated and I think we want to > ensure that case is always handled. Several of the non-mojo utility process > things I've looked at haven't handled failure correctly (including both safe > json and image decoder, now mojo), and I think it would be good to ensure this > is fixed going forward. Done. I'll make the callback mandatory. https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... content/public/browser/utility_process_mojo_client.h:34: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/06/10 11:13:54, Anand Mistry wrote: > DCHECK(!start_called_); Done. https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... content/public/browser/utility_process_mojo_client.h:54: void OnConnectionError() { on_error_callback_.Run(); } On 2016/06/10 11:13:54, Anand Mistry wrote: > If you don't make the error handler mandatory like I suggest above, you'll need > to check to see if this is null before running. Running a null base::Closure > will segfault. Acknowledged. https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... content/public/browser/utility_process_mojo_client.h:65: base::ThreadChecker thread_checker_; On 2016/06/10 11:13:54, Anand Mistry wrote: > On 2016/06/09 14:48:58, grt (slow) wrote: > > how about getting rid of this and instead move the one in the Impl class up > into > > that class's "protected:" block so it can be used here, too? > > Or get rid of the impl's thread checker. I don't think it's needed there. Got rid of impl's thread checker. https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... File content/public/browser/utility_process_mojo_client_impl.h (right): https://codereview.chromium.org/2049303002/diff/20001/content/public/browser/... content/public/browser/utility_process_mojo_client_impl.h:35: class Holder; On 2016/06/10 11:13:55, Anand Mistry wrote: > I'm not a fan of the name Holder... but I don't have a better suggestion. I think Helper is a bit better.
lgtm regarding my previous comments. https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... File components/safe_json/safe_json_parser_impl.cc (right): https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.cc:38: DCHECK(io_thread_checker_.CalledOnValidThread()); On 2016/06/10 18:13:23, Patrick Monette wrote: > On 2016/06/09 14:48:58, grt (slow) wrote: > > what's the advantage of doing these two things? why not remove > > io_thread_checker_ and use CURRENTLY_ON in the callback functions below? > > There's a comment explaining this with the declaration of > io_thread_checker_ in the .h file. Should I move the comment to the > .cc file? Ah, I missed that. The problem is that CURRENTLY_ON doesn't work during shutdown, but the correct thread is used nonetheless? Does the rest of Chrome not have a problem with this? https://codereview.chromium.org/2049303002/diff/40001/components/safe_json/sa... File components/safe_json/safe_json_parser_impl.cc (right): https://codereview.chromium.org/2049303002/diff/40001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.cc:98: // The parsing is done whether an error occured or not and this instance can language nit: "whether or not an error occurred, so this instance..." https://codereview.chromium.org/2049303002/diff/40001/content/public/browser/... File content/public/browser/utility_process_mojo_client.h (right): https://codereview.chromium.org/2049303002/diff/40001/content/public/browser/... content/public/browser/utility_process_mojo_client.h:26: explicit UtilityProcessMojoClient(const base::string16& process_name, nit: omit "explicit"
https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... File components/safe_json/safe_json_parser_impl.cc (right): https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.cc:38: DCHECK(io_thread_checker_.CalledOnValidThread()); On 2016/06/10 19:03:06, grt (slow) wrote: > On 2016/06/10 18:13:23, Patrick Monette wrote: > > On 2016/06/09 14:48:58, grt (slow) wrote: > > > what's the advantage of doing these two things? why not remove > > > io_thread_checker_ and use CURRENTLY_ON in the callback functions below? > > > > There's a comment explaining this with the declaration of > > io_thread_checker_ in the .h file. Should I move the comment to the > > .cc file? > > Ah, I missed that. The problem is that CURRENTLY_ON doesn't work during > shutdown, but the correct thread is used nonetheless? Does the rest of Chrome > not have a problem with this? Maybe amistry@ can answer that? :) https://codereview.chromium.org/2049303002/diff/40001/components/safe_json/sa... File components/safe_json/safe_json_parser_impl.cc (right): https://codereview.chromium.org/2049303002/diff/40001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.cc:98: // The parsing is done whether an error occured or not and this instance can On 2016/06/10 19:03:06, grt (slow) wrote: > language nit: "whether or not an error occurred, so this instance..." Done. https://codereview.chromium.org/2049303002/diff/40001/content/public/browser/... File content/public/browser/utility_process_mojo_client.h (right): https://codereview.chromium.org/2049303002/diff/40001/content/public/browser/... content/public/browser/utility_process_mojo_client.h:26: explicit UtilityProcessMojoClient(const base::string16& process_name, On 2016/06/10 19:03:06, grt (slow) wrote: > nit: omit "explicit" Done.
lgtm https://codereview.chromium.org/2049303002/diff/60001/components/safe_json/sa... File components/safe_json/safe_json_parser_impl.cc (right): https://codereview.chromium.org/2049303002/diff/60001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.cc:73: const base::Value* value = NULL; nullptr
https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... File components/safe_json/safe_json_parser_impl.cc (right): https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.cc:38: DCHECK(io_thread_checker_.CalledOnValidThread()); On 2016/06/10 21:29:46, Patrick Monette wrote: > On 2016/06/10 19:03:06, grt (slow) wrote: > > On 2016/06/10 18:13:23, Patrick Monette wrote: > > > On 2016/06/09 14:48:58, grt (slow) wrote: > > > > what's the advantage of doing these two things? why not remove > > > > io_thread_checker_ and use CURRENTLY_ON in the callback functions below? > > > > > > There's a comment explaining this with the declaration of > > > io_thread_checker_ in the .h file. Should I move the comment to the > > > .cc file? > > > > Ah, I missed that. The problem is that CURRENTLY_ON doesn't work during > > shutdown, but the correct thread is used nonetheless? Does the rest of Chrome > > not have a problem with this? > > Maybe amistry@ can answer that? :) Um... yes... this is a problem with the rest of Chrome.... kinda... It's only a problem with MessageLoop destruction observers. The reason it's a problem here is because the connection error callback will be run during MessageLoop shutdown. https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.cc:53: io_thread_checker_.DetachFromThread(); I know this was already there, but the DetachFromThread is only needed in the constructor. You don't need it here or in OnParseDone. https://codereview.chromium.org/2049303002/diff/80001/content/browser/utility... File content/browser/utility_process_mojo_client_browsertest.cc (right): https://codereview.chromium.org/2049303002/diff/80001/content/browser/utility... content/browser/utility_process_mojo_client_browsertest.cc:101: INSTANTIATE_TEST_CASE_P(TestCases, I think this is an abuse of parametertised tests. All the test cases and expectations are hidden inside common test code. Parameterised tests are supposed to be for the case where you have a set of tests that need to run under a set of different environments. i.e. different message loop types. This is just a case of a set of tests, with a common environment. A better way would be to have multiple tests, each calling a different service method and setting different expectations, and common code to do the setup, running and checking of expectations.
michaeln@chromium.org changed reviewers: + michaeln@chromium.org
some drive by comments about smart ptr usage https://codereview.chromium.org/2049303002/diff/80001/content/browser/utility... File content/browser/utility_process_mojo_client_impl.cc (right): https://codereview.chromium.org/2049303002/diff/80001/content/browser/utility... content/browser/utility_process_mojo_client_impl.cc:14: // Helper ref-counted class that takes care of managing the lifetime of the Generally we like to avoid refcounting. This class doesn't really need to be refcounted given it has a single consumer. It'd be fine to use a std::unique_ptr<Helper> and to call BrowserThread::DeleteSoon(IO, helper_.release). https://codereview.chromium.org/2049303002/diff/80001/content/browser/utility... content/browser/utility_process_mojo_client_impl.cc:37: void set_disable_sandbox() { disable_sandbox_ = true; } Presumably the setter is for use on the main thread. This could be racey, the value is read on the IO thread. https://codereview.chromium.org/2049303002/diff/80001/content/browser/utility... content/browser/utility_process_mojo_client_impl.cc:48: utility_host_ = UtilityProcessHost::Create(nullptr, nullptr)->AsWeakPtr(); What is the reason to use a WeakPtr<> here? It looks like this object strictly owns the utility_host_. Sp long as the object is destructed on the IO thread, the shutdown methods might not be needed at all.
As per jam@ decision, I merged back the the UtilityProcessMojoClient classes into one. Apart for that, the biggest change from the last patch set is the change in lifetime management for class Helper (now a unique_ptr<> instead of being ref-counted) the refactoring of the browser tests. PTAnL. https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... File components/safe_json/safe_json_parser_impl.cc (right): https://codereview.chromium.org/2049303002/diff/20001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.cc:53: io_thread_checker_.DetachFromThread(); On 2016/06/14 00:32:29, Anand Mistry wrote: > I know this was already there, but the DetachFromThread is only needed in the > constructor. You don't need it here or in OnParseDone. Removed. https://codereview.chromium.org/2049303002/diff/60001/components/safe_json/sa... File components/safe_json/safe_json_parser_impl.cc (right): https://codereview.chromium.org/2049303002/diff/60001/components/safe_json/sa... components/safe_json/safe_json_parser_impl.cc:73: const base::Value* value = NULL; On 2016/06/13 08:34:53, Bernhard Bauer wrote: > nullptr Done. https://codereview.chromium.org/2049303002/diff/80001/content/browser/utility... File content/browser/utility_process_mojo_client_browsertest.cc (right): https://codereview.chromium.org/2049303002/diff/80001/content/browser/utility... content/browser/utility_process_mojo_client_browsertest.cc:101: INSTANTIATE_TEST_CASE_P(TestCases, On 2016/06/14 00:32:29, Anand Mistry wrote: > I think this is an abuse of parametertised tests. All the test cases and > expectations are hidden inside common test code. Parameterised tests are > supposed to be for the case where you have a set of tests that need to run under > a set of different environments. i.e. different message loop types. This is just > a case of a set of tests, with a common environment. A better way would be to > have multiple tests, each calling a different service method and setting > different expectations, and common code to do the setup, running and checking of > expectations. I've switched out of parameterized tests. https://codereview.chromium.org/2049303002/diff/80001/content/browser/utility... File content/browser/utility_process_mojo_client_impl.cc (right): https://codereview.chromium.org/2049303002/diff/80001/content/browser/utility... content/browser/utility_process_mojo_client_impl.cc:14: // Helper ref-counted class that takes care of managing the lifetime of the On 2016/06/14 18:37:27, michaeln wrote: > Generally we like to avoid refcounting. This class doesn't really need to be > refcounted given it has a single consumer. > > It'd be fine to use a std::unique_ptr<Helper> and to call > BrowserThread::DeleteSoon(IO, helper_.release). Good idea. Done. https://codereview.chromium.org/2049303002/diff/80001/content/browser/utility... content/browser/utility_process_mojo_client_impl.cc:37: void set_disable_sandbox() { disable_sandbox_ = true; } On 2016/06/14 18:37:26, michaeln wrote: > Presumably the setter is for use on the main thread. This could be racey, the > value is read on the IO thread. The contract of the class is explicit about calling set_disable_sandbox() before calling Start(), which posts the task that will eventually read disable_sandbox_. I'll add a DCHECK(!start_called) so developers can immediately see their mistake if they do this. https://codereview.chromium.org/2049303002/diff/80001/content/browser/utility... content/browser/utility_process_mojo_client_impl.cc:48: utility_host_ = UtilityProcessHost::Create(nullptr, nullptr)->AsWeakPtr(); On 2016/06/14 18:37:27, michaeln wrote: > What is the reason to use a WeakPtr<> here? It looks like this object strictly > owns the utility_host_. Sp long as the object is destructed on the IO thread, > the shutdown methods might not be needed at all. utility_host_ manages its own lifetime. The WeakPtr<> is kept to be able to force a termination if it is still running.
michaeln@chromium.org changed reviewers: - michaeln@chromium.org
https://codereview.chromium.org/2049303002/diff/80001/content/browser/utility... File content/browser/utility_process_mojo_client_impl.cc (right): https://codereview.chromium.org/2049303002/diff/80001/content/browser/utility... content/browser/utility_process_mojo_client_impl.cc:48: utility_host_ = UtilityProcessHost::Create(nullptr, nullptr)->AsWeakPtr(); On 2016/06/14 21:23:32, Patrick Monette wrote: > On 2016/06/14 18:37:27, michaeln wrote: > > What is the reason to use a WeakPtr<> here? It looks like this object strictly > > owns the utility_host_. Sp long as the object is destructed on the IO thread, > > the shutdown methods might not be needed at all. > > utility_host_ manages its own lifetime. The WeakPtr<> is kept to be able to > force a termination if it is still running. i see, thnx, you might want a comment about that to explain the weakptr usage, a code comment around the call to delete utility_host_.get() would do it
LGTM Cool!
https://codereview.chromium.org/2049303002/diff/80001/content/browser/utility... File content/browser/utility_process_mojo_client_impl.cc (right): https://codereview.chromium.org/2049303002/diff/80001/content/browser/utility... content/browser/utility_process_mojo_client_impl.cc:48: utility_host_ = UtilityProcessHost::Create(nullptr, nullptr)->AsWeakPtr(); On 2016/06/14 22:10:50, michaeln wrote: > On 2016/06/14 21:23:32, Patrick Monette wrote: > > On 2016/06/14 18:37:27, michaeln wrote: > > > What is the reason to use a WeakPtr<> here? It looks like this object > strictly > > > owns the utility_host_. Sp long as the object is destructed on the IO > thread, > > > the shutdown methods might not be needed at all. > > > > utility_host_ manages its own lifetime. The WeakPtr<> is kept to be able to > > force a termination if it is still running. > > i see, thnx, you might want a comment about that to explain the weakptr usage, a > code comment around the call to delete utility_host_.get() would do it Done.
jam@ PTAL
Patchset #10 (id:200001) has been deleted
lgtm https://codereview.chromium.org/2049303002/diff/220001/content/public/browser... File content/public/browser/utility_process_mojo_client.h (right): https://codereview.chromium.org/2049303002/diff/220001/content/public/browser... content/public/browser/utility_process_mojo_client.h:23: // Implements a client to a mojo service running on a utility process. Takes nit: Mojo
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, bauerb@chromium.org, amistry@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/2049303002/#ps240001 (title: "s/mojo/Mojo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049303002/240001
Thanks for the reviews! https://codereview.chromium.org/2049303002/diff/220001/content/public/browser... File content/public/browser/utility_process_mojo_client.h (right): https://codereview.chromium.org/2049303002/diff/220001/content/public/browser... content/public/browser/utility_process_mojo_client.h:23: // Implements a client to a mojo service running on a utility process. Takes On 2016/06/17 15:47:34, jam wrote: > nit: Mojo Done twice
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by pmonette@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049303002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, amistry@chromium.org, jam@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2049303002/#ps260001 (title: "Fixed tests with --no-sandbox")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049303002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, amistry@chromium.org, jam@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2049303002/#ps280001 (title: "Fixed android sandbox browser tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049303002/280001
Message was sent while issue was closed.
Description was changed from ========== Add the UtilityProcessMojoClient class and convert SafeJsonParser This class takes care of the mundane job of managing the utility process lifetime and connecting to the remote Mojo service in an effort to make it very easy to add new Mojo services that run on a utility process. As a first example, SafeJsonParser is converted to use this class. BUG=618459 ========== to ========== Add the UtilityProcessMojoClient class and convert SafeJsonParser This class takes care of the mundane job of managing the utility process lifetime and connecting to the remote Mojo service in an effort to make it very easy to add new Mojo services that run on a utility process. As a first example, SafeJsonParser is converted to use this class. BUG=618459 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Add the UtilityProcessMojoClient class and convert SafeJsonParser This class takes care of the mundane job of managing the utility process lifetime and connecting to the remote Mojo service in an effort to make it very easy to add new Mojo services that run on a utility process. As a first example, SafeJsonParser is converted to use this class. BUG=618459 ========== to ========== Add the UtilityProcessMojoClient class and convert SafeJsonParser This class takes care of the mundane job of managing the utility process lifetime and connecting to the remote Mojo service in an effort to make it very easy to add new Mojo services that run on a utility process. As a first example, SafeJsonParser is converted to use this class. BUG=618459 Committed: https://crrev.com/e6f1ea1a8b2145019c3e7a7248ab1fc1d880b069 Cr-Commit-Position: refs/heads/master@{#400522} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/e6f1ea1a8b2145019c3e7a7248ab1fc1d880b069 Cr-Commit-Position: refs/heads/master@{#400522} |
