Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1920893003:
🐸 Check for 0-length files in ResourceExtractor (Closed)
DescriptionCheck for 0-length files in ResourceExtractor
Also moves from an on-disk version timestamp file to SharedPreferences
(should be faster).
BUG=606413
Committed: https://crrev.com/b28607d4c72261657ea9e24ea9195bc23a472b3f
Cr-Commit-Position: refs/heads/master@{#390068}
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : rename pref #Messages
Total messages: 19 (8 generated)
agrieve@chromium.org changed reviewers: + yfriedman@chromium.org
yfriedman@chromium.org changed reviewers: + torne@chromium.org
looks like torne already has substantive comments on the bug so makes sense for him to review
Description was changed from ========== Check for 0-length files in ResourceExtractor Also moves from an on-disk version timestamp file to SharedPreferences (should be faster). BUG=606413 ========== to ========== Check for 0-length files in ResourceExtractor Also moves from an on-disk version timestamp file to SharedPreferences (should be faster). BUG=606413 ==========
agrieve@chromium.org changed reviewers: + wnwen@chromium.org
On 2016/04/26 16:01:59, Yaron wrote: > looks like torne already has substantive comments on the bug so makes sense for > him to review +wnwen as well to make sure I did the SharedPreferences alright
This looks pretty reasonable to me, and seems like it should fix several possible classes of problem. LGTM
SharedPreferences lgtm % nit https://codereview.chromium.org/1920893003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ResourceExtractor.java (right): https://codereview.chromium.org/1920893003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ResourceExtractor.java:43: private static final String APP_VERSION_PREF = "ResourceExtractorVersion"; Please include the package as well, that's the new standard for shared prefs (lots of old ones lingering around though!). See TabIdManager.PREF_NEXT_ID: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
https://codereview.chromium.org/1920893003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ResourceExtractor.java (right): https://codereview.chromium.org/1920893003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ResourceExtractor.java:43: private static final String APP_VERSION_PREF = "ResourceExtractorVersion"; On 2016/04/27 13:35:15, Peter Wen wrote: > Please include the package as well, that's the new standard for shared prefs > (lots of old ones lingering around though!). > > See TabIdManager.PREF_NEXT_ID: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... Done.
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wnwen@chromium.org, torne@chromium.org Link to the patchset: https://codereview.chromium.org/1920893003/#ps40001 (title: "rename pref")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1920893003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1920893003/40001
lgtm https://codereview.chromium.org/1920893003/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ResourceExtractor.java (right): https://codereview.chromium.org/1920893003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ResourceExtractor.java:98: beginTraceSection("checkPakTimeStamp"); update https://codereview.chromium.org/1920893003/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ResourceExtractor.java:178: // Looks for a timestamp file on disk that indicates the version of the APK that Update comment
Message was sent while issue was closed.
Description was changed from ========== Check for 0-length files in ResourceExtractor Also moves from an on-disk version timestamp file to SharedPreferences (should be faster). BUG=606413 ========== to ========== Check for 0-length files in ResourceExtractor Also moves from an on-disk version timestamp file to SharedPreferences (should be faster). BUG=606413 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Check for 0-length files in ResourceExtractor Also moves from an on-disk version timestamp file to SharedPreferences (should be faster). BUG=606413 ========== to ========== Check for 0-length files in ResourceExtractor Also moves from an on-disk version timestamp file to SharedPreferences (should be faster). BUG=606413 Committed: https://crrev.com/b28607d4c72261657ea9e24ea9195bc23a472b3f Cr-Commit-Position: refs/heads/master@{#390068} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b28607d4c72261657ea9e24ea9195bc23a472b3f Cr-Commit-Position: refs/heads/master@{#390068}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1948033004/ by qyearsley@chromium.org. The reason for reverting is: Possibly regressed memory usage on android (http://crbug.com/607598) Speculative revert to see if vm_private_dirty_final_browser values go back down for android bots on the perf waterfall. |
