Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2851023003:
Moved OMA downloads out of DownloadManagerService (Closed)
|
Created:
3 years, 7 months ago by shaktisahu Modified:
3 years, 7 months ago CC:
chromium-reviews, David Trainor- moved to gerrit, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMoved OMA downloads out of DownloadManagerService
This CL moves most of the interactions with the android DownloadManager
from DownloadManagerService to DownloadManagerDelegate. OMA downloads
use android DownloadManager, so OMA related code and shared prefs were
moved to OMADownloadHandler. The broadcast receiver for DownloadManager
was also moved to DownloadManagerDelegate.
BUG=721139
Review-Url: https://codereview.chromium.org/2851023003
Cr-Commit-Position: refs/heads/master@{#471264}
Committed: https://chromium.googlesource.com/chromium/src/+/8eb8e12559dcda966f159d8f0912a0ef863ae105
Patch Set 1 #
Total comments: 32
Patch Set 2 : comments #Patch Set 3 : more #
Total comments: 1
Patch Set 4 : nits #
Total comments: 16
Patch Set 5 : comments #Patch Set 6 : decouple DownloadManagerDelegate and OMADownloadHandler #Patch Set 7 : Fixed tests #Patch Set 8 : Fix test #
Messages
Total messages: 36 (22 generated)
Description was changed from ========== Moved OMA downloads out of DownloadManagerService This CL moves most of the interactions with the android DownloadManager from DownloadManagerService to DownloadManagerDelegate. OMA downloads use android DownloadManager, so OMA related code and shared prefs were moved to OMADownloadHandler. The broadcast receiver for DownloadManager was also moved to DownloadManagerDelegate. BUG= ========== to ========== Moved OMA downloads out of DownloadManagerService This CL moves most of the interactions with the android DownloadManager from DownloadManagerService to DownloadManagerDelegate. OMA downloads use android DownloadManager, so OMA related code and shared prefs were moved to OMADownloadHandler. The broadcast receiver for DownloadManager was also moved to DownloadManagerDelegate. BUG= ==========
shaktisahu@chromium.org changed reviewers: + qinmin@chromium.org
shaktisahu@chromium.org changed reviewers: + dtrainor@chromium.org
Hi, Please take a look and let me know if this seems in the correct direction.
https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:440: boolean success = addCompletedDownload(item); We no longer need to do this if the MIME type is for dd file. Instead, we should just call let OMADownloadHandler to process the file. there is no need to add the download to Android DM and then later delete it from android DM when we parse the xml file. https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:661: mDownloadManagerDelegate.enqueueDownloadManagerRequest(item, notifyCompleted, this); notifyCompleted should always be false now, you can remove all the code affected by this variable. For dd file, it used to be downloaded by Android DownloadManager. So we use this variable to tell Android DM that notification shouldn't be shown after download completes because Chrome still need to parse the xml file. Now the dd file is downloaded by Chrome, so this variable is no longer used. https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java (right): https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:118: private final DownloadManagerService mDownloadManagerService; this is singleton, you don't need it as a member variable https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:1019: return true; nit: return true can go to the previous line
https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java (right): https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:49: public void setOMADownloadHandler(OMADownloadHandler omaDownloadHandler) { Javadoc? Or should we just mandate one in the constructor? Ah you probably can't if you extend BroadcastReceiver? https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:144: void removeCompletedDownload(String downloadGuid) { Future change suggestion: Some of these run on the UI thread and some run on the a background thread. Should we just make this class handle all of that? I'd even be fine with making them all sync in one class and adding a wrapper that adds the concept of callbacks and makes the calls async if we need both of those methods. If not they should just all be async here? https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:239: builder.setFileName(c.getString(c.getColumnIndex( Can you pull the setFileName out of the setDownloadInfo? https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:300: if (mSystemDownloadIdMap.size() == 0) { Can we check this even if downloadItem is null? https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:313: * TODO(qinmin): move this to DownloadManagerDelegate. Remove the TODO https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:316: * @param notifyCompleted Whether to notify the user after Downloadmanager completes the Downloadmanager -> DownloadManager https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:321: EnqueueDownloadRequestTask task = new EnqueueDownloadRequestTask(item, callback); Don't need variable. newEnqueueDownloadRequestTask(...).execute(notifyCompleted)? https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:346: private final DownloadItem mDownloadItem; Pull finals up top https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:358: public Boolean doInBackground(Boolean... booleans) { At this point should we just pass notifyCompleted in the constructor with the rest? Seems arbitrary to have this one used as a param. https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:367: // Use ERROR_UNHANDLED_HTTP_CODE so that it will be treated as Does this comment all fit on one line? https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:661: mDownloadManagerDelegate.enqueueDownloadManagerRequest(item, notifyCompleted, this); On 2017/05/01 21:47:05, qinmin wrote: > notifyCompleted should always be false now, you can remove all the code affected > by this variable. > > For dd file, it used to be downloaded by Android DownloadManager. So we use this > variable to tell Android DM that notification shouldn't be shown after download > completes because Chrome still need to parse the xml file. Now the dd file is > downloaded by Chrome, so this variable is no longer used. Nice! That cleans up a lot of the AsyncTask code as well :). https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java (right): https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:316: // Testing only ..... Remove :)
https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:661: mDownloadManagerDelegate.enqueueDownloadManagerRequest(item, notifyCompleted, this); On 2017/05/01 21:47:05, qinmin wrote: > notifyCompleted should always be false now, you can remove all the code affected > by this variable. > > For dd file, it used to be downloaded by Android DownloadManager. So we use this > variable to tell Android DM that notification shouldn't be shown after download > completes because Chrome still need to parse the xml file. Now the dd file is > downloaded by Chrome, so this variable is no longer used. ignore the notifyCompleted comment, we still need it due to INSTALL_NOTIFY_URI in OMADOwnloadHandler.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #2 (id:60001) has been deleted
Addressed all the comments except the comments about making all the functions in DownloadManagerDelegate async (I think it should be done in a future CL). Can you please take another look? https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java (right): https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:49: public void setOMADownloadHandler(OMADownloadHandler omaDownloadHandler) { On 2017/05/03 20:25:12, David Trainor-ping if over 24h wrote: > Javadoc? Or should we just mandate one in the constructor? Ah you probably > can't if you extend BroadcastReceiver? Done. https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:144: void removeCompletedDownload(String downloadGuid) { On 2017/05/03 20:25:12, David Trainor-ping if over 24h wrote: > Future change suggestion: Some of these run on the UI thread and some run on the > a background thread. Should we just make this class handle all of that? I'd > even be fine with making them all sync in one class and adding a wrapper that > adds the concept of callbacks and makes the calls async if we need both of those > methods. If not they should just all be async here? Ok. I think we should make them async here. That should be for a future CL though. https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:239: builder.setFileName(c.getString(c.getColumnIndex( On 2017/05/03 20:25:12, David Trainor-ping if over 24h wrote: > Can you pull the setFileName out of the setDownloadInfo? Done. https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:300: if (mSystemDownloadIdMap.size() == 0) { On 2017/05/03 20:25:11, David Trainor-ping if over 24h wrote: > Can we check this even if downloadItem is null? I added this to the above block as well. Now it should check for unregister whenever there is a remove. https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:313: * TODO(qinmin): move this to DownloadManagerDelegate. On 2017/05/03 20:25:11, David Trainor-ping if over 24h wrote: > Remove the TODO Done. https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:316: * @param notifyCompleted Whether to notify the user after Downloadmanager completes the On 2017/05/03 20:25:12, David Trainor-ping if over 24h wrote: > Downloadmanager -> DownloadManager Done. https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:321: EnqueueDownloadRequestTask task = new EnqueueDownloadRequestTask(item, callback); On 2017/05/03 20:25:12, David Trainor-ping if over 24h wrote: > Don't need variable. > newEnqueueDownloadRequestTask(...).execute(notifyCompleted)? Done. https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:346: private final DownloadItem mDownloadItem; On 2017/05/03 20:25:11, David Trainor-ping if over 24h wrote: > Pull finals up top Done. https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:358: public Boolean doInBackground(Boolean... booleans) { On 2017/05/03 20:25:12, David Trainor-ping if over 24h wrote: > At this point should we just pass notifyCompleted in the constructor with the > rest? Seems arbitrary to have this one used as a param. Done. https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:367: // Use ERROR_UNHANDLED_HTTP_CODE so that it will be treated as On 2017/05/03 20:25:12, David Trainor-ping if over 24h wrote: > Does this comment all fit on one line? Done. https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:440: boolean success = addCompletedDownload(item); On 2017/05/01 21:47:05, qinmin wrote: > We no longer need to do this if the MIME type is for dd file. Instead, we should > just call let OMADownloadHandler to process the file. there is no need to add > the download to Android DM and then later delete it from android DM when we > parse the xml file. Actually, the resulting downloadId seems important in keeping an entry in the map |mPendingOMADownloads| which is needed to track the pending downloads, it is subsequently updated with the download id of the dm file. This is also used in the sharedprefs to keep the pending downloads. Not sure if we should change all that code now. https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java (right): https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:118: private final DownloadManagerService mDownloadManagerService; On 2017/05/01 21:47:05, qinmin wrote: > this is singleton, you don't need it as a member variable Done. https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:316: // Testing only ..... On 2017/05/03 20:25:12, David Trainor-ping if over 24h wrote: > Remove :) Done. https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:1019: return true; On 2017/05/01 21:47:05, qinmin wrote: > nit: return true can go to the previous line Done.
lgtm % nits https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2851023003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:661: mDownloadManagerDelegate.enqueueDownloadManagerRequest(item, notifyCompleted, this); On 2017/05/05 23:47:53, qinmin wrote: > On 2017/05/01 21:47:05, qinmin wrote: > > notifyCompleted should always be false now, you can remove all the code > affected > > by this variable. > > > > For dd file, it used to be downloaded by Android DownloadManager. So we use > this > > variable to tell Android DM that notification shouldn't be shown after > download > > completes because Chrome still need to parse the xml file. Now the dd file is > > downloaded by Chrome, so this variable is no longer used. > > ignore the notifyCompleted comment, we still need it due to INSTALL_NOTIFY_URI > in OMADOwnloadHandler. (╯°□°）╯︵ ┻━┻ https://codereview.chromium.org/2851023003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java (right): https://codereview.chromium.org/2851023003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:704: System.out.println("shakti, clearPendingOMADownload"); Remove :)
https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java (right): https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:458: protected DownloadItem findDownloadItem(long downloadId) { use default access modifier, any subclasses? https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:509: protected void handleAutoOpenAfterDownload(DownloadItem download) { why is this protected? Any one inheriting this DownloadManagerService? https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java (right): https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:247: mDownloadManagerDelegate.setOMADownloadHandler(this); This is pretty bad practice, this creates a circular reference between DownloadManagerDelegate and OMADownloadHandler https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:291: mFreeSpace = Environment.getExternalStorageDirectory().getUsableSpace(); Shouldn't this be calculated onPostExecute()? Usable space could change during doInBackground() and onPostExecute(). Although quite uncommon if this is done by user, but remember that the OS can do lot of stuffs any moment. https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:699: public boolean handleDownloadManagerBroadcast(long downloadId) { use default access modifier https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:825: protected void clearPendingOMADownload(long downloadId, String installNotifyURI) { private https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:838: public void clearPendingOMADownloads() { default access modifier
https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java (right): https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerDelegate.java:458: protected DownloadItem findDownloadItem(long downloadId) { On 2017/05/10 18:35:16, qinmin wrote: > use default access modifier, any subclasses? Done. https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java (right): https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:509: protected void handleAutoOpenAfterDownload(DownloadItem download) { On 2017/05/10 18:35:16, qinmin wrote: > why is this protected? Any one inheriting this DownloadManagerService? Done. https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java (right): https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:291: mFreeSpace = Environment.getExternalStorageDirectory().getUsableSpace(); On 2017/05/10 18:35:16, qinmin wrote: > Shouldn't this be calculated onPostExecute()? Usable space could change during > doInBackground() and onPostExecute(). Although quite uncommon if this is done by > user, but remember that the OS can do lot of stuffs any moment. Actually, this was hitting strict mode violation as it shouldn't be done on UI thread. https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:699: public boolean handleDownloadManagerBroadcast(long downloadId) { On 2017/05/10 18:35:16, qinmin wrote: > use default access modifier Done. https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:825: protected void clearPendingOMADownload(long downloadId, String installNotifyURI) { On 2017/05/10 18:35:16, qinmin wrote: > private Done. https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:838: public void clearPendingOMADownloads() { On 2017/05/10 18:35:16, qinmin wrote: > default access modifier Done.
Description was changed from ========== Moved OMA downloads out of DownloadManagerService This CL moves most of the interactions with the android DownloadManager from DownloadManagerService to DownloadManagerDelegate. OMA downloads use android DownloadManager, so OMA related code and shared prefs were moved to OMADownloadHandler. The broadcast receiver for DownloadManager was also moved to DownloadManagerDelegate. BUG= ========== to ========== Moved OMA downloads out of DownloadManagerService This CL moves most of the interactions with the android DownloadManager from DownloadManagerService to DownloadManagerDelegate. OMA downloads use android DownloadManager, so OMA related code and shared prefs were moved to OMADownloadHandler. The broadcast receiver for DownloadManager was also moved to DownloadManagerDelegate. BUG=721139 ==========
https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java (right): https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:247: mDownloadManagerDelegate.setOMADownloadHandler(this); On 2017/05/10 18:35:16, qinmin wrote: > This is pretty bad practice, this creates a circular reference between > DownloadManagerDelegate and OMADownloadHandler hmm.. not sure how to do this otherwise. DownloadManagerDelegate needs to call the following upon receiving broadcasts. mOMADownloadHandler.handleDownloadManagerBroadcast(downloadId)
https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java (right): https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:247: mDownloadManagerDelegate.setOMADownloadHandler(this); On 2017/05/11 06:44:40, shaktisahu wrote: > On 2017/05/10 18:35:16, qinmin wrote: > > This is pretty bad practice, this creates a circular reference between > > DownloadManagerDelegate and OMADownloadHandler > > hmm.. not sure how to do this otherwise. DownloadManagerDelegate needs to call > the following upon receiving broadcasts. > mOMADownloadHandler.handleDownloadManagerBroadcast(downloadId) in DownloadManagerDelegate, mSystemDownloadIdMap is only used for tracking OMA downloads now, so you can Move it into this class, and so does the onReceive() call. And you can make queryDownloadResult() a static method of DownloadManagerDelegate. So DownloadManagerDelegate should no longer have a reference to OMADownloadHandler
https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java (right): https://codereview.chromium.org/2851023003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/OMADownloadHandler.java:247: mDownloadManagerDelegate.setOMADownloadHandler(this); On 2017/05/11 17:08:52, qinmin wrote: > On 2017/05/11 06:44:40, shaktisahu wrote: > > On 2017/05/10 18:35:16, qinmin wrote: > > > This is pretty bad practice, this creates a circular reference between > > > DownloadManagerDelegate and OMADownloadHandler > > > > hmm.. not sure how to do this otherwise. DownloadManagerDelegate needs to call > > the following upon receiving broadcasts. > > mOMADownloadHandler.handleDownloadManagerBroadcast(downloadId) > > in DownloadManagerDelegate, mSystemDownloadIdMap is only used for tracking OMA > downloads now, so you can Move it into this class, and so does the onReceive() > call. And you can make queryDownloadResult() a static method of > DownloadManagerDelegate. So DownloadManagerDelegate should no longer have a > reference to OMADownloadHandler > Thank you! I decoupled the OMADownloadHandler and DownloadManagerDelegate and now there is no cyclic dependency. I didn't need to make the functions in DownloadManagerDelegate static, though all of them can be static (except the mContext variable which can be passed on through individual functions or from ContextUtils.getApplicationContext()). Let me know if you still want those functions to be static.
lgtm
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shaktisahu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, qinmin@chromium.org Link to the patchset: https://codereview.chromium.org/2851023003/#ps220001 (title: "Fix test")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1494582709209290,
"parent_rev": "76e63b012333e8446d17e7866f77fb49fc2fc5c3", "commit_rev":
"8eb8e12559dcda966f159d8f0912a0ef863ae105"}
Message was sent while issue was closed.
Description was changed from ========== Moved OMA downloads out of DownloadManagerService This CL moves most of the interactions with the android DownloadManager from DownloadManagerService to DownloadManagerDelegate. OMA downloads use android DownloadManager, so OMA related code and shared prefs were moved to OMADownloadHandler. The broadcast receiver for DownloadManager was also moved to DownloadManagerDelegate. BUG=721139 ========== to ========== Moved OMA downloads out of DownloadManagerService This CL moves most of the interactions with the android DownloadManager from DownloadManagerService to DownloadManagerDelegate. OMA downloads use android DownloadManager, so OMA related code and shared prefs were moved to OMADownloadHandler. The broadcast receiver for DownloadManager was also moved to DownloadManagerDelegate. BUG=721139 Review-Url: https://codereview.chromium.org/2851023003 Cr-Commit-Position: refs/heads/master@{#471264} Committed: https://chromium.googlesource.com/chromium/src/+/8eb8e12559dcda966f159d8f0912... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:220001) as https://chromium.googlesource.com/chromium/src/+/8eb8e12559dcda966f159d8f0912... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
