Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1359383002:
webapps: Add cleanup task when opening up WebappActivity to clean old web apps (Closed)
|
Created:
5 years, 3 months ago by Lalit Maganti Modified:
5 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@webapp-cleanup Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionwebapps: Add cleanup task when opening up WebappActivity to clean old web apps
Since there is no sure way of knowing when a web app has been removed from
the Home screen, a heuristic has to be used instead.
The heuristic is that if the web app has not been opened in 3 months then it
can probably be safely removed. The full cleanup check is run once a month when
starting a web app so we don't choke the system every time a web app is opened.
BUG=535174
Committed: https://crrev.com/8d3b2c77b75ee7261ab91a7caf4fb2a25a3abc93
Cr-Commit-Position: refs/heads/master@{#350579}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address review comments #
Total comments: 10
Patch Set 3 : Fix remaining comments #Patch Set 4 : Fix nits #Patch Set 5 : Rebase on master #
Total comments: 4
Patch Set 6 : Address all remaining issues #Patch Set 7 : Fix remaining uissues #
Total comments: 10
Patch Set 8 : Address Mounir's comments #
Messages
Total messages: 19 (3 generated)
lalitm@google.com changed reviewers: + dfalcantara@chromium.org, mlamouri@chromium.org
Dan and Mounir: PTAL
Two comments on the description first: - "Since there is no sure fire way of " => s/fire // - Could you open a bug?
On 2015/09/23 14:04:28, Mounir Lamouri wrote: > Two comments on the description first: > - "Since there is no sure fire way of " => s/fire // > - Could you open a bug? Done both.
https://codereview.chromium.org/1359383002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/1359383002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:169: // Kick off the old web app cleanup (if we haven't alread) now that we have queued the nit: alread? I'd move this further down in the pipeline (maybe onStart or onResume). Even if it's an asynctask, it still takes cycles to kick it off. https://codereview.chromium.org/1359383002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1359383002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:140: /** Package private for use by WebappRegistry */ nit: could've sworn I'd put a comment last time about not bothering to mention why it's package private. can you delete the one above, as well? https://codereview.chromium.org/1359383002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/1359383002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:33: /* Represnts a period of 4 weeks in milliseconds */ /** Represents a period of 4 weeks in milliseconds. */ https://codereview.chromium.org/1359383002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:36: /* Represnts a period of 13 weeks in milliseconds */ same fix as above. rename to WEBAPP_UNOPENED_CLEANUP_DURATION? https://codereview.chromium.org/1359383002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:91: * Deletes the data for all "old" web apps. i.e. webapps which have not been opened by the user nit: web apps or webapps. instead of leaving useful information in just the commit message, put the information about cleaning up only once a month and deleting apps unused for three months here. without any context it's just an opaque blob of heuristics. https://codereview.chromium.org/1359383002/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java (right): https://codereview.chromium.org/1359383002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:192: SystemClock.setCurrentTimeMillis(WebappRegistry.FULL_CLEANUP_DURATION - 1); instead of mucking about with the clock, add some way of passing in the time when the WebappRegistry needs it. example of why mucking about with system settings is bad: you don't reset it at the end of the test, and I don't know if Robolectric handles that for you. https://codereview.chromium.org/1359383002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:194: addWebappsToRegistry("test"); nit: can you use something other than "test"? it's too generic.
https://codereview.chromium.org/1359383002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/1359383002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:169: // Kick off the old web app cleanup (if we haven't alread) now that we have queued the On 2015/09/24 10:27:11, dfalcantara wrote: > nit: alread? > > I'd move this further down in the pipeline (maybe onStart or onResume). Even if > it's an asynctask, it still takes cycles to kick it off. Moved to onResume as this point, UI work should have almost been done. https://codereview.chromium.org/1359383002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1359383002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:140: /** Package private for use by WebappRegistry */ On 2015/09/24 10:27:11, dfalcantara wrote: > nit: could've sworn I'd put a comment last time about not bothering to mention > why it's package private. can you delete the one above, as well? Think you said something related but not for package private AFAIK. Removed both. https://codereview.chromium.org/1359383002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/1359383002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:33: /* Represnts a period of 4 weeks in milliseconds */ On 2015/09/24 10:27:11, dfalcantara wrote: > /** Represents a period of 4 weeks in milliseconds. */ Done. https://codereview.chromium.org/1359383002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:36: /* Represnts a period of 13 weeks in milliseconds */ On 2015/09/24 10:27:12, dfalcantara wrote: > same fix as above. rename to WEBAPP_UNOPENED_CLEANUP_DURATION? Done. https://codereview.chromium.org/1359383002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:91: * Deletes the data for all "old" web apps. i.e. webapps which have not been opened by the user On 2015/09/24 10:27:11, dfalcantara wrote: > nit: web apps or webapps. > > instead of leaving useful information in just the commit message, put the > information about cleaning up only once a month and deleting apps unused for > three months here. without any context it's just an opaque blob of heuristics. Done. https://codereview.chromium.org/1359383002/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java (right): https://codereview.chromium.org/1359383002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:192: SystemClock.setCurrentTimeMillis(WebappRegistry.FULL_CLEANUP_DURATION - 1); On 2015/09/24 10:27:12, dfalcantara wrote: > instead of mucking about with the clock, add some way of passing in the time > when the WebappRegistry needs it. example of why mucking about with system > settings is bad: you don't reset it at the end of the test, and I don't know if > Robolectric handles that for you. Done. https://codereview.chromium.org/1359383002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:194: addWebappsToRegistry("test"); On 2015/09/24 10:27:12, dfalcantara wrote: > nit: can you use something other than "test"? it's too generic. Done.
https://codereview.chromium.org/1359383002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/1359383002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:33: /** Represnts a period of 4 weeks in milliseconds */ Represents. https://codereview.chromium.org/1359383002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:93: * does not proceed with cleanup. "Cleanup is run at most once a month"? https://codereview.chromium.org/1359383002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java (right): https://codereview.chromium.org/1359383002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:204: assertEquals(new HashSet<String>(Arrays.asList("test")), actual); teeeeeeeeeeeeeeeeest? https://codereview.chromium.org/1359383002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:231: assertEquals(new HashSet<String>(Arrays.asList("test")), actual); "test"? https://codereview.chromium.org/1359383002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:248: long lastUsed = currentTime - WebappRegistry.WEBAPP_UNOPENED_CLEANUP_DURATION; any reason why this doesn't +1 but the one above does?
https://codereview.chromium.org/1359383002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/1359383002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:33: /** Represnts a period of 4 weeks in milliseconds */ On 2015/09/24 12:45:09, dfalcantara wrote: > Represents. Done and below too. https://codereview.chromium.org/1359383002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:93: * does not proceed with cleanup. On 2015/09/24 12:45:09, dfalcantara wrote: > "Cleanup is run at most once a month"? Done. https://codereview.chromium.org/1359383002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java (right): https://codereview.chromium.org/1359383002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:204: assertEquals(new HashSet<String>(Arrays.asList("test")), actual); On 2015/09/24 12:45:10, dfalcantara wrote: > teeeeeeeeeeeeeeeeest? Done. https://codereview.chromium.org/1359383002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:231: assertEquals(new HashSet<String>(Arrays.asList("test")), actual); On 2015/09/24 12:45:10, dfalcantara wrote: > "test"? Done. https://codereview.chromium.org/1359383002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:248: long lastUsed = currentTime - WebappRegistry.WEBAPP_UNOPENED_CLEANUP_DURATION; On 2015/09/24 12:45:10, dfalcantara wrote: > any reason why this doesn't +1 but the one above does? The one above is just outside the cleanup window and this just is just inside - they are testing different things.
Dan: should have addressed your offline comments.
need to review again after you figure out why the test is passing, given the < and <= difference. https://codereview.chromium.org/1359383002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java (right): https://codereview.chromium.org/1359383002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:47: mSharedPreferences.edit().putLong(KEY_LAST_CLEANUP, 0).commit(); nit: make a private static final int INITIAL_TIME = 0, then make it so that currentTime = INITIAL_TIME + FULL_CLEANUP_DURATION so that's explicit https://codereview.chromium.org/1359383002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:242: assertEquals(0, lastCleanup); INITIAL_TIME
Should have sorted inconsistencies and all remaining issues. https://codereview.chromium.org/1359383002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java (right): https://codereview.chromium.org/1359383002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:47: mSharedPreferences.edit().putLong(KEY_LAST_CLEANUP, 0).commit(); On 2015/09/24 14:08:30, dfalcantara wrote: > nit: make a private static final int INITIAL_TIME = 0, then make it so that > currentTime = INITIAL_TIME + FULL_CLEANUP_DURATION so that's explicit Done. https://codereview.chromium.org/1359383002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:242: assertEquals(0, lastCleanup); On 2015/09/24 14:08:30, dfalcantara wrote: > INITIAL_TIME Done.
lgtm
lgtm with comments adressed https://codereview.chromium.org/1359383002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/1359383002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:166: } Doing that in onResume means that it would be called after being put back in the foreground, right? Is there a better place? https://codereview.chromium.org/1359383002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/1359383002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:34: static final long FULL_CLEANUP_DURATION = 4 * 7 * 24 * 60 * 60 * 1000; Could you use http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/TimeUnit.html :) https://codereview.chromium.org/1359383002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:37: static final long WEBAPP_UNOPENED_CLEANUP_DURATION = 13 * 7 * 24 * 60 * 60 * 1000; ditto https://codereview.chromium.org/1359383002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:103: if (currentTime - lastCleanup < FULL_CLEANUP_DURATION) return null; nit: I would use parenthesis: |if ((currentTime - lastUsed) < FULL_CLEANUP_DURATION) continue; https://codereview.chromium.org/1359383002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:109: if (currentTime - lastUsed < WEBAPP_UNOPENED_CLEANUP_DURATION) continue; ditto
Sending to CQ. https://codereview.chromium.org/1359383002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/1359383002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:166: } On 2015/09/24 15:20:04, Mounir Lamouri wrote: > Doing that in onResume means that it would be called after being put back in the > foreground, right? Is there a better place? After being put in foreground but also after all the layout work so there wouldn't be any delays in the startup. The other alternative is in onStart I guess. https://codereview.chromium.org/1359383002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java (right): https://codereview.chromium.org/1359383002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:34: static final long FULL_CLEANUP_DURATION = 4 * 7 * 24 * 60 * 60 * 1000; On 2015/09/24 15:20:04, Mounir Lamouri wrote: > Could you use > http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/TimeUnit.html :) Done. https://codereview.chromium.org/1359383002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:37: static final long WEBAPP_UNOPENED_CLEANUP_DURATION = 13 * 7 * 24 * 60 * 60 * 1000; On 2015/09/24 15:20:04, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1359383002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:103: if (currentTime - lastCleanup < FULL_CLEANUP_DURATION) return null; On 2015/09/24 15:20:04, Mounir Lamouri wrote: > nit: I would use parenthesis: |if ((currentTime - lastUsed) < > FULL_CLEANUP_DURATION) continue; Done. https://codereview.chromium.org/1359383002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:109: if (currentTime - lastUsed < WEBAPP_UNOPENED_CLEANUP_DURATION) continue; On 2015/09/24 15:20:04, Mounir Lamouri wrote: > ditto Done.
The CQ bit was checked by lalitm@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1359383002/#ps140001 (title: "Address Mounir's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1359383002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1359383002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/8d3b2c77b75ee7261ab91a7caf4fb2a25a3abc93 Cr-Commit-Position: refs/heads/master@{#350579} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
