Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2817293002:
Fix randomly disappearing icons in app list recent view. (Closed)
DescriptionFix randomly disappearing icons in app list recent view.
This fixes race condition when extension is installed.
BUG=711779
TEST=Manually on device. No flikering and no disappearing icons
detected.
Review-Url: https://codereview.chromium.org/2817293002
Cr-Commit-Position: refs/heads/master@{#464879}
Committed: https://chromium.googlesource.com/chromium/src/+/37568714f8eba4d6ff7eaf2257e1a785a80753e6
Patch Set 1 #Patch Set 2 : forgot UpdateIcon to match previous logic #
Total comments: 4
Patch Set 3 : rename function + comments #Patch Set 4 : fix win compilation #
Messages
Total messages: 24 (13 generated)
Description was changed from
==========
Fix randomly dissapearing icons in app list recent view.
This fixes race condition when extension get installed.
BUG=711779
TEST=Manually on device. No flikering and no disappearing icons
detected.
==========
to
==========
Fix randomly disappearing icons in app list recent view.
This fixes race condition when extension is installed.
BUG=711779
TEST=Manually on device. No flikering and no disappearing icons
detected.
==========
Description was changed from
==========
Fix randomly dissapearing icons in app list recent view.
This fixes race condition when extension get installed.
BUG=711779
TEST=Manually on device. No flikering and no disappearing icons
detected.
==========
to
==========
Fix randomly disappearing icons in app list recent view.
This fixes race condition when extension is installed.
BUG=711779
TEST=Manually on device. No flikering and no disappearing icons
detected.
==========
khmel@chromium.org changed reviewers: + xiyuan@chromium.org
Hi Xiyuan, Quite tricky case. Recent view is based on ExtensionAppResult. This element contains extension icon image. Image has observer on extension is removed: https://cs.chromium.org/chromium/src/extensions/browser/extension_icon_image.... As result icon data won't be loaded if NOTIFICATION_EXTENSION_REMOVED is issued. As result ExtensionAppResult might have icon without actual image. From other side, set of ExtensionAppResult is updated once any extension is installed or loaded. This leads to situation when ExtensionAppResult is constantly recreated. However new ExtensionAppResult is not used. Instead first created item is used and updated from newly created item. https://cs.chromium.org/chromium/src/ui/app_list/search/mixer.cc?dr&l=23 But in this case only title is updated. I cannot update icon together with title because this causes serious performance drop. So solution is recreate icon in case it was invalidated. PTAL Thanks!
Yep, extension update would make IconImage detached because of the notification and no longer provide the actual icon. If the hosting result is in the list serves UI, it will have the last loaded icon (probably blank). Good analysis. LGTM!
khmel@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Thank you Xiyuan for quick review! Adding Devlin for extension/browser/ approval PTAL
extensions lgtm https://codereview.chromium.org/2817293002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/extension_app_result.cc (right): https://codereview.chromium.org/2817293002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/extension_app_result.cc:53: void ExtensionAppResult::MaybeCreateIcon() { drive-by: perhaps this should be called CreateOrUpdateIcon()? https://codereview.chromium.org/2817293002/diff/20001/extensions/browser/exte... File extensions/browser/extension_icon_image.h (right): https://codereview.chromium.org/2817293002/diff/20001/extensions/browser/exte... extensions/browser/extension_icon_image.h:80: // Returns true if icon is attached to existing extension. grammar nit: if *the* icon is attached to *an* existing extension.
Thank you for review! https://codereview.chromium.org/2817293002/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/search/extension_app_result.cc (right): https://codereview.chromium.org/2817293002/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/search/extension_app_result.cc:53: void ExtensionAppResult::MaybeCreateIcon() { On 2017/04/15 00:42:54, Devlin wrote: > drive-by: perhaps this should be called CreateOrUpdateIcon()? Nice name, thanks! https://codereview.chromium.org/2817293002/diff/20001/extensions/browser/exte... File extensions/browser/extension_icon_image.h (right): https://codereview.chromium.org/2817293002/diff/20001/extensions/browser/exte... extensions/browser/extension_icon_image.h:80: // Returns true if icon is attached to existing extension. On 2017/04/15 00:42:54, Devlin wrote: > grammar nit: if *the* icon is attached to *an* existing extension. Done.
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2817293002/#ps40001 (title: "rename function + comments")
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
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2817293002/#ps60001 (title: "fix win compilation")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by khmel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492310965820470,
"parent_rev": "7a57143828b914abad790775f41ea97255ae9c11", "commit_rev":
"37568714f8eba4d6ff7eaf2257e1a785a80753e6"}
Message was sent while issue was closed.
Description was changed from
==========
Fix randomly disappearing icons in app list recent view.
This fixes race condition when extension is installed.
BUG=711779
TEST=Manually on device. No flikering and no disappearing icons
detected.
==========
to
==========
Fix randomly disappearing icons in app list recent view.
This fixes race condition when extension is installed.
BUG=711779
TEST=Manually on device. No flikering and no disappearing icons
detected.
Review-Url: https://codereview.chromium.org/2817293002
Cr-Commit-Position: refs/heads/master@{#464879}
Committed:
https://chromium.googlesource.com/chromium/src/+/37568714f8eba4d6ff7eaf2257e1...
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/37568714f8eba4d6ff7eaf2257e1... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
