[Android] Implement back key functionality in download manager

[Android] Implement back key functionality in download manager

On tablet, the back key already works, due to the browser caching a
navigation history. This CL implements the same thing on phones, by
caching a backstack history in download activity.

BUG=616324
Committed: https://crrev.com/f324214c3c190ff7a67e4711cdb8bbfd812a22cc
Cr-Commit-Position: refs/heads/master@{#414616}

[Ian Wen, 2016-08-24 23:08:23]

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

[Ian Wen, 2016-08-24 23:08:23]

PTAL

[gone, 2016-08-25 00:20:51]

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java
(right):

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:25:
private static final String LAST_URL_PREF = "pref-download-last-url";
private static final String PREF_LAST_URL =
        "org.chromium.chrome.browser.download.DownloadActivity.LAST_URL";

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:29:
// DownloadPage, by caching a backstack on its own.
/** Caches the stack of filters applied to let the user backtrack through their
history. */

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:32:
private DownloadUiObserver mUiObserver = new DownloadUiObserver() {
private final

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:40:
if (!mBackStack.isEmpty() && mBackStack.peek().equals(url)) return;
don't bother early returning if it's so short; might as well just use 
if (mBackStack.isEmpty() || !mBackStack.peek().equals(url))
mBackStack.push(url);

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:54:
mDownloadManagerUi.addObserver(mUiObserver);
newline between 54 and 55

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:55:
String lastUrl = ContextUtils.getAppSharedPreferences().getString(LAST_URL_PREF,
null);
Is keeping track of this really necessary?  The pref is never cleared, so in its
current state it looks like you will get stuck in a filtered view -- even when
you hit X to explicitly close the Activity.

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:56:
if (lastUrl != null) {
put on same line and remove braces?

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:64:
if (mBackStack.size() > 1) {
Why are you special casing the # here?  Took me a while to understand what was
happening.  Add a comment like this here:

// The top of the stack always represents the current filter.  When back is
pressed,
// the top is popped off and the new top indicates what filter to use.  If there
are
// no filters remaining, the Activity itself is closed.

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:75:
mDownloadManagerUi.removeObserver(mUiObserver);
Remove this line; it's automatically handled by the DownloadManagerUi.

[Ian Wen, 2016-08-25 02:30:43]

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java
(right):

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:25:
private static final String LAST_URL_PREF = "pref-download-last-url";
On 2016/08/25 00:20:51, dfalcantara wrote:
> private static final String PREF_LAST_URL =
>         "org.chromium.chrome.browser.download.DownloadActivity.LAST_URL";

Done.

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:29:
// DownloadPage, by caching a backstack on its own.
On 2016/08/25 00:20:51, dfalcantara wrote:
> /** Caches the stack of filters applied to let the user backtrack through
their
> history. */

Done.

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:32:
private DownloadUiObserver mUiObserver = new DownloadUiObserver() {
On 2016/08/25 00:20:51, dfalcantara wrote:
> private final

Done.

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:40:
if (!mBackStack.isEmpty() && mBackStack.peek().equals(url)) return;
On 2016/08/25 00:20:51, dfalcantara wrote:
> don't bother early returning if it's so short; might as well just use 
> if (mBackStack.isEmpty() || !mBackStack.peek().equals(url))
> mBackStack.push(url);

Done.

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:54:
mDownloadManagerUi.addObserver(mUiObserver);
On 2016/08/25 00:20:51, dfalcantara wrote:
> newline between 54 and 55

Done.

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:55:
String lastUrl = ContextUtils.getAppSharedPreferences().getString(LAST_URL_PREF,
null);
On 2016/08/25 00:20:51, dfalcantara wrote:
> Is keeping track of this really necessary?  The pref is never cleared, so in
its
> current state it looks like you will get stuck in a filtered view -- even when
> you hit X to explicitly close the Activity.

Removed. Done.

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:56:
if (lastUrl != null) {
On 2016/08/25 00:20:51, dfalcantara wrote:
> put on same line and remove braces?

Removed.

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:64:
if (mBackStack.size() > 1) {
On 2016/08/25 00:20:51, dfalcantara wrote:
> Why are you special casing the # here?  Took me a while to understand what was
> happening.  Add a comment like this here:
> 
> // The top of the stack always represents the current filter.  When back is
> pressed,
> // the top is popped off and the new top indicates what filter to use.  If
there
> are
> // no filters remaining, the Activity itself is closed.

Done.

https://chromiumcodereview.appspot.com/2271583006/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:75:
mDownloadManagerUi.removeObserver(mUiObserver);
On 2016/08/25 00:20:51, dfalcantara wrote:
> Remove this line; it's automatically handled by the DownloadManagerUi.

Done.

[gone, 2016-08-25 03:20:37]

lgtm

https://chromiumcodereview.appspot.com/2271583006/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java
(right):

https://chromiumcodereview.appspot.com/2271583006/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:29:
history. */
nit: Doesn't fit on one line, I guess?  Use proper syntax in that case

https://chromiumcodereview.appspot.com/2271583006/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:55:
// Call updateForUrl() here to align with how DownloadPage interact with the
manager ui.
nit: interacts with the {@link DownloadManagerUi}.

[Ian Wen, 2016-08-25 04:16:55]

https://chromiumcodereview.appspot.com/2271583006/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java
(right):

https://chromiumcodereview.appspot.com/2271583006/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:29:
history. */
On 2016/08/25 03:20:37, dfalcantara wrote:
> nit: Doesn't fit on one line, I guess?  Use proper syntax in that case

Done.

https://chromiumcodereview.appspot.com/2271583006/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadActivity.java:55:
// Call updateForUrl() here to align with how DownloadPage interact with the
manager ui.
On 2016/08/25 03:20:37, dfalcantara wrote:
> nit: interacts with the {@link DownloadManagerUi}.

Done.

[Ian Wen, 2016-08-25 14:48:13]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2016-08-25 14:48:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2271583006/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-25 14:48:36]

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, 2016-08-25 16:08:37]

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

[commit-bot: I haz the power, 2016-08-25 16:08:39]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ian Wen, 2016-08-26 00:44:18]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2016-08-26 00:44:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2271583006/#ps80001
(title: "fix test")

[commit-bot: I haz the power, 2016-08-26 00:45:06]

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, 2016-08-26 01:31:08]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-26 01:32:26]

Description was changed from

==========
[Android] Implement back key functionality in download manager

On tablet, the back key already works, due to the browser caching a
navigation history. This CL implements the same thing on phones, by
caching a backstack history in download activity.

BUG=616324
==========

to

==========
[Android] Implement back key functionality in download manager

On tablet, the back key already works, due to the browser caching a
navigation history. This CL implements the same thing on phones, by
caching a backstack history in download activity.

BUG=616324

Committed: https://crrev.com/f324214c3c190ff7a67e4711cdb8bbfd812a22cc
Cr-Commit-Position: refs/heads/master@{#414616}
==========

[commit-bot: I haz the power, 2016-08-26 01:32:27]

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