Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2747983002:
arc: Fix crash on deferred app launch. (Closed)
Descriptionarc: Fix crash on deferred app launch.
This fixes regression due recent shelf refactoring when meaning
of TYPE_APP AND TYPE_SHORTCUT_APP was changed. ARC defered
launcher is allowed only over shortcuts.
TEST=Manually
BUG=701152
Review-Url: https://codereview.chromium.org/2747983002
Cr-Commit-Position: refs/heads/master@{#456830}
Committed: https://chromium.googlesource.com/chromium/src/+/b1a39b125d0cc6b6994b92089ebc877b693a6ddd
Patch Set 1 #
Total comments: 6
Patch Set 2 : refactored #
Total comments: 4
Patch Set 3 : nits (comment updated) #
Messages
Total messages: 14 (5 generated)
khmel@chromium.org changed reviewers: + msw@chromium.org
Hi Mike IIUC this regression due https://codereview.chromium.org/2545923004 PTAL Thanks!
https://codereview.chromium.org/2747983002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc (right): https://codereview.chromium.org/2747983002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:192: return true; What does it mean if LauncherItemController::IsShortcut() != (ShelfItem::type == TYPE_APP_SHORTCUT)? Instead, could this launcher item controller subclass ensure that the associated shelf item has type ash::TYPE_APP_SHORTCUT (perhaps by setting the item type to that value in the ctor)? Or is the issue that non-"IsShortcut" items were marked as TYPE_APP_SHORTCUT? Either way, we should avoid ShelfItemDelegate/LauncherItemController expansion going forward. https://codereview.chromium.org/2747983002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc (right): https://codereview.chromium.org/2747983002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc:203: if (existing_controller && !existing_controller->IsShortcut()) Can you write an automated test that ensures this doesn't crash/regress?
https://codereview.chromium.org/2747983002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc (right): https://codereview.chromium.org/2747983002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:192: return true; On 2017/03/14 01:24:59, msw wrote: > What does it mean if LauncherItemController::IsShortcut() != (ShelfItem::type == > TYPE_APP_SHORTCUT)? Instead, could this launcher item controller subclass ensure > that the associated shelf item has type ash::TYPE_APP_SHORTCUT (perhaps by > setting the item type to that value in the ctor)? Or is the issue that > non-"IsShortcut" items were marked as TYPE_APP_SHORTCUT? Either way, we should > avoid ShelfItemDelegate/LauncherItemController expansion going forward. Hi, I don't clear understand your idea. The reason of this crash is impossibility to determine if shelf item represents running app or just a shortcut. With recent refactoring ash::TYPE_APP_SHORTCUT represents and shortcut and running app. When app is not running it has type ash::TYPE_APP_SHORTCUT. When app is running its shelf item still has type ash::TYPE_APP_SHORTCUT. Before in this scenario it was changed from ash::TYPE_APP_SHORTCUT to ash::TYPE_APP. Do you suggest to restore functionality by passing type to base class as it was here: https://codereview.chromium.org/2545923004/diff/80001/chrome/browser/ui/ash/l... ?
https://codereview.chromium.org/2747983002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc (right): https://codereview.chromium.org/2747983002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:192: return true; On 2017/03/14 15:25:44, khmel wrote: > On 2017/03/14 01:24:59, msw wrote: > > What does it mean if LauncherItemController::IsShortcut() != (ShelfItem::type > == > > TYPE_APP_SHORTCUT)? Instead, could this launcher item controller subclass > ensure > > that the associated shelf item has type ash::TYPE_APP_SHORTCUT (perhaps by > > setting the item type to that value in the ctor)? Or is the issue that > > non-"IsShortcut" items were marked as TYPE_APP_SHORTCUT? Either way, we should > > avoid ShelfItemDelegate/LauncherItemController expansion going forward. > > Hi, > I don't clear understand your idea. The reason of this crash is impossibility to > determine if shelf item represents running app or just a shortcut. With recent > refactoring ash::TYPE_APP_SHORTCUT represents and shortcut and running app. When > app is not running it has type ash::TYPE_APP_SHORTCUT. When app is running its > shelf item still has type ash::TYPE_APP_SHORTCUT. Before in this scenario it was > changed from ash::TYPE_APP_SHORTCUT to ash::TYPE_APP. Do you suggest to restore > functionality by passing type to base class as it was here: > https://codereview.chromium.org/2545923004/diff/80001/chrome/browser/ui/ash/l... > ? My idea is to update and check ShelfItem struct values, instead of calling controller instance functions. So, something should be updating the ShelfItem's |type| and/or |status| when the item changes from a shortcut to a running app. There are two possibilities: 1) Running apps should have ShelfItem::type == ash::TYPE_APP, but shortcuts (pinned items that are not running) should have ShelfItem::type == ash::TYPE_APP_SHORTCUT. See ShelfItem::type here: https://cs.chromium.org/chromium/src/ash/common/shelf/shelf_item_types.h?l=25 2) Even better, we disregard ShelfItem::type (both should eventually just be app...) and we check the ShelfItem::status value (either STATUS_CLOSED or not). See ShelfItem::status here: https://cs.chromium.org/chromium/src/ash/common/shelf/shelf_item_types.h?l=34 I hope that's clearer. What do you think?
Thanks for our offline discussion. PTAL updated version https://codereview.chromium.org/2747983002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc (right): https://codereview.chromium.org/2747983002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/app_shortcut_launcher_item_controller.cc:192: return true; On 2017/03/14 17:19:32, msw wrote: > On 2017/03/14 15:25:44, khmel wrote: > > On 2017/03/14 01:24:59, msw wrote: > > > What does it mean if LauncherItemController::IsShortcut() != > (ShelfItem::type > > == > > > TYPE_APP_SHORTCUT)? Instead, could this launcher item controller subclass > > ensure > > > that the associated shelf item has type ash::TYPE_APP_SHORTCUT (perhaps by > > > setting the item type to that value in the ctor)? Or is the issue that > > > non-"IsShortcut" items were marked as TYPE_APP_SHORTCUT? Either way, we > should > > > avoid ShelfItemDelegate/LauncherItemController expansion going forward. > > > > Hi, > > I don't clear understand your idea. The reason of this crash is impossibility > to > > determine if shelf item represents running app or just a shortcut. With recent > > refactoring ash::TYPE_APP_SHORTCUT represents and shortcut and running app. > When > > app is not running it has type ash::TYPE_APP_SHORTCUT. When app is running its > > shelf item still has type ash::TYPE_APP_SHORTCUT. Before in this scenario it > was > > changed from ash::TYPE_APP_SHORTCUT to ash::TYPE_APP. Do you suggest to > restore > > functionality by passing type to base class as it was here: > > > https://codereview.chromium.org/2545923004/diff/80001/chrome/browser/ui/ash/l... > > ? > > My idea is to update and check ShelfItem struct values, instead of calling > controller instance functions. So, something should be updating the ShelfItem's > |type| and/or |status| when the item changes from a shortcut to a running app. > There are two possibilities: > 1) Running apps should have ShelfItem::type == ash::TYPE_APP, but shortcuts > (pinned items that are not running) should have ShelfItem::type == > ash::TYPE_APP_SHORTCUT. See ShelfItem::type here: > https://cs.chromium.org/chromium/src/ash/common/shelf/shelf_item_types.h?l=25 > 2) Even better, we disregard ShelfItem::type (both should eventually just be > app...) and we check the ShelfItem::status value (either STATUS_CLOSED or not). > See ShelfItem::status here: > https://cs.chromium.org/chromium/src/ash/common/shelf/shelf_item_types.h?l=34 > I hope that's clearer. What do you think? Done as per offline discussion. https://codereview.chromium.org/2747983002/diff/1/chrome/browser/ui/ash/launc... File chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc (right): https://codereview.chromium.org/2747983002/diff/1/chrome/browser/ui/ash/launc... chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc:203: if (existing_controller && !existing_controller->IsShortcut()) On 2017/03/14 01:24:59, msw wrote: > Can you write an automated test that ensures this doesn't crash/regress? Done.
lgtm with nits https://codereview.chromium.org/2747983002/diff/20001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc (right): https://codereview.chromium.org/2747983002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1935: // Test that validates deferred controler does not override active app nit: consider "// Ensure the deferred controller does not override the active app controller." Also, optionally mention the crash and/or cite the bug#? https://codereview.chromium.org/2747983002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1973: // This should switch shelf item into active state. nit: Does 'active' here contradict the 'STATUS_CLOSED' check below?
https://codereview.chromium.org/2747983002/diff/20001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc (right): https://codereview.chromium.org/2747983002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1935: // Test that validates deferred controler does not override active app On 2017/03/14 20:28:08, msw wrote: > nit: consider "// Ensure the deferred controller does not override the active > app controller." Also, optionally mention the crash and/or cite the bug#? Thanks for ready to use text :) https://codereview.chromium.org/2747983002/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:1973: // This should switch shelf item into active state. On 2017/03/14 20:28:08, msw wrote: > nit: Does 'active' here contradict the 'STATUS_CLOSED' check below? nice catch.
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2747983002/#ps40001 (title: "nits (comment updated)")
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": 40001, "attempt_start_ts": 1489523535607140,
"parent_rev": "65dcca5f8602932cb80baf9b0b2c739bcca41495", "commit_rev":
"b1a39b125d0cc6b6994b92089ebc877b693a6ddd"}
Message was sent while issue was closed.
Description was changed from ========== arc: Fix crash on deferred app launch. This fixes regression due recent shelf refactoring when meaning of TYPE_APP AND TYPE_SHORTCUT_APP was changed. ARC defered launcher is allowed only over shortcuts. TEST=Manually BUG=701152 ========== to ========== arc: Fix crash on deferred app launch. This fixes regression due recent shelf refactoring when meaning of TYPE_APP AND TYPE_SHORTCUT_APP was changed. ARC defered launcher is allowed only over shortcuts. TEST=Manually BUG=701152 Review-Url: https://codereview.chromium.org/2747983002 Cr-Commit-Position: refs/heads/master@{#456830} Committed: https://chromium.googlesource.com/chromium/src/+/b1a39b125d0cc6b6994b92089ebc... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b1a39b125d0cc6b6994b92089ebc... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
