Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
639233002:
Remote assistance on Chrome OS Part IV - It2MeHost (Closed)
|
Created:
6 years, 2 months ago by kelvinp Modified:
5 years, 7 months ago Reviewers:
Ken Rockot(use gerrit already), jochen (gone - plz use gerrit), Wez, Dmitry Polukhin, rmsousa, agl, Jamie, not at google - send to devlin CC:
chromium-apps-reviews_chromium.org, chromium-reviews, chromoting-reviews_chromium.org, extensions-reviews_chromium.org, mkwst+moarreviews-ipc_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRemote assistance on Chrome OS Part IV - It2MeHost
This CL links the it2me host to the Chrome binary on ChromeOS
behind a flag.
The following changes are made to the it2me host so that it can be run in the browser process.
1. Initializes SSL server sockets and specific CPU media features on ChromeOS startup.
2. Fixes a crash in it2me shutdown by making It2meHost owns the ChromotingHostContext.
3. Replace the blocking shutdown wait on PolicyWatcher with a callback.
Implements policy_watcher on ChromeOS using policy services.
4. Re-use existing threads, url request context getters and policy service on ChromeOS.
5. Fixed a incorrect DCHECK regarding the color format of the frames captured on ChromeOS.
BUG=334087
Committed: https://crrev.com/54dde6f02d121ff745e66b57205583087ff720ec
Cr-Commit-Position: refs/heads/master@{#302034}
Committed: https://crrev.com/561074cfd46c253dcdc456fdc63693aff4d1be32
Cr-Commit-Position: refs/heads/master@{#302162}
Patch Set 1 : #
Total comments: 3
Patch Set 2 : DEPS clean up and ChromotingHostContext clean up #
Total comments: 7
Patch Set 3 : Move DEPS to remoting/host #
Total comments: 42
Patch Set 4 : Address feedbacks #
Total comments: 87
Patch Set 5 : Address Wez's feedbacks #Patch Set 6 : Fix typo #Patch Set 7 : Merge with ToT #Patch Set 8 : ChromotingHostContext cleanup #
Total comments: 44
Patch Set 9 : AuraDesktopCapturer clean up #
Total comments: 24
Patch Set 10 : #
Total comments: 23
Patch Set 11 : #
Total comments: 1
Patch Set 12 : Rebase on ToT #Patch Set 13 : Fix build error on ozone #Messages
Total messages: 75 (21 generated)
kelvinp@chromium.org changed reviewers: + kalman@chromium.org
kelvinp@chromium.org changed reviewers: + jochen@chromium.org, rmsousa@chromium.org, sergeyu@chromium.org
Patchset #1 (id:1) has been deleted
PTAL. kalman@ Can I have a CL for the following files: chrome/browser/DEPS chrome/browser/extensions/api/DEPS chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc jochen@ Can I have a CL for the dependencies added to DEPS: '+chrome/browser', '+components/policy/core/common', '+content/public/browser',
Could you put in a BUG? I'm not the best person to review this because I have no context, is there somebody else working on it2me that can?
this is a cyclic dependency (remoting -> chrome browser -> remoting), just from looking at the DEPS files not lgtm, sorry https://codereview.chromium.org/639233002/diff/20001/remoting/DEPS File remoting/DEPS (right): https://codereview.chromium.org/639233002/diff/20001/remoting/DEPS#newcode26 remoting/DEPS:26: "+components/policy/core/common", why are those in remoting/ and not in remoting/host/ ?
On 9 Oct 2014 06:02, <kelvinp@chromium.org> wrote: > > Reviewers: kalman, rmsousa, Sergey Ulanov, jochen, > > Message: > PTAL. > > kalman@ > Can I have a CL for the following files: > chrome/browser/DEPS > chrome/browser/extensions/api/DEPS > chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc > > jochen@ > Can I have a CL for the dependencies added to DEPS: > '+chrome/browser', > '+components/policy/core/common', > '+content/public/browser', > > Description: > Remote assistance on Chrome OS Part IV - It2MeHost > > This CL links the it2me host to the Chrome binary on ChromeOS > behind a flag. > The following changes are made to the it2me host so that it can be run in the > browser process. > 1. Initializes SSL server sockets and specific CPU media features on ChromeOS > startup. > 2. Fixes a crash in it2me shutdown by making ChromotingHostContext ref-counted. Don't do this please. There should be no reason to ref-count that structure! > 3. Replace the blocking shutdown wait on PolicyWatcher with a callback. > 4. Re-use existing threads, url request context getters and policy service on > ChromeOS. > > > Please review this at https://codereview.chromium.org/639233002/ > > SVN Base: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+551, -143 lines): > M chrome/browser/DEPS > M chrome/browser/chrome_content_browser_client.cc > M chrome/browser/extensions/api/DEPS > M chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc > M chrome/chrome_browser_extensions.gypi > M remoting/DEPS > M remoting/host/basic_desktop_environment.cc > M remoting/host/chromeos/aura_desktop_capturer.cc > M remoting/host/chromeos/aura_desktop_capturer_unittest.cc > A remoting/host/chromeos/browser_mock_objects.cc > A remoting/host/chromeos/chrome_browser_main_extra_parts_remoting.h > A + remoting/host/chromeos/chrome_browser_main_extra_parts_remoting.cc > M remoting/host/chromoting_host_context.h > M remoting/host/chromoting_host_context.cc > A remoting/host/chromoting_host_context_impl.h > A remoting/host/chromoting_host_context_impl.cc > A remoting/host/chromoting_host_context_impl_chromeos.cc > M remoting/host/chromoting_host_context_unittest.cc > A remoting/host/continue_window_chromeos.cc > A remoting/host/disconnect_window_chromeos.cc > M remoting/host/it2me/it2me_host.h > M remoting/host/it2me/it2me_host.cc > M remoting/host/it2me/it2me_native_messaging_host.h > M remoting/host/it2me/it2me_native_messaging_host.cc > M remoting/host/it2me/it2me_native_messaging_host_main.cc > M remoting/host/it2me/it2me_native_messaging_host_unittest.cc > M remoting/host/policy_hack/policy_watcher.h > M remoting/host/policy_hack/policy_watcher.cc > A remoting/host/policy_hack/policy_watcher_chromeos.cc > M remoting/host/policy_hack/policy_watcher_unittest.cc > M remoting/host/remoting_me2me_host.cc > M remoting/remoting_host.gypi > M remoting/remoting_test.gypi > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 9 Oct 2014 12:52, "Wez" <wez@google.com> wrote: > > On 9 Oct 2014 06:02, <kelvinp@chromium.org> wrote: > > > > Reviewers: kalman, rmsousa, Sergey Ulanov, jochen, > > > > Message: > > PTAL. > > > > kalman@ > > Can I have a CL for the following files: > > chrome/browser/DEPS > > chrome/browser/extensions/api/DEPS > > chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc > > > > jochen@ > > Can I have a CL for the dependencies added to DEPS: > > '+chrome/browser', > > '+components/policy/core/common', > > '+content/public/browser', > > > > Description: > > Remote assistance on Chrome OS Part IV - It2MeHost > > > > This CL links the it2me host to the Chrome binary on ChromeOS > > behind a flag. > > The following changes are made to the it2me host so that it can be run > in the > > browser process. > > 1. Initializes SSL server sockets and specific CPU media features on > ChromeOS > > startup. > > 2. Fixes a crash in it2me shutdown by making ChromotingHostContext > ref-counted. > > Don't do this please. There should be no reason to ref-count that > structure! > > > 3. Replace the blocking shutdown wait on PolicyWatcher with a callback. > > 4. Re-use existing threads, url request context getters and policy > service on > > ChromeOS. > > > > > > Please review this at https://codereview.chromium.org/639233002/ > > > > SVN Base: https://chromium.googlesource.com/chromium/src.git@master > > > > Affected files (+551, -143 lines): > > M chrome/browser/DEPS > > M chrome/browser/chrome_content_browser_client.cc > > M chrome/browser/extensions/api/DEPS > > M > chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc > > M chrome/chrome_browser_extensions.gypi > > M remoting/DEPS > > M remoting/host/basic_desktop_environment.cc > > M remoting/host/chromeos/aura_desktop_capturer.cc > > M remoting/host/chromeos/aura_desktop_capturer_unittest.cc > > A remoting/host/chromeos/browser_mock_objects.cc > > A remoting/host/chromeos/chrome_browser_main_extra_parts_remoting.h > > A + remoting/host/chromeos/chrome_browser_main_extra_parts_remoting.cc > > M remoting/host/chromoting_host_context.h > > M remoting/host/chromoting_host_context.cc > > A remoting/host/chromoting_host_context_impl.h > > A remoting/host/chromoting_host_context_impl.cc > > A remoting/host/chromoting_host_context_impl_chromeos.cc > > M remoting/host/chromoting_host_context_unittest.cc > > A remoting/host/continue_window_chromeos.cc > > A remoting/host/disconnect_window_chromeos.cc > > M remoting/host/it2me/it2me_host.h > > M remoting/host/it2me/it2me_host.cc > > M remoting/host/it2me/it2me_native_messaging_host.h > > M remoting/host/it2me/it2me_native_messaging_host.cc > > M remoting/host/it2me/it2me_native_messaging_host_main.cc > > M remoting/host/it2me/it2me_native_messaging_host_unittest.cc > > M remoting/host/policy_hack/policy_watcher.h > > M remoting/host/policy_hack/policy_watcher.cc > > A remoting/host/policy_hack/policy_watcher_chromeos.cc > > M remoting/host/policy_hack/policy_watcher_unittest.cc > > M remoting/host/remoting_me2me_host.cc > > M remoting/remoting_host.gypi > > M remoting/remoting_test.gypi > > > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
@Wez. The It2MeNativeMessagingHost currently owns the ChromotingHostContext and the It2meHost(which is ref-counted). The It2MeHost also has a weak pointer to the ChromotingHostContext. During shutdown, the It2MeNativeMessagingHost destroys the underlying ChromotingHostContext, but it doesn't decrement the ref-count of the It2meHost, so the It2meHost can continues its shutdown on the network thread(to post a session-terminate message back to the client). At this point, the It2MeHost is referencing a dangling reference of the ChromotingHostcontext. I think one option that I have is to make the It2MeHost to own the ChromotingHostContext.
On 2014/10/09 18:56:33, kelvinp wrote:
> @Wez. The It2MeNativeMessagingHost currently owns the ChromotingHostContext
and
> the It2meHost(which is ref-counted). The It2MeHost also has a weak pointer to
> the ChromotingHostContext.
^^^^^^^^^^^^
As far as I can tell It2meHost is actually use a bare pointer, not a WeakPtr,
which would explain the problems you're having.
> During shutdown, the It2MeNativeMessagingHost
> destroys the underlying ChromotingHostContext, but it doesn't decrement the
> ref-count of the It2meHost, so the It2meHost can continues its shutdown on the
> network thread(to post a session-terminate message back to the client). At
this
> point, the It2MeHost is referencing a dangling reference of the
> ChromotingHostcontext.
> I think one option that I have is to make the It2MeHost to own the
> ChromotingHostContext.
Ideally nobody should own the context; it's designed to be created & the various
task runners passed explicitly to the various classes during initialization, and
once initialization is done it is just dropped.
In the HostProcess we don't follow that pattern, though, so HostProcess has to
be passed the context and own it, and is responsible for tearing it down at the
right time.
On 2014/10/09 19:41:12, Wez wrote: > On 2014/10/09 18:56:33, kelvinp wrote: > > @Wez. The It2MeNativeMessagingHost currently owns the ChromotingHostContext > and > > the It2meHost(which is ref-counted). The It2MeHost also has a weak pointer to > > the ChromotingHostContext. > ^^^^^^^^^^^^ > > As far as I can tell It2meHost is actually use a bare pointer, not a WeakPtr, > which would explain the problems you're having. > > > During shutdown, the It2MeNativeMessagingHost > > destroys the underlying ChromotingHostContext, but it doesn't decrement the > > ref-count of the It2meHost, so the It2meHost can continues its shutdown on the > > network thread(to post a session-terminate message back to the client). At > this > > point, the It2MeHost is referencing a dangling reference of the > > ChromotingHostcontext. > > I think one option that I have is to make the It2MeHost to own the > > ChromotingHostContext. > > Ideally nobody should own the context; it's designed to be created & the various > task runners passed explicitly to the various classes during initialization, and > once initialization is done it is just dropped. > > In the HostProcess we don't follow that pattern, though, so HostProcess has to > be passed the context and own it, and is responsible for tearing it down at the > right time. If I understand your suggestion correct, basically I can modify It2Me host to simply cache the resources that it needs from the ChromotingHostContext (mainly the task runners, which are ref-counted anyways) instead of owning the context itself. I agree that once initialization is done, it is no longer required to keep the context alive. However, it would require some deep surgery to determine when initialization is done, especially when we re-create the It2MeHost on each connection request. So I will will just let the It2MeNativeMessagingHost own the host context and destroy it upon shutdown.
On 2014/10/09 21:17:52, kelvinp wrote: > On 2014/10/09 19:41:12, Wez wrote: > > On 2014/10/09 18:56:33, kelvinp wrote: > > > @Wez. The It2MeNativeMessagingHost currently owns the ChromotingHostContext > > and > > > the It2meHost(which is ref-counted). The It2MeHost also has a weak pointer > to > > > the ChromotingHostContext. > > ^^^^^^^^^^^^ > > > > As far as I can tell It2meHost is actually use a bare pointer, not a WeakPtr, > > which would explain the problems you're having. > > > > > During shutdown, the It2MeNativeMessagingHost > > > destroys the underlying ChromotingHostContext, but it doesn't decrement the > > > ref-count of the It2meHost, so the It2meHost can continues its shutdown on > the > > > network thread(to post a session-terminate message back to the client). At > > this > > > point, the It2MeHost is referencing a dangling reference of the > > > ChromotingHostcontext. > > > I think one option that I have is to make the It2MeHost to own the > > > ChromotingHostContext. > > > > Ideally nobody should own the context; it's designed to be created & the > various > > task runners passed explicitly to the various classes during initialization, > and > > once initialization is done it is just dropped. > > > > In the HostProcess we don't follow that pattern, though, so HostProcess has to > > be passed the context and own it, and is responsible for tearing it down at > the > > right time. > > If I understand your suggestion correct, basically I can modify It2Me host to > simply > cache the resources that it needs from the ChromotingHostContext (mainly the > task runners, which are ref-counted anyways) instead of owning the context > itself. Yes. > I agree that once initialization is done, it is no longer required to keep the > context > alive. However, it would require some deep surgery to determine when > initialization is > done, especially when we re-create the It2MeHost on each connection request. Initialization is done when the initializer or constructor to which the context was passed has returned to the caller - at that point you can tear down the context safely. The init/ctor method should have pulled out all the stuff that it, and any sub-components or other dependencies, need, before returning. > So I will will just let the It2MeNativeMessagingHost own the host context and > destroy it upon shutdown. What I'm saying is: 1. Have the code that creates It2meHost instances "own" the context. 2. Have the It2meHost take references to the task runners etc that it, or sub-components need.
kelvinp@chromium.org changed reviewers: + dpolukhin@chromium.org, jamiewalch@chromium.org - sergeyu@chromium.org
PTAL @Wez. I have changed the object lifetime of the ChromotingHostContext as discussed. @jochen. Good point. I have removed the dependency on chrome/browser from remoting/host. The new dependency graph is simply chrome/browser/extensions -> remoting/host/it2me -> content/public/browser @kalman. Bug added to the CL. Ideally, sergeyu@ would be the best person to review this chang. However, he is on paternity leave right now so I think you are my best bet :) In short, this CL creates an extra entry in the BuiltInHost table to create the it2meHost behind a chrome flag. Let me know if you have more questions. dpolukhin@ Can I have an CL on the changes under: chrome/browser/chromeos/chrome_browser_main_chromeos.cc In short, in order to support remote assistance on Chrome OS. We would need to initialize SSL Server sockets on startup. Please let me know if there is a better place to add the initialization logic.
LGTM for change in chrome/browser/chromeos/chrome_browser_main_chromeos.cc
https://codereview.chromium.org/639233002/diff/90001/remoting/DEPS File remoting/DEPS (right): https://codereview.chromium.org/639233002/diff/90001/remoting/DEPS#newcode3 remoting/DEPS:3: "+content/public/browser", shouldn't this be in remoting/host/DEPS https://codereview.chromium.org/639233002/diff/90001/remoting/DEPS#newcode4 remoting/DEPS:4: "+components/policy/core/common", shouldn't these two be in remoting/host/DEPS? https://codereview.chromium.org/639233002/diff/90001/remoting/DEPS#newcode4 remoting/DEPS:4: "+components/policy/core/common", shouldn't these two be in remoting/host/DEPS? https://codereview.chromium.org/639233002/diff/90001/remoting/DEPS#newcode4 remoting/DEPS:4: "+components/policy/core/common", shouldn't these two be in remoting/host/DEPS?
PTAL https://codereview.chromium.org/639233002/diff/90001/remoting/DEPS File remoting/DEPS (right): https://codereview.chromium.org/639233002/diff/90001/remoting/DEPS#newcode3 remoting/DEPS:3: "+content/public/browser", On 2014/10/13 08:19:57, jochen wrote: > shouldn't this be in remoting/host/DEPS Done. https://codereview.chromium.org/639233002/diff/90001/remoting/DEPS#newcode4 remoting/DEPS:4: "+components/policy/core/common", On 2014/10/13 08:19:56, jochen wrote: > shouldn't these two be in remoting/host/DEPS? Done. https://codereview.chromium.org/639233002/diff/90001/remoting/DEPS#newcode4 remoting/DEPS:4: "+components/policy/core/common", On 2014/10/13 08:19:58, jochen wrote: > shouldn't these two be in remoting/host/DEPS? Done.
kalman@chromium.org changed reviewers: + rockot@chromium.org
me -> rockot
On 2014/10/13 22:16:08, kalman wrote: > me -> rockot extensions stuff lgtm
This looks generally good, depending on the answer to the first 2 comments. Please wait for a review from Wez on the pointer ownership changes. https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.cc (left): https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:43: DCHECK_EQ(kRGBA_8888_SkColorType, bitmap->info().colorType()) Out of curiosity: was this broken before? Are we the first feature to exercise this code? https://codereview.chromium.org/639233002/diff/320001/remoting/host/continue_... File remoting/host/continue_window_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:9: class ContinueWindowAura : public ContinueWindow { This isn't really an "Aura" implementation, just an empty implementation. If this is supposed to remain empty, please name it appropriately, and add a code comment explaining why these UI elements aren't needed in Chrome OS. If you do plan to add Aura implementations of this later on, please add a TODO to that effect. Also, please explain why it is safe to land this change without the UI elements (e.g. if the it2me code is currently guaranteed to be unreachable) https://codereview.chromium.org/639233002/diff/320001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1417: void HostProcess::PostPolicyWatcherShutdown() { nit: OnPolicyWatcherShutdown. "Post" is an ambiguous term - we typically use it with its verb meaning (i.e. to post something to a task), so to use it with its adverb meaning (i.e. this will happen after the policy watcher is shutdown) is confusing.
https://codereview.chromium.org/639233002/diff/320001/chrome/browser/extensio... File chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/320001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:35: // used for testing by ExtensionApiTest::NativeMessagingBasic. Is this comment still correct? It looks like it now supports more than just echo. https://codereview.chromium.org/639233002/diff/320001/remoting/host/basic_des... File remoting/host/basic_desktop_environment.cc (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/basic_des... remoting/host/basic_desktop_environment.cc:68: #if defined(OS_CHROMEOS) This doesn't feel like the right place for this ifdef. ScreenCapture::Create is a per-platform static method; it feels like we should have a ChromeOS implementation that returns an AuraDesktopCapturer, in which case this file wouldn't need to be changed. https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.cc (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:44: << "DesktopFrame objects always hold BGRA data."; There seem to be a couple of changes that flip the endianness, but there's nothing in the CL description. If it's an unrelated bugfix, consider moving it to another CL. If not, please mention it in the description. https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromotin... File remoting/host/chromoting_host_context.cc (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:37: context->audio_task_runner_ = AutoThread::CreateWithType( This code reads a lot like a ctor. Could you add extra parameters to the ctor for the various threads? It would add a compiler check to ensure that both Create methods are supplying the same threads etc. I think the creation of the audio, input and encode threads could also be left in the ctor, since the only difference is the thread they use to join. You could either make that another ctor parameter, or perhaps use the file thread to join on all platforms (check with someone who knows the architecture better than I do first; it might not work if you do that). https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:64: // on the UI thread of the browser process. This comment implies that CreateOnChromeOS must also be called on the UI thread, which should probably be mentioned in the header. https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:98: // Thread joining is blocking and it can only be done on threads that allows s/allows/allow/ https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromotin... File remoting/host/chromoting_host_context.h (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:18: } // namespace net s/net/policy/ https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:30: // Creates threads and URLRequestContextGetter for use by a host. The previous sense was more consistent with the other comments in this file. https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:78: // Verify that all threads has started. s/has/have/ https://codereview.chromium.org/639233002/diff/320001/remoting/host/continue_... File remoting/host/continue_window_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:29: void ContinueWindowAura::ShowUi() { These empty methods should all have NOTIMPLEMENTED(). https://codereview.chromium.org/639233002/diff/320001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:248: task_runner_->PostTask(FROM_HERE, Why has this changed? https://codereview.chromium.org/639233002/diff/320001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:126: // Called when |policy_watcher_| has stopped listening for changes so that it s/so that/and/ https://codereview.chromium.org/639233002/diff/320001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:127: // safe to delete the |policy_watcher_| object. s/safe/is safe/ https://codereview.chromium.org/639233002/diff/320001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:143: scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_; It's not clear to me why this new member is needed. It seems only to be used to save a call to host_context_->network_task_runner() and it makes what would otherwise be a very small diff to this file much harder to review. https://codereview.chromium.org/639233002/diff/320001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_native_messaging_host.h (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/it2me/it2... remoting/host/it2me/it2me_native_messaging_host.h:36: scoped_ptr<It2MeHostFactory> factory); Having both a Create method and a ctor doesn't seem right here. Could you have Create/CreateForChromeOS like you did for the host context? https://codereview.chromium.org/639233002/diff/320001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:38: scoped_ptr<base::DictionaryValue> GetPolicyValues(const PolicyMap& map); I don't think this needs to be a separate method, unless you think you're going to use it elsewhere. If it was inlined in Reload, you wouldn't need a DCHECK on the result (because it would clearly be the result of "new"), and the reader wouldn't have to think about the circumstances under which it might be null. https://codereview.chromium.org/639233002/diff/320001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:53: const PolicyMap& previous, Indentation. https://codereview.chromium.org/639233002/diff/320001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (left): https://codereview.chromium.org/639233002/diff/320001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:387: task_runner->DeleteSoon(FROM_HERE, context_.release()); Why has this been deleted?
Apologies for the delay; will take a look ASAP! On 14 October 2014 02:18, <jamiewalch@chromium.org> wrote: > > https://codereview.chromium.org/639233002/diff/320001/ > chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc > File > chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc > (right): > > https://codereview.chromium.org/639233002/diff/320001/ > chrome/browser/extensions/api/messaging/native_message_host_ > chromeos.cc#newcode35 > chrome/browser/extensions/api/messaging/native_message_host_ > chromeos.cc:35: > // used for testing by ExtensionApiTest::NativeMessagingBasic. > Is this comment still correct? It looks like it now supports more than > just echo. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/basic_desktop_environment.cc > File remoting/host/basic_desktop_environment.cc (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/basic_desktop_environment.cc#newcode68 > remoting/host/basic_desktop_environment.cc:68: #if defined(OS_CHROMEOS) > This doesn't feel like the right place for this ifdef. > ScreenCapture::Create is a per-platform static method; it feels like we > should have a ChromeOS implementation that returns an > AuraDesktopCapturer, in which case this file wouldn't need to be > changed. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromeos/aura_desktop_capturer.cc > File remoting/host/chromeos/aura_desktop_capturer.cc (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromeos/aura_desktop_capturer.cc#newcode44 > remoting/host/chromeos/aura_desktop_capturer.cc:44: << "DesktopFrame > objects always hold BGRA data."; > There seem to be a couple of changes that flip the endianness, but > there's nothing in the CL description. If it's an unrelated bugfix, > consider moving it to another CL. If not, please mention it in the > description. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromoting_host_context.cc > File remoting/host/chromoting_host_context.cc (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromoting_host_context.cc#newcode37 > remoting/host/chromoting_host_context.cc:37: context->audio_task_runner_ > = AutoThread::CreateWithType( > This code reads a lot like a ctor. Could you add extra parameters to the > ctor for the various threads? It would add a compiler check to ensure > that both Create methods are supplying the same threads etc. > > I think the creation of the audio, input and encode threads could also > be left in the ctor, since the only difference is the thread they use to > join. You could either make that another ctor parameter, or perhaps use > the file thread to join on all platforms (check with someone who knows > the architecture better than I do first; it might not work if you do > that). > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromoting_host_context.cc#newcode64 > remoting/host/chromoting_host_context.cc:64: // on the UI thread of the > browser process. > This comment implies that CreateOnChromeOS must also be called on the UI > thread, which should probably be mentioned in the header. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromoting_host_context.cc#newcode98 > remoting/host/chromoting_host_context.cc:98: // Thread joining is > blocking and it can only be done on threads that allows > s/allows/allow/ > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromoting_host_context.h > File remoting/host/chromoting_host_context.h (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromoting_host_context.h#newcode18 > remoting/host/chromoting_host_context.h:18: } // namespace net > s/net/policy/ > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromoting_host_context.h#newcode30 > remoting/host/chromoting_host_context.h:30: // Creates threads and > URLRequestContextGetter for use by a host. > The previous sense was more consistent with the other comments in this > file. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromoting_host_context.h#newcode78 > remoting/host/chromoting_host_context.h:78: // Verify that all threads > has started. > s/has/have/ > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/continue_window_chromeos.cc > File remoting/host/continue_window_chromeos.cc (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/continue_window_chromeos.cc#newcode29 > remoting/host/continue_window_chromeos.cc:29: void > ContinueWindowAura::ShowUi() { > These empty methods should all have NOTIMPLEMENTED(). > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/it2me/it2me_host.cc > File remoting/host/it2me/it2me_host.cc (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/it2me/it2me_host.cc#newcode248 > remoting/host/it2me/it2me_host.cc:248: task_runner_->PostTask(FROM_HERE, > Why has this changed? > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/it2me/it2me_host.h > File remoting/host/it2me/it2me_host.h (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/it2me/it2me_host.h#newcode126 > remoting/host/it2me/it2me_host.h:126: // Called when |policy_watcher_| > has stopped listening for changes so that it > s/so that/and/ > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/it2me/it2me_host.h#newcode127 > remoting/host/it2me/it2me_host.h:127: // safe to delete the > |policy_watcher_| object. > s/safe/is safe/ > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/it2me/it2me_host.h#newcode143 > remoting/host/it2me/it2me_host.h:143: > scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_; > It's not clear to me why this new member is needed. It seems only to be > used to save a call to host_context_->network_task_runner() and it makes > what would otherwise be a very small diff to this file much harder to > review. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/it2me/it2me_native_messaging_host.h > File remoting/host/it2me/it2me_native_messaging_host.h (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/it2me/it2me_native_messaging_host.h#newcode36 > remoting/host/it2me/it2me_native_messaging_host.h:36: > scoped_ptr<It2MeHostFactory> factory); > Having both a Create method and a ctor doesn't seem right here. Could > you have Create/CreateForChromeOS like you did for the host context? > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/policy_hack/policy_watcher_chromeos.cc > File remoting/host/policy_hack/policy_watcher_chromeos.cc (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/policy_hack/policy_watcher_chromeos.cc#newcode38 > remoting/host/policy_hack/policy_watcher_chromeos.cc:38: > scoped_ptr<base::DictionaryValue> GetPolicyValues(const PolicyMap& map); > I don't think this needs to be a separate method, unless you think > you're going to use it elsewhere. If it was inlined in Reload, you > wouldn't need a DCHECK on the result (because it would clearly be the > result of "new"), and the reader wouldn't have to think about the > circumstances under which it might be null. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/policy_hack/policy_watcher_chromeos.cc#newcode53 > remoting/host/policy_hack/policy_watcher_chromeos.cc:53: const > PolicyMap& previous, > Indentation. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/remoting_me2me_host.cc > File remoting/host/remoting_me2me_host.cc (left): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/remoting_me2me_host.cc#oldcode387 > remoting/host/remoting_me2me_host.cc:387: > task_runner->DeleteSoon(FROM_HERE, context_.release()); > Why has this been deleted? > > https://codereview.chromium.org/639233002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
deps lgtm
Patchset #4 (id:420001) has been deleted
kelvinp@chromium.org changed reviewers: + agl@chromium.org, evan@chromium.org
PTAL @agl I have added dependencies to net/url_request in remoting/DEPS. Can I have an LGTM for the change? @evan. I have added a ScopedAllowedIO in AutoThread.cc, to allow joining and I would like to have some feedback from you. The remote assistance host creates threads (e.g. encoding) on the UI thread of the browser process, which needs to be deleted when the host shutdown. The remote assistance host uses AutoThread as its threading primitive, which will only shut itself down when there are no more outstanding references to it. As a result, it is guaranteed that at the point of shutdown, there will be no outstanding tasks in the task_runner and the join will not be blocking. https://codereview.chromium.org/639233002/diff/320001/chrome/browser/extensio... File chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/320001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:35: // used for testing by ExtensionApiTest::NativeMessagingBasic. On 2014/10/14 01:18:41, Jamie wrote: > Is this comment still correct? It looks like it now supports more than just > echo. Comment updated https://codereview.chromium.org/639233002/diff/320001/remoting/host/basic_des... File remoting/host/basic_desktop_environment.cc (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/basic_des... remoting/host/basic_desktop_environment.cc:68: #if defined(OS_CHROMEOS) On 2014/10/14 01:18:41, Jamie wrote: > This doesn't feel like the right place for this ifdef. ScreenCapture::Create is > a per-platform static method; it feels like we should have a ChromeOS > implementation that returns an AuraDesktopCapturer, in which case this file > wouldn't need to be changed. Done. https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.cc (left): https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:43: DCHECK_EQ(kRGBA_8888_SkColorType, bitmap->info().colorType()) On 2014/10/14 00:18:25, rmsousa wrote: > Out of curiosity: was this broken before? Are we the first feature to exercise > this code? Yes, I created this file two weeks ago but it is not being exercised other than unittest. I create the DCHECK based on the comments on webrtc::DeskopFrame, which is apparently incorrect. https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.cc (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:44: << "DesktopFrame objects always hold BGRA data."; On 2014/10/14 01:18:41, Jamie wrote: > There seem to be a couple of changes that flip the endianness, but there's > nothing in the CL description. If it's an unrelated bugfix, consider moving it > to another CL. If not, please mention it in the description. This is required otherwise the DCHECK will fail after the remoting session is connected. I will update the CL description. https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromotin... File remoting/host/chromoting_host_context.cc (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:37: context->audio_task_runner_ = AutoThread::CreateWithType( On 2014/10/14 01:18:41, Jamie wrote: > This code reads a lot like a ctor. Could you add extra parameters to the ctor > for the various threads? It would add a compiler check to ensure that both > Create methods are supplying the same threads etc. > > I think the creation of the audio, input and encode threads could also be left > in the ctor, since the only difference is the thread they use to join. You could > either make that another ctor parameter, or perhaps use the file thread to join > on all platforms (check with someone who knows the architecture better than I do > first; it might not work if you do that). I spoke to wez. He suggested instead of joining on the FILE Thread. We should add a ScopedAllowIO on auto thread instead. https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:64: // on the UI thread of the browser process. On 2014/10/14 01:18:41, Jamie wrote: > This comment implies that CreateOnChromeOS must also be called on the UI thread, > which should probably be mentioned in the header. Done. https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:98: // Thread joining is blocking and it can only be done on threads that allows On 2014/10/14 01:18:41, Jamie wrote: > s/allows/allow/ Done. https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromotin... File remoting/host/chromoting_host_context.h (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:18: } // namespace net On 2014/10/14 01:18:41, Jamie wrote: > s/net/policy/ Done. https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:30: // Creates threads and URLRequestContextGetter for use by a host. On 2014/10/14 01:18:42, Jamie wrote: > The previous sense was more consistent with the other comments in this file. Done. https://codereview.chromium.org/639233002/diff/320001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:78: // Verify that all threads has started. On 2014/10/14 01:18:42, Jamie wrote: > s/has/have/ Done. https://codereview.chromium.org/639233002/diff/320001/remoting/host/continue_... File remoting/host/continue_window_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:9: class ContinueWindowAura : public ContinueWindow { On 2014/10/14 00:18:25, rmsousa wrote: > This isn't really an "Aura" implementation, just an empty implementation. > > If this is supposed to remain empty, please name it appropriately, and add a > code comment explaining why these UI elements aren't needed in Chrome OS. > > If you do plan to add Aura implementations of this later on, please add a TODO > to that effect. Also, please explain why it is safe to land this change without > the UI elements (e.g. if the it2me code is currently guaranteed to be > unreachable) Done. https://codereview.chromium.org/639233002/diff/320001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:29: void ContinueWindowAura::ShowUi() { On 2014/10/14 01:18:42, Jamie wrote: > These empty methods should all have NOTIMPLEMENTED(). Those function will be called on ChromeOS and can't have NOTIMPLEMENTED(). I will add a TODO statement https://codereview.chromium.org/639233002/diff/320001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:248: task_runner_->PostTask(FROM_HERE, On 2014/10/14 01:18:42, Jamie wrote: > Why has this changed? As per wez, comment, we are trying to take reference on the task_runner of the ChromotingHostContext instead of holding on to the host. https://codereview.chromium.org/639233002/diff/320001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:126: // Called when |policy_watcher_| has stopped listening for changes so that it On 2014/10/14 01:18:42, Jamie wrote: > s/so that/and/ Done. https://codereview.chromium.org/639233002/diff/320001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:127: // safe to delete the |policy_watcher_| object. On 2014/10/14 01:18:42, Jamie wrote: > s/safe/is safe/ Done. https://codereview.chromium.org/639233002/diff/320001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:143: scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_; On 2014/10/14 01:18:42, Jamie wrote: > It's not clear to me why this new member is needed. It seems only to be used to > save a call to host_context_->network_task_runner() and it makes what would > otherwise be a very small diff to this file much harder to review. The ChromotingHostContext is owned by the It2meNativeMessaginHost and it will be deleted when It2meNativeMessagingHost is destroyed. During shutdown, It2meHost outlives It2meNativeMessagingHost so that I can clean itself up on the network_task_runner. We can no longer get the network_task_runner from the host_context at this point as it is already destroyed. https://codereview.chromium.org/639233002/diff/320001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_native_messaging_host.h (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/it2me/it2... remoting/host/it2me/it2me_native_messaging_host.h:36: scoped_ptr<It2MeHostFactory> factory); On 2014/10/14 01:18:42, Jamie wrote: > Having both a Create method and a ctor doesn't seem right here. Could you have > Create/CreateForChromeOS like you did for the host context? Done. https://codereview.chromium.org/639233002/diff/320001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:38: scoped_ptr<base::DictionaryValue> GetPolicyValues(const PolicyMap& map); On 2014/10/14 01:18:42, Jamie wrote: > I don't think this needs to be a separate method, unless you think you're going > to use it elsewhere. If it was inlined in Reload, you wouldn't need a DCHECK on > the result (because it would clearly be the result of "new"), and the reader > wouldn't have to think about the circumstances under which it might be null. Done. https://codereview.chromium.org/639233002/diff/320001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:53: const PolicyMap& previous, On 2014/10/14 01:18:42, Jamie wrote: > Indentation. Done. https://codereview.chromium.org/639233002/diff/320001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (left): https://codereview.chromium.org/639233002/diff/320001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:387: task_runner->DeleteSoon(FROM_HERE, context_.release()); On 2014/10/14 01:18:42, Jamie wrote: > Why has this been deleted? Good catch https://codereview.chromium.org/639233002/diff/320001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/639233002/diff/320001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1417: void HostProcess::PostPolicyWatcherShutdown() { On 2014/10/14 00:18:25, rmsousa wrote: > nit: OnPolicyWatcherShutdown. > "Post" is an ambiguous term - we typically use it with its verb meaning (i.e. to > post something to a task), so to use it with its adverb meaning (i.e. this will > happen after the policy watcher is shutdown) is confusing. Done.
Hey, I haven't worked on Chrome in a long time, you'll probably need someone else to review your ScopedAllowIO change. Maybe jam@. On Wed, Oct 15, 2014 at 4:03 PM, <kelvinp@chromium.org> wrote: > PTAL > > @agl > I have added dependencies to net/url_request in remoting/DEPS. > Can I have an LGTM for the change? > > @evan. > I have added a ScopedAllowedIO in AutoThread.cc, to allow joining and I > would > like to have some feedback from you. > The remote assistance host creates threads (e.g. encoding) on the UI > thread of > the browser process, which needs to be deleted when the host shutdown. > The remote assistance host uses AutoThread as its threading primitive, > which > will only shut itself down when there are no more outstanding references > to it. > As a result, it is guaranteed that at the point of shutdown, there will be > no > outstanding tasks in the task_runner and the join will not be blocking. > > > > > > https://codereview.chromium.org/639233002/diff/320001/ > chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc > File > chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc > (right): > > https://codereview.chromium.org/639233002/diff/320001/ > chrome/browser/extensions/api/messaging/native_message_host_ > chromeos.cc#newcode35 > chrome/browser/extensions/api/messaging/native_message_host_ > chromeos.cc:35: > // used for testing by ExtensionApiTest::NativeMessagingBasic. > On 2014/10/14 01:18:41, Jamie wrote: > >> Is this comment still correct? It looks like it now supports more than >> > just > >> echo. >> > > Comment updated > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/basic_desktop_environment.cc > File remoting/host/basic_desktop_environment.cc (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/basic_desktop_environment.cc#newcode68 > remoting/host/basic_desktop_environment.cc:68: #if defined(OS_CHROMEOS) > On 2014/10/14 01:18:41, Jamie wrote: > >> This doesn't feel like the right place for this ifdef. >> > ScreenCapture::Create is > >> a per-platform static method; it feels like we should have a ChromeOS >> implementation that returns an AuraDesktopCapturer, in which case this >> > file > >> wouldn't need to be changed. >> > > Done. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromeos/aura_desktop_capturer.cc > File remoting/host/chromeos/aura_desktop_capturer.cc (left): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromeos/aura_desktop_capturer.cc#oldcode43 > remoting/host/chromeos/aura_desktop_capturer.cc:43: > DCHECK_EQ(kRGBA_8888_SkColorType, bitmap->info().colorType()) > On 2014/10/14 00:18:25, rmsousa wrote: > >> Out of curiosity: was this broken before? Are we the first feature to >> > exercise > >> this code? >> > > Yes, I created this file two weeks ago but it is not being exercised > other than unittest. I create the DCHECK based on the comments on > webrtc::DeskopFrame, which is apparently incorrect. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromeos/aura_desktop_capturer.cc > File remoting/host/chromeos/aura_desktop_capturer.cc (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromeos/aura_desktop_capturer.cc#newcode44 > remoting/host/chromeos/aura_desktop_capturer.cc:44: << "DesktopFrame > objects always hold BGRA data."; > On 2014/10/14 01:18:41, Jamie wrote: > >> There seem to be a couple of changes that flip the endianness, but >> > there's > >> nothing in the CL description. If it's an unrelated bugfix, consider >> > moving it > >> to another CL. If not, please mention it in the description. >> > > This is required otherwise the DCHECK will fail after the remoting > session is connected. I will update the CL description. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromoting_host_context.cc > File remoting/host/chromoting_host_context.cc (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromoting_host_context.cc#newcode37 > remoting/host/chromoting_host_context.cc:37: context->audio_task_runner_ > = AutoThread::CreateWithType( > On 2014/10/14 01:18:41, Jamie wrote: > >> This code reads a lot like a ctor. Could you add extra parameters to >> > the ctor > >> for the various threads? It would add a compiler check to ensure that >> > both > >> Create methods are supplying the same threads etc. >> > > I think the creation of the audio, input and encode threads could also >> > be left > >> in the ctor, since the only difference is the thread they use to join. >> > You could > >> either make that another ctor parameter, or perhaps use the file >> > thread to join > >> on all platforms (check with someone who knows the architecture better >> > than I do > >> first; it might not work if you do that). >> > > I spoke to wez. He suggested instead of joining on the FILE Thread. We > should add a ScopedAllowIO on auto thread instead. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromoting_host_context.cc#newcode64 > remoting/host/chromoting_host_context.cc:64: // on the UI thread of the > browser process. > On 2014/10/14 01:18:41, Jamie wrote: > >> This comment implies that CreateOnChromeOS must also be called on the >> > UI thread, > >> which should probably be mentioned in the header. >> > > Done. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromoting_host_context.cc#newcode98 > remoting/host/chromoting_host_context.cc:98: // Thread joining is > blocking and it can only be done on threads that allows > On 2014/10/14 01:18:41, Jamie wrote: > >> s/allows/allow/ >> > > Done. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromoting_host_context.h > File remoting/host/chromoting_host_context.h (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromoting_host_context.h#newcode18 > remoting/host/chromoting_host_context.h:18: } // namespace net > On 2014/10/14 01:18:41, Jamie wrote: > >> s/net/policy/ >> > > Done. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromoting_host_context.h#newcode30 > remoting/host/chromoting_host_context.h:30: // Creates threads and > URLRequestContextGetter for use by a host. > On 2014/10/14 01:18:42, Jamie wrote: > >> The previous sense was more consistent with the other comments in this >> > file. > > Done. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/chromoting_host_context.h#newcode78 > remoting/host/chromoting_host_context.h:78: // Verify that all threads > has started. > On 2014/10/14 01:18:42, Jamie wrote: > >> s/has/have/ >> > > Done. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/continue_window_chromeos.cc > File remoting/host/continue_window_chromeos.cc (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/continue_window_chromeos.cc#newcode9 > remoting/host/continue_window_chromeos.cc:9: class ContinueWindowAura : > public ContinueWindow { > On 2014/10/14 00:18:25, rmsousa wrote: > >> This isn't really an "Aura" implementation, just an empty >> > implementation. > > If this is supposed to remain empty, please name it appropriately, and >> > add a > >> code comment explaining why these UI elements aren't needed in Chrome >> > OS. > > If you do plan to add Aura implementations of this later on, please >> > add a TODO > >> to that effect. Also, please explain why it is safe to land this >> > change without > >> the UI elements (e.g. if the it2me code is currently guaranteed to be >> unreachable) >> > > Done. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/continue_window_chromeos.cc#newcode29 > remoting/host/continue_window_chromeos.cc:29: void > ContinueWindowAura::ShowUi() { > On 2014/10/14 01:18:42, Jamie wrote: > >> These empty methods should all have NOTIMPLEMENTED(). >> > > Those function will be called on ChromeOS and can't have > NOTIMPLEMENTED(). I will add a TODO statement > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/it2me/it2me_host.cc > File remoting/host/it2me/it2me_host.cc (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/it2me/it2me_host.cc#newcode248 > remoting/host/it2me/it2me_host.cc:248: task_runner_->PostTask(FROM_HERE, > On 2014/10/14 01:18:42, Jamie wrote: > >> Why has this changed? >> > > As per wez, comment, we are trying to take reference on the task_runner > of the ChromotingHostContext instead of holding on to the host. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/it2me/it2me_host.h > File remoting/host/it2me/it2me_host.h (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/it2me/it2me_host.h#newcode126 > remoting/host/it2me/it2me_host.h:126: // Called when |policy_watcher_| > has stopped listening for changes so that it > On 2014/10/14 01:18:42, Jamie wrote: > >> s/so that/and/ >> > > Done. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/it2me/it2me_host.h#newcode127 > remoting/host/it2me/it2me_host.h:127: // safe to delete the > |policy_watcher_| object. > On 2014/10/14 01:18:42, Jamie wrote: > >> s/safe/is safe/ >> > > Done. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/it2me/it2me_host.h#newcode143 > remoting/host/it2me/it2me_host.h:143: > scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_; > On 2014/10/14 01:18:42, Jamie wrote: > >> It's not clear to me why this new member is needed. It seems only to >> > be used to > >> save a call to host_context_->network_task_runner() and it makes what >> > would > >> otherwise be a very small diff to this file much harder to review. >> > > The ChromotingHostContext is owned by the It2meNativeMessaginHost and it > will be deleted when It2meNativeMessagingHost is destroyed. During > shutdown, It2meHost outlives It2meNativeMessagingHost so that I can > clean itself up on the network_task_runner. We can no longer get the > network_task_runner from the host_context at this point as it is already > destroyed. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/it2me/it2me_native_messaging_host.h > File remoting/host/it2me/it2me_native_messaging_host.h (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/it2me/it2me_native_messaging_host.h#newcode36 > remoting/host/it2me/it2me_native_messaging_host.h:36: > scoped_ptr<It2MeHostFactory> factory); > On 2014/10/14 01:18:42, Jamie wrote: > >> Having both a Create method and a ctor doesn't seem right here. Could >> > you have > >> Create/CreateForChromeOS like you did for the host context? >> > > Done. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/policy_hack/policy_watcher_chromeos.cc > File remoting/host/policy_hack/policy_watcher_chromeos.cc (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/policy_hack/policy_watcher_chromeos.cc#newcode38 > remoting/host/policy_hack/policy_watcher_chromeos.cc:38: > scoped_ptr<base::DictionaryValue> GetPolicyValues(const PolicyMap& map); > On 2014/10/14 01:18:42, Jamie wrote: > >> I don't think this needs to be a separate method, unless you think >> > you're going > >> to use it elsewhere. If it was inlined in Reload, you wouldn't need a >> > DCHECK on > >> the result (because it would clearly be the result of "new"), and the >> > reader > >> wouldn't have to think about the circumstances under which it might be >> > null. > > Done. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/policy_hack/policy_watcher_chromeos.cc#newcode53 > remoting/host/policy_hack/policy_watcher_chromeos.cc:53: const > PolicyMap& previous, > On 2014/10/14 01:18:42, Jamie wrote: > >> Indentation. >> > > Done. > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/remoting_me2me_host.cc > File remoting/host/remoting_me2me_host.cc (left): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/remoting_me2me_host.cc#oldcode387 > remoting/host/remoting_me2me_host.cc:387: > task_runner->DeleteSoon(FROM_HERE, context_.release()); > On 2014/10/14 01:18:42, Jamie wrote: > >> Why has this been deleted? >> > > Good catch > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/remoting_me2me_host.cc > File remoting/host/remoting_me2me_host.cc (right): > > https://codereview.chromium.org/639233002/diff/320001/ > remoting/host/remoting_me2me_host.cc#newcode1417 > remoting/host/remoting_me2me_host.cc:1417: void > HostProcess::PostPolicyWatcherShutdown() { > On 2014/10/14 00:18:25, rmsousa wrote: > >> nit: OnPolicyWatcherShutdown. >> "Post" is an ambiguous term - we typically use it with its verb >> > meaning (i.e. to > >> post something to a task), so to use it with its adverb meaning (i.e. >> > this will > >> happen after the policy watcher is shutdown) is confusing. >> > > Done. > > https://codereview.chromium.org/639233002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM. If you already depend on net/socket then net/url_request shouldn't be a problem.
kelvinp@chromium.org changed reviewers: + jam@chromium.org - evan@chromium.org
PTAL @jam. I have added a ScopedAllowedIO in AutoThread.cc, to allow joining and I would like to have some feedback from you. The remote assistance host creates threads (e.g. encoding) on the UI thread of the browser process, which needs to be deleted when the host shutdown. The remote assistance host uses AutoThread as its threading primitive, which will only shut itself down when there are no more outstanding references to it. As a result, it is guaranteed that at the point of shutdown, there will be no outstanding tasks in the task_runner and the join will not be blocking.
On 2014/10/16 22:45:52, kelvinp wrote: > PTAL > > @jam. > I have added a ScopedAllowedIO in AutoThread.cc, to allow joining and I would > like to have some feedback from you. did you mean to add ScopedAllowWait instead? I think you did, and note in the future please send tryjobs before reviews, as it would catch stuff like this. > The remote assistance host creates threads (e.g. encoding) on the UI thread of > the browser process, which needs to be deleted when the host shutdown. to make sure i understand, what is "host"? would there only ever be one request at a time? if so, what about after starting the threads we just leave them running? i.e. why do we need to join? > The remote assistance host uses AutoThread as its threading primitive, which > will only shut itself down when there are no more outstanding references to it. > As a result, it is guaranteed that at the point of shutdown, there will be no > outstanding tasks in the task_runner and the join will not be blocking.
https://codereview.chromium.org/639233002/diff/20001/remoting/host/it2me/it2m... File remoting/host/it2me/it2me_native_messaging_host.cc (right): https://codereview.chromium.org/639233002/diff/20001/remoting/host/it2me/it2m... remoting/host/it2me/it2me_native_messaging_host.cc:56: // base::DoNothing is passed in as the quit closure. This comment is in the wrong place, and misses the point. AutoThreadTaskRunner is a TaskRunner with the special property that it will continue to process tasks until no references remain, at least. The QuitClosure we usually pass does the simple thing of stopping the underlying TaskRunner, but in general the point is that the underlying TaskRunner _must not stop processing tasks_ until that callback is invoked. So by passing DoNothing you're violating that contract, and you may find that things break at browser shutdown. https://codereview.chromium.org/639233002/diff/440001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/440001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:308: media::InitializeCPUSpecificMediaFeatures(); Is this also required only for the RA host? Is it also something that needs to be done while we're single-threaded, or could we defer it to IT2Me host initialization? https://codereview.chromium.org/639233002/diff/440001/chrome/browser/extensio... File chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/440001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:34: // A simple NativeMesageHost that mimics the implementation of typo https://codereview.chromium.org/639233002/diff/440001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:111: // chrome and fetch the list of allowed_origins from the manifest. nit: Is there a bug filed for that cleanup? https://codereview.chromium.org/639233002/diff/440001/remoting/base/auto_thre... File remoting/base/auto_thread.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/base/auto_thre... remoting/base/auto_thread.cc:118: // non-blocking as its task runner will always be empty at shutdown. This guarantee actually doesn't hold if you do the ScopedAllowIO here, since a caller can create an AutoThread and manage its lifetime manually, in which case this Join() _can_ block. The guarantee only holds in JoinAndDeleteThread(), so the ScopedAllowIO should be there. https://codereview.chromium.org/639233002/diff/440001/remoting/base/auto_thre... remoting/base/auto_thread.cc:119: base::ThreadRestrictions::ScopedAllowIO allow_io; I'd recommend making this change in a separate CL, since IIRC you'll need a special approval from a base::Thread owner for adding another use of ScopedAllowIO. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:119: bool AuraDesktopCapturer::GetScreenList(ScreenList* screens) { Do you ever intend to implement these? If so then please include a reference to the bug # for that work. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:131: namespace webrtc { nit: Blank line between namespace and //static lines. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.h (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.h:24: // on the Browser UI thread. Update this comment? https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.h:25: class AuraDesktopCapturer : public webrtc::ScreenCapturer { AuraScreenCapturer? https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... File remoting/host/chromoting_host_context.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:43: AutoThread::Create("ChromotingEncodeThread", ui_task_runner); It's strange to have some of the task runners passed in here, but to create a few in this class; I think it'd be cleaner to move these initializations into the Create*() functions. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:68: new ChromotingHostContext(ui_task_runner.get(), Don't use .get() here. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:83: namespace { Since this is a single function definition, use static rather than an anonymous namespace. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:87: #if defined(OS_CHROMEOS) Why not #ifdef# the entire contents of CreateForChromeOS, and this function? https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:136: url_request_context_getter_.get(); The comment in the header indicates that this tests for threads being started (hence by suggested renaming ;) but this actually tests for all must-have fields being set, not just threads. Perhaps just call this IsInitialized(), and update the comment. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... File remoting/host/chromoting_host_context.h (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:41: policy::PolicyService* policy_service); What are the lifetime/ownership properties of |policy_service|? https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:74: policy::PolicyService* policy_service(); Add a comment to clarify how this is used / why it's needed? https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:74: policy::PolicyService* policy_service(); Wouldn't it be cleaner to push the PolicyWatcher out into the context, so that calling code can initialize it appropriately for the platform? https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:82: AutoThreadTaskRunner* video_capture_task_runner, Why are these bare pointers? https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:88: bool VerifyInitialized(); AreThreadsStarted()? https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:88: bool VerifyInitialized(); Under what circumstances could the threads _not_ be started? They're passed in to the ctor, after all? https://codereview.chromium.org/639233002/diff/440001/remoting/host/continue_... File remoting/host/continue_window_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:12: class ContinueWindowAura : public ContinueWindow { This should be inside an anonymous namespace. https://codereview.chromium.org/639233002/diff/440001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:18: // ContinueWindow overrides. nit: s/overrides/interface https://codereview.chromium.org/639233002/diff/440001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:33: // TODO(kelvinp): Implement this on Chrome OS. Is there a bug # for that work? https://codereview.chromium.org/639233002/diff/440001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:42: return scoped_ptr<HostWindow>(new ContinueWindowAura()); make_scoped_ptr(new ...) https://codereview.chromium.org/639233002/diff/440001/remoting/host/disconnec... File remoting/host/disconnect_window_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/disconnec... remoting/host/disconnect_window_chromeos.cc:14: // A place holder implementation for the Disconnect window on ChromeOS. Refer to bug # for that work. https://codereview.chromium.org/639233002/diff/440001/remoting/host/disconnec... remoting/host/disconnect_window_chromeos.cc:20: // HostWindow overrides. s/overrides/interface https://codereview.chromium.org/639233002/diff/440001/remoting/host/disconnec... remoting/host/disconnect_window_chromeos.cc:44: return scoped_ptr<HostWindow>(new DisconnectWindowAura()); make_scoped_ptr() https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:127: // safe to delete the |policy_watcher_| object. nit: ... safe to delete it. https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:141: ChromotingHostContext* host_context_; That doesn't make sense; the It2Me object is ref-counted, so the caller has no control over its lifetime. Can you just have the It2MeHost own the context? You create a new one for each host instance anyway. https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:143: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; Add a comment, or better still rename this to ui_task_runner, to clarify the distinction between it and the network task runner, below. https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:144: scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_; Why have you pulled this out of the host context? https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_native_messaging_host.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... remoting/host/it2me/it2me_native_messaging_host.cc:71: scoped_ptr<extensions::NativeMessageHost> It2MeNativeMessagingHost::Create( This is also // static https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... remoting/host/it2me/it2me_native_messaging_host.cc:76: factory.Pass())); Why is this Create() method parameterized on It2MeHostFactory, but the one for ChromeOS not? Similarly, why does this version require a contet to be passed in, and the ChromeOS version not? Why not always have it passed in, and fix the calling code to do the dirty work of creating it, rather than polluting this class? https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... remoting/host/it2me/it2me_native_messaging_host_unittest.cc:443: ChromotingHostContext::Create(host_task_runner_.get()), Don't use get() here. https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.cc:168: if (!OnPolicyWatcherThread()) { This kind of trampoline doesn't need a separate function to call on to; you can just post the task if you're on the wrong thread and Bind() it back to StopWatching, as this code did previously. If you really want to use PostTaskAndReply then you may as well always post, to avoid overcomplicating this method. https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher.h (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:48: ChromotingHostContext* context, Don't pass the ChromotingHostContext around; it's not designed for that. Pull out the bits you need and pass them in to the PolicyWatcher. This method should be: static scoped_ptr<PolicyWatcher> Create(...) https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:23: virtual ~PolicyWatcherChromeOS() {} Don't inline virtuals, even destructors (except in pure interface classes :) https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:25: // PolicyService::Observer implementation. s/implementation/interface https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:31: // PolicyWatcher overrides. s/overrides/interface https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:44: PolicyService* policy_service) What's the lifetime guarantee/requirement on policy service? https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:55: void PolicyWatcherChromeOS::Reload(){ nit: Space between () and { https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:57: PolicyNamespace(POLICY_DOMAIN_CHROME, std::string())); This is redundant in the OnPolicyUpdated case, since it passes you the namespace and the previous & current maps; you only need this Reload() logic at start-of-day so can you move this logic into OnPolicyUpdated and then have Reload() call that with these paremeters in place? https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher_unittest.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_unittest.cc:102: EXPECT_CALL(*this, PostPolicyWatcherShutdown()).Times(1); You shouldn't mix call expectations in with actually running the code; they need to be set up before you start running code. In this case you will already have called e.g. StartWatching()so the policy watcher will be active at the point where you're adding this now expectation into the mix.
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/639233002/460001
The CQ bit was unchecked by wez@chromium.org
Patchset #5 (id:460001) has been deleted
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/639233002/480001
@wez. Feedback addressed. @jam. The remote assistance host is the OS component that allows IT support to connect to it. It runs in the browser process and it is responsible for tasks like screen capturing, encoding and sending frames over the network, and injecting input. You are correct that there can only be one ongoing remote assistance session at a time. However, since we create a new host (and thus new threads) for each incoming connection request, we should destroy those threads at the end of the remote assistance session. https://codereview.chromium.org/639233002/diff/20001/remoting/host/it2me/it2m... File remoting/host/it2me/it2me_native_messaging_host.cc (right): https://codereview.chromium.org/639233002/diff/20001/remoting/host/it2me/it2m... remoting/host/it2me/it2me_native_messaging_host.cc:56: // base::DoNothing is passed in as the quit closure. On 2014/10/17 17:57:59, Wez wrote: > This comment is in the wrong place, and misses the point. > > AutoThreadTaskRunner is a TaskRunner with the special property that it will > continue to process tasks until no references remain, at least. The QuitClosure > we usually pass does the simple thing of stopping the underlying TaskRunner, but > in general the point is that the underlying TaskRunner _must not stop processing > tasks_ until that callback is invoked. So by passing DoNothing you're violating > that contract, and you may find that things break at browser shutdown. I can update the comment. Yes, I agree that this is a hack, but this is the best solution that I can come across so far to reconcile the two contraditing threading ownership philosophies in Chrome and remoting. Chrome has an explicit ownership model in the sense that those task runners should only be destroyed by the browser thread. By definition, the assumptions of the AutoThreadTaskRunner won't hold. I have tested the shutdown scenarios extensively and I have found no crashes. There is only a leak in the url_request_context_getter when the entire chrome process goes away, which I think it is ok. https://codereview.chromium.org/639233002/diff/440001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/440001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:308: media::InitializeCPUSpecificMediaFeatures(); On 2014/10/17 17:57:59, Wez wrote: > Is this also required only for the RA host? > > Is it also something that needs to be done while we're single-threaded, or could > we defer it to IT2Me host initialization? Done. https://codereview.chromium.org/639233002/diff/440001/chrome/browser/extensio... File chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/440001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:34: // A simple NativeMesageHost that mimics the implementation of On 2014/10/17 17:57:59, Wez wrote: > typo I think both mimics and implementation are spelled correctly https://codereview.chromium.org/639233002/diff/440001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:111: // chrome and fetch the list of allowed_origins from the manifest. On 2014/10/17 17:57:59, Wez wrote: > nit: Is there a bug filed for that cleanup? Done. https://codereview.chromium.org/639233002/diff/440001/remoting/base/auto_thre... File remoting/base/auto_thread.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/base/auto_thre... remoting/base/auto_thread.cc:118: // non-blocking as its task runner will always be empty at shutdown. On 2014/10/17 17:57:59, Wez wrote: > This guarantee actually doesn't hold if you do the ScopedAllowIO here, since a > caller can create an AutoThread and manage its lifetime manually, in which case > this Join() _can_ block. > > The guarantee only holds in JoinAndDeleteThread(), so the ScopedAllowIO should > be there. Done. https://codereview.chromium.org/639233002/diff/440001/remoting/base/auto_thre... remoting/base/auto_thread.cc:119: base::ThreadRestrictions::ScopedAllowIO allow_io; On 2014/10/17 17:57:59, Wez wrote: > I'd recommend making this change in a separate CL, since IIRC you'll need a > special approval from a base::Thread owner for adding another use of > ScopedAllowIO. I have already added jam on this CL. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:119: bool AuraDesktopCapturer::GetScreenList(ScreenList* screens) { On 2014/10/17 17:57:59, Wez wrote: > Do you ever intend to implement these? If so then please include a reference to > the bug # for that work. No plans for supporting multiple desktop on ChromeOS https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:131: namespace webrtc { On 2014/10/17 17:57:59, Wez wrote: > nit: Blank line between namespace and //static lines. Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.h (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.h:24: // on the Browser UI thread. On 2014/10/17 17:57:59, Wez wrote: > Update this comment? Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.h:25: class AuraDesktopCapturer : public webrtc::ScreenCapturer { On 2014/10/17 17:58:00, Wez wrote: > AuraScreenCapturer? Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... File remoting/host/chromoting_host_context.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:43: AutoThread::Create("ChromotingEncodeThread", ui_task_runner); On 2014/10/17 17:58:00, Wez wrote: > It's strange to have some of the task runners passed in here, but to create a > few in this class; I think it'd be cleaner to move these initializations into > the Create*() functions. Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:68: new ChromotingHostContext(ui_task_runner.get(), On 2014/10/17 17:58:00, Wez wrote: > Don't use .get() here. Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:83: namespace { On 2014/10/17 17:58:00, Wez wrote: > Since this is a single function definition, use static rather than an anonymous > namespace. What is the benefits of using a single function definition over an anonymous namespace? https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:87: #if defined(OS_CHROMEOS) On 2014/10/17 17:58:00, Wez wrote: > Why not #ifdef# the entire contents of CreateForChromeOS, and this function? I have moved the chromeos factory to a separate file to avoid ifdefs https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:136: url_request_context_getter_.get(); On 2014/10/17 17:58:00, Wez wrote: > The comment in the header indicates that this tests for threads being started > (hence by suggested renaming ;) but this actually tests for all must-have fields > being set, not just threads. > > Perhaps just call this IsInitialized(), and update the comment. Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... File remoting/host/chromoting_host_context.h (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:41: policy::PolicyService* policy_service); On 2014/10/17 17:58:00, Wez wrote: > What are the lifetime/ownership properties of |policy_service|? Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:74: policy::PolicyService* policy_service(); On 2014/10/17 17:58:00, Wez wrote: > Wouldn't it be cleaner to push the PolicyWatcher out into the context, so that > calling code can initialize it appropriately for the platform? Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:74: policy::PolicyService* policy_service(); On 2014/10/17 17:58:00, Wez wrote: > Wouldn't it be cleaner to push the PolicyWatcher out into the context, so that > calling code can initialize it appropriately for the platform? Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:82: AutoThreadTaskRunner* video_capture_task_runner, On 2014/10/17 17:58:00, Wez wrote: > Why are these bare pointers? Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:88: bool VerifyInitialized(); On 2014/10/17 17:58:00, Wez wrote: > Under what circumstances could the threads _not_ be started? They're passed in > to the ctor, after all? Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/continue_... File remoting/host/continue_window_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:12: class ContinueWindowAura : public ContinueWindow { On 2014/10/17 17:58:00, Wez wrote: > This should be inside an anonymous namespace. Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:18: // ContinueWindow overrides. On 2014/10/17 17:58:00, Wez wrote: > nit: s/overrides/interface Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:33: // TODO(kelvinp): Implement this on Chrome OS. On 2014/10/17 17:58:00, Wez wrote: > Is there a bug # for that work? Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:42: return scoped_ptr<HostWindow>(new ContinueWindowAura()); On 2014/10/17 17:58:00, Wez wrote: > make_scoped_ptr(new ...) Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/disconnec... File remoting/host/disconnect_window_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/disconnec... remoting/host/disconnect_window_chromeos.cc:14: // A place holder implementation for the Disconnect window on ChromeOS. On 2014/10/17 17:58:01, Wez wrote: > Refer to bug # for that work. Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/disconnec... remoting/host/disconnect_window_chromeos.cc:20: // HostWindow overrides. On 2014/10/17 17:58:01, Wez wrote: > s/overrides/interface Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/disconnec... remoting/host/disconnect_window_chromeos.cc:44: return scoped_ptr<HostWindow>(new DisconnectWindowAura()); On 2014/10/17 17:58:00, Wez wrote: > make_scoped_ptr() Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:127: // safe to delete the |policy_watcher_| object. On 2014/10/17 17:58:01, Wez wrote: > nit: ... safe to delete it. Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:141: ChromotingHostContext* host_context_; On 2014/10/17 17:58:01, Wez wrote: > That doesn't make sense; the It2Me object is ref-counted, so the caller has no > control over its lifetime. > > Can you just have the It2MeHost own the context? You create a new one for each > host instance anyway. We can but this would mean that it2meHost would need a context factory as a new context is created each time. https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:143: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2014/10/17 17:58:01, Wez wrote: > Add a comment, or better still rename this to ui_task_runner, to clarify the > distinction between it and the network task runner, below. I have removed network_task_runner. https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:144: scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_; On 2014/10/17 17:58:01, Wez wrote: > Why have you pulled this out of the host context? Because It2MeHost outlives the host context and we still need the network_task_runner() on shutdown. https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_native_messaging_host.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... remoting/host/it2me/it2me_native_messaging_host.cc:71: scoped_ptr<extensions::NativeMessageHost> It2MeNativeMessagingHost::Create( On 2014/10/17 17:58:01, Wez wrote: > This is also // static Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... remoting/host/it2me/it2me_native_messaging_host.cc:76: factory.Pass())); On 2014/10/17 17:58:01, Wez wrote: > Why is this Create() method parameterized on It2MeHostFactory, but the one for > ChromeOS not? > > Similarly, why does this version require a contet to be passed in, and the > ChromeOS version not? Why not always have it passed in, and fix the calling code > to do the dirty work of creating it, rather than polluting this class? Good Idea. In this case, I can simply get rid of the create function. https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/it2me/it2... remoting/host/it2me/it2me_native_messaging_host_unittest.cc:443: ChromotingHostContext::Create(host_task_runner_.get()), On 2014/10/17 17:58:01, Wez wrote: > Don't use get() here. Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.cc:168: if (!OnPolicyWatcherThread()) { On 2014/10/17 17:58:01, Wez wrote: > This kind of trampoline doesn't need a separate function to call on to; you can > just post the task if you're on the wrong thread and Bind() it back to > StopWatching, as this code did previously. > > If you really want to use PostTaskAndReply then you may as well always post, to > avoid overcomplicating this method. I use PostTaskAndReply as I want the callback is be invoked on the caller thread. I will make it always post instead. https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher.h (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:48: ChromotingHostContext* context, On 2014/10/17 17:58:01, Wez wrote: > Don't pass the ChromotingHostContext around; it's not designed for that. Pull > out the bits you need and pass them in to the PolicyWatcher. > > This method should be: > static scoped_ptr<PolicyWatcher> Create(...) I will avoid passing the context but pass the policy_service instead. Fixing the scoped_ptr is beyond the scope of this change though. https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:23: virtual ~PolicyWatcherChromeOS() {} On 2014/10/17 17:58:01, Wez wrote: > Don't inline virtuals, even destructors (except in pure interface classes :) Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:25: // PolicyService::Observer implementation. On 2014/10/17 17:58:01, Wez wrote: > s/implementation/interface Seems like sergey prefers implementation and you prefer interface :) https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:31: // PolicyWatcher overrides. On 2014/10/17 17:58:01, Wez wrote: > s/overrides/interface Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:44: PolicyService* policy_service) On 2014/10/17 17:58:01, Wez wrote: > What's the lifetime guarantee/requirement on policy service? Good point. Comments added. https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:55: void PolicyWatcherChromeOS::Reload(){ On 2014/10/17 17:58:01, Wez wrote: > nit: Space between () and { Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:57: PolicyNamespace(POLICY_DOMAIN_CHROME, std::string())); On 2014/10/17 17:58:01, Wez wrote: > This is redundant in the OnPolicyUpdated case, since it passes you the namespace > and the previous & current maps; you only need this Reload() logic at > start-of-day so can you move this logic into OnPolicyUpdated and then have > Reload() call that with these paremeters in place? Done. https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher_unittest.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_unittest.cc:102: EXPECT_CALL(*this, PostPolicyWatcherShutdown()).Times(1); On 2014/10/17 17:58:01, Wez wrote: > You shouldn't mix call expectations in with actually running the code; they need > to be set up before you start running code. In this case you will already have > called e.g. StartWatching()so the policy watcher will be active at the point > where you're adding this now expectation into the mix. I felt the same way when I saw the EXPECT_EQ in the original code as well. But then I realized that StopWatching is manually called in the test case. The test here is trying to verify that whenever StopWatching is called, the appropriate callback is invoked (or stop_event is signaled before this change) and not so much about whether StopWatching is being called. I am going to leave it as it is.
@jam, Actually, I do mean AllowIO. See PlatformThread::Join
void PlatformThread::Join(PlatformThreadHandle thread_handle) {
// Joining another thread may block the current thread for a long time, since
// the thread referred to by |thread_handle| may still be running long-lived /
// blocking tasks.
base::ThreadRestrictions::AssertIOAllowed();
CHECK_EQ(0, pthread_join(thread_handle.handle_, NULL));
}
Patchset #5 (id:480001) has been deleted
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/639233002/500001
On 2014/10/20 00:42:58, kelvinp wrote:
> @jam, Actually, I do mean AllowIO. See PlatformThread::Join
>
> void PlatformThread::Join(PlatformThreadHandle thread_handle) {
> // Joining another thread may block the current thread for a long time,
since
> // the thread referred to by |thread_handle| may still be running long-lived
/
> // blocking tasks.
> base::ThreadRestrictions::AssertIOAllowed();
> CHECK_EQ(0, pthread_join(thread_handle.handle_, NULL));
> }
As per wez's feedback, I am going to move the ThreadRestrictions to a separate
change.
For now, I will use the FILE thread for as the joiner.
https://codereview.chromium.org/639233002/diff/440001/chrome/browser/extensio... File chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/440001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:34: // A simple NativeMesageHost that mimics the implementation of On 2014/10/17 17:57:59, Wez wrote: > typo Still typo ;)
The CQ bit was unchecked by wez@chromium.org
Please be careful not to hit the CQ box until you have LGTMs from all relevant reviewers. :) On 19 Oct 2014 18:07, <commit-bot@chromium.org> wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/639233002/500001 > > > https://codereview.chromium.org/639233002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/10/20 01:17:35, Wez wrote: > Please be careful not to hit the CQ box until you have LGTMs from all > relevant reviewers. :) > On 19 Oct 2014 18:07, <mailto:commit-bot@chromium.org> wrote: > > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/639233002/500001 > > > > > > https://codereview.chromium.org/639233002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I see the typo now. I am counting on the fact that the tree has been red for 1+ days so that the commit won't check the code it. I will wait for your LGTM.
On 2014/10/20 00:21:18, kelvinp wrote: > @wez. Feedback addressed. > > @jam. The remote assistance host is the OS component that allows IT support to > connect to it. It runs in the browser process and it is responsible for tasks > like screen capturing, encoding and sending frames over the network, and > injecting input. > > You are correct that there can only be one ongoing remote assistance session at > a time. However, since we create a new host (and thus new threads) for each > incoming connection request, we should destroy those threads at the end of the > remote assistance session. I'm curious, why did you not just have a thread that is lazily created, and then one it's created it can stay around and be used for future assistance sessions? btw i don't think you need me for any owners reviews?
kelvinp@chromium.org changed reviewers: - jam@chromium.org
@jam, Yes, that's is correct, I don't need an LGTM for this CL. Thank you for your time and your inputs. I have removed you from the reviewer's list.
@wez. I have fixed the face-palming typo ;( (good catch) and changed PolicyWatcher::Create to return a scoped_ptr. I would like to check-in the change today as there are two more CL depending on this. It would be great if you can take a look of the updated CL today.
On 2014/10/20 18:18:37, kelvinp wrote: > @wez. I have fixed the face-palming typo ;( (good catch) and changed > PolicyWatcher::Create to return a scoped_ptr. I would like to check-in the > change today as there are two more CL depending on this. It would be great if > you can take a look of the updated CL today. @wez. In this CL, 1. Gets rid of the ChromotingHostContext factories and brought back the Create functions. 2. Removed policy_watcher from the ChromotingHostContext and move it to the It2MeHostFactory 3. Defines a copy method on ChromotingHostContext PTAL
Patchset #8 (id:560001) has been deleted
https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromeos/... File remoting/host/chromeos/aura_desktop_capturer.cc (right): https://codereview.chromium.org/639233002/diff/440001/remoting/host/chromeos/... remoting/host/chromeos/aura_desktop_capturer.cc:119: bool AuraDesktopCapturer::GetScreenList(ScreenList* screens) { On 2014/10/20 00:21:16, kelvinp wrote: > On 2014/10/17 17:57:59, Wez wrote: > > Do you ever intend to implement these? If so then please include a reference > to > > the bug # for that work. > > No plans for supporting multiple desktop on ChromeOS OK; as discussed let's revert this and take the hit in the CreateVideoCapturer API instead. https://codereview.chromium.org/639233002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:111: // Therefore, base::DoNothing is passed in as the quit closure. This comment is out-of-date and doesn't belong here. https://codereview.chromium.org/639233002/diff/580001/remoting/host/DEPS File remoting/host/DEPS (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/DEPS#newc... remoting/host/DEPS:4: "+content/public/browser", This seems a scary dep to pull in to remoting/host? Could it be restricted to code under remoting/host/it2me, for example? https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromeos/... File remoting/host/chromeos/aura_screen_capturer.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromeos/... remoting/host/chromeos/aura_screen_capturer.cc:120: // Multiple monitors capturing are not supported on ChromeOS. nit: Multi-monitor capturing is not supported yet. https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... File remoting/host/chromoting_host_context.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:115: url_request_context_getter)); nit: Rather than create them all in temporaries and then pass them, it may read more cleanly to create the non-ifdef'd ones in the ctor i.e: new ChromotingHostContext( ..., AutoThread::CreateWithType(...), ...); https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:121: // on the UI thread of the browser process. nit: the latter is a req of the GetMessageLoopProxyForThread API, so do you need to re-state it here? https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:125: base::Bind(&base::DoNothing)); nit: add brief comment explaining the issue w/ using DoNothing here. https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:145: // that allows blocking I/O, which is required by thread joining. I thought we had fixed AutoThread? https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:160: video_encode_task_runner, url_request_context_getter)); More readable to have one line per task-runner here. You could also create all but the joiner task runner in-line in the ctor params. https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... File remoting/host/chromoting_host_context.h (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:32: // Must be called on the UI thread of the browser process. nit: Document what it does, e.g. attaches task runners to the relevant browser threads. https://codereview.chromium.org/639233002/diff/580001/remoting/host/continue_... File remoting/host/continue_window_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:17: virtual ~ContinueWindowAura(); override? https://codereview.chromium.org/639233002/diff/580001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:35: // TODO(kelvinp): Implement this on Chrome OS (See crbug.com/424908). NOTIMPLEMENTED()? https://codereview.chromium.org/639233002/diff/580001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:39: // TODO(kelvinp): Implement this on Chrome OS (See crbug.com/424908). As above? https://codereview.chromium.org/639233002/diff/580001/remoting/host/disconnec... File remoting/host/disconnect_window_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/disconnec... remoting/host/disconnect_window_chromeos.cc:18: virtual ~DisconnectWindowAura(); override https://codereview.chromium.org/639233002/diff/580001/remoting/host/disconnec... remoting/host/disconnect_window_chromeos.cc:37: // TODO(kelvinp): Implement this on ChromeOS (See crbug.com/424908). NOTIMPLEMENTED https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:314: if (!host_context_->network_task_runner()->BelongsToCurrentThread()) { Previous code assumed/required that this be called on network thread; why has this changed; looks like that's the thread passed in below, still? https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:187: // It2MeHost object. It2MeHost objects are ref-counted, so how can the caller possibly guarantee that? https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_native_messaging_host.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... remoting/host/it2me/it2me_native_messaging_host.cc:48: task_runner_(context->ui_task_runner()), Why pull this out here, rather than just referring to it this way in task_runner()? https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_native_messaging_host_main.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... remoting/host/it2me/it2me_native_messaging_host_main.cc:116: scoped_ptr<It2MeHostFactory> factory(new It2MeHostFactory(nullptr)); nit: Add a comment to explain the nullptr https://codereview.chromium.org/639233002/diff/580001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher.h (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:112: virtual void StopWatchingOnPolicyWatcherThread(); Why virtual? https://codereview.chromium.org/639233002/diff/580001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:22: // of PolicyWatcherChromeOS. This is a contract on the Create API, so put the comment there.
Patchset #9 (id:600001) has been deleted
@wez PTAL https://codereview.chromium.org/639233002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc:111: // Therefore, base::DoNothing is passed in as the quit closure. On 2014/10/24 00:28:47, Wez wrote: > This comment is out-of-date and doesn't belong here. Done. https://codereview.chromium.org/639233002/diff/580001/remoting/host/DEPS File remoting/host/DEPS (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/DEPS#newc... remoting/host/DEPS:4: "+content/public/browser", On 2014/10/24 00:28:47, Wez wrote: > This seems a scary dep to pull in to remoting/host? > > Could it be restricted to code under remoting/host/it2me, for example? It is currently referenced by the chromoting_host_context, which lives in remoting/host. https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromeos/... File remoting/host/chromeos/aura_screen_capturer.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromeos/... remoting/host/chromeos/aura_screen_capturer.cc:120: // Multiple monitors capturing are not supported on ChromeOS. On 2014/10/24 00:28:47, Wez wrote: > nit: Multi-monitor capturing is not supported yet. Revert to AuraDesktopCapturer https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... File remoting/host/chromoting_host_context.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:115: url_request_context_getter)); On 2014/10/24 00:28:48, Wez wrote: > nit: Rather than create them all in temporaries and then pass them, it may read > more cleanly to create the non-ifdef'd ones in the ctor i.e: > > new ChromotingHostContext( > ..., > AutoThread::CreateWithType(...), > ...); Done. https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:121: // on the UI thread of the browser process. On 2014/10/24 00:28:47, Wez wrote: > nit: the latter is a req of the GetMessageLoopProxyForThread API, so do you need > to re-state it here? Done. https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:125: base::Bind(&base::DoNothing)); On 2014/10/24 00:28:47, Wez wrote: > nit: add brief comment explaining the issue w/ using DoNothing here. Done. https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:145: // that allows blocking I/O, which is required by thread joining. On 2014/10/24 00:28:48, Wez wrote: > I thought we had fixed AutoThread? Close but not yet. We agreed to separate that in a separate CL :) https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:160: video_encode_task_runner, url_request_context_getter)); On 2014/10/24 00:28:48, Wez wrote: > More readable to have one line per task-runner here. You could also create all > but the joiner task runner in-line in the ctor params. Done. https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... File remoting/host/chromoting_host_context.h (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:32: // Must be called on the UI thread of the browser process. On 2014/10/24 00:28:48, Wez wrote: > nit: Document what it does, e.g. attaches task runners to the relevant browser > threads. Done. https://codereview.chromium.org/639233002/diff/580001/remoting/host/continue_... File remoting/host/continue_window_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:17: virtual ~ContinueWindowAura(); On 2014/10/24 00:28:48, Wez wrote: > override? Done. https://codereview.chromium.org/639233002/diff/580001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:35: // TODO(kelvinp): Implement this on Chrome OS (See crbug.com/424908). On 2014/10/24 00:28:48, Wez wrote: > NOTIMPLEMENTED()? Done. https://codereview.chromium.org/639233002/diff/580001/remoting/host/continue_... remoting/host/continue_window_chromeos.cc:39: // TODO(kelvinp): Implement this on Chrome OS (See crbug.com/424908). On 2014/10/24 00:28:48, Wez wrote: > As above? Done. https://codereview.chromium.org/639233002/diff/580001/remoting/host/disconnec... File remoting/host/disconnect_window_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/disconnec... remoting/host/disconnect_window_chromeos.cc:18: virtual ~DisconnectWindowAura(); On 2014/10/24 00:28:48, Wez wrote: > override Done. https://codereview.chromium.org/639233002/diff/580001/remoting/host/disconnec... remoting/host/disconnect_window_chromeos.cc:37: // TODO(kelvinp): Implement this on ChromeOS (See crbug.com/424908). On 2014/10/24 00:28:48, Wez wrote: > NOTIMPLEMENTED Done. https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:314: if (!host_context_->network_task_runner()->BelongsToCurrentThread()) { On 2014/10/24 00:28:48, Wez wrote: > Previous code assumed/required that this be called on network thread; why has > this changed; looks like that's the thread passed in below, still? Because on ChromeOS, the policy_watcher runs in the ui_task_runner of the browser process. https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:187: // It2MeHost object. On 2014/10/24 00:28:48, Wez wrote: > It2MeHost objects are ref-counted, so how can the caller possibly guarantee > that? Comments updated. https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_native_messaging_host.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... remoting/host/it2me/it2me_native_messaging_host.cc:48: task_runner_(context->ui_task_runner()), On 2014/10/24 00:28:48, Wez wrote: > Why pull this out here, rather than just referring to it this way in > task_runner()? Done. https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_native_messaging_host_main.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... remoting/host/it2me/it2me_native_messaging_host_main.cc:116: scoped_ptr<It2MeHostFactory> factory(new It2MeHostFactory(nullptr)); On 2014/10/24 00:28:48, Wez wrote: > nit: Add a comment to explain the nullptr Done. https://codereview.chromium.org/639233002/diff/580001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher.h (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:112: virtual void StopWatchingOnPolicyWatcherThread(); On 2014/10/24 00:28:48, Wez wrote: > Why virtual? Done. https://codereview.chromium.org/639233002/diff/580001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher_chromeos.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher_chromeos.cc:22: // of PolicyWatcherChromeOS. On 2014/10/24 00:28:48, Wez wrote: > This is a contract on the Create API, so put the comment there. Done.
https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... File remoting/host/chromoting_host_context.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:145: // that allows blocking I/O, which is required by thread joining. On 2014/10/24 21:39:41, kelvinp wrote: > On 2014/10/24 00:28:48, Wez wrote: > > I thought we had fixed AutoThread? > > Close but not yet. We agreed to separate that in a separate CL :) I meant land that separate CL so you don't need to hack around it in this one... https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:314: if (!host_context_->network_task_runner()->BelongsToCurrentThread()) { On 2014/10/24 21:39:42, kelvinp wrote: > On 2014/10/24 00:28:48, Wez wrote: > > Previous code assumed/required that this be called on network thread; why has > > this changed; looks like that's the thread passed in below, still? > > Because on ChromeOS, the policy_watcher runs in the ui_task_runner of the > browser process. Right, and on other platforms it runs on some other thread... so what changed? Or are you saying we run it on network thread on other platforms..? Or on other platforms are we hopping back to ther caller (network) thread to dispatch the callbacks? https://codereview.chromium.org/639233002/diff/620001/remoting/host/basic_des... File remoting/host/basic_desktop_environment.cc (right): https://codereview.chromium.org/639233002/diff/620001/remoting/host/basic_des... remoting/host/basic_desktop_environment.cc:13: #endif // defined(OS_CHROMEOS) nit: Don't need the trailing comment for a single-line #ifdef block https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... File remoting/host/chromoting_host_context.cc (right): https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:119: AutoThreadTaskRunner* GetTaskRunner(content::BrowserThread::ID id) { scoped_refptr<> https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:119: AutoThreadTaskRunner* GetTaskRunner(content::BrowserThread::ID id) { Suggest WrapBrowserThread() as more descriptive name for this. https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:125: // is passed in as the quit closure. Let's add a bug, and refer to it here, for fixing that somehow. https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:147: GetTaskRunner(content::BrowserThread::FILE), You already wrapped FILE, above; just re-use that here. https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:151: GetTaskRunner(content::BrowserThread::UI), // video_capture_task_runner You already wrapped UI, above; re-use that here. https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:153: base::MessageLoop::TYPE_IO), Why does encode need an I/O thread...? https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... File remoting/host/chromoting_host_context.h (right): https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:33: // host. Must be called on the UI thread of the browser process. Still need to document why |url_request_context_getter| needs passing in. https://codereview.chromium.org/639233002/diff/620001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/639233002/diff/620001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:188: // |policy_service| is a global singleton available from the browser process. If this is really a singleton then just pull in the dependency in the CrOS-specific PolicyWatcher impl, directly (you can also have a Set*ForTest for testing of course), rather than pollute this interface with impossible-to-meet requirements on callers! https://codereview.chromium.org/639233002/diff/620001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher.h (right): https://codereview.chromium.org/639233002/diff/620001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:45: // Implemented by each platform. This message loop of |task_runner| should be "This message loop of" seems redundant https://codereview.chromium.org/639233002/diff/620001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:46: // an IO message loop. |policy_service| is currently only used on ChromeOS. What is task_runner used for by the PolicyWatcher?
PTAL https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... File remoting/host/chromoting_host_context.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:145: // that allows blocking I/O, which is required by thread joining. On 2014/10/24 23:29:26, Wez wrote: > On 2014/10/24 21:39:41, kelvinp wrote: > > On 2014/10/24 00:28:48, Wez wrote: > > > I thought we had fixed AutoThread? > > > > Close but not yet. We agreed to separate that in a separate CL :) > > I meant land that separate CL so you don't need to hack around it in this one... I will create a separate CL. https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/639233002/diff/580001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:314: if (!host_context_->network_task_runner()->BelongsToCurrentThread()) { On 2014/10/24 23:29:26, Wez wrote: > On 2014/10/24 21:39:42, kelvinp wrote: > > On 2014/10/24 00:28:48, Wez wrote: > > > Previous code assumed/required that this be called on network thread; why > has > > > this changed; looks like that's the thread passed in below, still? > > > > Because on ChromeOS, the policy_watcher runs in the ui_task_runner of the > > browser process. > > Right, and on other platforms it runs on some other thread... so what changed? > Or are you saying we run it on network thread on other platforms..? Or on other > platforms are we hopping back to ther caller (network) thread to dispatch the > callbacks? The policy watcher supports being called from any thread as under the hood, it re-posts the call to its task_runner. On other platforms, we always run the policy watcher on the network_task_runner, and the policy_watcher and it calls us back on the network_task_runner as well. On chrome OS, we run the policy watcher on the UI thread and it calls us back on the ui_task_runner. Therefore, we need to call ourselves again in the network_task_runner. https://codereview.chromium.org/639233002/diff/620001/remoting/host/basic_des... File remoting/host/basic_desktop_environment.cc (right): https://codereview.chromium.org/639233002/diff/620001/remoting/host/basic_des... remoting/host/basic_desktop_environment.cc:13: #endif // defined(OS_CHROMEOS) On 2014/10/24 23:29:27, Wez wrote: > nit: Don't need the trailing comment for a single-line #ifdef block Done. https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... File remoting/host/chromoting_host_context.cc (right): https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:119: AutoThreadTaskRunner* GetTaskRunner(content::BrowserThread::ID id) { On 2014/10/24 23:29:27, Wez wrote: > scoped_refptr<> Done. https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:119: AutoThreadTaskRunner* GetTaskRunner(content::BrowserThread::ID id) { On 2014/10/24 23:29:27, Wez wrote: > Suggest WrapBrowserThread() as more descriptive name for this. Done. https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:125: // is passed in as the quit closure. On 2014/10/24 23:29:27, Wez wrote: > Let's add a bug, and refer to it here, for fixing that somehow. Done. https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:147: GetTaskRunner(content::BrowserThread::FILE), On 2014/10/24 23:29:27, Wez wrote: > You already wrapped FILE, above; just re-use that here. Done. https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:151: GetTaskRunner(content::BrowserThread::UI), // video_capture_task_runner On 2014/10/24 23:29:27, Wez wrote: > You already wrapped UI, above; re-use that here. Done. https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:153: base::MessageLoop::TYPE_IO), On 2014/10/24 23:29:27, Wez wrote: > Why does encode need an I/O thread...? I am not sure. The implementation on the existing platform uses a task runner with TYPE_IO. https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... File remoting/host/chromoting_host_context.h (right): https://codereview.chromium.org/639233002/diff/620001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:33: // host. Must be called on the UI thread of the browser process. On 2014/10/24 23:29:27, Wez wrote: > Still need to document why |url_request_context_getter| needs passing in. Done. https://codereview.chromium.org/639233002/diff/620001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/639233002/diff/620001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:188: // |policy_service| is a global singleton available from the browser process. On 2014/10/24 23:29:27, Wez wrote: > If this is really a singleton then just pull in the dependency in the > CrOS-specific PolicyWatcher impl, directly (you can also have a Set*ForTest for > testing of course), rather than pollute this interface with impossible-to-meet > requirements on callers! It is actually a global object instead of a singleton. I can't pull it in directly as that would introduce include dependencies from remoting to browser, which would create a circular include dependency https://codereview.chromium.org/639233002/diff/620001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher.h (right): https://codereview.chromium.org/639233002/diff/620001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:45: // Implemented by each platform. This message loop of |task_runner| should be On 2014/10/24 23:29:27, Wez wrote: > "This message loop of" seems redundant Done. https://codereview.chromium.org/639233002/diff/620001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:46: // an IO message loop. |policy_service| is currently only used on ChromeOS. On 2014/10/24 23:29:27, Wez wrote: > What is task_runner used for by the PolicyWatcher? Done.
https://codereview.chromium.org/639233002/diff/620001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/639233002/diff/620001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:188: // |policy_service| is a global singleton available from the browser process. On 2014/10/29 01:22:52, kelvinp wrote: > On 2014/10/24 23:29:27, Wez wrote: > > If this is really a singleton then just pull in the dependency in the > > CrOS-specific PolicyWatcher impl, directly (you can also have a Set*ForTest > for > > testing of course), rather than pollute this interface with impossible-to-meet > > requirements on callers! > > It is actually a global object instead of a singleton. I can't pull it in > directly as that would introduce include dependencies from remoting to browser, > which would create a circular include dependency :( OK... can we instead move this parameter to a set_policty_service() setter, to avoid having to pass NULL in on non-CrOS, please? https://codereview.chromium.org/639233002/diff/640001/remoting/host/chromotin... File remoting/host/chromoting_host_context.cc (right): https://codereview.chromium.org/639233002/diff/640001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:121: // This is a hack. AutoThreadTaskRunner is a TaskRunner with the special nit: Remove "This is a hack" https://codereview.chromium.org/639233002/diff/640001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:127: // TODO(kelvinp): Remove the hack (See crbug.com/428187). Suggest: Remove... -> Fix this. https://codereview.chromium.org/639233002/diff/640001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:143: WrapBrowserThread(content::BrowserThread::FILE); Add a reference to the bug for fixing AutoThread so that that restriction isn't necessary. https://codereview.chromium.org/639233002/diff/640001/remoting/host/chromotin... File remoting/host/chromoting_host_context.h (right): https://codereview.chromium.org/639233002/diff/640001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:40: // Instead, we re-use the |url_request_context_getter| in the browser process. If this is the only reason you need to re-use the browser's getter then can't you pass the chromoting host context off to an IO capable thread to be deleted, so that there is no reference to the getter remaining on a non-IO thread? https://codereview.chromium.org/639233002/diff/640001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/639233002/diff/640001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:315: // |network_task_runner| on other platforms. That's just an implementation detail of the CrOS policy watcher though - wouldn't it be cleaner to fix that to post the update on the |task_runner| that is passed to it? https://codereview.chromium.org/639233002/diff/640001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher.h (right): https://codereview.chromium.org/639233002/diff/640001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:45: // Implemented by each platform. This |task_runner| should be an IO message Remove "This" - it doesn't make sense grammatically. https://codereview.chromium.org/639233002/diff/640001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:47: // |task_runner| so it can be called from any thread. |policy_service| is It's also the task runner on which the watcher will call callbacks. https://codereview.chromium.org/639233002/diff/640001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:47: // |task_runner| so it can be called from any thread. |policy_service| is This isn't true under ChromeOS, though. Is it even a property we care about - we seem to only use it from the same thread? https://codereview.chromium.org/639233002/diff/640001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:49: // |policy_service| remains valid for the lifetime of PolicyWatcherChromeOS. This would be cleaner with separate CrOS and non-CrOS creator functions, IMO. https://codereview.chromium.org/639233002/diff/640001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:49: // |policy_service| remains valid for the lifetime of PolicyWatcherChromeOS. The caller does not see PolicyWatcherChromeOS - it's an implementation detail - so don't mention it in the comment.
Patchset #11 (id:660001) has been deleted
@wez PTAL https://codereview.chromium.org/639233002/diff/620001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/639233002/diff/620001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:188: // |policy_service| is a global singleton available from the browser process. On 2014/10/29 18:27:50, Wez wrote: > On 2014/10/29 01:22:52, kelvinp wrote: > > On 2014/10/24 23:29:27, Wez wrote: > > > If this is really a singleton then just pull in the dependency in the > > > CrOS-specific PolicyWatcher impl, directly (you can also have a Set*ForTest > > for > > > testing of course), rather than pollute this interface with > impossible-to-meet > > > requirements on callers! > > > > It is actually a global object instead of a singleton. I can't pull it in > > directly as that would introduce include dependencies from remoting to > browser, > > which would create a circular include dependency > > :( > > OK... can we instead move this parameter to a set_policty_service() setter, to > avoid having to pass NULL in on non-CrOS, please? Agreed. https://codereview.chromium.org/639233002/diff/640001/remoting/host/chromotin... File remoting/host/chromoting_host_context.cc (right): https://codereview.chromium.org/639233002/diff/640001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:121: // This is a hack. AutoThreadTaskRunner is a TaskRunner with the special On 2014/10/29 18:27:51, Wez wrote: > nit: Remove "This is a hack" Done. https://codereview.chromium.org/639233002/diff/640001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:127: // TODO(kelvinp): Remove the hack (See crbug.com/428187). On 2014/10/29 18:27:50, Wez wrote: > Suggest: Remove... -> Fix this. Done. https://codereview.chromium.org/639233002/diff/640001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:143: WrapBrowserThread(content::BrowserThread::FILE); On 2014/10/29 18:27:50, Wez wrote: > Add a reference to the bug for fixing AutoThread so that that restriction isn't > necessary. Done. https://codereview.chromium.org/639233002/diff/640001/remoting/host/chromotin... File remoting/host/chromoting_host_context.h (right): https://codereview.chromium.org/639233002/diff/640001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:40: // Instead, we re-use the |url_request_context_getter| in the browser process. On 2014/10/29 18:27:51, Wez wrote: > If this is the only reason you need to re-use the browser's getter then can't > you pass the chromoting host context off to an IO capable thread to be deleted, > so that there is no reference to the getter remaining on a non-IO thread? Your suggestion involves changing the threading model on shut down that is risky and requires sufficient testing. Given the existing approach works fine and we would like to land the feature in M40. Unless that are significant advantages of not passing the |url_request_context_getter|. I would prefer to leave the code as it is. https://codereview.chromium.org/639233002/diff/640001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/639233002/diff/640001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:315: // |network_task_runner| on other platforms. On 2014/10/29 18:27:51, Wez wrote: > That's just an implementation detail of the CrOS policy watcher though - > wouldn't it be cleaner to fix that to post the update on the |task_runner| that > is passed to it? Unfortunately, We can't. In ChromeOS, |task_runner| is the |ui_task_runner| and we would always like to handle the policy update on the |network_task_runner| https://codereview.chromium.org/639233002/diff/640001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher.h (right): https://codereview.chromium.org/639233002/diff/640001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:45: // Implemented by each platform. This |task_runner| should be an IO message On 2014/10/29 18:27:51, Wez wrote: > Remove "This" - it doesn't make sense grammatically. Done. https://codereview.chromium.org/639233002/diff/640001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:47: // |task_runner| so it can be called from any thread. |policy_service| is On 2014/10/29 18:27:51, Wez wrote: > This isn't true under ChromeOS, though. Is it even a property we care about - we > seem to only use it from the same thread? To be honest, I don't know the historical context of why the policy_watcher reposts on the existing thread. I merely document it off the existing behavior of the code. In order to avoid documenting unnecessary rationale, I am going to remove the comment regarding the task_runner. No information is better than information that converting existing behavior into a rationale :) https://codereview.chromium.org/639233002/diff/640001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:49: // |policy_service| remains valid for the lifetime of PolicyWatcherChromeOS. On 2014/10/29 18:27:51, Wez wrote: > The caller does not see PolicyWatcherChromeOS - it's an implementation detail - > so don't mention it in the comment. Done. https://codereview.chromium.org/639233002/diff/640001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:49: // |policy_service| remains valid for the lifetime of PolicyWatcherChromeOS. On 2014/10/29 18:27:51, Wez wrote: > This would be cleaner with separate CrOS and non-CrOS creator functions, IMO. Totally, but that would also introduce ifdef's on the call site. In the long run, we would want to get rid of policy hack (our custom cross platform implementation) and move towards the policy_service in the policy component anyways, so leaving the policy_service in the Ctor is not as bad as it seems.
https://codereview.chromium.org/639233002/diff/640001/remoting/host/chromotin... File remoting/host/chromoting_host_context.h (right): https://codereview.chromium.org/639233002/diff/640001/remoting/host/chromotin... remoting/host/chromoting_host_context.h:40: // Instead, we re-use the |url_request_context_getter| in the browser process. On 2014/10/29 22:20:17, kelvinp wrote: > On 2014/10/29 18:27:51, Wez wrote: > > If this is the only reason you need to re-use the browser's getter then can't > > you pass the chromoting host context off to an IO capable thread to be > deleted, > > so that there is no reference to the getter remaining on a non-IO thread? > > Your suggestion involves changing the threading model on shut down that is risky > and requires sufficient testing. Given the existing approach works fine and we > would like to land the feature in M40. Unless that are significant advantages > of not passing the |url_request_context_getter|. I would prefer to leave the > code as it is. Acknowledged. https://codereview.chromium.org/639233002/diff/640001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/639233002/diff/640001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:315: // |network_task_runner| on other platforms. On 2014/10/29 22:20:17, kelvinp wrote: > On 2014/10/29 18:27:51, Wez wrote: > > That's just an implementation detail of the CrOS policy watcher though - > > wouldn't it be cleaner to fix that to post the update on the |task_runner| > that > > is passed to it? > > Unfortunately, We can't. In ChromeOS, |task_runner| is the |ui_task_runner| and > we would always like to handle the policy update on the |network_task_runner| No it isn't - it's still passed the network task runner (see line 488), but then internally it picks up the UI task runner. https://codereview.chromium.org/639233002/diff/640001/remoting/host/policy_ha... File remoting/host/policy_hack/policy_watcher.h (right): https://codereview.chromium.org/639233002/diff/640001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:47: // |task_runner| so it can be called from any thread. |policy_service| is On 2014/10/29 22:20:17, kelvinp wrote: > On 2014/10/29 18:27:51, Wez wrote: > > This isn't true under ChromeOS, though. Is it even a property we care about - > we > > seem to only use it from the same thread? > > To be honest, I don't know the historical context of why the policy_watcher > reposts on the existing thread. I merely document it off the existing behavior > of the code. In order to avoid documenting unnecessary rationale, I am going to > remove the comment regarding the task_runner. No information is better than > information that converting existing behavior into a rationale :) Acknowledged. https://codereview.chromium.org/639233002/diff/640001/remoting/host/policy_ha... remoting/host/policy_hack/policy_watcher.h:49: // |policy_service| remains valid for the lifetime of PolicyWatcherChromeOS. On 2014/10/29 22:20:17, kelvinp wrote: > On 2014/10/29 18:27:51, Wez wrote: > > This would be cleaner with separate CrOS and non-CrOS creator functions, IMO. > > Totally, but that would also introduce ifdef's on the call site. In the long > run, we would want to get rid of policy hack (our custom cross platform > implementation) and move towards the policy_service in the policy component > anyways, so leaving the policy_service in the Ctor is not as bad as it seems. Acknowledged. https://codereview.chromium.org/639233002/diff/680001/remoting/host/chromotin... File remoting/host/chromoting_host_context.cc (right): https://codereview.chromium.org/639233002/diff/680001/remoting/host/chromotin... remoting/host/chromoting_host_context.cc:126: // is passed in as the quit closure. TODO(kelvinp): Fix this (See nit: Fine to leave the TODO starting on the next line; TODOs usually get their own line. :)
lgtm
Note that your CL description needs updating before this lands.
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/639233002/700001
Message was sent while issue was closed.
Committed patchset #12 (id:700001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/54dde6f02d121ff745e66b57205583087ff720ec Cr-Commit-Position: refs/heads/master@{#302034}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:700001) has been created in https://codereview.chromium.org/686373002/ by kelvinp@chromium.org. The reason for reverting is: Reverting due to build failure on Linux Ozone builder. http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/639233002/720001
Message was sent while issue was closed.
Committed patchset #13 (id:720001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/561074cfd46c253dcdc456fdc63693aff4d1be32 Cr-Commit-Position: refs/heads/master@{#302162} |
