Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(122)

Issue 2754363004: OfflineContentProvider changes to start service (Closed)

Created:
3 years, 9 months ago by David Trainor- moved to gerrit
Modified:
3 years, 9 months ago
Reviewers:
qinmin, gone, fgorski
CC:
chromium-reviews, asanka, mikecase+watch_chromium.org, jbudorick+watch_chromium.org, agrieve+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

OfflineContentProvider changes to start service Start bridging the OfflineContentProvider code to the DownloadNotificationService. This patch just adds the Java hooks to connect the OfflineContentProvider pipeline with a class that spins up and manages the lifecycle of the service. A future patch will refactor the service so we can actually make the appropriate calls on it. BUG=691805

Patch Set 1 #

Total comments: 34
Unified diffs Side-by-side diffs Delta from patch set Stats (+676 lines, -3 lines) Patch
M chrome/android/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java View 2 chunks +2 lines, -0 lines 2 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java View 3 chunks +4 lines, -1 line 0 comments Download
A chrome/android/java/src/org/chromium/chrome/browser/download/items/DownloadNotificationServiceNotifierUi.java View 1 chunk +125 lines, -0 lines 8 comments Download
A chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotifier.java View 1 chunk +198 lines, -0 lines 6 comments Download
A chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotifierFactory.java View 1 chunk +50 lines, -0 lines 7 comments Download
M chrome/android/java_sources.gni View 2 chunks +4 lines, -0 lines 0 comments Download
A chrome/android/junit/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotifierUnitTest.java View 1 chunk +257 lines, -0 lines 0 comments Download
M components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/ContentId.java View 1 chunk +25 lines, -0 lines 5 comments Download
M components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineItem.java View 1 chunk +7 lines, -2 lines 5 comments Download
M components/offline_items_collection/core/offline_item_state.h View 1 chunk +2 lines, -0 lines 1 comment Download
M tools/android/eclipse/.classpath View 1 chunk +1 line, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 9 (3 generated)
David Trainor- moved to gerrit
First patch at looking at how to bridge to the notification service. I'm building this ...
3 years, 9 months ago (2017-03-18 00:40:49 UTC) #2
David Trainor- moved to gerrit
+fgorski since he's been reviewing the set of CLs since the beginning as well :). ...
3 years, 9 months ago (2017-03-18 00:45:41 UTC) #4
gone
https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode1172 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1172: OfflineContentAggregatorNotifierFactory.create(getApplicationContext()); ContextUtils.getApplicationContext() https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org/chromium/chrome/browser/download/items/DownloadNotificationServiceNotifierUi.java File chrome/android/java/src/org/chromium/chrome/browser/download/items/DownloadNotificationServiceNotifierUi.java (right): https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org/chromium/chrome/browser/download/items/DownloadNotificationServiceNotifierUi.java#newcode48 chrome/android/java/src/org/chromium/chrome/browser/download/items/DownloadNotificationServiceNotifierUi.java:48: mApplicationContext ...
3 years, 9 months ago (2017-03-20 19:03:37 UTC) #5
fgorski
This is mostly java so Dan's covering it nicely. I only have a couple of ...
3 years, 9 months ago (2017-03-20 20:19:20 UTC) #6
qinmin
lgtm % comments https://codereview.chromium.org/2754363004/diff/1/components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/ContentId.java File components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/ContentId.java (right): https://codereview.chromium.org/2754363004/diff/1/components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/ContentId.java#newcode15 components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/ContentId.java:15: public String namespace; nit: public final ...
3 years, 9 months ago (2017-03-22 07:53:58 UTC) #7
David Trainor- moved to gerrit
3 years, 9 months ago (2017-03-25 03:31:13 UTC) #8
Sorry about that!  I'm abandoning this due to a much simpler alternative :(.  My
bad for the wasted review :(.

https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1172:
OfflineContentAggregatorNotifierFactory.create(getApplicationContext());
On 2017/03/20 19:03:35, dfalcantara (load balance plz) wrote:
> ContextUtils.getApplicationContext()

Hmm we use getApplicationContext() throughout this file.  I can use ContextUtils
here, but maybe we should just migrate all sources if that's the desired usage?

https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/items/DownloadNotificationServiceNotifierUi.java
(right):

https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/items/DownloadNotificationServiceNotifierUi.java:48:
mApplicationContext = context.getApplicationContext();
On 2017/03/20 19:03:36, dfalcantara (load balance plz) wrote:
> Do you have a good reason not to use ContextUtils.getApplicationContext()?

Eh just a bit cleaner to pass in dependencies.  I'm fine getting rid of this as
long as the unit tests work nicely :).

https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/items/DownloadNotificationServiceNotifierUi.java:66:
mIsServiceBound = true;
On 2017/03/20 19:03:35, dfalcantara (load balance plz) wrote:
> seems like this boolean should go where you're actually starting and binding
the
> service

I didn't do it originally because I wanted to make startAndBindService and
unbindService mockable for unit tests.  Ended up not testing this layer (which
may or may not be a great idea...).  Will remove those helpers.

https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/items/DownloadNotificationServiceNotifierUi.java:79:
mBoundService = null;
On 2017/03/20 20:19:20, fgorski wrote:
> why not reset the values in unbindService?

See above comment on testing.  Will change though :).

https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotifier.java
(right):

https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotifier.java:62:
/** Any updates from {@code mProvider} that have not been propagated to {@code
mUi} yet. */
On 2017/03/20 19:03:36, dfalcantara (load balance plz) wrote:
> @link #mUi, etc

Done.

https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotifier.java:69:
* A list of 'active' {@link OfflineItem}'s as currently known by this class. 
'Active' means
On 2017/03/20 19:03:36, dfalcantara (load balance plz) wrote:
> no apostrophe before s

Done.

https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotifier.java:110:
if (!mUi.onUiNeeded(mUiReadyObserver)) break;
On 2017/03/20 19:03:36, dfalcantara (load balance plz) wrote:
> does this really need to happen for every single item?  Everything after the
> first attempt looks like a no-op.

I think the main worry is that the service could decide to shut itself down. 
This needs to be fixed, but it's a different lifecycle issue.  I'll add a TODO
because that's a good point.

https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotifierFactory.java
(right):

https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotifierFactory.java:25:
* @param context
On 2017/03/20 19:03:36, dfalcantara (load balance plz) wrote:
> fill this in

Done.

https://codereview.chromium.org/2754363004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/items/OfflineContentAggregatorNotifierFactory.java:28:
if (sNotifier != null) return;
On 2017/03/20 19:03:36, dfalcantara (load balance plz) wrote:
> Should synchronize on an Object to prevent this from being created twice. 
> You've got no guards against what thread is actually being used to call this.

Put an assert check for UI thread.  LMK if you'd like more strict requirements.

https://codereview.chromium.org/2754363004/diff/1/components/offline_items_co...
File
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/ContentId.java
(right):

https://codereview.chromium.org/2754363004/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/ContentId.java:23:

On 2017/03/20 19:03:36, dfalcantara (load balance plz) wrote:
> Also randomly added?

See comment in OfflineItem.java.

https://codereview.chromium.org/2754363004/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/ContentId.java:35:
ContentId lhs = (ContentId) o;
On 2017/03/20 20:19:20, fgorski wrote:
> rhs?
> Both because this is right of equals and you put it on the right below.

Done.

https://codereview.chromium.org/2754363004/diff/1/components/offline_items_co...
File
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineItem.java
(right):

https://codereview.chromium.org/2754363004/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineItem.java:48:
this.id = new ContentId();
On 2017/03/20 19:03:36, dfalcantara (load balance plz) wrote:
> why add the this. for one of them but not the other two?

Ah sorry I used to allow passing in an id, so I had "this.id = id;"  Didn't
remove the this.

https://codereview.chromium.org/2754363004/diff/1/components/offline_items_co...
components/offline_items_collection/core/android/java/src/org/chromium/components/offline_items_collection/OfflineItem.java:55:
return getClass().getSimpleName() + "@" + Integer.toHexString(hashCode()) + " ["
+ id + "]";
On 2017/03/20 19:03:36, dfalcantara (load balance plz) wrote:
> was this thing automatically added?

No but it makes debugging unit tests easier as mockito likes to print out the
object when things don't work as expected.

Powered by Google App Engine
This is Rietveld 408576698