[Android] Update Protected Media Identifier Settings UI to reflect exceptions.

[Android] Update Protected Media Identifier Settings UI to reflect exceptions.

Protected Media Settings UI no longer has a custom preference screen
and now shares WebsitePreferences with other ContentSettings.

The explanatory text has been shifted entirely to a webpage which is
linked. The reset credentials button has been moved to the options menu.

Also the permission prompt(infobar) now points to the help page instead of the
preference screen as it no longer contains any explanatory text.

Screenshots of implementation:
https://drive.google.com/open?id=0B6xQNbSUHV8BfmZuRzdUelg4LUZyd1lObEh2dFNXRktpa2JkQVdFbFVMWkFQbGVnaFFadW8&authuser=0

BUG=467666, 469783
Committed: https://crrev.com/7079464faaa5d2d2903fb701213bddcd2fc7bec8
Cr-Commit-Position: refs/heads/master@{#327834}

[knn, 2015-04-22 17:47:59]

knn@chromium.org changed reviewers:
+ ddorwin@chromium.org, mariakhomenko@chromium.org, xhwang@chromium.org,
yusufo@chromium.org

[knn, 2015-04-22 17:48:00]

{xhwang,ddorwin}@chromium.org: Please review the overall change and
protected_media_identifier_infobar_delegate.cc

mariakhomenko@chromium.org: Please review changes in chrome/android/java/s*

yusufo@chromium.org: Please review changes in chrome/android/java/res/*

[Maria, 2015-04-22 18:58:52]

mariakhomenko@chromium.org changed reviewers:
+ newt@chromium.org

[Maria, 2015-04-22 18:58:53]

I think Newton would be a better reviewer for this change -- can you take a look
at chrome/android/java code?

[knn, 2015-04-24 12:35:51]

On 2015/04/22 18:58:53, Maria wrote:
> I think Newton would be a better reviewer for this change -- can you take a
look
> at chrome/android/java code?

Added screenshots to the issue description.

[xhwang, 2015-04-27 18:05:28]

On 2015/04/24 12:35:51, knn wrote:
> On 2015/04/22 18:58:53, Maria wrote:
> > I think Newton would be a better reviewer for this change -- can you take a
> look
> > at chrome/android/java code?
> 
> Added screenshots to the issue description.

In the screenshot we are still using "DENY" instead of "BLOCK". I thought we
should use "BLOCK" for consistency?

Other than that, the screenshots sgtm. I'll defer to some UI people to review
the details of this CL.

[knn, 2015-04-28 10:26:03]

On 2015/04/27 18:05:28, xhwang wrote:
> On 2015/04/24 12:35:51, knn wrote:
> > On 2015/04/22 18:58:53, Maria wrote:
> > > I think Newton would be a better reviewer for this change -- can you take
a
> > look
> > > at chrome/android/java code?
> > 
> > Added screenshots to the issue description.
> 
> In the screenshot we are still using "DENY" instead of "BLOCK". I thought we
> should use "BLOCK" for consistency?
> 
> Other than that, the screenshots sgtm. I'll defer to some UI people to review
> the details of this CL.

@Xiaohan, as I pointed out, the settings strings are already inconsistent which
this change cannot fix.
My understanding from the mail thread was that, Rebecca is working with the
Security team to make all Settings strings consistent and no additional work is
required from my end.

It would be good to land this before the M44 feature freeze this Friday as this
is a UI regression in a sense.
Let me know if I can do anything to catalyze that.

[knn, 2015-04-29 20:06:01]

On 2015/04/28 10:26:03, knn wrote:
> On 2015/04/27 18:05:28, xhwang wrote:
> > On 2015/04/24 12:35:51, knn wrote:
> > > On 2015/04/22 18:58:53, Maria wrote:
> > > > I think Newton would be a better reviewer for this change -- can you
take
> a
> > > look
> > > > at chrome/android/java code?
> > > 
> > > Added screenshots to the issue description.
> > 
> > In the screenshot we are still using "DENY" instead of "BLOCK". I thought we
> > should use "BLOCK" for consistency?
> > 
> > Other than that, the screenshots sgtm. I'll defer to some UI people to
review
> > the details of this CL.
> 
> @Xiaohan, as I pointed out, the settings strings are already inconsistent
which
> this change cannot fix.
> My understanding from the mail thread was that, Rebecca is working with the
> Security team to make all Settings strings consistent and no additional work
is
> required from my end.
> 
> It would be good to land this before the M44 feature freeze this Friday as
this
> is a UI regression in a sense.
> Let me know if I can do anything to catalyze that.

Friendly ping?

[xhwang, 2015-04-29 20:17:37]

On 2015/04/29 20:06:01, knn wrote:
> On 2015/04/28 10:26:03, knn wrote:
> > On 2015/04/27 18:05:28, xhwang wrote:
> > > On 2015/04/24 12:35:51, knn wrote:
> > > > On 2015/04/22 18:58:53, Maria wrote:
> > > > > I think Newton would be a better reviewer for this change -- can you
> take
> > a
> > > > look
> > > > > at chrome/android/java code?
> > > > 
> > > > Added screenshots to the issue description.
> > > 
> > > In the screenshot we are still using "DENY" instead of "BLOCK". I thought
we
> > > should use "BLOCK" for consistency?
> > > 
> > > Other than that, the screenshots sgtm. I'll defer to some UI people to
> review
> > > the details of this CL.
> > 
> > @Xiaohan, as I pointed out, the settings strings are already inconsistent
> which
> > this change cannot fix.
> > My understanding from the mail thread was that, Rebecca is working with the
> > Security team to make all Settings strings consistent and no additional work
> is
> > required from my end.
> > 
> > It would be good to land this before the M44 feature freeze this Friday as
> this
> > is a UI regression in a sense.
> > Let me know if I can do anything to catalyze that.
> 
> Friendly ping?

Thanks for the explanation. The current plan sgtm.

[newt (away), 2015-04-30 00:31:59]

The preferences-related code looks mostly good. Just a few small comments
inline. Someone else should review the infobar changes -- I don't have any
context about what's changing there.

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromiumApplication.java
(right):

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromiumApplication.java:90:
* Opens a protected content settings page, if available.
delete orphaned comment

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/Preferences.java
(right):

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/Preferences.java:241:
MenuItem help =
How about using Menu.CATEGORY_SECONDARY and deleting MENU_LAST? e.g

    menu.add(Menu.NONE, R.id.menu_id_help_general, Menu.CATEGORY_SECONDARY,
R.string.menu_help);

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java
(right):

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:326:
if (!mFilter.showProtectedMediaSites(mCategoryFilter)) return;
Early returns are dangerous. Other developers may add code further down in the
method and not realize that their code isn't being run. This early return in
particular is very easy to miss, so don't use it: put this entire block inside
the if statement.

A good rule of thumb for early returns is: only use an early return at the
beginning of a method if the entire method shouldn't be run because something is
null, not initialized, already destroyed, etc.

For example, this is OK:

    public void fireLasers() {
        if (mIsDestroyed) return;

        // lots of code
        // to fire lasers
    }

But don't use an early return just to reduce indentation in the bottom section
of a multi-part method. Before you know it, there will be a new bottom-er
section in that method.

(Not so) fun fact: Aurimas and I recently discovered some code that had existed
in our codebase for several years, but had never been run due to a sneaky early
return a few lines earlier in a big startup-related method. The result was that
certain widgets and preferences weren't getting updated on startup. Not good.
(https://chrome-internal-review.googlesource.com/212950)

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:536:
globalToggle.setDrawDivider(false);
I think we should keep the divider, i.e. remove this line

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:651:
@Override
add

    // ProtectedContentResetCredentialConfirmDialogFragment.Listener:

above this method

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsiteSettingsCategoryFilter.java
(right):

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsiteSettingsCategoryFilter.java:139:
public boolean showProtectedMediaSites(String filterValue) {
move this above showPushNotificationsSites() to keep things alphabetized

https://codereview.chromium.org/1091253005/diff/1/chrome/browser/media/protec...
File chrome/browser/media/protected_media_identifier_infobar_delegate.cc
(right):

https://codereview.chromium.org/1091253005/diff/1/chrome/browser/media/protec...
chrome/browser/media/protected_media_identifier_infobar_delegate.cc:118: const
GURL learn_more_url(chrome::kEnhancedPlaybackNotificationLearnMoreURL);
why did you remove "#elif defined(OS_CHROMEOS)"? This changes the behavior on
other platforms, which doesn't seem intentional. Is this even used on other
platforms besides Android and Chrome OS?

[newt (away), 2015-04-30 00:54:26]

p.s. Thanks for the detailed commit message and the screenshots. They were both
very helpful :)

[knn, 2015-04-30 16:00:36]

Thanks Newton.

The only change to the infobar/permission prompt is it is now pointing to a help
webpage instead of the preference page as the explanation from the preference
page has been removed. This is also what Camera/Mic settings do.
Sorry about the lack of context, this discussion happened over 2 bugs and also
over a mail thread. :(

Also, Xiaohan and David are owners of the infobar file so I expect them to
review this as well.

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromiumApplication.java
(right):

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromiumApplication.java:90:
* Opens a protected content settings page, if available.
On 2015/04/30 00:31:59, newt wrote:
> delete orphaned comment

Sorry about this. Done.

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/Preferences.java
(right):

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/Preferences.java:241:
MenuItem help =
On 2015/04/30 00:31:59, newt wrote:
> How about using Menu.CATEGORY_SECONDARY and deleting MENU_LAST? e.g
> 
>     menu.add(Menu.NONE, R.id.menu_id_help_general, Menu.CATEGORY_SECONDARY,
> R.string.menu_help);

Indeed, Should've seen this. Thanks!

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java
(right):

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:326:
if (!mFilter.showProtectedMediaSites(mCategoryFilter)) return;
On 2015/04/30 00:31:59, newt wrote:
> Early returns are dangerous. Other developers may add code further down in the
> method and not realize that their code isn't being run. This early return in
> particular is very easy to miss, so don't use it: put this entire block inside
> the if statement.
> 
> A good rule of thumb for early returns is: only use an early return at the
> beginning of a method if the entire method shouldn't be run because something
is
> null, not initialized, already destroyed, etc.
> 
> For example, this is OK:
> 
>     public void fireLasers() {
>         if (mIsDestroyed) return;
> 
>         // lots of code
>         // to fire lasers
>     }
> 
> But don't use an early return just to reduce indentation in the bottom section
> of a multi-part method. Before you know it, there will be a new bottom-er
> section in that method.
> 
> (Not so) fun fact: Aurimas and I recently discovered some code that had
existed
> in our codebase for several years, but had never been run due to a sneaky
early
> return a few lines earlier in a big startup-related method. The result was
that
> certain widgets and preferences weren't getting updated on startup. Not good.
> (https://chrome-internal-review.googlesource.com/212950)

Looks really nasty, Done. Thanks for the explanation!

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:536:
globalToggle.setDrawDivider(false);
On 2015/04/30 00:31:59, newt wrote:
> I think we should keep the divider, i.e. remove this line

Done.

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferences.java:651:
@Override
On 2015/04/30 00:31:59, newt wrote:
> add
> 
>     // ProtectedContentResetCredentialConfirmDialogFragment.Listener:
> 
> above this method

Done.

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsiteSettingsCategoryFilter.java
(right):

https://codereview.chromium.org/1091253005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsiteSettingsCategoryFilter.java:139:
public boolean showProtectedMediaSites(String filterValue) {
On 2015/04/30 00:31:59, newt wrote:
> move this above showPushNotificationsSites() to keep things alphabetized

Done.

https://codereview.chromium.org/1091253005/diff/1/chrome/browser/media/protec...
File chrome/browser/media/protected_media_identifier_infobar_delegate.cc
(right):

https://codereview.chromium.org/1091253005/diff/1/chrome/browser/media/protec...
chrome/browser/media/protected_media_identifier_infobar_delegate.cc:118: const
GURL learn_more_url(chrome::kEnhancedPlaybackNotificationLearnMoreURL);
On 2015/04/30 00:31:59, newt wrote:
> why did you remove "#elif defined(OS_CHROMEOS)"? This changes the behavior on
> other platforms, which doesn't seem intentional. Is this even used on other
> platforms besides Android and Chrome OS?

This entire class is only used in Android and Chrome OS. Since now there is no
different implementations for Android vs ChromeOS, I have removed the ugly
#ifdefs.

The Create method should be the ideal place for any NOTIMPLEMENTED() although I
think even that is excessive.

[newt (away), 2015-04-30 17:47:17]

one last comment, then lgtm

https://codereview.chromium.org/1091253005/diff/20001/chrome/browser/media/pr...
File chrome/browser/media/protected_media_identifier_infobar_delegate.cc
(right):

https://codereview.chromium.org/1091253005/diff/20001/chrome/browser/media/pr...
chrome/browser/media/protected_media_identifier_infobar_delegate.cc:17: #if
defined(OS_ANDROID) || defined(OS_CHROMEOS)
remove this #ifdef too

[knn, 2015-04-30 18:42:44]

Thanks Newton, Fixed that.

Xiaohan, David: Please have a look if you have any additional comments.

https://codereview.chromium.org/1091253005/diff/20001/chrome/browser/media/pr...
File chrome/browser/media/protected_media_identifier_infobar_delegate.cc
(right):

https://codereview.chromium.org/1091253005/diff/20001/chrome/browser/media/pr...
chrome/browser/media/protected_media_identifier_infobar_delegate.cc:17: #if
defined(OS_ANDROID) || defined(OS_CHROMEOS)
On 2015/04/30 17:47:17, newt wrote:
> remove this #ifdef too

Done.

[xhwang, 2015-04-30 23:31:32]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-04-30 23:31:32]

lgtm

[xhwang, 2015-04-30 23:31:32]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1091253005/#ps40001
(title: "Missed a change in prev patch")

[commit-bot: I haz the power, 2015-04-30 23:32:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1091253005/40001

[commit-bot: I haz the power, 2015-04-30 23:38:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-30 23:39:53]

Patchset 3 (id:??) landed as
https://crrev.com/7079464faaa5d2d2903fb701213bddcd2fc7bec8
Cr-Commit-Position: refs/heads/master@{#327834}