[Home] Update util methods to open bookmarks/downloads/history

[Home] Update util methods to open bookmarks/downloads/history

Downloads, history and bookmarks all have utility methods that open
their UIs. When Chrome Home is enabled, opening these UIs should
open the bottom sheet and show the corresponding BottomSheetContent.
Typed urls (e.g. chrome://history) will still open as native pages
rather than in the sheet.

Also adds tests for the BottomSheetContentController.

BUG=716250
Review-Url: https://codereview.chromium.org/2861453002
Cr-Commit-Position: refs/heads/master@{#469349}
Committed: https://chromium.googlesource.com/chromium/src/+/5ac6612a4ee3e4c8f261f77f022aa800bbd3487a

[Theresa, 2017-05-02 19:25:51]

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

[commit-bot: I haz the power, 2017-05-02 19:26:30]

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-05-02 20:29:14]

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

[commit-bot: I haz the power, 2017-05-02 20:29:15]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-05-02 21:12:04]

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

[commit-bot: I haz the power, 2017-05-02 21:12:33]

Dry run: CQ is trying da patch.

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

[Theresa, 2017-05-02 21:13:10]

twellington@chromium.org changed reviewers:
+ dfalcantara@chromium.org, mdjones@chromium.org

[Theresa, 2017-05-02 21:13:11]

mdjones@ - ptal at BottomSheet*
dfalcantara@ - ptal at BookmarkUtils, DownloadUtils, and HistoryManagerUtils

Videos in the bug.

[commit-bot: I haz the power, 2017-05-02 22:13:37]

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

[commit-bot: I haz the power, 2017-05-02 22:13:38]

Dry run: This issue passed the CQ dry run.

[mdjones, 2017-05-02 22:18:30]

Is it the intended behavior to show b/h/d outside of Chrome Home ever? URLs are
routed through the bottom sheet, we can intercept them there and open the bottom
sheet if necessary.

https://codereview.chromium.org/2861453002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2861453002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:200:
mBottomSheet.setSheetState(BottomSheet.SHEET_STATE_HALF, true);
Why only expand to half in this case?

[Theresa, 2017-05-02 22:27:34]

On 2017/05/02 22:18:30, mdjones wrote:
> Is it the intended behavior to show b/h/d outside of Chrome Home ever? URLs
are
> routed through the bottom sheet, we can intercept them there and open the
bottom
> sheet if necessary.
> 
>
https://codereview.chromium.org/2861453002/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
> (right):
> 
>
https://codereview.chromium.org/2861453002/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:200:
> mBottomSheet.setSheetState(BottomSheet.SHEET_STATE_HALF, true);
> Why only expand to half in this case?

For Downloads, if Chrome isn't running, the intent created when the notification
is clicked will open the DownloadActivity. I think that's okay (for now at
least).

All 3 surfaces can be bookmarked on tablets. When the bookmark is opened on a
phone, the native page loads inside a tab. Those URLs won't get routed through
the bottom sheet; we have to leave native page support for tablets anyway, so I
think it's okay to leave this behavior as is since it's an odd use case to
support.

chrome://history can be typed into the URL bar. I think that if the user is
explicitly typing in the URL instead of opening the bottom sheet we should let
them get to the native page inside a tab version. UX may feel differently, but I
think for now it's okay to leave as-is.

[Theresa, 2017-05-02 22:28:10]

https://codereview.chromium.org/2861453002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2861453002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:200:
mBottomSheet.setSheetState(BottomSheet.SHEET_STATE_HALF, true);
On 2017/05/02 22:18:30, mdjones wrote:
> Why only expand to half in this case?

That was an oversight. I'll change it to SHEET_STATE_FULL.

[gone, 2017-05-03 18:15:27]

lgtm

https://codereview.chromium.org/2861453002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2861453002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:121:
if (activity instanceof ChromeActivity
Are you ever expecting a bottom sheet to exist in something other than a
ChromeTabbedActivity?  Kind of surprised getBottomSheet() is on ChromeActivity.

https://codereview.chromium.org/2861453002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentControllerTest.java
(right):

https://codereview.chromium.org/2861453002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentControllerTest.java:37:
mObserver.mOpenedCallbackHelper.waitForCallback(initialOpenedCount, 1);
check that the other ones haven't been called?

[Theresa, 2017-05-03 22:24:59]

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

[Theresa, 2017-05-03 22:25:32]

https://codereview.chromium.org/2861453002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2861453002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:121:
if (activity instanceof ChromeActivity
On 2017/05/03 18:15:27, slow (dfalcantara) wrote:
> Are you ever expecting a bottom sheet to exist in something other than a
> ChromeTabbedActivity?  Kind of surprised getBottomSheet() is on
ChromeActivity.

I don't think we'll have a bottom sheet on anything besides
ChromeTabbedActivity. Matt, what do you think about moving bottom sheet
ownership to ChromeTabbedActivity?

https://codereview.chromium.org/2861453002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2861453002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:200:
mBottomSheet.setSheetState(BottomSheet.SHEET_STATE_HALF, true);
On 2017/05/02 22:28:09, Theresa wrote:
> On 2017/05/02 22:18:30, mdjones wrote:
> > Why only expand to half in this case?
> 
> That was an oversight. I'll change it to SHEET_STATE_FULL.

Done.

https://codereview.chromium.org/2861453002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentControllerTest.java
(right):

https://codereview.chromium.org/2861453002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentControllerTest.java:37:
mObserver.mOpenedCallbackHelper.waitForCallback(initialOpenedCount, 1);
On 2017/05/03 18:15:27, slow (dfalcantara) wrote:
> check that the other ones haven't been called?

Done.

[commit-bot: I haz the power, 2017-05-03 22:25:40]

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-05-03 23:43:14]

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

[commit-bot: I haz the power, 2017-05-03 23:43:15]

Dry run: This issue passed the CQ dry run.

[mdjones, 2017-05-04 15:55:58]

lgtm

https://codereview.chromium.org/2861453002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
(right):

https://codereview.chromium.org/2861453002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:121:
if (activity instanceof ChromeActivity
On 2017/05/03 22:25:32, Theresa wrote:
> On 2017/05/03 18:15:27, slow (dfalcantara) wrote:
> > Are you ever expecting a bottom sheet to exist in something other than a
> > ChromeTabbedActivity?  Kind of surprised getBottomSheet() is on
> ChromeActivity.
> 
> I don't think we'll have a bottom sheet on anything besides
> ChromeTabbedActivity. Matt, what do you think about moving bottom sheet
> ownership to ChromeTabbedActivity?

We could, but it was convenient to have it there because ChromeActivity does
most of the initial view setting and inflation.

[Theresa, 2017-05-04 15:57:58]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2017-05-04 15:57:59]

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/2861453002/#ps60001
(title: "[Home] Update util methods to open bookmarks/downloads/history")

[commit-bot: I haz the power, 2017-05-04 15:58:12]

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-05-04 16:03:53]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493913478711530,
"parent_rev": "644d16e85e314eea64abdb2ebd30b11ed731f5c9", "commit_rev":
"5ac6612a4ee3e4c8f261f77f022aa800bbd3487a"}

[commit-bot: I haz the power, 2017-05-04 16:04:54]

Description was changed from

==========
[Home] Update util methods to open bookmarks/downloads/history

Downloads, history and bookmarks all have utility methods that open
their UIs. When Chrome Home is enabled, opening these UIs should
open the bottom sheet and show the corresponding BottomSheetContent.
Typed urls (e.g. chrome://history) will still open as native pages
rather than in the sheet.

Also adds tests for the BottomSheetContentController.

BUG=716250
==========

to

==========
[Home] Update util methods to open bookmarks/downloads/history

Downloads, history and bookmarks all have utility methods that open
their UIs. When Chrome Home is enabled, opening these UIs should
open the bottom sheet and show the corresponding BottomSheetContent.
Typed urls (e.g. chrome://history) will still open as native pages
rather than in the sheet.

Also adds tests for the BottomSheetContentController.

BUG=716250

Review-Url: https://codereview.chromium.org/2861453002
Cr-Commit-Position: refs/heads/master@{#469349}
Committed:
https://chromium.googlesource.com/chromium/src/+/5ac6612a4ee3e4c8f261f77f022a...
==========

[commit-bot: I haz the power, 2017-05-04 16:04:55]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/5ac6612a4ee3e4c8f261f77f022a...