[Android History] Add clear browsing data button and privacy disclaimers

[Android History] Add clear browsing data button and privacy disclaimers

Adds an optional header to DateDividedAdapter and adds a new layout
with a clear browsing data button and privacy disclaimers as a header
in the history ui.

BUG=654071
Committed: https://crrev.com/a6ba8d8ccca0f615e7f830cefc1fac7af2acf4bd
Cr-Commit-Position: refs/heads/master@{#437931}

[Theresa, 2016-12-09 21:50:20]

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

[commit-bot: I haz the power, 2016-12-09 21:50:54]

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

[Theresa, 2016-12-09 21:55:58]

Description was changed from

==========
[Android History] Add clear browsing data button and privacy disclaimers

Adds a optional header to DateDividedAdapter and adds a new layout
with a clear browsing data button and privacy disclaimers as a header
in the history ui.

BUG=654071
==========

to

==========
[Android History] Add clear browsing data button and privacy disclaimers

Adds an optional header to DateDividedAdapter and adds a new layout
with a clear browsing data button and privacy disclaimers as a header
in the history ui.

BUG=654071
==========

[Theresa, 2016-12-09 22:35:09]

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

[Theresa, 2016-12-09 22:35:10]

ianwen@ - ptal at DateDividedAdapter changes
dfalcantara@ - ptal at everything

Screenshots are in the bug

[Theresa, 2016-12-09 23:02:35]

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

[commit-bot: I haz the power, 2016-12-09 23:03:48]

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, 2016-12-10 01:05:29]

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

[commit-bot: I haz the power, 2016-12-10 01:05:30]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[gone, 2016-12-12 19:14:13]

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/history_header.xml (right):

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/history_header.xml:27:
style="@style/ButtonCompatBorderless"
swap style and marginTop lines?

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java
(right):

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:343:
clear(false);
Feels like this should still be called by #loadItems() directly to avoid
situations where another set of items is loaded before it's been cleared. 
Should be asserted in some way that doesn't conflict with HistoryAdapter, at
least.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java
(right):

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java:153:
Button cbdButton = (Button) v.findViewById(R.id.clear_browsing_data_button);
I think you can get by with just View cbdButton.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java
(right):

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java:162:
mActivity.startActivity(intent);
For sanity's sake?
IntentUtils#safeStartActivity()

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java
(right):

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:125:
private boolean mIsHeader;
"Header" is a little too generic because the dates can be considered "headers"
for their sections.  Maybe mIsListHeader?

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:178:
// If this is the header, there is only one item.
What happens if you don't add this clause?  I wonder if it makes sense to change
the constructor to take in a timestamp instead of a TimedItem... if you changed
it so it just took a timestamp and changed the single callsite to immediately
add the TimedItem to the list, then I think you'd be able to avoid storing the
dummy item.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:220:
public static final int TYPE_HEADER = 2;
TYPE_HEADER should be 0 since it's at the top.  I don't think anything'll break.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:307:
mGroups.add(header);
Should assert the size is 0 at this point.  The add() will incorrectly put the
header at the end of the list if stuff is already there.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:348:
return pair.second == -1 ? TYPE_HEADER : pair.second == 0 ? TYPE_DATE :
TYPE_NORMAL;
1) Should just expand this into a set of conditionals now that there's three
possibilities.

2) Should pull out -1 as a new constant that indicates that it's a header, I
think.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:360:
}
Should assert false at this point; I don't know how you could create anything
except those three.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:369:
} else if (!(holder instanceof HeaderViewHolder)) {
null !instanceof HeaderViewHolder, so this would likely crash if it ever hit
that.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:384:
int i = position;
Move the int i line down to the for loop area.

[Theresa, 2016-12-12 20:04:04]

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

[commit-bot: I haz the power, 2016-12-12 20:05:46]

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

[Theresa, 2016-12-12 20:08:10]

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

[Theresa, 2016-12-12 20:09:04]

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/history_header.xml (right):

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/history_header.xml:27:
style="@style/ButtonCompatBorderless"
On 2016/12/12 19:14:13, dfalcantara (check my queue) wrote:
> swap style and marginTop lines?

Done.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java
(right):

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:343:
clear(false);
On 2016/12/12 19:14:13, dfalcantara (check my queue) wrote:
> Feels like this should still be called by #loadItems() directly to avoid
> situations where another set of items is loaded before it's been cleared. 
> Should be asserted in some way that doesn't conflict with HistoryAdapter, at
> least.

As discussed offline, I updated the JavaDoc for #loadItems to change the promise
about whether data is cleared since History shouldn't clear items before loading
more.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java
(right):

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java:153:
Button cbdButton = (Button) v.findViewById(R.id.clear_browsing_data_button);
On 2016/12/12 19:14:13, dfalcantara (check my queue) wrote:
> I think you can get by with just View cbdButton.

Done.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java
(right):

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java:162:
mActivity.startActivity(intent);
On 2016/12/12 19:14:13, dfalcantara (check my queue) wrote:
> For sanity's sake?
> IntentUtils#safeStartActivity()

Done.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java
(right):

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:125:
private boolean mIsHeader;
On 2016/12/12 19:14:13, dfalcantara (check my queue) wrote:
> "Header" is a little too generic because the dates can be considered "headers"
> for their sections.  Maybe mIsListHeader?

Done.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:178:
// If this is the header, there is only one item.
On 2016/12/12 19:14:13, dfalcantara (check my queue) wrote:
> What happens if you don't add this clause?  I wonder if it makes sense to
change
> the constructor to take in a timestamp instead of a TimedItem... if you
changed
> it so it just took a timestamp and changed the single callsite to immediately
> add the TimedItem to the list, then I think you'd be able to avoid storing the
> dummy item.

Done.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:220:
public static final int TYPE_HEADER = 2;
On 2016/12/12 19:14:13, dfalcantara (check my queue) wrote:
> TYPE_HEADER should be 0 since it's at the top.  I don't think anything'll
break.

Done.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:307:
mGroups.add(header);
On 2016/12/12 19:14:13, dfalcantara (check my queue) wrote:
> Should assert the size is 0 at this point.  The add() will incorrectly put the
> header at the end of the list if stuff is already there.

Done.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:348:
return pair.second == -1 ? TYPE_HEADER : pair.second == 0 ? TYPE_DATE :
TYPE_NORMAL;
On 2016/12/12 19:14:13, dfalcantara (check my queue) wrote:
> 1) Should just expand this into a set of conditionals now that there's three
> possibilities.
> 
> 2) Should pull out -1 as a new constant that indicates that it's a header, I
> think.

Done. I made TYPE_HEADER = -1.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:360:
}
On 2016/12/12 19:14:13, dfalcantara (check my queue) wrote:
> Should assert false at this point; I don't know how you could create anything
> except those three.

Done.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:369:
} else if (!(holder instanceof HeaderViewHolder)) {
On 2016/12/12 19:14:13, dfalcantara (check my queue) wrote:
> null !instanceof HeaderViewHolder, so this would likely crash if it ever hit
> that.

I don't think that null can be passed in for the holder. If it can be null, we
are are already calling bindViewHolderForTimedItem() because the current else
block doesn't have a conditional.

https://codereview.chromium.org/2562123003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:384:
int i = position;
On 2016/12/12 19:14:13, dfalcantara (check my queue) wrote:
> Move the int i line down to the for loop area.

Done.

[commit-bot: I haz the power, 2016-12-12 20:09:57]

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

[gone, 2016-12-12 21:06:12]

lgtm

[Theresa, 2016-12-12 21:07:49]

The CQ bit was unchecked by twellington@chromium.org

[Theresa, 2016-12-12 21:07:49]

The CQ bit was checked by twellington@chromium.org

[commit-bot: I haz the power, 2016-12-12 21:09:09]

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-12-12 22:24:40]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481576869688160,
"parent_rev": "96f144f9d95c8a6539c2445cb78645f36d7965e6", "commit_rev":
"2afb4c1a09f43ff1a8e5ccc5bf37dbec44d02847"}

[commit-bot: I haz the power, 2016-12-12 22:25:08]

Description was changed from

==========
[Android History] Add clear browsing data button and privacy disclaimers

Adds an optional header to DateDividedAdapter and adds a new layout
with a clear browsing data button and privacy disclaimers as a header
in the history ui.

BUG=654071
==========

to

==========
[Android History] Add clear browsing data button and privacy disclaimers

Adds an optional header to DateDividedAdapter and adds a new layout
with a clear browsing data button and privacy disclaimers as a header
in the history ui.

BUG=654071

Review-Url: https://codereview.chromium.org/2562123003
==========

[commit-bot: I haz the power, 2016-12-12 22:25:09]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-12 22:28:56]

Description was changed from

==========
[Android History] Add clear browsing data button and privacy disclaimers

Adds an optional header to DateDividedAdapter and adds a new layout
with a clear browsing data button and privacy disclaimers as a header
in the history ui.

BUG=654071

Review-Url: https://codereview.chromium.org/2562123003
==========

to

==========
[Android History] Add clear browsing data button and privacy disclaimers

Adds an optional header to DateDividedAdapter and adds a new layout
with a clear browsing data button and privacy disclaimers as a header
in the history ui.

BUG=654071

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

[commit-bot: I haz the power, 2016-12-12 22:28:57]

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