Add support for pulling icons for OfflineItems

Add support for pulling icons for OfflineItems

- Add the concept of OfflineItemVisuals, which are heavyweight visual
components associated with an OfflineItem.  Make that API async and
by request only.
- Hook up the OfflineItemVisuals to the current bridge pipeline and
notification system.  This involves building a small cache in the bridge
that handles passing these to the notification layer so that these are
queried infrequently.  In the future, having a unified cache mechanism
would be better, but right now notifications and downloads home have
very different caching characteristics.
- Added support for showing the icons in notifications.  There are still
some jumps where the icon vanishes and reappears because we process the
action before notifying native.  A follow up patch should look into
removing setting those notifications early.

BUG=691805
Review-Url: https://codereview.chromium.org/2811803006
Cr-Commit-Position: refs/heads/master@{#465146}
Committed: https://chromium.googlesource.com/chromium/src/+/cdde15e4fb35971f7008d9fadfa3192d301ac763

[David Trainor- moved to gerrit, 2017-04-12 07:12:40]

wip

[David Trainor- moved to gerrit, 2017-04-12 20:59:55]

dtrainor@chromium.org changed reviewers:
+ nyquist@chromium.org

[nyquist, 2017-04-13 05:08:25]

lgtm!

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

https://codereview.chromium.org/2811803006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadInfo.java:237:
.setIcon(visuals != null ? visuals.icon : null)
Do you find this way just as easy to read as:
.setIcon(visuals == null ? null : visuals.icon)
?
If so, please keep it :-)

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

https://codereview.chromium.org/2811803006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:1223:
// TODO(dtrainor): Shouldwe just hit the delegate and rely on that to update the
Nit: "Should we" here and below.
Or rewrite and do something like: "Consider hitting the delegate and rely on
that to update the state."

https://codereview.chromium.org/2811803006/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotificationBridgeUi.java
(right):

https://codereview.chromium.org/2811803006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotificationBridgeUi.java:127:
if (!needsVisualsForUi(item)) {
If you flip this else around you don't need the ! which might make it easier to
read. And if you do that, maybe you could do:
if (needsVisualsForUi(item)) {
  if (!mVisualsCache.containsKey(item.id)) {
    ...
instead of retrieving it at first? And move the getting-of-visual-part to just
before you put the item to the UI?

I don't feel strongly about this style though, particularly since you get that
pesky ! for the containsKey() part.

https://codereview.chromium.org/2811803006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotificationBridgeUi.java:176:
case OfflineItemState.IN_PROGRESS:
Often I want to ask the author to add . // Intentional fall through. at the end
of each line. But I think it's pretty clear it's intentional here.

https://codereview.chromium.org/2811803006/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotifierBridgeUiTest.java
(right):

https://codereview.chromium.org/2811803006/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotifierBridgeUiTest.java:62:
OfflineContentAggregatorNotificationBridgeUi bridge =
Should this and the verify-line below be part of a setup method instead? @Before
or what ever it's called nowadays. And then the opposite with the two lines at
the end? If so, you could just move the bridge to a class member instead. Or
does that mess up the mocking and verify, etc.?

https://codereview.chromium.org/2811803006/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotifierBridgeUiTest.java:89:
// TODO(dtrainor): Flesh out these validations.
Was this meant as a TODO-before-submit? Or just a normal TODO? I'm fine either
way, just wondered.

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
File
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineContentAggregatorBridge.java
(right):

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineContentAggregatorBridge.java:99:
if (areItemsAvailable()) {
I don't feel strongly, but we often early out instead to remove indents in
chrome it seems like. Up to you though. It's super short and reads well as is as
well.

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
File
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineItemVisuals.java
(right):

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineItemVisuals.java:16:
public class OfflineItemVisuals { public Bitmap icon; }
Wow, clang-format seems to like perl-style coding :-p

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
File
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/bridges/OfflineItemVisualsBridge.java
(right):

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/bridges/OfflineItemVisualsBridge.java:20:
public class OfflineItemVisualsBridge {
Nit: also add final modifier to class?

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
File components/offline_items_collection/core/offline_content_provider.h
(right):

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
components/offline_items_collection/core/offline_content_provider.h:88: // |id|
or |nullptr| if one doesn't exist.  The caller should post any
The "caller"? Do you mean the implementor of the interface?

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
File components/offline_items_collection/core/offline_item.h (right):

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
components/offline_items_collection/core/offline_item.h:153: // Can be |nullptr|
if there is no valid icon for this item.
Is gfx::Image a pointer?

[David Trainor- moved to gerrit, 2017-04-13 07:05:41]

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

[commit-bot: I haz the power, 2017-04-13 07:06:01]

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-04-13 07:14:19]

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

[commit-bot: I haz the power, 2017-04-13 07:14:20]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[David Trainor- moved to gerrit, 2017-04-13 07:20:24]

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

https://codereview.chromium.org/2811803006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadInfo.java:237:
.setIcon(visuals != null ? visuals.icon : null)
On 2017/04/13 05:08:24, nyquist wrote:
> Do you find this way just as easy to read as:
> .setIcon(visuals == null ? null : visuals.icon)
> ?
> If so, please keep it :-)

Happy to change :).

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

https://codereview.chromium.org/2811803006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java:1223:
// TODO(dtrainor): Shouldwe just hit the delegate and rely on that to update the
On 2017/04/13 05:08:24, nyquist wrote:
> Nit: "Should we" here and below.
> Or rewrite and do something like: "Consider hitting the delegate and rely on
> that to update the state."

Done.

https://codereview.chromium.org/2811803006/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotificationBridgeUi.java
(right):

https://codereview.chromium.org/2811803006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotificationBridgeUi.java:127:
if (!needsVisualsForUi(item)) {
On 2017/04/13 05:08:24, nyquist wrote:
> If you flip this else around you don't need the ! which might make it easier
to
> read. And if you do that, maybe you could do:
> if (needsVisualsForUi(item)) {
>   if (!mVisualsCache.containsKey(item.id)) {
>     ...
> instead of retrieving it at first? And move the getting-of-visual-part to just
> before you put the item to the UI?
> 
> I don't feel strongly about this style though, particularly since you get that
> pesky ! for the containsKey() part.

Done.

https://codereview.chromium.org/2811803006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotificationBridgeUi.java:176:
case OfflineItemState.IN_PROGRESS:
On 2017/04/13 05:08:24, nyquist wrote:
> Often I want to ask the author to add . // Intentional fall through. at the
end
> of each line. But I think it's pretty clear it's intentional here.

Yeah I wasn't sure what looked nicer, but this looked cleaner than a long if
block.  I agree the fallthrough would probably be overkill in this case.

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
File
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineContentAggregatorBridge.java
(right):

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineContentAggregatorBridge.java:99:
if (areItemsAvailable()) {
On 2017/04/13 05:08:24, nyquist wrote:
> I don't feel strongly, but we often early out instead to remove indents in
> chrome it seems like. Up to you though. It's super short and reads well as is
as
> well.

Ah you're right.  Yeah it's my old habits of trying to have a single return
point bleeding through.  Fixed.

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
File
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineItemVisuals.java
(right):

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineItemVisuals.java:16:
public class OfflineItemVisuals { public Bitmap icon; }
On 2017/04/13 05:08:24, nyquist wrote:
> Wow, clang-format seems to like perl-style coding :-p

I was surprised as well... :/

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
File
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/bridges/OfflineItemVisualsBridge.java
(right):

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/bridges/OfflineItemVisualsBridge.java:20:
public class OfflineItemVisualsBridge {
On 2017/04/13 05:08:24, nyquist wrote:
> Nit: also add final modifier to class?

Done.

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
File components/offline_items_collection/core/offline_content_provider.h
(right):

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
components/offline_items_collection/core/offline_content_provider.h:88: // |id|
or |nullptr| if one doesn't exist.  The caller should post any
On 2017/04/13 05:08:25, nyquist wrote:
> The "caller"? Do you mean the implementor of the interface?

Ah yeah sorry thanks!

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
File components/offline_items_collection/core/offline_item.h (right):

https://codereview.chromium.org/2811803006/diff/20001/components/offline_item...
components/offline_items_collection/core/offline_item.h:153: // Can be |nullptr|
if there is no valid icon for this item.
On 2017/04/13 05:08:25, nyquist wrote:
> Is gfx::Image a pointer?

:/ crap yeah I changed it once I realized the internal storage was ref counted.

[David Trainor- moved to gerrit, 2017-04-13 07:21:31]

Description was changed from

==========
Add support for pulling icons for OfflineItems

- Add the concept of OfflineItemVisuals, which are heavyweight visual
components associated with an OfflineItem.  Make that API async and
by request only.
- Hook up the OfflineItemVisuals to the current bridge pipeline and
notification system.  This involves building a small cache in the bridge
that handles passing these to the notification layer so that these are
queried infrequently.  In the future, having a unified cache mechanism
would be better, but right now notifications and downloads home have
very different caching characteristics.
- Added support for showing the icons in notifications.  There are still
some jumps where the icon vanishes and reappears because we process the
action before notifying native.  A follow up patch should look into
removing setting those notifications early.
- Added a MultiMap to make storing multiple entries for a list easier.

BUG=691805
==========

to

==========
Add support for pulling icons for OfflineItems

- Add the concept of OfflineItemVisuals, which are heavyweight visual
components associated with an OfflineItem.  Make that API async and
by request only.
- Hook up the OfflineItemVisuals to the current bridge pipeline and
notification system.  This involves building a small cache in the bridge
that handles passing these to the notification layer so that these are
queried infrequently.  In the future, having a unified cache mechanism
would be better, but right now notifications and downloads home have
very different caching characteristics.
- Added support for showing the icons in notifications.  There are still
some jumps where the icon vanishes and reappears because we process the
action before notifying native.  A follow up patch should look into
removing setting those notifications early.

BUG=691805
==========

[David Trainor- moved to gerrit, 2017-04-13 07:26:54]

dtrainor@chromium.org changed reviewers:
+ msw@chromium.org

[David Trainor- moved to gerrit, 2017-04-13 07:26:54]

msw@ can I get an OWNERS review for adding ui/gfx to DEPS? :)

[msw, 2017-04-13 16:41:22]

lgtm

[David Trainor- moved to gerrit, 2017-04-14 01:30:51]

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

[commit-bot: I haz the power, 2017-04-14 01:31:14]

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-04-14 02:03:07]

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

[commit-bot: I haz the power, 2017-04-14 02:03:09]

Dry run: 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...)

[David Trainor- moved to gerrit, 2017-04-14 23:32:33]

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

[commit-bot: I haz the power, 2017-04-14 23:33:02]

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-04-15 00:02:55]

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

[commit-bot: I haz the power, 2017-04-15 00:02:56]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[David Trainor- moved to gerrit, 2017-04-17 20:07:32]

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

[commit-bot: I haz the power, 2017-04-17 20:07:57]

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-04-17 21:27:48]

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

[commit-bot: I haz the power, 2017-04-17 21:27:49]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[David Trainor- moved to gerrit, 2017-04-17 23:30:48]

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

[commit-bot: I haz the power, 2017-04-17 23:31:23]

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-04-18 01:18:42]

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

[commit-bot: I haz the power, 2017-04-18 01:18:44]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[David Trainor- moved to gerrit, 2017-04-18 03:47:56]

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

[commit-bot: I haz the power, 2017-04-18 03:48:23]

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-04-18 05:02:23]

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

[commit-bot: I haz the power, 2017-04-18 05:02:24]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2017-04-18 05:03:03]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2017-04-18 05:03:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2811803006/#ps150001
(title: "More findbugs")

[David Trainor- moved to gerrit, 2017-04-18 05:03:10]

The CQ bit was unchecked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2017-04-18 05:04:30]

The CQ bit was checked by dtrainor@chromium.org

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

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-04-18 05:09:11]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1492491870365680,
"parent_rev": "52c5ec950ba33ce26507840a34fd8690f86fe801", "commit_rev":
"cdde15e4fb35971f7008d9fadfa3192d301ac763"}

[commit-bot: I haz the power, 2017-04-18 05:09:18]

Description was changed from

==========
Add support for pulling icons for OfflineItems

- Add the concept of OfflineItemVisuals, which are heavyweight visual
components associated with an OfflineItem.  Make that API async and
by request only.
- Hook up the OfflineItemVisuals to the current bridge pipeline and
notification system.  This involves building a small cache in the bridge
that handles passing these to the notification layer so that these are
queried infrequently.  In the future, having a unified cache mechanism
would be better, but right now notifications and downloads home have
very different caching characteristics.
- Added support for showing the icons in notifications.  There are still
some jumps where the icon vanishes and reappears because we process the
action before notifying native.  A follow up patch should look into
removing setting those notifications early.

BUG=691805
==========

to

==========
Add support for pulling icons for OfflineItems

- Add the concept of OfflineItemVisuals, which are heavyweight visual
components associated with an OfflineItem.  Make that API async and
by request only.
- Hook up the OfflineItemVisuals to the current bridge pipeline and
notification system.  This involves building a small cache in the bridge
that handles passing these to the notification layer so that these are
queried infrequently.  In the future, having a unified cache mechanism
would be better, but right now notifications and downloads home have
very different caching characteristics.
- Added support for showing the icons in notifications.  There are still
some jumps where the icon vanishes and reappears because we process the
action before notifying native.  A follow up patch should look into
removing setting those notifications early.

BUG=691805

Review-Url: https://codereview.chromium.org/2811803006
Cr-Commit-Position: refs/heads/master@{#465146}
Committed:
https://chromium.googlesource.com/chromium/src/+/cdde15e4fb35971f7008d9fadfa3...
==========

[commit-bot: I haz the power, 2017-04-18 05:09:20]

Committed patchset #9 (id:150001) as
https://chromium.googlesource.com/chromium/src/+/cdde15e4fb35971f7008d9fadfa3...