Enable download home by default

Enable download home by default

enable this for M57 and beyond

BUG=685317
Review-Url: https://codereview.chromium.org/2651233002
Cr-Commit-Position: refs/heads/master@{#446420}
Committed: https://chromium.googlesource.com/chromium/src/+/2476c81a93b0cb04c10c8186308fbb5cf1271627

[qinmin, 2017-01-25 19:35:58]

qinmin@chromium.org changed reviewers:
+ bauerb@chromium.org, dfalcantara@chromium.org, dtrainor@chromium.org

[qinmin, 2017-01-25 19:35:58]

PTAL

[Bernhard Bauer, 2017-01-25 20:16:25]

LGTM

https://codereview.chromium.org/2651233002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2651233002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:247:
// Clean up unused shared prefs. TODO(qinmin): remove this in later versions.
Can you add a specific milestone please? :)

[qinmin, 2017-01-25 20:25:18]

qinmin@chromium.org changed reviewers:
+ twellington@chromium.org

[qinmin, 2017-01-25 20:25:18]

Theresa, would you please take a look if this affects page actions?

[gone, 2017-01-25 20:55:09]

lgtm % question

https://codereview.chromium.org/2651233002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java
(right):

https://codereview.chromium.org/2651233002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:130:
menu.findItem(R.id.downloads_menu_id).setVisible(true);
This should just be changed in the menu XML directly now that it's not
conditional.  It might already be visible by default so you might just be able
to delete the line entirely.  Can you check?

https://codereview.chromium.org/2651233002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarTablet.java
(left):

https://codereview.chromium.org/2651233002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarTablet.java:580:
return (!shouldShowDeleteButton() && !DownloadUtils.isDownloadHomeEnabled())
You also deleted !shouldShowDeleteButton(); does this change anything?

[qinmin, 2017-01-25 22:42:24]

https://codereview.chromium.org/2651233002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java
(right):

https://codereview.chromium.org/2651233002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:130:
menu.findItem(R.id.downloads_menu_id).setVisible(true);
On 2017/01/25 20:55:09, dfalcantara (load balance plz) wrote:
> This should just be changed in the menu XML directly now that it's not
> conditional.  It might already be visible by default so you might just be able
> to delete the line entirely.  Can you check?

yes, it works without this line

https://codereview.chromium.org/2651233002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2651233002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:247:
// Clean up unused shared prefs. TODO(qinmin): remove this in later versions.
On 2017/01/25 20:16:25, Bernhard Bauer wrote:
> Can you add a specific milestone please? :)

Done.

https://codereview.chromium.org/2651233002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarTablet.java
(left):

https://codereview.chromium.org/2651233002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarTablet.java:580:
return (!shouldShowDeleteButton() && !DownloadUtils.isDownloadHomeEnabled())
On 2017/01/25 20:55:09, dfalcantara (load balance plz) wrote:
> You also deleted !shouldShowDeleteButton(); does this change anything?

since DownloadUtils.isDownloadHomeEnabled() always returns true,
(!shouldShowDeleteButton() && !DownloadUtils.isDownloadHomeEnabled()) should now
always be false, so it won't impact the final result

[Theresa, 2017-01-25 22:56:49]

lgtm % nit

https://codereview.chromium.org/2651233002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2651233002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:247:
// Clean up unused shared prefs. TODO(qinmin): remove this after M58.
nit: What if the user is updating from M56 to M59 (or M65)?

[qinmin, 2017-01-26 00:18:12]

https://codereview.chromium.org/2651233002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2651233002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:247:
// Clean up unused shared prefs. TODO(qinmin): remove this after M58.
On 2017/01/25 22:56:49, Theresa wrote:
> nit: What if the user is updating from M56 to M59 (or M65)?

Unfortunately, that will leave the sharedpref uncleaned. Hopefully, a majority
of the user will either upgrade to M57 or M58, and thus they won't have such
issue.

[qinmin, 2017-01-26 00:18:17]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2017-01-26 00:18:18]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2651233002/#ps40001
(title: "comments")

[commit-bot: I haz the power, 2017-01-26 00:19:01]

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

[Theresa, 2017-01-26 00:22:21]

https://codereview.chromium.org/2651233002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2651233002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:247:
// Clean up unused shared prefs. TODO(qinmin): remove this after M58.
On 2017/01/26 00:18:12, qinmin wrote:
> On 2017/01/25 22:56:49, Theresa wrote:
> > nit: What if the user is updating from M56 to M59 (or M65)?
> 
> Unfortunately, that will leave the sharedpref uncleaned. Hopefully, a majority
> of the user will either upgrade to M57 or M58, and thus they won't have such
> issue.

Why not just leave this line until a more distant milestone?

[commit-bot: I haz the power, 2017-01-26 00:34:25]

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

[commit-bot: I haz the power, 2017-01-26 00:34:26]

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

[qinmin, 2017-01-26 00:39:53]

https://codereview.chromium.org/2651233002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2651233002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:247:
// Clean up unused shared prefs. TODO(qinmin): remove this after M58.
On 2017/01/26 00:22:21, Theresa wrote:
> On 2017/01/26 00:18:12, qinmin wrote:
> > On 2017/01/25 22:56:49, Theresa wrote:
> > > nit: What if the user is updating from M56 to M59 (or M65)?
> > 
> > Unfortunately, that will leave the sharedpref uncleaned. Hopefully, a
majority
> > of the user will either upgrade to M57 or M58, and thus they won't have such
> > issue.
> 
> Why not just leave this line until a more distant milestone?

ok, changed this to M61, that is 30 weeks since M56, more than half a year since
the flag is gone.

[qinmin, 2017-01-26 07:24:26]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2017-01-26 07:24:26]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
twellington@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2651233002/#ps60001
(title: "change TODO")

[commit-bot: I haz the power, 2017-01-26 07:24: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-01-26 08:11:05]

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

[commit-bot: I haz the power, 2017-01-26 08:11:06]

Failed to apply patch for
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:
While running git apply --index -p1;
  error: patch failed:
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:78
  error:
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:
patch does not apply

Patch:      
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
Index:
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
b/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
index
67b208c0afe1a4d731c6d0fc583bf45783671bff..4edfa0889c7d224d272c5e0cb22c5717a9e516b9
100644
---
a/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
+++
b/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
@@ -9,7 +9,6 @@ import android.app.PendingIntent;
 import android.content.ActivityNotFoundException;
 import android.content.Context;
 import android.content.Intent;
-import android.content.SharedPreferences;
 import android.graphics.Bitmap;
 import android.graphics.BitmapFactory;
 import android.graphics.Color;
@@ -29,7 +28,6 @@ import org.chromium.base.VisibleForTesting;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.base.metrics.RecordUserAction;
 import org.chromium.chrome.R;
-import org.chromium.chrome.browser.ChromeFeatureList;
 import org.chromium.chrome.browser.ChromeTabbedActivity;
 import org.chromium.chrome.browser.IntentHandler;
 import org.chromium.chrome.browser.UrlConstants;
@@ -78,38 +76,15 @@ public class DownloadUtils {
     private static final String EXTRA_IS_OFF_THE_RECORD =
             "org.chromium.chrome.browser.download.IS_OFF_THE_RECORD";
 
-    private static final String PREF_IS_DOWNLOAD_HOME_ENABLED =
-            "org.chromium.chrome.browser.download.IS_DOWNLOAD_HOME_ENABLED";
-
     private static final long BYTES_PER_KILOBYTE = 1024;
     private static final long BYTES_PER_MEGABYTE = 1024 * 1024;
     private static final long BYTES_PER_GIGABYTE = 1024 * 1024 * 1024;
 
     /**
-     * @return Whether or not the Download Home is enabled.
-     */
-    public static boolean isDownloadHomeEnabled() {
-        SharedPreferences preferences = ContextUtils.getAppSharedPreferences();
-        return preferences.getBoolean(PREF_IS_DOWNLOAD_HOME_ENABLED, false);
-    }
-
-    /**
-     * Caches the native flag that enables the Download Home in
SharedPreferences.
-     * This is necessary because the DownloadActivity can be opened before
native has been loaded.
-     */
-    public static void cacheIsDownloadHomeEnabled() {
-        boolean isEnabled = ChromeFeatureList.isEnabled("DownloadsUi");
-        SharedPreferences preferences = ContextUtils.getAppSharedPreferences();
-        preferences.edit().putBoolean(PREF_IS_DOWNLOAD_HOME_ENABLED,
isEnabled).apply();
-    }
-
-    /**
      * Displays the download manager UI. Note the UI is different on tablets
and on phones.
      * @return Whether the UI was shown.
      */
     public static boolean showDownloadManager(@Nullable Activity activity,
@Nullable Tab tab) {
-        if (!isDownloadHomeEnabled()) return false;
-
         // Figure out what tab was last being viewed by the user.
         if (activity == null) activity =
ApplicationStatus.getLastTrackedFocusedActivity();
         if (tab == null && activity instanceof ChromeTabbedActivity) {

[qinmin, 2017-01-26 18:19:18]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2017-01-26 18:19:18]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
twellington@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2651233002/#ps30013
(title: "rebase")

[commit-bot: I haz the power, 2017-01-26 18:19:45]

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-01-26 20:09:30]

CQ is committing da patch.

Bot data: {"patchset_id": 30013, "attempt_start_ts": 1485454758014400,
"parent_rev": "02d1c14d47b948cf6b4eb49c3b4cef697525f03e", "commit_rev":
"2476c81a93b0cb04c10c8186308fbb5cf1271627"}

[commit-bot: I haz the power, 2017-01-26 20:10:07]

Description was changed from

==========
Enable download home by default

enable this for M57 and beyond

BUG=685317
==========

to

==========
Enable download home by default

enable this for M57 and beyond

BUG=685317

Review-Url: https://codereview.chromium.org/2651233002
Cr-Commit-Position: refs/heads/master@{#446420}
Committed:
https://chromium.googlesource.com/chromium/src/+/2476c81a93b0cb04c10c8186308f...
==========

[commit-bot: I haz the power, 2017-01-26 20:10:08]

Committed patchset #5 (id:30013) as
https://chromium.googlesource.com/chromium/src/+/2476c81a93b0cb04c10c8186308f...