Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2258113003:
[Android] Fix Chrome widget no display issue on some devices (Closed)
DescriptionFix the problems that on some models of phones such as Sony Xperia Z3 & Z5, Chrome widget blocks in showing the bookmark information.
It comes with two problems and both happens when there are thousands of
bookmark to display:
1. If Chrome is not initialized, GC might recycle the BookmarkLoader
before BookmarkModel is loaded which blocks retrieving bookmark info.
2. Even Chrome is already intialized, fetching meta data is not always
guaranteed to wait until onDataChanged is finished(even though Android doc
claims it is). So need to manually check if bookmark info is loaded and
keep refreshing if not.
BUG=621643
Committed: https://crrev.com/24462918aee20854070ea38434c05d1da845884e
Cr-Commit-Position: refs/heads/master@{#415684}
Patch Set 1 #
Total comments: 10
Patch Set 2 : update based on Ian's comments and add thread annotations to fix lint error #
Total comments: 4
Patch Set 3 : update based on Ian's comments #Patch Set 4 : update in case mPreferences.getString returns null and fires and NullPointerException error #
Total comments: 2
Patch Set 5 : update based on Ian's comment #Messages
Total messages: 24 (9 generated)
The CQ bit was checked by ltian@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
ltian@chromium.org changed reviewers: + ianwen@chromium.org, tedchoc@chromium.org
First thing first. You should shorten your CL title. How about "[Android] Fix Chrome widget blank screen issue on some devices"?
On 2016/08/22 18:24:40, Ian Wen wrote: > First thing first. You should shorten your CL title. How about "[Android] Fix > Chrome widget blank screen issue on some devices"? It might still have 2-3 seconds showing blank page when loading BookmarkModel and favicons. Is "[Android] Fix Chrome widget no display issue on some devices" better?
https://codereview.chromium.org/2258113003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java (right): https://codereview.chromium.org/2258113003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java:170: // TODO(ltian): Auto-generated constructor stub This constructor is not needed. The default java one will just work. https://codereview.chromium.org/2258113003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java:383: //Separate BookmarkLoader initialization from construction to maintain a reference Thanks for the comments! I think it should be a bit more concise: "A reference of BookmarkLoader on binder thread is needed to prevent the object from being garbage collected on Ui thread." No need to mention Sony here, as this is a legit behavior in Java and it might happen on any devices with memory pressure. https://codereview.chromium.org/2258113003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java:435: //On some models (Sony Xperia Z3 & Z5), fetching meta data (getCount, getLoadingView) I would rephrase it to be: On some sony devices, getCount() might be triggered even before onDatasetChanged() returns. If this happens, refresh the widget until the bookmark is loaded. https://codereview.chromium.org/2258113003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java:439: refreshWidget(); I am curious that if loading mCurrentFolder takes a long time, how many times will we call refreshWidget(). Is it going to be a spinlock for the binder thread? If so it would be a very expensive and we shouldn't do this. I'm thinking maybe we should manually set a handler, and post refreshWidget() in every 500ms? https://en.wikipedia.org/wiki/Spinlock https://codereview.chromium.org/2258113003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java:441: } else if (!mPreferences.getString(PREF_CURRENT_FOLDER, null) This will actually crash if there is no PREF_CURRENT_FOLDER set in preferences, because you default the value to be null. Also you are comparing a BookmarkId object with BookmarkId#getId()#ToString(), which is wrong as well. Try BookmarkId#getBookmarkIdFromString(), and then directly compare the two bookmark id objects.
On 2016/08/22 18:30:52, ltian wrote: > On 2016/08/22 18:24:40, Ian Wen wrote: > > First thing first. You should shorten your CL title. How about "[Android] Fix > > Chrome widget blank screen issue on some devices"? > > It might still have 2-3 seconds showing blank page when loading BookmarkModel > and favicons. Is "[Android] Fix Chrome widget no display issue on some devices" > better? Sure. This looks okay as well.
Description was changed from ========== Fix the problems that on some models of phones such as Sony Xperia Z3 & Z5, Chrome widget blocks in showing the bookmark information. It comes with two problems and both happens when there are thousands of bookmark to display: 1. If Chrome is not initialized, GC might recycle the BookmarkLoader before BookmarkModel is loaded which blocks retrieving bookmark info. 2. Even Chrome is already intialized, fetching meta data is not always guaranteed to wait until onDataChanged is finished(even though Android doc claims it is). So need to manually check if bookmark info is loaded and keep refreshing if not. BUG=621643 ========== to ========== Fix the problems that on some models of phones such as Sony Xperia Z3 & Z5, Chrome widget blocks in showing the bookmark information. It comes with two problems and both happens when there are thousands of bookmark to display: 1. If Chrome is not initialized, GC might recycle the BookmarkLoader before BookmarkModel is loaded which blocks retrieving bookmark info. 2. Even Chrome is already intialized, fetching meta data is not always guaranteed to wait until onDataChanged is finished(even though Android doc claims it is). So need to manually check if bookmark info is loaded and keep refreshing if not. BUG=621643 ==========
https://codereview.chromium.org/2258113003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java (right): https://codereview.chromium.org/2258113003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java:170: // TODO(ltian): Auto-generated constructor stub On 2016/08/22 18:47:13, Ian Wen wrote: > This constructor is not needed. The default java one will just work. Done. https://codereview.chromium.org/2258113003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java:383: //Separate BookmarkLoader initialization from construction to maintain a reference On 2016/08/22 18:47:13, Ian Wen wrote: > Thanks for the comments! > > I think it should be a bit more concise: "A reference of BookmarkLoader on > binder thread is needed to prevent the object from being garbage collected on Ui > thread." > > No need to mention Sony here, as this is a legit behavior in Java and it might > happen on any devices with memory pressure. Done. https://codereview.chromium.org/2258113003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java:435: //On some models (Sony Xperia Z3 & Z5), fetching meta data (getCount, getLoadingView) On 2016/08/22 18:47:14, Ian Wen wrote: > I would rephrase it to be: On some sony devices, getCount() might be triggered > even before onDatasetChanged() returns. If this happens, refresh the widget > until the bookmark is loaded. Done. https://codereview.chromium.org/2258113003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java:439: refreshWidget(); On 2016/08/22 18:47:14, Ian Wen wrote: > I am curious that if loading mCurrentFolder takes a long time, how many times > will we call refreshWidget(). Is it going to be a spinlock for the binder > thread? If so it would be a very expensive and we shouldn't do this. I'm > thinking maybe we should manually set a handler, and post refreshWidget() in > every 500ms? > > https://en.wikipedia.org/wiki/Spinlock It takes ~1.5s to load mCurrentFolder for >2000 bookmarks, if Chrome is already initialized. The getCount() is not immediately called. It is called only ~0.5s before onDatasetChanged() is finished. So is it a better idea to have a handler, wait for 0.5~1s and then post refreshWidget()? https://codereview.chromium.org/2258113003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java:441: } else if (!mPreferences.getString(PREF_CURRENT_FOLDER, null) On 2016/08/22 18:47:14, Ian Wen wrote: > This will actually crash if there is no PREF_CURRENT_FOLDER set in preferences, > because you default the value to be null. > > Also you are comparing a BookmarkId object with BookmarkId#getId()#ToString(), > which is wrong as well. Try BookmarkId#getBookmarkIdFromString(), and then > directly compare the two bookmark id objects. If PREF_CURRENT_FOLDER is not in preferences, mCurrentFolder will also be null and it will step into the first check and directly call refreshWidget(). The reason for the comparison is because in updateBookmarkList(), it populates the preferences by: mPreferences.edit().putString(PREF_CURRENT_FOLDER,mCurrentFolder.folder.id.toString()) .apply().
https://codereview.chromium.org/2258113003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java (right): https://codereview.chromium.org/2258113003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java:385: //prevent it from being garbaged collected by UI thread. s/garbaged/garbage GC will not run "by UI thread". It is simply a JVM mechanism. I would remove "by UI thread" here. https://codereview.chromium.org/2258113003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java:443: } else if (!mPreferences.getString(PREF_CURRENT_FOLDER, null) 1. What's the purpose of this if block? 2. You should combine #444 and #435 to remove duplicate code.
https://codereview.chromium.org/2258113003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java (right): https://codereview.chromium.org/2258113003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java:385: //prevent it from being garbaged collected by UI thread. On 2016/08/30 23:12:28, Ian Wen wrote: > s/garbaged/garbage > > GC will not run "by UI thread". It is simply a JVM mechanism. I would remove "by > UI thread" here. Done. https://codereview.chromium.org/2258113003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java:443: } else if (!mPreferences.getString(PREF_CURRENT_FOLDER, null) On 2016/08/30 23:12:28, Ian Wen wrote: > 1. What's the purpose of this if block? > 2. You should combine #444 and #435 to remove duplicate code. Done.
https://codereview.chromium.org/2258113003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java (right): https://codereview.chromium.org/2258113003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java:435: if (mCurrentFolder == null || !mPreferences.getString(PREF_CURRENT_FOLDER, new String()) Use "" instead of new String(). "" will create an adhoc empty string, and it is less verbose.
https://codereview.chromium.org/2258113003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java (right): https://codereview.chromium.org/2258113003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java:435: if (mCurrentFolder == null || !mPreferences.getString(PREF_CURRENT_FOLDER, new String()) On 2016/08/31 01:10:02, Ian Wen wrote: > Use "" instead of new String(). > > "" will create an adhoc empty string, and it is less verbose. Done.
lgtm!
The CQ bit was checked by ianwen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix the problems that on some models of phones such as Sony Xperia Z3 & Z5, Chrome widget blocks in showing the bookmark information. It comes with two problems and both happens when there are thousands of bookmark to display: 1. If Chrome is not initialized, GC might recycle the BookmarkLoader before BookmarkModel is loaded which blocks retrieving bookmark info. 2. Even Chrome is already intialized, fetching meta data is not always guaranteed to wait until onDataChanged is finished(even though Android doc claims it is). So need to manually check if bookmark info is loaded and keep refreshing if not. BUG=621643 ========== to ========== Fix the problems that on some models of phones such as Sony Xperia Z3 & Z5, Chrome widget blocks in showing the bookmark information. It comes with two problems and both happens when there are thousands of bookmark to display: 1. If Chrome is not initialized, GC might recycle the BookmarkLoader before BookmarkModel is loaded which blocks retrieving bookmark info. 2. Even Chrome is already intialized, fetching meta data is not always guaranteed to wait until onDataChanged is finished(even though Android doc claims it is). So need to manually check if bookmark info is loaded and keep refreshing if not. BUG=621643 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix the problems that on some models of phones such as Sony Xperia Z3 & Z5, Chrome widget blocks in showing the bookmark information. It comes with two problems and both happens when there are thousands of bookmark to display: 1. If Chrome is not initialized, GC might recycle the BookmarkLoader before BookmarkModel is loaded which blocks retrieving bookmark info. 2. Even Chrome is already intialized, fetching meta data is not always guaranteed to wait until onDataChanged is finished(even though Android doc claims it is). So need to manually check if bookmark info is loaded and keep refreshing if not. BUG=621643 ========== to ========== Fix the problems that on some models of phones such as Sony Xperia Z3 & Z5, Chrome widget blocks in showing the bookmark information. It comes with two problems and both happens when there are thousands of bookmark to display: 1. If Chrome is not initialized, GC might recycle the BookmarkLoader before BookmarkModel is loaded which blocks retrieving bookmark info. 2. Even Chrome is already intialized, fetching meta data is not always guaranteed to wait until onDataChanged is finished(even though Android doc claims it is). So need to manually check if bookmark info is loaded and keep refreshing if not. BUG=621643 Committed: https://crrev.com/24462918aee20854070ea38434c05d1da845884e Cr-Commit-Position: refs/heads/master@{#415684} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/24462918aee20854070ea38434c05d1da845884e Cr-Commit-Position: refs/heads/master@{#415684} |
