Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2891023002:
[cast_channel] Make CastSocket not inherit from ApiResource (Closed)
Description[cast_channel] Make CastSocket not inherit from ApiResource
We are moving cast_channel related files from //extensions to //components and need to remove extensions dependencies.
Add CastChannelService as BrowserContextKeyedService to replace extensions::ApiResourceManager and manage cast sockets.
BUG=723944
Review-Url: https://codereview.chromium.org/2891023002
Cr-Commit-Position: refs/heads/master@{#475057}
Committed: https://chromium.googlesource.com/chromium/src/+/6d0877aff7c0a14b421d2635cf60da4c4d25a948
Patch Set 1 #
Total comments: 34
Patch Set 2 : resolve code review comments from Mark #
Total comments: 6
Patch Set 3 : resolve code review comments from Mark #
Total comments: 12
Patch Set 4 : resolve code review comments from Derek #Patch Set 5 : resolve code review comments from Derek #Messages
Total messages: 38 (24 generated)
zhaobin@chromium.org changed reviewers: + mfoltz@chromium.org
Partial comments (didn't complete review, apologies) https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_channel_service.cc (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.cc:22: int id = sockets_.size() + 1; This will assign duplicate ids as the size of sockets_ changes. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.cc:29: DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); DCHECK(channel_id > 0) https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.cc:43: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); Since sockets_ is created on the IO thread it follows that this API must also be called on the IO thread, so you could DCHECK_CURRENTLY_ON throughout. However thread_checker_ is more friendly for unit testing IIUC. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_channel_service.h (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.h:21: // This class manages cast sockets created by CastChannelAPI. Does this comment belong with CastChannelService? https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.h:23: class CastSocketData { Bikeshed: CastSocketRegistry? https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.h:28: // Adds |socket| to |manager_| and returns the new channel_id. Takes ownership Where is |manager_| declared? https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.h:30: int AddSocket(CastSocket* socket); When ownership is transferred via parameter, it preferable to use a moveable type, in this case std::unique_ptr<CastSocket> https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.h:34: void RemoveSocket(int channel_id); Nit: Consider returning std::unique_ptr<CastSocket> as this class is now relinquishing ownership. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.h:52: class CastChannelService : public KeyedService { Bikeshed: Slight preference to name this class CastSocketManager, but I don't feel strongly https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.h:58: CastSocketData* GetOrCreateSocketData(); What thread is expected to invoke this method? https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_channel_service_factory.h (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service_factory.h:18: class CastChannelServiceFactory : public BrowserContextKeyedServiceFactory { I think a CastSocket created by one profile (browser context) could be shared with other profiles. Ultimately the set of cast sockets will be determined by the set of Cast devices available on the user's LAN, which is not a function of the browser profile. The Cast MRP should be responsible for not exposing any information across profiles inappropriately (I don't think it cares whether the socket is per-profile or shared). I think this is fine to land as it matches existing behavior; but does examination of the Cast MRP code reveal any issues with sharing sockets? If not, then the CastChannelService could be refactored as a LazySingleton without the need for a factory :)
Remaining comments https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_channel_service.cc (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.cc:34: DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); DCHECK(channel_id > 0) https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_channel_service_factory.cc (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service_factory.cc:46: return context; This will return separate services for a normal profile and its incognito profile. If we decide to keep this as a profile-keyed service, it would make sense to return the same instance for incognito (to be consistent with Media Router). https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket.h:112: virtual const std::string& owner_extension_id() const = 0; Is this still necessary? I think this is only used by ApiResourceManager but I could be wrong. Since we'll have sockets created by the browser (and not any extension), this will eventually be empty (and possibly unused).
zhaobin@chromium.org changed reviewers: + imcheng@chromium.org
+imcheng@ https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_channel_service.cc (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.cc:22: int id = sockets_.size() + 1; On 2017/05/18 18:02:26, mark a. foltz wrote: > This will assign duplicate ids as the size of sockets_ changes. Done. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.cc:29: DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); On 2017/05/18 18:02:26, mark a. foltz wrote: > DCHECK(channel_id > 0) Done. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.cc:34: DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); On 2017/05/18 19:12:04, mark a. foltz wrote: > DCHECK(channel_id > 0) Done. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.cc:43: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); On 2017/05/18 18:02:26, mark a. foltz wrote: > Since sockets_ is created on the IO thread it follows that this API must also be > called on the IO thread, so you could DCHECK_CURRENTLY_ON throughout. However > thread_checker_ is more friendly for unit testing IIUC. Seems unable to use |thread_checker_| here. CastChannelService is created/destroyed on the UI thread, but this API must be called on the IO thread. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_channel_service.h (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.h:21: // This class manages cast sockets created by CastChannelAPI. On 2017/05/18 18:02:26, mark a. foltz wrote: > Does this comment belong with CastChannelService? CastSocketRegistry does the actual management...Modified a little. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.h:23: class CastSocketData { On 2017/05/18 18:02:26, mark a. foltz wrote: > Bikeshed: CastSocketRegistry? Done. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.h:28: // Adds |socket| to |manager_| and returns the new channel_id. Takes ownership On 2017/05/18 18:02:26, mark a. foltz wrote: > Where is |manager_| declared? Done. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.h:30: int AddSocket(CastSocket* socket); On 2017/05/18 18:02:26, mark a. foltz wrote: > When ownership is transferred via parameter, it preferable to use a moveable > type, in this case std::unique_ptr<CastSocket> Done. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.h:34: void RemoveSocket(int channel_id); On 2017/05/18 18:02:26, mark a. foltz wrote: > Nit: Consider returning std::unique_ptr<CastSocket> as this class is now > relinquishing ownership. Done. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.h:52: class CastChannelService : public KeyedService { On 2017/05/18 18:02:26, mark a. foltz wrote: > Bikeshed: Slight preference to name this class CastSocketManager, but I don't > feel strongly It is a KeyedService, would like to keep Service...CastSocketService? https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.h:58: CastSocketData* GetOrCreateSocketData(); On 2017/05/18 18:02:26, mark a. foltz wrote: > What thread is expected to invoke this method? Done. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_channel_service_factory.cc (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service_factory.cc:46: return context; On 2017/05/18 19:12:05, mark a. foltz wrote: > This will return separate services for a normal profile and its incognito > profile. If we decide to keep this as a profile-keyed service, it would make > sense to return the same instance for incognito (to be consistent with Media > Router). Verified that CastChannelAPI has the same browser context instance for both normal profile and incognito profile. Added a comment to CastChannelServiceFactory::GetForBrowserContext() function. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_channel_service_factory.h (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service_factory.h:18: class CastChannelServiceFactory : public BrowserContextKeyedServiceFactory { On 2017/05/18 18:02:27, mark a. foltz wrote: > I think a CastSocket created by one profile (browser context) could be shared > with other profiles. Ultimately the set of cast sockets will be determined by > the set of Cast devices available on the user's LAN, which is not a function of > the browser profile. The Cast MRP should be responsible for not exposing any > information across profiles inappropriately (I don't think it cares whether the > socket is per-profile or shared). > > I think this is fine to land as it matches existing behavior; but does > examination of the Cast MRP code reveal any issues with sharing sockets? > > If not, then the CastChannelService could be refactored as a LazySingleton > without the need for a factory :) Created crbug.com/725717 https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket.h:112: virtual const std::string& owner_extension_id() const = 0; On 2017/05/18 19:12:05, mark a. foltz wrote: > Is this still necessary? I think this is only used by ApiResourceManager but I > could be wrong. > > Since we'll have sockets created by the browser (and not any extension), this > will eventually be empty (and possibly unused). Still needed when dispatch UI event... https://cs.chromium.org/chromium/src/extensions/browser/api/cast_channel/cast...
LGTM with a couple of nit picks https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_channel_service.cc (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.cc:43: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); On 2017/05/24 at 01:51:39, zhaobin wrote: > On 2017/05/18 18:02:26, mark a. foltz wrote: > > Since sockets_ is created on the IO thread it follows that this API must also be > > called on the IO thread, so you could DCHECK_CURRENTLY_ON throughout. However > > thread_checker_ is more friendly for unit testing IIUC. > > Seems unable to use |thread_checker_| here. CastChannelService is created/destroyed on the UI thread, but this API must be called on the IO thread. This should be called out in the comments. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_channel_service.h (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.h:52: class CastChannelService : public KeyedService { On 2017/05/24 at 01:51:40, zhaobin wrote: > On 2017/05/18 18:02:26, mark a. foltz wrote: > > Bikeshed: Slight preference to name this class CastSocketManager, but I don't > > feel strongly > > It is a KeyedService, would like to keep Service...CastSocketService? Yeah that makes sense. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_channel_service_factory.cc (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service_factory.cc:46: return context; On 2017/05/24 at 01:51:40, zhaobin wrote: > On 2017/05/18 19:12:05, mark a. foltz wrote: > > This will return separate services for a normal profile and its incognito > > profile. If we decide to keep this as a profile-keyed service, it would make > > sense to return the same instance for incognito (to be consistent with Media > > Router). > > Verified that CastChannelAPI has the same browser context instance for both normal profile and incognito profile. Added a comment to CastChannelServiceFactory::GetForBrowserContext() function. Cool :) https://codereview.chromium.org/2891023002/diff/20001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_service.h (right): https://codereview.chromium.org/2891023002/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_service.h:37: // Returns the socket corresponding to |channel_id| if one exists, or null nit: nullptr https://codereview.chromium.org/2891023002/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_service.h:43: static int id_; last_channel_id_ https://codereview.chromium.org/2891023002/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_service.h:45: // The collection of CastSocket. .. keyed by channel_id
Quick comment on lifetime. Most keyed services are created in advance (on browser startup) and there is a way to express dependencies from one keyed service on another (for example from the CastChannelAPI to to the CastChannelService). However I think lazy initialization here is okay since there is no reason to create this service for users that don't discover Cast devices. You might check the keyed service documentation in case there are any concerns.
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_channel_service.cc (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_channel_service.cc:43: DCHECK_CURRENTLY_ON(content::BrowserThread::IO); On 2017/05/24 17:44:10, mark a. foltz wrote: > On 2017/05/24 at 01:51:39, zhaobin wrote: > > On 2017/05/18 18:02:26, mark a. foltz wrote: > > > Since sockets_ is created on the IO thread it follows that this API must > also be > > > called on the IO thread, so you could DCHECK_CURRENTLY_ON throughout. > However > > > thread_checker_ is more friendly for unit testing IIUC. > > > > Seems unable to use |thread_checker_| here. CastChannelService is > created/destroyed on the UI thread, but this API must be called on the IO > thread. > > This should be called out in the comments. Done. https://codereview.chromium.org/2891023002/diff/20001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_service.h (right): https://codereview.chromium.org/2891023002/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_service.h:37: // Returns the socket corresponding to |channel_id| if one exists, or null On 2017/05/24 17:44:10, mark a. foltz wrote: > nit: nullptr Done. https://codereview.chromium.org/2891023002/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_service.h:43: static int id_; On 2017/05/24 17:44:10, mark a. foltz wrote: > last_channel_id_ Done. https://codereview.chromium.org/2891023002/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_service.h:45: // The collection of CastSocket. On 2017/05/24 17:44:10, mark a. foltz wrote: > .. keyed by channel_id Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good, just a couple of questions that I need some clarifying. https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket.h:112: virtual const std::string& owner_extension_id() const = 0; On 2017/05/24 01:51:40, zhaobin wrote: > On 2017/05/18 19:12:05, mark a. foltz wrote: > > Is this still necessary? I think this is only used by ApiResourceManager but > I > > could be wrong. > > > > Since we'll have sockets created by the browser (and not any extension), this > > will eventually be empty (and possibly unused). > > Still needed when dispatch UI event... > > https://cs.chromium.org/chromium/src/extensions/browser/api/cast_channel/cast... Can we just use extension_->id() (which is used to populate owner_extension_id) in the ExtensionFunction class? https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket.h:61: explicit CastSocket(); Remove this constructor since CastSocket is to be pure virtual. https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket.h:65: static const char* service_name() { return "CastSocketImplManager"; } Can this be removed? https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket.h:185: static const char* service_name() { return "CastSocketManager"; } Can this be removed? https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_service_unittest.cc (right): https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_service_unittest.cc:28: auto* registry = cast_socket_service_.GetOrCreateSocketRegistry(); The counter used for allocating socket ids is a static variable. I think it might cause problems in this test. Can you try to run this test with --gtest_repeat=2 to see if that's the case? https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_test_util.cc (right): https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_test_util.cc:38: : mock_transport_(new MockCastTransport), MockCastTransport() for consistency.
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/2891023002/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket.h:112: virtual const std::string& owner_extension_id() const = 0; On 2017/05/24 20:24:32, imcheng wrote: > On 2017/05/24 01:51:40, zhaobin wrote: > > On 2017/05/18 19:12:05, mark a. foltz wrote: > > > Is this still necessary? I think this is only used by ApiResourceManager > but > > I > > > could be wrong. > > > > > > Since we'll have sockets created by the browser (and not any extension), > this > > > will eventually be empty (and possibly unused). > > > > Still needed when dispatch UI event... > > > > > https://cs.chromium.org/chromium/src/extensions/browser/api/cast_channel/cast... > > Can we just use extension_->id() (which is used to populate owner_extension_id) > in the ExtensionFunction class? Done. https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket.h (right): https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket.h:61: explicit CastSocket(); On 2017/05/24 20:24:32, imcheng wrote: > Remove this constructor since CastSocket is to be pure virtual. Done. https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket.h:65: static const char* service_name() { return "CastSocketImplManager"; } On 2017/05/24 20:24:32, imcheng wrote: > Can this be removed? Done. https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket.h:185: static const char* service_name() { return "CastSocketManager"; } On 2017/05/24 20:24:32, imcheng wrote: > Can this be removed? Done. https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_service_unittest.cc (right): https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_service_unittest.cc:28: auto* registry = cast_socket_service_.GetOrCreateSocketRegistry(); On 2017/05/24 20:24:32, imcheng wrote: > The counter used for allocating socket ids is a static variable. I think it > might cause problems in this test. Can you try to run this test with > --gtest_repeat=2 to see if that's the case? --gtest_repeat=2 passed (think it starts a new process for the second run?) https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_test_util.cc (right): https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_test_util.cc:38: : mock_transport_(new MockCastTransport), On 2017/05/24 20:24:32, imcheng wrote: > MockCastTransport() for consistency. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_service_unittest.cc (right): https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_service_unittest.cc:28: auto* registry = cast_socket_service_.GetOrCreateSocketRegistry(); On 2017/05/24 22:15:07, zhaobin wrote: > On 2017/05/24 20:24:32, imcheng wrote: > > The counter used for allocating socket ids is a static variable. I think it > > might cause problems in this test. Can you try to run this test with > > --gtest_repeat=2 to see if that's the case? > > --gtest_repeat=2 passed (think it starts a new process for the second run?) Ah ok. I would still try not to assume the ids are (0, 1, 2) since the counter could be modified by other code (e.g., if the test process ran TestRemoveAndGetSocket then TestAddSocket). Maybe it is sufficient to check that the ids of the sockets aren't equal?
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
zhaobin@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
+rdevlin.cronin@ to review: extensions/browser/BUILD.gn https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_service_unittest.cc (right): https://codereview.chromium.org/2891023002/diff/40001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_service_unittest.cc:28: auto* registry = cast_socket_service_.GetOrCreateSocketRegistry(); On 2017/05/25 20:20:17, imcheng wrote: > On 2017/05/24 22:15:07, zhaobin wrote: > > On 2017/05/24 20:24:32, imcheng wrote: > > > The counter used for allocating socket ids is a static variable. I think it > > > might cause problems in this test. Can you try to run this test with > > > --gtest_repeat=2 to see if that's the case? > > > > --gtest_repeat=2 passed (think it starts a new process for the second run?) > > Ah ok. I would still try not to assume the ids are (0, 1, 2) since the counter > could be modified by other code (e.g., if the test process ran > TestRemoveAndGetSocket then TestAddSocket). Maybe it is sufficient to check that > the ids of the sockets aren't equal? Done.
extensions/browser/BUILD.gn lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zhaobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2891023002/#ps80001 (title: "resolve code review comments from Derek")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495821368912090,
"parent_rev": "f7b1b07daa4fb6337f9806a00e3a299aeb613641", "commit_rev":
"6d0877aff7c0a14b421d2635cf60da4c4d25a948"}
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495821368912090,
"parent_rev": "f7b1b07daa4fb6337f9806a00e3a299aeb613641", "commit_rev":
"6d0877aff7c0a14b421d2635cf60da4c4d25a948"}
Message was sent while issue was closed.
Description was changed from ========== [cast_channel] Make CastSocket not inherit from ApiResource We are moving cast_channel related files from //extensions to //components and need to remove extensions dependencies. Add CastChannelService as BrowserContextKeyedService to replace extensions::ApiResourceManager and manage cast sockets. BUG=723944 ========== to ========== [cast_channel] Make CastSocket not inherit from ApiResource We are moving cast_channel related files from //extensions to //components and need to remove extensions dependencies. Add CastChannelService as BrowserContextKeyedService to replace extensions::ApiResourceManager and manage cast sockets. BUG=723944 Review-Url: https://codereview.chromium.org/2891023002 Cr-Commit-Position: refs/heads/master@{#475057} Committed: https://chromium.googlesource.com/chromium/src/+/6d0877aff7c0a14b421d2635cf60... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/6d0877aff7c0a14b421d2635cf60... |
