Refactor media permission dispatcher class and create proxy class for non-UI threads.

Refactor media permission dispatcher class and create proxy class for non-UI threads.

Refactor media permission dispatcher into
1) a base class which manages pending PermissionService calls and the dispatching of the callback.
2) a sub class which uses the base class to talk to Mojo service.

Both classes are not thread safe, the same as before and can only be used by UI thread.

Also create a new subclass whose purpose is to enable non-UI threads calling into this class through the proxy and receive the callback on its own thread.

This is used by the ongoing CL https://codereview.chromium.org/1349823004/

TBR=xhwang@chromium.org

BUG=520101
Committed: https://crrev.com/f4282b031b403e1358125e0db371bda17cac604a
Cr-Commit-Position: refs/heads/master@{#350689}

[guoweis_left_chromium, 2015-09-21 19:18:21]

guoweis@chromium.org changed reviewers:
+ ddorwin@chromium.org

[guoweis_left_chromium, 2015-09-21 19:19:53]

guoweis@chromium.org changed reviewers:
- ddorwin@chromium.org

[guoweis_left_chromium, 2015-09-21 19:21:41]

guoweis@chromium.org changed reviewers:
+ ddorwin@chromium.org

[guoweis_left_chromium, 2015-09-21 19:21:41]

PTAL. The proxy class is modeled after It2MeConfirmationDialogProxy
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/it2m....

[guoweis_left_chromium, 2015-09-21 19:34:40]

Patchset #1 (id:1) has been deleted

[guoweis_left_chromium, 2015-09-21 19:56:23]

guoweis@chromium.org changed reviewers:
+ xhwang@chromium.org

[guoweis_left_chromium, 2015-09-21 19:57:09]

guoweis@chromium.org changed reviewers:
- ddorwin@chromium.org, xhwang@chromium.org

[guoweis_left_chromium, 2015-09-21 20:00:38]

guoweis@chromium.org changed reviewers:
+ perkj@chromium.org

[guoweis_left_chromium, 2015-09-21 20:00:39]

PTAL.

[perkj_chrome, 2015-09-22 08:24:15]

perkj@chromium.org changed reviewers:
+ guidou@chromium.org

[perkj_chrome, 2015-09-22 08:24:16]

Guido, is this related to what you want to do to?

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher.h (right):

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher.h:38: 
Please re-add thread checker to enforce that this is single threaded since the
base functions are still used. 
You can have a protected method tread_checker() if you want.

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher_impl.cc (right):

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher_impl.cc:60:
base::Unretained(this),
Why is it safe to use Unretained?  Looks scary to me. Why not use weakptr?

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher_impl.h (right):

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher_impl.h:26: class
CONTENT_EXPORT MediaPermissionDispatcherImpl
Why do you need this extra level? I understand you migh want the proxy but why
not just to the same thing from MediaPermissionDispatcher and don't add
MediaPermissionDispatcherImpl.

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher_proxy.cc (right):

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher_proxy.cc:71:
base::Unretained(this), request_id));
is this safe?

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher_proxy.cc:108:
base::Bind(&Core::HasPermission, base::Unretained(core_.get()), type,
Why is Unretained safe here? Document.

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher_proxy.cc:118: <<
"RequestPermission is not supported in MediaPermissionDispatcherProxy";
why not implement this too at the same time?

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher_proxy.h (right):

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher_proxy.h:33: void
HasPermission(Type type,
protected:

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4991: scoped_ptr<media::MediaPermission>
RenderFrameImpl::CreateMediaPermissionProxy(
Where is this used? Can it be moved away from the RenderFrame ?

[guoweis_left_chromium, 2015-09-22 18:48:49]

PTAL.

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher.h (right):

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher.h:38: 
On 2015/09/22 08:24:16, perkj wrote:
> Please re-add thread checker to enforce that this is single threaded since the
> base functions are still used. 
> You can have a protected method tread_checker() if you want.

Done.

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher_impl.cc (right):

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher_impl.cc:60:
base::Unretained(this),
On 2015/09/22 08:24:16, perkj wrote:
> Why is it safe to use Unretained?  Looks scary to me. Why not use weakptr? 

Done.

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher_impl.h (right):

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher_impl.h:26: class
CONTENT_EXPORT MediaPermissionDispatcherImpl
On 2015/09/22 08:24:16, perkj wrote:
> Why do you need this extra level? I understand you migh want the proxy but why
> not just to the same thing from MediaPermissionDispatcher and don't add
> MediaPermissionDispatcherImpl. 

The MediaPermissionDispatcherImpl takes render_frame as parameter for ctor. I
don't need that for proxy class. However, proxy does need some support for
mananginng pending PermissionService calls which is intertwined inside
GetPermissionStatus and RequestPermission in the original implementations.

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher_proxy.cc (right):

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher_proxy.cc:71:
base::Unretained(this), request_id));
On 2015/09/22 08:24:16, perkj wrote:
> is this safe?

Done.

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher_proxy.cc:108:
base::Bind(&Core::HasPermission, base::Unretained(core_.get()), type,
On 2015/09/22 08:24:16, perkj wrote:
> Why is Unretained safe here? Document. 

Done.

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher_proxy.cc:118: <<
"RequestPermission is not supported in MediaPermissionDispatcherProxy";
On 2015/09/22 08:24:16, perkj wrote:
> why not implement this too at the same time? 

I believe further down, this could trigger a prompt and I'd like that to be an
intentional decision.

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher_proxy.h (right):

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher_proxy.h:33: void
HasPermission(Type type,
On 2015/09/22 08:24:16, perkj wrote:
> protected:

I'm not sure I understand this comment. They are public in the base class. Why
do we want to move them into protected?

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4991: scoped_ptr<media::MediaPermission>
RenderFrameImpl::CreateMediaPermissionProxy(
On 2015/09/22 08:24:16, perkj wrote:
> Where is this used? Can it be moved away from the RenderFrame ? 

This is used by the follow up CL.

[perkj_chrome, 2015-09-23 13:25:23]

You will need to find someone to review the RenderFrame change as well. 
But with the below fixed I am happy.

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher_proxy.h (right):

https://codereview.chromium.org/1351443005/diff/20001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher_proxy.h:33: void
HasPermission(Type type,
On 2015/09/22 18:48:49, guoweis_chromium wrote:
> On 2015/09/22 08:24:16, perkj wrote:
> > protected:
> 
> I'm not sure I understand this comment. They are public in the base class. Why
> do we want to move them into protected?

This is up to you. But my thinging was that a user of this class uses this
through the MediaPermission interface and not the
MediaPermissionDispatcherProxy. As you say, it does not really matter since they
are public in the beginning so in practice they are public. 

I asked two colleagues, one voted they should public as you say since they are
public in the base class and one voted they should be private... I could not
find anything in the style guide so I would leave them as public....

https://codereview.chromium.org/1351443005/diff/40001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher_impl.cc (right):

https://codereview.chromium.org/1351443005/diff/40001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher_impl.cc:61:
base::Unretained(this),
not done?

https://codereview.chromium.org/1351443005/diff/40001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher_proxy.cc (right):

https://codereview.chromium.org/1351443005/diff/40001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher_proxy.cc:50: //
|PermissionServices on the UI thread.
nit - remove extra | around PermissionServices

[guoweis_left_chromium, 2015-09-23 16:27:35]

guoweis@chromium.org changed reviewers:
+ kinuko@chromium.org

[guoweis_left_chromium, 2015-09-23 16:27:36]

kinuko@chromium.org: Please review changes in render_frame_impl.*

https://codereview.chromium.org/1351443005/diff/40001/content/renderer/media/...
File content/renderer/media/media_permission_dispatcher_impl.cc (right):

https://codereview.chromium.org/1351443005/diff/40001/content/renderer/media/...
content/renderer/media/media_permission_dispatcher_impl.cc:61:
base::Unretained(this),
On 2015/09/23 13:25:23, perkj wrote:
> not done?

Missed this. Done.

[guoweis_left_chromium, 2015-09-24 16:18:51]

guoweis@chromium.org changed reviewers:
- kinuko@chromium.org

[guoweis_left_chromium, 2015-09-24 16:19:14]

guoweis@chromium.org changed reviewers:
+ sievers@chromium.org

[guoweis_left_chromium, 2015-09-24 16:19:15]

sievers@chromium.org: Please review changes in render_frame_impl.*

[no sievers, 2015-09-24 18:20:44]

On 2015/09/24 16:19:15, guoweis_chromium wrote:
> mailto:sievers@chromium.org: Please review changes in render_frame_impl.*

lgtm

[guoweis_left_chromium, 2015-09-24 18:24:34]

The CQ bit was checked by guoweis@chromium.org

[commit-bot: I haz the power, 2015-09-24 18:26:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1351443005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1351443005/80001

[perkj_chrome, 2015-09-24 18:26:52]

media lgtm

[commit-bot: I haz the power, 2015-09-24 18:55:34]

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

[commit-bot: I haz the power, 2015-09-24 18:55:35]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[guoweis_left_chromium, 2015-09-24 18:55:51]

The CQ bit was checked by guoweis@chromium.org

[commit-bot: I haz the power, 2015-09-24 18:57:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1351443005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1351443005/80001

[commit-bot: I haz the power, 2015-09-24 20:00:19]

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

[commit-bot: I haz the power, 2015-09-24 20:00:20]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[guoweis2, 2015-09-24 21:31:20]

The CQ bit was checked by guoweis@google.com

[guoweis2, 2015-09-24 21:31:20]

The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org,
sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1351443005/#ps100001
(title: "Fix windows build break.")

[commit-bot: I haz the power, 2015-09-24 21:32:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1351443005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1351443005/100001

[commit-bot: I haz the power, 2015-09-24 23:05:31]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-09-24 23:06:09]

Patchset 5 (id:??) landed as
https://crrev.com/f4282b031b403e1358125e0db371bda17cac604a
Cr-Commit-Position: refs/heads/master@{#350689}