[Dial] Refactor DialRegistry and DeviceDescriptionFetcher so they can be used by MediaSinkService

[Dial] Refactor DialRegistry and DeviceDescriptionFetcher so they can be used by MediaSinkService

- Make DialRegistry listen to multiple observers (DialAPI and MediaSinkService)
- Make DeviceDescriptionFetcher run on both UI (for extension functions) and IO (for MediaSinkService) threads

BUG=687375
Review-Url: https://codereview.chromium.org/2702503003
Cr-Commit-Position: refs/heads/master@{#455192}
Committed: https://chromium.googlesource.com/chromium/src/+/1b26f93c8009b26acc32161860c6dd1aac3d9818

[zhaobin, 2017-02-16 18:33:15]

zhaobin@chromium.org changed reviewers:
+ imcheng@chromium.org, mfoltz@chromium.org

[imcheng, 2017-02-22 03:45:05]

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/dial/device_description_fetcher.cc (right):

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/device_description_fetcher.cc:50:
DCHECK_CURRENTLY_ON(thread_id_);
If you are checking that the methods are being called on the same thread as the
one the object is created in, then you can just use a base::ThreadChecker.

https://cs.chromium.org/chromium/src/base/threading/thread_checker.h?q=base::...

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/dial/device_description_fetcher.h (right):

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/device_description_fetcher.h:32: class
DeviceDescriptionFetcher : public net::URLFetcherDelegate {
I would make it clear that an instance must only be used exclusively on the UI
thread or exclusively on the IO thread (e.g., you can't create it on UI thread
then call Start() on IO thread). On the other hand, I feel it would it simpler
if we only allowed this from the IO thread, as that means fewer thread hops for
both DialMediaSinkService and dial.fetchDeviceDescription implementation. The
net::URLRequestContextGetter itself is RefCountedThreadSafe which means you can
pass it from UI to IO thread. Mark, WDYT?

[zhaobin, 2017-02-24 05:39:47]

Patchset #2 (id:20001) has been deleted

[mark a. foltz, 2017-03-01 21:44:00]

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/dial/device_description_fetcher.cc (right):

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/device_description_fetcher.cc:50:
DCHECK_CURRENTLY_ON(thread_id_);
On 2017/02/22 at 03:45:04, imcheng wrote:
> If you are checking that the methods are being called on the same thread as
the one the object is created in, then you can just use a base::ThreadChecker.
> 
>
https://cs.chromium.org/chromium/src/base/threading/thread_checker.h?q=base::...

+1

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/dial/device_description_fetcher.h (right):

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/device_description_fetcher.h:31: // This
class is not thread safe and must be used on the UI or IO thread.
It's safe to use the URLFetcher from either thread, so this class should also be
safe.  However the public API must use the thread checker to prevent
cross-thread usage.

Nit: I would just leave the comment above as "not thread safe"

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/dial/dial_registry.cc (right):

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/dial_registry.cc:368: for (auto* observer :
observers_)
for (auto& observer : observers_) (from base::ObserverList)

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/dial/dial_registry.h (right):

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/dial_registry.h:60: DialRegistry(Observer*
dial_api,
Can we use the Register API below instead of also passing via ctor?  (If this is
a big change, fine to leave a TODO).

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/dial_registry.h:189: std::set<Observer*>
observers_;
Can you use base::ObserverList?

[zhaobin, 2017-03-02 07:04:35]

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/dial/device_description_fetcher.cc (right):

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/device_description_fetcher.cc:50:
DCHECK_CURRENTLY_ON(thread_id_);
On 2017/03/01 21:44:00, mark a. foltz wrote:
> On 2017/02/22 at 03:45:04, imcheng wrote:
> > If you are checking that the methods are being called on the same thread as
> the one the object is created in, then you can just use a base::ThreadChecker.
> > 
> >
>
https://cs.chromium.org/chromium/src/base/threading/thread_checker.h?q=base::...
> 
> +1

Done.

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/dial/device_description_fetcher.h (right):

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/device_description_fetcher.h:31: // This
class is not thread safe and must be used on the UI or IO thread.
On 2017/03/01 21:44:00, mark a. foltz wrote:
> It's safe to use the URLFetcher from either thread, so this class should also
be
> safe.  However the public API must use the thread checker to prevent
> cross-thread usage.
> 
> Nit: I would just leave the comment above as "not thread safe" 

Done.

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/device_description_fetcher.h:32: class
DeviceDescriptionFetcher : public net::URLFetcherDelegate {
On 2017/02/22 03:45:04, imcheng wrote:
> I would make it clear that an instance must only be used exclusively on the UI
> thread or exclusively on the IO thread (e.g., you can't create it on UI thread
> then call Start() on IO thread). On the other hand, I feel it would it simpler
> if we only allowed this from the IO thread, as that means fewer thread hops
for
> both DialMediaSinkService and dial.fetchDeviceDescription implementation. The
> net::URLRequestContextGetter itself is RefCountedThreadSafe which means you
can
> pass it from UI to IO thread. Mark, WDYT?

Acknowledged.

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/dial/dial_registry.cc (right):

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/dial_registry.cc:368: for (auto* observer :
observers_)
On 2017/03/01 21:44:00, mark a. foltz wrote:
> for (auto& observer : observers_) (from base::ObserverList)

Done.

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/dial/dial_registry.h (right):

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/dial_registry.h:60: DialRegistry(Observer*
dial_api,
On 2017/03/01 21:44:00, mark a. foltz wrote:
> Can we use the Register API below instead of also passing via ctor?  (If this
is
> a big change, fine to leave a TODO).

Done.

https://codereview.chromium.org/2702503003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/dial_registry.h:189: std::set<Observer*>
observers_;
On 2017/03/01 21:44:00, mark a. foltz wrote:
> Can you use base::ObserverList?

Done.

[mark a. foltz, 2017-03-02 17:20:48]

lgtm

[zhaobin, 2017-03-02 20:25:38]

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 20:26:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 21:22:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 21:22:52]

Dry run: This issue passed the CQ dry run.

[imcheng, 2017-03-06 20:56:30]

lgtm after comments addressed.

https://codereview.chromium.org/2702503003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/dial/device_description_fetcher.cc (right):

https://codereview.chromium.org/2702503003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/dial/device_description_fetcher.cc:32:
BrowserThread::ID thread_id,
thread_id can be removed now.

https://codereview.chromium.org/2702503003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/dial/device_description_fetcher.cc:39:
DCHECK(thread_checker_.CalledOnValidThread());
this DCHECK is not necessary since it's just been created.

https://codereview.chromium.org/2702503003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/dial/device_description_fetcher.h (right):

https://codereview.chromium.org/2702503003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/dial/device_description_fetcher.h:42:
net::URLRequestContextGetter* request_context,
I believe this should be a scoped_refptr; this object is passed from UI thread
to IO thread. URLRequestContextGetter inherits RefCountedThreadSafe.

https://codereview.chromium.org/2702503003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/dial/dial_registry.h (right):

https://codereview.chromium.org/2702503003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/dial/dial_registry.h:61: DialRegistry(const
base::TimeDelta& refresh_interval,
base::TimeDelta is small enough that usually it is passed by value instead of
const ref.

[zhaobin, 2017-03-07 00:13:18]

https://codereview.chromium.org/2702503003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/dial/device_description_fetcher.cc (right):

https://codereview.chromium.org/2702503003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/dial/device_description_fetcher.cc:32:
BrowserThread::ID thread_id,
On 2017/03/06 20:56:30, imcheng wrote:
> thread_id can be removed now.

Done.

https://codereview.chromium.org/2702503003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/dial/device_description_fetcher.cc:39:
DCHECK(thread_checker_.CalledOnValidThread());
On 2017/03/06 20:56:30, imcheng wrote:
> this DCHECK is not necessary since it's just been created.

Done.

https://codereview.chromium.org/2702503003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/dial/device_description_fetcher.h (right):

https://codereview.chromium.org/2702503003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/dial/device_description_fetcher.h:42:
net::URLRequestContextGetter* request_context,
On 2017/03/06 20:56:30, imcheng wrote:
> I believe this should be a scoped_refptr; this object is passed from UI thread
> to IO thread. URLRequestContextGetter inherits RefCountedThreadSafe.

Pass in raw pointer (Profile::GetRequestContext() returns raw pointer) and store
it as scoped_refptr?

https://codereview.chromium.org/2702503003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/dial/dial_registry.h (right):

https://codereview.chromium.org/2702503003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/dial/dial_registry.h:61: DialRegistry(const
base::TimeDelta& refresh_interval,
On 2017/03/06 20:56:30, imcheng wrote:
> base::TimeDelta is small enough that usually it is passed by value instead of
> const ref.

Done.

[zhaobin, 2017-03-07 00:37:20]

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 00:37:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 03:28:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 03:28:59]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[zhaobin, 2017-03-07 18:24:49]

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 18:26:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 19:12:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 19:12:14]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-03-07 20:02:09]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-03-07 20:02:10]

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/2702503003/#ps60001
(title: "resolve code review comments from Derek")

[commit-bot: I haz the power, 2017-03-07 20:05:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 20:16:27]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488916929588670,
"parent_rev": "7f84aee35099f2ddf92308987375481aa3ab76bf", "commit_rev":
"1b26f93c8009b26acc32161860c6dd1aac3d9818"}

[commit-bot: I haz the power, 2017-03-07 20:17:15]

Description was changed from

==========
[Dial] Refactor DialRegistry and DeviceDescriptionFetcher so they can be used by
MediaSinkService

- Make DialRegistry listen to multiple observers (DialAPI and MediaSinkService)
- Make DeviceDescriptionFetcher run on both UI (for extension functions) and IO
(for MediaSinkService) threads

BUG=687375
==========

to

==========
[Dial] Refactor DialRegistry and DeviceDescriptionFetcher so they can be used by
MediaSinkService

- Make DialRegistry listen to multiple observers (DialAPI and MediaSinkService)
- Make DeviceDescriptionFetcher run on both UI (for extension functions) and IO
(for MediaSinkService) threads

BUG=687375

Review-Url: https://codereview.chromium.org/2702503003
Cr-Commit-Position: refs/heads/master@{#455192}
Committed:
https://chromium.googlesource.com/chromium/src/+/1b26f93c8009b26acc32161860c6...
==========

[commit-bot: I haz the power, 2017-03-07 20:17:16]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/1b26f93c8009b26acc32161860c6...

[imcheng, 2017-03-07 20:32:58]

Sorry for the late comment, but it's fine to address in a follow up patch when
the refactored code is used in IO thread.

https://codereview.chromium.org/2702503003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/dial/device_description_fetcher.h (right):

https://codereview.chromium.org/2702503003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/dial/device_description_fetcher.h:42:
net::URLRequestContextGetter* request_context,
On 2017/03/07 00:13:18, zhaobin wrote:
> On 2017/03/06 20:56:30, imcheng wrote:
> > I believe this should be a scoped_refptr; this object is passed from UI
thread
> > to IO thread. URLRequestContextGetter inherits RefCountedThreadSafe.
> 
> Pass in raw pointer (Profile::GetRequestContext() returns raw pointer) and
store
> it as scoped_refptr?

Most likely you will pass in a scoped_refptr here, but not a big deal until you
start to use this on the IO thread. The reason is that if this object is created
on the IO thread, then you would need to pass in a net::URLRequestContextGetter
from the UI thread first. This means it should already be wrapped in a
scoped_refptr when posting the task, since you can't pass in a raw pointer to a
ref counted type  as a function argument in base::Bind.

More info here:
https://www.chromium.org/developers/coding-style/important-abstractions-and-d...