Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1286973003:
webapps: introduce helper class to store extended set of data (Closed)
|
Created:
5 years, 4 months ago by Lalit Maganti Modified:
5 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionwebapps: introduce helper class to store extended set of data
Storing data in the intent is not scalable and leads to
ugly methods which accept many parameters.
Here, a new class is introduced which allows more data to
be stored about webapps without pushing them all into the
intent.
Other usecases include updating this data from the manifest
every time the webapp is opened and this is especially
useful for the introduction of the splashscreen icon as it
can be put here after the intent is created.
This is a part of a group of CLs:
(1) https://codereview.chromium.org/1286973003 (this)
(2) https://codereview.chromium.org/1310223002
(3) https://codereview.chromium.org/1239923002
BUG=508627
Committed: https://crrev.com/8d4bb0fdd64d96a3cdf0d9414781c60c51a39b70
Cr-Commit-Position: refs/heads/master@{#345970}
Patch Set 1 #Patch Set 2 : Remove unecessary patch #Patch Set 3 : Fix compile #
Total comments: 4
Patch Set 4 : Make stuff async #Patch Set 5 : Make Put async as well #Patch Set 6 : Add accessed date to storage #
Total comments: 10
Patch Set 7 : Address review comments #Patch Set 8 : Address missed comment #
Total comments: 21
Patch Set 9 : Address review comments #
Total comments: 17
Patch Set 10 : Fix review comments #Patch Set 11 : Fix minor issues #
Total comments: 15
Patch Set 12 : Fix oversights #Patch Set 13 : Fix review comments and remaining issues #Patch Set 14 : Add tests and refactor utils into ShortcutHelper #Patch Set 15 : Fix minor mistakes in comments #Patch Set 16 : Switch from Instrumentation to Unit test #
Total comments: 9
Patch Set 17 : Remove usage of AtomicBoolean #Patch Set 18 : Address comments on review #Patch Set 19 : Fix comment #Patch Set 20 : Fix assert in callback #
Total comments: 4
Patch Set 21 : Fix nit in previous patch #Patch Set 22 : Make findbugs happy #Patch Set 23 : Fix minor issue #
Messages
Total messages: 51 (9 generated)
lalitm@google.com changed reviewers: + dfalcantara@chromium.org, mlamouri@chromium.org
Just a quick sketch up of an idea to resolve stuffing all webapp data into an intent. Would appreciate feedback on whether the approach taken is along the right lines :)
Huh, very clever. I was concerned about having one large SharedPreferences file for all the web apps (the entire file is cached and stored on startup), but having each web app have its own SharedPreferences is probably alright performance wise. This doesn't, however, account for the problem of SharedPreferences that are kept around for web apps that we haven't seen in forever. Do you have a solution for that too? https://chromiumcodereview.appspot.com/1286973003/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://chromiumcodereview.appspot.com/1286973003/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:48: .commit(); you probably want to use apply() instead. commit() is synchronous.
On 2015/08/12 22:32:32, dfalcantara wrote: > Huh, very clever. I was concerned about having one large SharedPreferences file > for all the web apps (the entire file is cached and stored on startup), but > having each web app have its own SharedPreferences is probably alright > performance wise. This doesn't, however, account for the problem of > SharedPreferences that are kept around for web apps that we haven't seen in > forever. Do you have a solution for that too? > > https://chromiumcodereview.appspot.com/1286973003/diff/40001/chrome/android/j... > File > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java > (right): > > https://chromiumcodereview.appspot.com/1286973003/diff/40001/chrome/android/j... > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:48: > .commit(); > you probably want to use apply() instead. commit() is synchronous. So Mounir and I talked briefly about this and our idea is to have som sort of scheduled cleanup of webapp data every once in a while. Whether the webapp data would be deleted or not would depend on the time since the user last opened the webapp - i.e. the idea is to store the time the webapp was opened inside the shared preference file and update it every time the user opens it. I hope to have a sketch of this idea by end of today.
https://codereview.chromium.org/1286973003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1286973003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:32: public Bitmap getSplashIcon() { Is that call synchronous? Is that going to slow down the startup?
https://codereview.chromium.org/1286973003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1286973003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:32: public Bitmap getSplashIcon() { On 2015/08/13 11:21:01, Mounir Lamouri wrote: > Is that call synchronous? Is that going to slow down the startup? Yes and yes. I've made it async and introduced callbacks. https://codereview.chromium.org/1286973003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:48: .commit(); On 2015/08/12 22:32:32, dfalcantara wrote: > you probably want to use apply() instead. commit() is synchronous. Done.
Dan: could you please take a look at this patch before I add the registry code to it?
https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:94: return fetchSharedPreference(); Is there any reason why you set these AsyncTasks this way (with abstract methods)? Instead of abstracting away the AsyncTasks with new classes that don't do much, you should just use AsyncTask directly and put the code into doInBackground().
https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:94: return fetchSharedPreference(); On 2015/08/24 20:22:46, dfalcantara wrote: > Is there any reason why you set these AsyncTasks this way (with abstract > methods)? > > Instead of abstracting away the AsyncTasks with new classes that don't do much, > you should just use AsyncTask directly and put the code into doInBackground(). Right now this doesn't make any sense to add this indirection but with the eventual aim of making this class hold all data for webapps, it makes much more sense to extract as much code (especially the callback code in FetchTask) as possible.
https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:94: return fetchSharedPreference(); On 2015/08/24 20:34:23, Lalit Maganti wrote: > On 2015/08/24 20:22:46, dfalcantara wrote: > > Is there any reason why you set these AsyncTasks this way (with abstract > > methods)? > > > > Instead of abstracting away the AsyncTasks with new classes that don't do > much, > > you should just use AsyncTask directly and put the code into doInBackground(). > > Right now this doesn't make any sense to add this indirection but with the > eventual aim of making this class hold all data for webapps, it makes much more > sense to extract as much code (especially the callback code in FetchTask) as > possible. I'd suggest dealing with abstracting it away when you actually have other data to deal with. Right now it just makes the code look more complicated than it really is. PutTask (in its current state) just adds another layer between the task and doInBackground() since it doesn't even have a callback to worry about.
https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:94: return fetchSharedPreference(); On 2015/08/24 20:37:33, dfalcantara wrote: > On 2015/08/24 20:34:23, Lalit Maganti wrote: > > On 2015/08/24 20:22:46, dfalcantara wrote: > > > Is there any reason why you set these AsyncTasks this way (with abstract > > > methods)? > > > > > > Instead of abstracting away the AsyncTasks with new classes that don't do > > much, > > > you should just use AsyncTask directly and put the code into > doInBackground(). > > > > Right now this doesn't make any sense to add this indirection but with the > > eventual aim of making this class hold all data for webapps, it makes much > more > > sense to extract as much code (especially the callback code in FetchTask) as > > possible. > > I'd suggest dealing with abstracting it away when you actually have other data > to deal with. Right now it just makes the code look more complicated than it > really is. PutTask (in its current state) just adds another layer between the > task and doInBackground() since it doesn't even have a callback to worry about. The eternal struggle between abstraction and complexity :P OK for Put I agree there isn't much benefit. I personally feel that it's a good idea to keep Fetch because I'm almost certain it will be used and the usecase for it. If you feel otherwise though I will remove.
https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:94: return fetchSharedPreference(); On 2015/08/24 20:43:20, Lalit Maganti wrote: > On 2015/08/24 20:37:33, dfalcantara wrote: > > On 2015/08/24 20:34:23, Lalit Maganti wrote: > > > On 2015/08/24 20:22:46, dfalcantara wrote: > > > > Is there any reason why you set these AsyncTasks this way (with abstract > > > > methods)? > > > > > > > > Instead of abstracting away the AsyncTasks with new classes that don't do > > > much, > > > > you should just use AsyncTask directly and put the code into > > doInBackground(). > > > > > > Right now this doesn't make any sense to add this indirection but with the > > > eventual aim of making this class hold all data for webapps, it makes much > > more > > > sense to extract as much code (especially the callback code in FetchTask) as > > > possible. > > > > I'd suggest dealing with abstracting it away when you actually have other data > > to deal with. Right now it just makes the code look more complicated than it > > really is. PutTask (in its current state) just adds another layer between the > > task and doInBackground() since it doesn't even have a callback to worry > about. > > The eternal struggle between abstraction and complexity :P > > OK for Put I agree there isn't much benefit. I personally feel that it's a good > idea to keep Fetch because I'm almost certain it will be used and the usecase > for it. If you feel otherwise though I will remove. Do you have a concrete second use case for abstracting FetchTask? If your second use case is also an icon I'd suggest changing FetchTask to FetchIconTask, which takes in the preference name -- the logic for dealing with empty encoded bitmap strings will likely be shared. If you're going to make the FetchTask retrieve generic strings, I imagine you'd want a different class for that, anyway, since the AsyncTask signatures would be different.
https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:94: return fetchSharedPreference(); On 2015/08/24 20:57:36, dfalcantara wrote: > On 2015/08/24 20:43:20, Lalit Maganti wrote: > > On 2015/08/24 20:37:33, dfalcantara wrote: > > > On 2015/08/24 20:34:23, Lalit Maganti wrote: > > > > On 2015/08/24 20:22:46, dfalcantara wrote: > > > > > Is there any reason why you set these AsyncTasks this way (with abstract > > > > > methods)? > > > > > > > > > > Instead of abstracting away the AsyncTasks with new classes that don't > do > > > > much, > > > > > you should just use AsyncTask directly and put the code into > > > doInBackground(). > > > > > > > > Right now this doesn't make any sense to add this indirection but with the > > > > eventual aim of making this class hold all data for webapps, it makes much > > > more > > > > sense to extract as much code (especially the callback code in FetchTask) > as > > > > possible. > > > > > > I'd suggest dealing with abstracting it away when you actually have other > data > > > to deal with. Right now it just makes the code look more complicated than > it > > > really is. PutTask (in its current state) just adds another layer between > the > > > task and doInBackground() since it doesn't even have a callback to worry > > about. > > > > The eternal struggle between abstraction and complexity :P > > > > OK for Put I agree there isn't much benefit. I personally feel that it's a > good > > idea to keep Fetch because I'm almost certain it will be used and the usecase > > for it. If you feel otherwise though I will remove. > > Do you have a concrete second use case for abstracting FetchTask? If your > second use case is also an icon I'd suggest changing FetchTask to FetchIconTask, > which takes in the preference name -- the logic for dealing with empty encoded > bitmap strings will likely be shared. If you're going to make the FetchTask > retrieve generic strings, I imagine you'd want a different class for that, > anyway, since the AsyncTask signatures would be different. I'd imagine we'd use this for name/short name as well as the recents icon yes. The signature difference is accounted for in the generic T.
https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:38: String icon = mPreferences.getString("splash_icon", ""); Pull these keys into private static final Strings. https://chromiumcodereview.appspot.com/1286973003/diff/100001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:94: return fetchSharedPreference(); On 2015/08/24 21:00:16, Lalit Maganti wrote: > On 2015/08/24 20:57:36, dfalcantara wrote: > > On 2015/08/24 20:43:20, Lalit Maganti wrote: > > > On 2015/08/24 20:37:33, dfalcantara wrote: > > > > On 2015/08/24 20:34:23, Lalit Maganti wrote: > > > > > On 2015/08/24 20:22:46, dfalcantara wrote: > > > > > > Is there any reason why you set these AsyncTasks this way (with > abstract > > > > > > methods)? > > > > > > > > > > > > Instead of abstracting away the AsyncTasks with new classes that don't > > do > > > > > much, > > > > > > you should just use AsyncTask directly and put the code into > > > > doInBackground(). > > > > > > > > > > Right now this doesn't make any sense to add this indirection but with > the > > > > > eventual aim of making this class hold all data for webapps, it makes > much > > > > more > > > > > sense to extract as much code (especially the callback code in > FetchTask) > > as > > > > > possible. > > > > > > > > I'd suggest dealing with abstracting it away when you actually have other > > data > > > > to deal with. Right now it just makes the code look more complicated than > > it > > > > really is. PutTask (in its current state) just adds another layer between > > the > > > > task and doInBackground() since it doesn't even have a callback to worry > > > about. > > > > > > The eternal struggle between abstraction and complexity :P > > > > > > OK for Put I agree there isn't much benefit. I personally feel that it's a > > good > > > idea to keep Fetch because I'm almost certain it will be used and the > usecase > > > for it. If you feel otherwise though I will remove. > > > > Do you have a concrete second use case for abstracting FetchTask? If your > > second use case is also an icon I'd suggest changing FetchTask to > FetchIconTask, > > which takes in the preference name -- the logic for dealing with empty encoded > > bitmap strings will likely be shared. If you're going to make the FetchTask > > retrieve generic strings, I imagine you'd want a different class for that, > > anyway, since the AsyncTask signatures would be different. > > I'd imagine we'd use this for name/short name as well as the recents icon yes. > The signature difference is accounted for in the generic T. I'd still suggest separating the image and string fetches out into different AsyncTask types. The icon saving/loading and encoding/decoding logic will have to be shared eventually; they may as well be done in the AsyncTask subclass itself rather than having effectively duplicated fetchSharedPreference() calls. I guess an alternative is to have a generic WebappDataStorage.fetchBitmap() call that takes in a generic preference name and then abstracts away the decoding, but that'd still be more complicated than just putting it into the AsyncTask itself.
https://codereview.chromium.org/1286973003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1286973003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:38: String icon = mPreferences.getString("splash_icon", ""); On 2015/08/24 21:15:16, dfalcantara wrote: > Pull these keys into private static final Strings. Done. https://codereview.chromium.org/1286973003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:94: return fetchSharedPreference(); On 2015/08/24 21:15:16, dfalcantara wrote: > On 2015/08/24 21:00:16, Lalit Maganti wrote: > > On 2015/08/24 20:57:36, dfalcantara wrote: > > > On 2015/08/24 20:43:20, Lalit Maganti wrote: > > > > On 2015/08/24 20:37:33, dfalcantara wrote: > > > > > On 2015/08/24 20:34:23, Lalit Maganti wrote: > > > > > > On 2015/08/24 20:22:46, dfalcantara wrote: > > > > > > > Is there any reason why you set these AsyncTasks this way (with > > abstract > > > > > > > methods)? > > > > > > > > > > > > > > Instead of abstracting away the AsyncTasks with new classes that > don't > > > do > > > > > > much, > > > > > > > you should just use AsyncTask directly and put the code into > > > > > doInBackground(). > > > > > > > > > > > > Right now this doesn't make any sense to add this indirection but with > > the > > > > > > eventual aim of making this class hold all data for webapps, it makes > > much > > > > > more > > > > > > sense to extract as much code (especially the callback code in > > FetchTask) > > > as > > > > > > possible. > > > > > > > > > > I'd suggest dealing with abstracting it away when you actually have > other > > > data > > > > > to deal with. Right now it just makes the code look more complicated > than > > > it > > > > > really is. PutTask (in its current state) just adds another layer > between > > > the > > > > > task and doInBackground() since it doesn't even have a callback to worry > > > > about. > > > > > > > > The eternal struggle between abstraction and complexity :P > > > > > > > > OK for Put I agree there isn't much benefit. I personally feel that it's a > > > good > > > > idea to keep Fetch because I'm almost certain it will be used and the > > usecase > > > > for it. If you feel otherwise though I will remove. > > > > > > Do you have a concrete second use case for abstracting FetchTask? If your > > > second use case is also an icon I'd suggest changing FetchTask to > > FetchIconTask, > > > which takes in the preference name -- the logic for dealing with empty > encoded > > > bitmap strings will likely be shared. If you're going to make the FetchTask > > > retrieve generic strings, I imagine you'd want a different class for that, > > > anyway, since the AsyncTask signatures would be different. > > > > I'd imagine we'd use this for name/short name as well as the recents icon yes. > > The signature difference is accounted for in the generic T. > > I'd still suggest separating the image and string fetches out into different > AsyncTask types. The icon saving/loading and encoding/decoding logic will have > to be shared eventually; they may as well be done in the AsyncTask subclass > itself rather than having effectively duplicated fetchSharedPreference() calls. > I guess an alternative is to have a generic WebappDataStorage.fetchBitmap() call > that takes in a generic preference name and then abstracts away the decoding, > but that'd still be more complicated than just putting it into the AsyncTask > itself. OK I've split off the icon saving/loading logic into their own classes. For now, the benefit of doing the same for the string classes is negligible so I'd leave that till when the code for new values are added.
https://chromiumcodereview.appspot.com/1286973003/diff/140001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://chromiumcodereview.appspot.com/1286973003/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:29: Context.MODE_PRIVATE); nit: indentation is off. https://chromiumcodereview.appspot.com/1286973003/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:33: public static WebappDataStorage open(Context context, String webappId) { nit: all public methods require javadocs. https://chromiumcodereview.appspot.com/1286973003/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:43: return this; If you're returning "this" in order to make a builder pattern of WebappDataStorage.putSplashIcon().putOtherThing().putThirdThing(), I'd strongly advise against it. Pass in everything you need to save in one big function so only one AsyncTask and one SharedPreference write is incurred. https://chromiumcodereview.appspot.com/1286973003/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:66: .commit(); Didn't we suggest using apply() here? You said you addressed it in PS4. This is making me doubly-paranoid about checking all of the other comments I'd left on your other CLs, since I know it's happened before. https://chromiumcodereview.appspot.com/1286973003/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:76: public interface Callback<T> { nit: Either call this a FetchCallback (Callback is too generic) or make it a member of the BitmapFetchTask. https://chromiumcodereview.appspot.com/1286973003/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:93: String icon = mPreferences.getString(mKey, ""); use null instead of "" https://chromiumcodereview.appspot.com/1286973003/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:109: nit: remove the newline between the fields; you're not describing what they're for anyway. https://chromiumcodereview.appspot.com/1286973003/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:125: .commit(); commit() again.
Dan: please see my inline comments. I've responded to your feedback as well as some comments of my own about why I've changed stuff. https://codereview.chromium.org/1286973003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1286973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:29: Context.MODE_PRIVATE); On 2015/08/26 01:28:37, dfalcantara wrote: > nit: indentation is off. Done. https://codereview.chromium.org/1286973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:30: updateLastAccessedDate(); Mounir and Dan: I've modified the lastAccessedDate flow because when we try to cleanup eventually, we'll modify the last accessed date when we try and read it. I've also renamed it to lastAccessedTime since this better reflects its purpose. https://codereview.chromium.org/1286973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:33: public static WebappDataStorage open(Context context, String webappId) { On 2015/08/26 01:28:37, dfalcantara wrote: > nit: all public methods require javadocs. Done. https://codereview.chromium.org/1286973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:43: return this; On 2015/08/26 01:28:37, dfalcantara wrote: > If you're returning "this" in order to make a builder pattern of > WebappDataStorage.putSplashIcon().putOtherThing().putThirdThing(), I'd strongly > advise against it. Pass in everything you need to save in one big function so > only one AsyncTask and one SharedPreference write is incurred. Yes my intention was indeed to make a builder pattern. Refactored to do as you suggest (The BitmapPutTask no longer makes sense if we're going to have one big AsyncTask so I've got rid of it and inlined the AsyncTask.. https://codereview.chromium.org/1286973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:46: public void getLastAccessedDate(final Callback<String> callback) { See above for why I changed this. https://codereview.chromium.org/1286973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:50: return mPreferences.getString(KEY_LAST_ACCESSED, ""); Changed this to null as well to be consistent. https://codereview.chromium.org/1286973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:60: public WebappDataStorage updateLastAccessedDate() { See above for why I've changed this. https://codereview.chromium.org/1286973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:66: .commit(); On 2015/08/26 01:28:37, dfalcantara wrote: > Didn't we suggest using apply() here? You said you addressed it in PS4. This > is making me doubly-paranoid about checking all of the other comments I'd left > on your other CLs, since I know it's happened before. Sorry this was actually intentional. I switched it back to commit because AsyncTask is already running in the background so there is no reason to further delay the commit. I should have noted it down in a comment when I changed it. Sorry again. https://codereview.chromium.org/1286973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:76: public interface Callback<T> { On 2015/08/26 01:28:38, dfalcantara wrote: > nit: Either call this a FetchCallback (Callback is too generic) or make it a > member of the BitmapFetchTask. Done. https://codereview.chromium.org/1286973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:93: String icon = mPreferences.getString(mKey, ""); On 2015/08/26 01:28:37, dfalcantara wrote: > use null instead of "" Done. https://codereview.chromium.org/1286973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:109: On 2015/08/26 01:28:37, dfalcantara wrote: > nit: remove the newline between the fields; you're not describing what they're > for anyway. Removed the key field anyway to reflect your suggestion to make put as one function. https://codereview.chromium.org/1286973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:125: .commit(); On 2015/08/26 01:28:37, dfalcantara wrote: > commit() again. See above.
https://codereview.chromium.org/1286973003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1286973003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:43: return this; On 2015/08/26 09:45:44, Lalit Maganti wrote: > On 2015/08/26 01:28:37, dfalcantara wrote: > > If you're returning "this" in order to make a builder pattern of > > WebappDataStorage.putSplashIcon().putOtherThing().putThirdThing(), I'd > strongly > > advise against it. Pass in everything you need to save in one big function so > > only one AsyncTask and one SharedPreference write is incurred. > > Yes my intention was indeed to make a builder pattern. > > Refactored to do as you suggest (The BitmapPutTask no longer makes sense if > we're going to have one big AsyncTask so I've got rid of it and inlined the > AsyncTask.. OK scratch that - I haven't inlined the AsyncTask but I have refactored it to simply take data - it just uses the static variables for keys now.
Could you have tests for that? https://codereview.chromium.org/1286973003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1286973003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:23: private static final String KEY_LAST_ACCESSED = "last_accessed"; nit: last_used. "used" is a more common term. For example LRU/MRU. https://codereview.chromium.org/1286973003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:27: private WebappDataStorage(Context context, String webappId) { style: unless the coding style says otherwise, I would move the private methods after the public ones.
https://chromiumcodereview.appspot.com/1286973003/diff/160001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://chromiumcodereview.appspot.com/1286973003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:34: * Implementation detail: This WebappDataStorage object returned utilizes nits: (1) Don't bother saying "Implementation detail:". Callers of the function won't care and it's unnecessary verbosity. (2) The comment about using SharedPreference affects everything about the class, so move it up to the class comment. https://chromiumcodereview.appspot.com/1286973003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:48: * the open function) asynchrously. nit: Be explicit, especially since this is a comment and they quickly drift away from the function names during a refactoring. the open function -> {@ref WebappDataStorage#open(Context, String)} https://chromiumcodereview.appspot.com/1286973003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:59: * Retrieve the splashcreen image associated with with the current webapp nit: If it's not too late, we should standardize on "splash screen". That's the correct term, and we've already got too many other screwed up single-word names in these classes ("webapps", "homescreen"). This should be handled before it becomes more onerous to fix later. https://chromiumcodereview.appspot.com/1286973003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:71: * @param splashscreenImage The image which should be shown on the splashcreen of the webapp. nit: splashcreen? https://chromiumcodereview.appspot.com/1286973003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:73: public void update(final Bitmap splashscreenImage) { update() is vague. Use updateSplashscreenImage() or updateSplashScreenImage() since there's only one thing. Change the function name when you actually have a need to add more data instead of just a vague idea of future class usage.
https://chromiumcodereview.appspot.com/1286973003/diff/160001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://chromiumcodereview.appspot.com/1286973003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:48: * the open function) asynchrously. On 2015/08/26 21:03:09, dfalcantara wrote: > nit: > Be explicit, especially since this is a comment and they quickly drift away from > the function names during a refactoring. > > the open function -> {@ref WebappDataStorage#open(Context, String)} er @link
https://codereview.chromium.org/1286973003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1286973003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:23: private static final String KEY_LAST_ACCESSED = "last_accessed"; On 2015/08/26 at 13:41:04, Mounir Lamouri wrote: > nit: last_used. "used" is a more common term. For example LRU/MRU. Done. https://codereview.chromium.org/1286973003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:27: private WebappDataStorage(Context context, String webappId) { On 2015/08/26 at 13:41:04, Mounir Lamouri wrote: > style: unless the coding style says otherwise, I would move the private methods after the public ones. Moved to be after the public static methods similar to WebappInfo. https://codereview.chromium.org/1286973003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:34: * Implementation detail: This WebappDataStorage object returned utilizes On 2015/08/26 at 21:03:09, dfalcantara wrote: > nits: > > (1) Don't bother saying "Implementation detail:". Callers of the function won't care and it's unnecessary verbosity. > > (2) The comment about using SharedPreference affects everything about the class, so move it up to the class comment. (1) Ack. (2) Done. https://codereview.chromium.org/1286973003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:48: * the open function) asynchrously. On 2015/08/26 at 21:04:57, dfalcantara wrote: > On 2015/08/26 21:03:09, dfalcantara wrote: > > nit: > > Be explicit, especially since this is a comment and they quickly drift away from > > the function names during a refactoring. > > > > the open function -> {@ref WebappDataStorage#open(Context, String)} > > er @link Done. https://codereview.chromium.org/1286973003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:59: * Retrieve the splashcreen image associated with with the current webapp On 2015/08/26 at 21:03:09, dfalcantara wrote: > nit: If it's not too late, we should standardize on "splash screen". That's the correct term, and we've already got too many other screwed up single-word names in these classes ("webapps", "homescreen"). This should be handled before it becomes more onerous to fix later. Changed but see comment below about camel casing. https://codereview.chromium.org/1286973003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:71: * @param splashscreenImage The image which should be shown on the splashcreen of the webapp. On 2015/08/26 at 21:03:09, dfalcantara wrote: > nit: splashcreen? Fixed. https://codereview.chromium.org/1286973003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:73: public void update(final Bitmap splashscreenImage) { On 2015/08/26 at 21:03:09, dfalcantara wrote: > update() is vague. Use updateSplashscreenImage() or updateSplashScreenImage() since there's only one thing. Change the function name when you actually have a need to add more data instead of just a vague idea of future class usage. Changed to updateSplashscreenImage. However, if we are switching to "splash screen" the second way makes more sense but would be inconsistent with the rest of the file. Do you want me to change the camel casing everywhere?
https://codereview.chromium.org/1286973003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1286973003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:73: public void update(final Bitmap splashscreenImage) { On 2015/08/26 21:20:41, Lalit Maganti wrote: > On 2015/08/26 at 21:03:09, dfalcantara wrote: > > update() is vague. Use updateSplashscreenImage() or updateSplashScreenImage() > since there's only one thing. Change the function name when you actually have a > need to add more data instead of just a vague idea of future class usage. > > Changed to updateSplashscreenImage. However, if we are switching to "splash > screen" the second way makes more sense but would be inconsistent with the rest > of the file. Do you want me to change the camel casing everywhere? Ideally, yes. That's one reason why we got stuck calling "web apps" "webapps" -- we used Webapp instead of WebApp for everything. It's also the same reason why the code has "infobar" and "InfoBar" all over the place. That can be done as a cleanup CL to avoid bloating this CL up more, but it should be done sooner rather than later. https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:41: * Retrieve the time which this WebappDataStorage was last opened using nit: Asynchronously retrieves the time which this WebappDataStorage was last opened using {@link WebappDataStorage#open(Context, String)}. https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:58: * Retrieve the splash screen image associated with with the current webapp nit: Asynchronously retrieves the splash screen image associated with the current webapp. https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:69: * data. nit: wrap at 100. https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:95: .putLong(KEY_LAST_USED, System.currentTimeMillis()) nit: indent by 8. https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:104: * Callback class used when items are requested from the storage. nit: Called after data has been retrieved from storage. https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:107: public void run(T readObject); run() -> onDataRetrieved(). https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:113: nit: remove newline?
I forget: did we agree on having an explicit registry or iterate over shared prefs files? And if we are iterating should the registry form a part of this patch or a dependent one? https://codereview.chromium.org/1286973003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1286973003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:73: public void update(final Bitmap splashscreenImage) { On 2015/08/26 at 21:56:14, dfalcantara wrote: > On 2015/08/26 21:20:41, Lalit Maganti wrote: > > On 2015/08/26 at 21:03:09, dfalcantara wrote: > > > update() is vague. Use updateSplashscreenImage() or updateSplashScreenImage() > > since there's only one thing. Change the function name when you actually have a > > need to add more data instead of just a vague idea of future class usage. > > > > Changed to updateSplashscreenImage. However, if we are switching to "splash > > screen" the second way makes more sense but would be inconsistent with the rest > > of the file. Do you want me to change the camel casing everywhere? > > Ideally, yes. That's one reason why we got stuck calling "web apps" "webapps" -- we used Webapp instead of WebApp for everything. It's also the same reason why the code has "infobar" and "InfoBar" all over the place. That can be done as a cleanup CL to avoid bloating this CL up more, but it should be done sooner rather than later. It's OK as this is the first patch that actually uses "splashscreen". I've updated this one and will update the dependent ones as well to match. https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:41: * Retrieve the time which this WebappDataStorage was last opened using On 2015/08/26 at 21:56:14, dfalcantara wrote: > nit: Asynchronously retrieves the time which this WebappDataStorage was last opened using {@link WebappDataStorage#open(Context, String)}. Done. https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:58: * Retrieve the splash screen image associated with with the current webapp On 2015/08/26 at 21:56:14, dfalcantara wrote: > nit: Asynchronously retrieves the splash screen image associated with the current webapp. Done. https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:69: * data. On 2015/08/26 at 21:56:14, dfalcantara wrote: > nit: wrap at 100. Done. https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:95: .putLong(KEY_LAST_USED, System.currentTimeMillis()) On 2015/08/26 at 21:56:14, dfalcantara wrote: > nit: indent by 8. Done. https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:104: * Callback class used when items are requested from the storage. On 2015/08/26 at 21:56:14, dfalcantara wrote: > nit: Called after data has been retrieved from storage. Done. https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:107: public void run(T readObject); On 2015/08/26 at 21:56:14, dfalcantara wrote: > run() -> onDataRetrieved(). Done. https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:113: On 2015/08/26 at 21:56:14, dfalcantara wrote: > nit: remove newline? Done. https://codereview.chromium.org/1286973003/diff/180002/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:151: .putString(KEY_SPLASH_ICON, splashImageString) Also indented at 8 to match above.
Scratch "And if we are iterating should the registry form a part of this patch or a dependent one?" and replace with "And if we are using a registry should it form part of this patch or a dependent one?"
I think we settled on using a registry, which can come after this CL. But yeah, add some unit tests. It's silly that I was about to l-g-t-m it when a main retrieval function wasn't doing what it was actually supposed to be doing.
Decided to test it with an instrumentation test as there is a heavy reliance on SharedPreferences and AdvancedMockContext makes testing that quite easy. I've also introduced a little bit of refactoring into this CL to reduce code duplication which would have been required.
Can you check whether the test can be run using roboelectrik?
On 2015/08/27 at 14:09:47, mlamouri wrote: > Can you check whether the test can be run using roboelectrik? Your wish is my command :P It's now a unit test but there is a Roboelectric bug which has led to an interesting hack. I may submit a patch to Roboelectric to fix the issue. Not sure how tough it'll be though.
lgtm. Thanks for using a roboelectric test! :) https://codereview.chromium.org/1286973003/diff/290001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1286973003/diff/290001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:16: * to persist the data to disk. nit: I would add a comment explaining that every time .open() is called, the LAST_USED time is update. It can be checked without updating it using .getLastUsedTime() https://codereview.chromium.org/1286973003/diff/290001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:83: callback.onDataRetrieved(result == -1L ? null : result); -1 shouldn't happen, right? Should that assert? https://codereview.chromium.org/1286973003/diff/290001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:120: System.out.println(mPreferences.getString(mKey, null)); nit: remove? https://codereview.chromium.org/1286973003/diff/290001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java (right): https://codereview.chromium.org/1286973003/diff/290001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:87: System.out.println(ShortcutHelper.encodeBitmapAsString(expected)); nit: remove
https://codereview.chromium.org/1286973003/diff/290001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1286973003/diff/290001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:83: callback.onDataRetrieved(result == -1L ? null : result); On 2015/08/27 at 16:18:23, Mounir Lamouri wrote: > -1 shouldn't happen, right? Should that assert? -1 will be returned if there is no last used time. Although extremely unlikely that getLastUsedTime would be called without a webapp existing it could theoretically happen. https://codereview.chromium.org/1286973003/diff/290001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:120: System.out.println(mPreferences.getString(mKey, null)); On 2015/08/27 at 16:18:23, Mounir Lamouri wrote: > nit: remove? Whoops missed removing this one. Done. https://codereview.chromium.org/1286973003/diff/290001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java (right): https://codereview.chromium.org/1286973003/diff/290001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:87: System.out.println(ShortcutHelper.encodeBitmapAsString(expected)); On 2015/08/27 at 16:18:23, Mounir Lamouri wrote: > nit: remove And this :/ Done.
https://codereview.chromium.org/1286973003/diff/290001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1286973003/diff/290001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:83: callback.onDataRetrieved(result == -1L ? null : result); On 2015/08/27 at 16:24:34, Lalit Maganti wrote: > On 2015/08/27 at 16:18:23, Mounir Lamouri wrote: > > -1 shouldn't happen, right? Should that assert? > > -1 will be returned if there is no last used time. Although extremely unlikely that getLastUsedTime would be called without a webapp existing it could theoretically happen. That's the point. Can you add an assert?
https://codereview.chromium.org/1286973003/diff/290001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java (right): https://codereview.chromium.org/1286973003/diff/290001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:83: callback.onDataRetrieved(result == -1L ? null : result); On 2015/08/27 at 16:29:30, Mounir Lamouri wrote: > On 2015/08/27 at 16:24:34, Lalit Maganti wrote: > > On 2015/08/27 at 16:18:23, Mounir Lamouri wrote: > > > -1 shouldn't happen, right? Should that assert? > > > > -1 will be returned if there is no last used time. Although extremely unlikely that getLastUsedTime would be called without a webapp existing it could theoretically happen. > > That's the point. Can you add an assert? OK done.
lgtm mostly. https://codereview.chromium.org/1286973003/diff/370001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/1286973003/diff/370001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:124: String encodedIcon = icon == null ? "" : encodeBitmapAsString(icon); nit: This icon == null check should be at the beginning of encodeBitmapAsString for the same reason that decodeBitmapFromString checks for an empty string. https://codereview.chromium.org/1286973003/diff/370001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java (right): https://codereview.chromium.org/1286973003/diff/370001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:51: public void testLastUsedRetrieval() throws InterruptedException { You should test for the default unset case, too.
lgtm again https://chromiumcodereview.appspot.com/1286973003/diff/370001/chrome/android/... File chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java (right): https://chromiumcodereview.appspot.com/1286973003/diff/370001/chrome/android/... chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:51: public void testLastUsedRetrieval() throws InterruptedException { On 2015/08/27 18:24:20, dfalcantara wrote: > You should test for the default unset case, too. Apparently we assert on unset variables now, so that's cool.
https://codereview.chromium.org/1286973003/diff/370001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/1286973003/diff/370001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:124: String encodedIcon = icon == null ? "" : encodeBitmapAsString(icon); On 2015/08/27 at 18:24:20, dfalcantara wrote: > nit: This icon == null check should be at the beginning of encodeBitmapAsString for the same reason that decodeBitmapFromString checks for an empty string. Done.
The CQ bit was checked by lalitm@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/1286973003/#ps390001 (title: "Fix nit in previous patch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1286973003/390001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1286973003/390001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
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/1286973003/#ps280005 (title: "Make findbugs happy")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1286973003/280005 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1286973003/280005
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
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/1286973003/#ps410001 (title: "Fix minor issue")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1286973003/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1286973003/410001
Message was sent while issue was closed.
Committed patchset #23 (id:410001)
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/8d4bb0fdd64d96a3cdf0d9414781c60c51a39b70 Cr-Commit-Position: refs/heads/master@{#345970} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
