Support "learn more" link for EME in PermissionRequestManager code-path on Android

Support "learn more" link for EME in PermissionRequestManager code-path on Android

This patch adds the "learn more" link for EME in the infobar and modal
prompts on Android when the PermissionRequestManager is enabled. Since
only EME requires this link, this is for now hard-coded into
PermissionPromptAndroid (if we want other permission requests to have
this, we would probably move it to PermissionRequest).

Note that currently the UI for this will be wrong for infobars, as the
infobar message is something like "https://permission.site wants to",
and we add "Learn more" after that. In a later patch I'll make the
infobars from the PRM match the existing ones (have everything in the
message).

BUG=606138
Review-Url: https://codereview.chromium.org/2919323002
Cr-Commit-Position: refs/heads/master@{#477532}
Committed: https://chromium.googlesource.com/chromium/src/+/663a25accbad02b9f3cb8d7b60e5dc0f05a9f530

[Timothy Loh, 2017-06-05 06:01:44]

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

[commit-bot: I haz the power, 2017-06-05 06:01:49]

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-06-05 06:53:33]

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

[commit-bot: I haz the power, 2017-06-05 06:53:34]

Dry run: This issue passed the CQ dry run.

[Timothy Loh, 2017-06-06 02:57:35]

Description was changed from

==========
wip eme link text

BUG=
==========

to

==========
Support "learn more" link for EME in PermissionRequestManager code-path on
Android

This patch adds the "learn more" link for EME in the infobar and modal
prompts on Android when the PermissionRequestManager is enabled.

BUG=606138
==========

[Timothy Loh, 2017-06-06 02:57:35]

timloh@chromium.org changed reviewers:
+ raymes@chromium.org

[Timothy Loh, 2017-06-06 03:03:27]

Description was changed from

==========
Support "learn more" link for EME in PermissionRequestManager code-path on
Android

This patch adds the "learn more" link for EME in the infobar and modal
prompts on Android when the PermissionRequestManager is enabled.

BUG=606138
==========

to

==========
Support "learn more" link for EME in PermissionRequestManager code-path on
Android

This patch adds the "learn more" link for EME in the infobar and modal
prompts on Android when the PermissionRequestManager is enabled. Since
only EME requires this link, this is for now hard-coded into
PermissionPromptAndroid (if we want other permission requests to have
this, we would probably move it to PermissionRequest).

BUG=606138
==========

[Timothy Loh, 2017-06-06 03:36:17]

Description was changed from

==========
Support "learn more" link for EME in PermissionRequestManager code-path on
Android

This patch adds the "learn more" link for EME in the infobar and modal
prompts on Android when the PermissionRequestManager is enabled. Since
only EME requires this link, this is for now hard-coded into
PermissionPromptAndroid (if we want other permission requests to have
this, we would probably move it to PermissionRequest).

BUG=606138
==========

to

==========
Support "learn more" link for EME in PermissionRequestManager code-path on
Android

This patch adds the "learn more" link for EME in the infobar and modal
prompts on Android when the PermissionRequestManager is enabled. Since
only EME requires this link, this is for now hard-coded into
PermissionPromptAndroid (if we want other permission requests to have
this, we would probably move it to PermissionRequest).

Note that currently the UI for this will be wrong for infobars, as the
infobar message is something like "https://permission.site wants to",
and we add "Learn more" after that. In a later patch I'll make the
infobars from the PRM match the existing ones (have everything in the
message).

BUG=606138
==========

[Timothy Loh, 2017-06-06 03:36:56]

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

[commit-bot: I haz the power, 2017-06-06 03:37:28]

Dry run: CQ is trying da patch.

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

[Timothy Loh, 2017-06-06 03:45:25]

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

[commit-bot: I haz the power, 2017-06-06 03:45:49]

Dry run: CQ is trying da patch.

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

[Timothy Loh, 2017-06-06 03:46:35]

timloh@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[Timothy Loh, 2017-06-06 03:46:36]

raymes@: PTAL at everything
dfalcantara@: PTAL at grouped_permission_infobar.cc and
GroupedPermissionInfoBar.java.

https://codereview.chromium.org/2919323002/diff/60001/chrome/browser/ui/permi...
File chrome/browser/ui/permission_bubble/mock_permission_prompt.cc (right):

https://codereview.chromium.org/2919323002/diff/60001/chrome/browser/ui/permi...
chrome/browser/ui/permission_bubble/mock_permission_prompt.cc:23: // The actual
prompt will call these, ensure we have test coverage.
Not sure if I should add an instrumentation test for EME or just do this. This
is simpler and adds coverage for other request types compared to a test for EME
so I preferred it.

[commit-bot: I haz the power, 2017-06-06 05:09:30]

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

[commit-bot: I haz the power, 2017-06-06 05:09:31]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-06-06 05:14:18]

lg overall, just a couple of small things

https://codereview.chromium.org/2919323002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_prompt_android.cc (right):

https://codereview.chromium.org/2919323002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_prompt_android.cc:137:
CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER)
nit: {}

https://codereview.chromium.org/2919323002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_prompt_android.cc:144:
CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER)
nit: {}

https://codereview.chromium.org/2919323002/diff/60001/chrome/browser/ui/permi...
File chrome/browser/ui/permission_bubble/mock_permission_prompt.cc (right):

https://codereview.chromium.org/2919323002/diff/60001/chrome/browser/ui/permi...
chrome/browser/ui/permission_bubble/mock_permission_prompt.cc:23: // The actual
prompt will call these, ensure we have test coverage.
On 2017/06/06 03:46:36, Timothy Loh wrote:
> Not sure if I should add an instrumentation test for EME or just do this. This
> is simpler and adds coverage for other request types compared to a test for
EME
> so I preferred it.

I would rather check that the results are meaningful in some way. e.g. either by
storing the results in a member variable and comparing them in the test harness,
or just EXPECT'ing here that they are non-null.

[Timothy Loh, 2017-06-06 09:28:16]

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

[commit-bot: I haz the power, 2017-06-06 09:28:28]

Dry run: CQ is trying da patch.

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

[Timothy Loh, 2017-06-06 09:29:15]

https://codereview.chromium.org/2919323002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_prompt_android.cc (right):

https://codereview.chromium.org/2919323002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_prompt_android.cc:137:
CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER)
On 2017/06/06 05:14:18, raymes wrote:
> nit: {}

Done.

https://codereview.chromium.org/2919323002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_prompt_android.cc:144:
CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER)
On 2017/06/06 05:14:18, raymes wrote:
> nit: {}

Done.

https://codereview.chromium.org/2919323002/diff/60001/chrome/browser/ui/permi...
File chrome/browser/ui/permission_bubble/mock_permission_prompt.cc (right):

https://codereview.chromium.org/2919323002/diff/60001/chrome/browser/ui/permi...
chrome/browser/ui/permission_bubble/mock_permission_prompt.cc:23: // The actual
prompt will call these, ensure we have test coverage.
On 2017/06/06 05:14:18, raymes wrote:
> On 2017/06/06 03:46:36, Timothy Loh wrote:
> > Not sure if I should add an instrumentation test for EME or just do this.
This
> > is simpler and adds coverage for other request types compared to a test for
> EME
> > so I preferred it.
> 
> I would rather check that the results are meaningful in some way. e.g. either
by
> storing the results in a member variable and comparing them in the test
harness,
> or just EXPECT'ing here that they are non-null.

Not sure this is really an appropriate place to have EXPECTs, but I added some.
WDYT?

[commit-bot: I haz the power, 2017-06-06 10:23:47]

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

[commit-bot: I haz the power, 2017-06-06 10:23:48]

Dry run: This issue passed the CQ dry run.

[gone, 2017-06-06 17:17:34]

android lgtm

[raymes, 2017-06-06 22:53:33]

lgtm

https://codereview.chromium.org/2919323002/diff/60001/chrome/browser/ui/permi...
File chrome/browser/ui/permission_bubble/mock_permission_prompt.cc (right):

https://codereview.chromium.org/2919323002/diff/60001/chrome/browser/ui/permi...
chrome/browser/ui/permission_bubble/mock_permission_prompt.cc:23: // The actual
prompt will call these, ensure we have test coverage.
On 2017/06/06 09:29:15, Timothy Loh wrote:
> On 2017/06/06 05:14:18, raymes wrote:
> > On 2017/06/06 03:46:36, Timothy Loh wrote:
> > > Not sure if I should add an instrumentation test for EME or just do this.
> This
> > > is simpler and adds coverage for other request types compared to a test
for
> > EME
> > > so I preferred it.
> > 
> > I would rather check that the results are meaningful in some way. e.g.
either
> by
> > storing the results in a member variable and comparing them in the test
> harness,
> > or just EXPECT'ing here that they are non-null.
> 
> Not sure this is really an appropriate place to have EXPECTs, but I added
some.
> WDYT?

It's definitely not ideal but I think it's slightly preferable to just calling
the functions. I consider this as part of the test infrastructure.

[Timothy Loh, 2017-06-07 02:54:25]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2017-06-07 02:54:40]

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-06-07 02:59:56]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496804065989000,
"parent_rev": "711fb3b4e484eb70f5a89bbbbbc9a741d23cb427", "commit_rev":
"663a25accbad02b9f3cb8d7b60e5dc0f05a9f530"}

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

Description was changed from

==========
Support "learn more" link for EME in PermissionRequestManager code-path on
Android

This patch adds the "learn more" link for EME in the infobar and modal
prompts on Android when the PermissionRequestManager is enabled. Since
only EME requires this link, this is for now hard-coded into
PermissionPromptAndroid (if we want other permission requests to have
this, we would probably move it to PermissionRequest).

Note that currently the UI for this will be wrong for infobars, as the
infobar message is something like "https://permission.site wants to",
and we add "Learn more" after that. In a later patch I'll make the
infobars from the PRM match the existing ones (have everything in the
message).

BUG=606138
==========

to

==========
Support "learn more" link for EME in PermissionRequestManager code-path on
Android

This patch adds the "learn more" link for EME in the infobar and modal
prompts on Android when the PermissionRequestManager is enabled. Since
only EME requires this link, this is for now hard-coded into
PermissionPromptAndroid (if we want other permission requests to have
this, we would probably move it to PermissionRequest).

Note that currently the UI for this will be wrong for infobars, as the
infobar message is something like "https://permission.site wants to",
and we add "Learn more" after that. In a later patch I'll make the
infobars from the PRM match the existing ones (have everything in the
message).

BUG=606138

Review-Url: https://codereview.chromium.org/2919323002
Cr-Commit-Position: refs/heads/master@{#477532}
Committed:
https://chromium.googlesource.com/chromium/src/+/663a25accbad02b9f3cb8d7b60e5...
==========

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

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/663a25accbad02b9f3cb8d7b60e5...