Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2540433002:
Reland "Propagate information about how ARC apps are launched" (Closed)
|
Created:
4 years ago by Luis Héctor Chávez Modified:
4 years ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, sadrul, yusukes+watch_chromium.org, tdresser+watch_chromium.org, tfarina, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, kalyank, davemoore+watch_chromium.org, Matt Giuca, sky Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReland "Propagate information about how ARC apps are launched"
This change lets ARC know whether an app should be launched in Touch
Mode[1] or not.
1: http://android-developers.blogspot.com/2008/12/touch-mode.html
BUG=669146
TEST=ARC app starts in touch mode when activated using
mouse/trackpad/touch
TEST=ARC app starts in focus mode when activated using
keyboard.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/b5898c35b2415dbf8b5d9aaa19a6b13b0510d1e7
Committed: https://crrev.com/fa0327a360d5e676680a4b5337a607958c0512d0
Cr-Original-Commit-Position: refs/heads/master@{#435115}
Cr-Commit-Position: refs/heads/master@{#435327}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed unit tests #
Total comments: 2
Patch Set 3 : Moved mouse/touch detectors to ARC code #
Total comments: 4
Patch Set 4 : Dedupe some code + propagate flags from JS #
Total comments: 3
Patch Set 5 : Propagate flag from deferred launcher #
Total comments: 2
Patch Set 6 : git cl lint #Patch Set 7 : Rebase to ToT #
Total comments: 18
Patch Set 8 : Addressed review feedback #Patch Set 9 : Fixed use-after-free #Messages
Total messages: 66 (39 generated)
The CQ bit was checked by lhchavez@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...
lhchavez@chromium.org changed reviewers: + khmel@chromium.org, sky@chromium.org, stevenjb@chromium.org
PTAL khmel: chrome/browser/ui/app_list/arc stevenjb: chrome/browser/* sky: ui/*
https://codereview.chromium.org/2540433002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2540433002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_utils.cc:220: extras.SetBoolean("inTouchMode", Can we make sure that sequence of operations is correct? For example we might have 2 app launch close to each other. One in touch mode and second is not. Is is guaranty here that touch mode will be valid on the app launch time? Another concern: App can be launched from Android itself. In this case last mode set from app launcher will affect it.
https://codereview.chromium.org/2540433002/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2540433002/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc/arc_app_utils.cc:220: extras.SetBoolean("inTouchMode", On 2016/11/28 20:27:02, khmel wrote: > Can we make sure that sequence of operations is correct? > > For example we might have 2 app launch close to each other. One in touch mode > and second is not. Is is guaranty here that touch mode will be valid on the app > launch time? I don't think such a guarantee is possible. Touch mode is a system-global state, so the value of touch mode will be undefined for the first app. > > Another concern: > App can be launched from Android itself. In this case last mode set from app > launcher will affect it. Since touch mode is a system-global state, the app launched from Android itself will preserve the value of touch mode from the first app. e.g. if the app was launched using the keyboard, it will still be out of touch mode, regardless of how the first app was invoked.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_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 lhchavez@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...
https://codereview.chromium.org/2540433002/diff/20001/ui/events/event_utils.cc File ui/events/event_utils.cc (right): https://codereview.chromium.org/2540433002/diff/20001/ui/events/event_utils.c... ui/events/event_utils.cc:133: bool IsMouseEventFromFlags(int event_flags) { AFAICT these functions are only used in your code. I would prefer the functions exist there.
https://codereview.chromium.org/2540433002/diff/20001/ui/events/event_utils.cc File ui/events/event_utils.cc (right): https://codereview.chromium.org/2540433002/diff/20001/ui/events/event_utils.c... ui/events/event_utils.cc:133: bool IsMouseEventFromFlags(int event_flags) { On 2016/11/28 21:35:26, sky wrote: > AFAICT these functions are only used in your code. I would prefer the functions > exist there. Done.
lhchavez@chromium.org changed reviewers: - sky@chromium.org
-sky@ as reviewer.
https://codereview.chromium.org/2540433002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2540433002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_utils.cc:225: if (intent_helper_instance) { Nit: please log error if instance is not available or (D)CHECK? Can we also do it similar to GetAppInstance(kMinVersion, kLaunchAppStr)? There is other code here that uses intent_helper. https://codereview.chromium.org/2540433002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2540433002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:2012: arc::LaunchAndroidSettingsApp(profile, ui::EF_NONE); It is more likely that this function is called by mouse clicking from settings. Can we extract any flag from https://cs.chromium.org/chromium/src/chrome/browser/resources/options/browser...
Description was changed from ========== Propagate information about how ARC apps are launched This change lets ARC know whether an app should be launched in Touch Mode[1] or not. 1: http://android-developers.blogspot.com/2008/12/touch-mode.html BUG=669146 TEST=ARC app starts in touch mode when activated using mouse/trackpad/touch TEST=ARC app starts in focus mode when activated using keyboard. ========== to ========== Propagate information about how ARC apps are launched This change lets ARC know whether an app should be launched in Touch Mode[1] or not. 1: http://android-developers.blogspot.com/2008/12/touch-mode.html BUG=669146 TEST=ARC app starts in touch mode when activated using mouse/trackpad/touch TEST=ARC app starts in focus mode when activated using keyboard. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2540433002/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2540433002/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_utils.cc:225: if (intent_helper_instance) { On 2016/11/28 22:08:51, khmel wrote: > Nit: please log error if instance is not available or (D)CHECK? > Can we also do it similar to GetAppInstance(kMinVersion, kLaunchAppStr)? There > is other code here that uses intent_helper. GetInstanceForMethod already performs logging. We definitely should not DCHECK because there's no guarantee of the ordering of the services in Android being available. Did something similar to GetAppInstance. https://codereview.chromium.org/2540433002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2540433002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:2012: arc::LaunchAndroidSettingsApp(profile, ui::EF_NONE); On 2016/11/28 22:08:52, khmel wrote: > It is more likely that this function is called by mouse clicking from settings. > Can we extract any flag from > https://cs.chromium.org/chromium/src/chrome/browser/resources/options/browser... Done.
The CQ bit was checked by lhchavez@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...
lgtm with nit https://codereview.chromium.org/2540433002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2540433002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_utils.cc:308: app_id); nit: It would be beneficial to pass |event_flags| to deferred controller to store them for deferred launch. Otherwise all apps, started in deferred mode appear as started from keyboard.
https://codereview.chromium.org/2540433002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2540433002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_utils.cc:308: app_id); On 2016/11/28 23:02:43, khmel wrote: > nit: It would be beneficial to pass |event_flags| to deferred controller to > store them for deferred launch. Otherwise all apps, started in deferred mode > appear as started from keyboard. Done.
https://codereview.chromium.org/2540433002/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2540433002/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_app_utils.cc:308: app_id); On 2016/11/28 23:20:53, Luis Héctor Chávez wrote: > On 2016/11/28 23:02:43, khmel wrote: > > nit: It would be beneficial to pass |event_flags| to deferred controller to > > store them for deferred launch. Otherwise all apps, started in deferred mode > > appear as started from keyboard. > > Done. Thanks! https://codereview.chromium.org/2540433002/diff/80001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc (right): https://codereview.chromium.org/2540433002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc:7: #include <string> nit: Remove it here. You included it in .h file.
https://codereview.chromium.org/2540433002/diff/80001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc (right): https://codereview.chromium.org/2540433002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc:7: #include <string> On 2016/11/28 23:23:48, khmel wrote: > nit: Remove it here. You included it in .h file. Done.
The CQ bit was checked by lhchavez@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lhchavez@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...
stevenjb@ gentle ping?
https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.cc (right): https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.cc:24: // Launching the app by app id in landscape mode. nit: Update comment wrt event_flags. https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.js:875: var isMouseOrTouch = e.detail != 0; I find isMouseOrTouch confusing. Maybe use isKeyboardEvent instead? Also, the comment just describes 'clicks', which makes it unclear whether that would be set for a touch 'tap' (presumably it is?). https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:245: "org.chromium.arc.intent_helper.SET_IN_TOUCH_MODE", We should also define this as a constant above. https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:353: "org.chromium.arc.intent_helper.SHOW_TALKBACK_SETTINGS", This too. https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.h (right): https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.h:33: // Registers deferred Arc app launch. Can you briefly document this class? It's not clear to me what this does, and why passing event_flags to RegisterDeferredLaunch makes sense. https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_item_controller.h (right): https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_item_controller.h:23: int event_flags, Please document this class too, and document event_flags_ below since this is an "item", not an event like "activate" or "launch". https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc (right): https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc:263: arc::LaunchApp(profile(), app_id, ui::EF_NONE); It seems to me like the default event for these should be a click event? https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2014: int flags = ui::EF_LEFT_MOUSE_BUTTON; Again, since what we really care about is whether this was a keyboard event, I think the parameter would be more clear as "from_keyboard". https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2017: flags = ui::EF_NONE; nit: int flags = is_mouse_or_touch ? ui::EF_LEFT_MOUSE_BUTTON : ui::EF_NONE;
https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.cc (right): https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_launcher.cc:24: // Launching the app by app id in landscape mode. On 2016/11/29 20:17:09, stevenjb wrote: > nit: Update comment wrt event_flags. Done. https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.js:875: var isMouseOrTouch = e.detail != 0; On 2016/11/29 20:17:10, stevenjb wrote: > I find isMouseOrTouch confusing. Maybe use isKeyboardEvent instead? Also, the > comment just describes 'clicks', which makes it unclear whether that would be > set for a touch 'tap' (presumably it is?). Done. https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_utils.cc (right): https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:245: "org.chromium.arc.intent_helper.SET_IN_TOUCH_MODE", On 2016/11/29 20:17:10, stevenjb wrote: > We should also define this as a constant above. Done. https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_utils.cc:353: "org.chromium.arc.intent_helper.SHOW_TALKBACK_SETTINGS", On 2016/11/29 20:17:10, stevenjb wrote: > This too. Done. https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.h (right): https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.h:33: // Registers deferred Arc app launch. On 2016/11/29 20:17:10, stevenjb wrote: > Can you briefly document this class? It's not clear to me what this does, and > why passing event_flags to RegisterDeferredLaunch makes sense. Done. https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_item_controller.h (right): https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_item_controller.h:23: int event_flags, On 2016/11/29 20:17:10, stevenjb wrote: > Please document this class too, and document event_flags_ below since this is an > "item", not an event like "activate" or "launch". Done. https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc (right): https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc:263: arc::LaunchApp(profile(), app_id, ui::EF_NONE); On 2016/11/29 20:17:10, stevenjb wrote: > It seems to me like the default event for these should be a click event? It does not have any perceivable difference from the test's perspective, but changed to left click. https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2014: int flags = ui::EF_LEFT_MOUSE_BUTTON; On 2016/11/29 20:17:10, stevenjb wrote: > Again, since what we really care about is whether this was a keyboard event, I > think the parameter would be more clear as "from_keyboard". Done. https://codereview.chromium.org/2540433002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:2017: flags = ui::EF_NONE; On 2016/11/29 20:17:10, stevenjb wrote: > nit: > > int flags = is_mouse_or_touch ? ui::EF_LEFT_MOUSE_BUTTON : ui::EF_NONE; Done.
The CQ bit was checked by lhchavez@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...
Thanks for the clarifying class comments. LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lhchavez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from khmel@chromium.org Link to the patchset: https://codereview.chromium.org/2540433002/#ps130001 (title: "Addressed review feedback")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by lhchavez@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": 130001, "attempt_start_ts": 1480470297034610,
"parent_rev": "1fdc4cd0fa4d0d0ab5974b409cbe97a59939e4a1", "commit_rev":
"7e6c15e808bfad6d7642aab19d88160542553a78"}
CQ is committing da patch.
Bot data: {"patchset_id": 130001, "attempt_start_ts": 1480470297034610,
"parent_rev": "1fdc4cd0fa4d0d0ab5974b409cbe97a59939e4a1", "commit_rev":
"7e6c15e808bfad6d7642aab19d88160542553a78"}
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== Propagate information about how ARC apps are launched This change lets ARC know whether an app should be launched in Touch Mode[1] or not. 1: http://android-developers.blogspot.com/2008/12/touch-mode.html BUG=669146 TEST=ARC app starts in touch mode when activated using mouse/trackpad/touch TEST=ARC app starts in focus mode when activated using keyboard. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Propagate information about how ARC apps are launched This change lets ARC know whether an app should be launched in Touch Mode[1] or not. 1: http://android-developers.blogspot.com/2008/12/touch-mode.html BUG=669146 TEST=ARC app starts in touch mode when activated using mouse/trackpad/touch TEST=ARC app starts in focus mode when activated using keyboard. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b5898c35b2415dbf8b5d9aaa19a6b13b0510d1e7 Cr-Commit-Position: refs/heads/master@{#435115} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b5898c35b2415dbf8b5d9aaa19a6b13b0510d1e7 Cr-Commit-Position: refs/heads/master@{#435115}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:130001) has been created in https://codereview.chromium.org/2540713003/ by battre@chromium.org. The reason for reverting is: Memory use after free (http://crbug.com/669146#c2).
Message was sent while issue was closed.
Description was changed from ========== Propagate information about how ARC apps are launched This change lets ARC know whether an app should be launched in Touch Mode[1] or not. 1: http://android-developers.blogspot.com/2008/12/touch-mode.html BUG=669146 TEST=ARC app starts in touch mode when activated using mouse/trackpad/touch TEST=ARC app starts in focus mode when activated using keyboard. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b5898c35b2415dbf8b5d9aaa19a6b13b0510d1e7 Cr-Commit-Position: refs/heads/master@{#435115} ========== to ========== Propagate information about how ARC apps are launched This change lets ARC know whether an app should be launched in Touch Mode[1] or not. 1: http://android-developers.blogspot.com/2008/12/touch-mode.html BUG=669146 TEST=ARC app starts in touch mode when activated using mouse/trackpad/touch TEST=ARC app starts in focus mode when activated using keyboard. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b5898c35b2415dbf8b5d9aaa19a6b13b0510d1e7 Cr-Commit-Position: refs/heads/master@{#435115} ==========
The CQ bit was checked by lhchavez@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...
There was a very dumb on my part use-after-free that caused this change to be reverted. This has been fixed and tested locally with the same configuration as the ASan builder with the following: ./unit_tests --gtest_filter=ChromeLauncherControllerImplWithArcTest.ArcDeferredLaunch ./browser_tests --gtest_filter=ArcAppDeferredLauncherBrowserTest* Didn't pass locally prior to the last patchset (as expected), but now both pass. PTAAL
Description was changed from ========== Propagate information about how ARC apps are launched This change lets ARC know whether an app should be launched in Touch Mode[1] or not. 1: http://android-developers.blogspot.com/2008/12/touch-mode.html BUG=669146 TEST=ARC app starts in touch mode when activated using mouse/trackpad/touch TEST=ARC app starts in focus mode when activated using keyboard. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b5898c35b2415dbf8b5d9aaa19a6b13b0510d1e7 Cr-Commit-Position: refs/heads/master@{#435115} ========== to ========== Reland "Propagate information about how ARC apps are launched" This change lets ARC know whether an app should be launched in Touch Mode[1] or not. 1: http://android-developers.blogspot.com/2008/12/touch-mode.html BUG=669146 TEST=ARC app starts in touch mode when activated using mouse/trackpad/touch TEST=ARC app starts in focus mode when activated using keyboard. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b5898c35b2415dbf8b5d9aaa19a6b13b0510d1e7 Cr-Commit-Position: refs/heads/master@{#435115} ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/30 16:44:32, Luis Héctor Chávez wrote: > There was a very dumb on my part use-after-free that caused this change to be > reverted. This has been fixed and tested locally with the same configuration as > the ASan builder with the following: > > ./unit_tests > --gtest_filter=ChromeLauncherControllerImplWithArcTest.ArcDeferredLaunch > ./browser_tests --gtest_filter=ArcAppDeferredLauncherBrowserTest* > > Didn't pass locally prior to the last patchset (as expected), but now both pass. > > PTAAL Thanks for fix! lgtm
The CQ bit was checked by lhchavez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2540433002/#ps150001 (title: "Fixed use-after-free")
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": 150001, "attempt_start_ts": 1480528359842030,
"parent_rev": "a5035303b30a24bd00d1c92dcc5747058de66b4b", "commit_rev":
"9e55d36b542ed1ca0a8197ce830e7151025c5b85"}
Message was sent while issue was closed.
Description was changed from ========== Reland "Propagate information about how ARC apps are launched" This change lets ARC know whether an app should be launched in Touch Mode[1] or not. 1: http://android-developers.blogspot.com/2008/12/touch-mode.html BUG=669146 TEST=ARC app starts in touch mode when activated using mouse/trackpad/touch TEST=ARC app starts in focus mode when activated using keyboard. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b5898c35b2415dbf8b5d9aaa19a6b13b0510d1e7 Cr-Commit-Position: refs/heads/master@{#435115} ========== to ========== Reland "Propagate information about how ARC apps are launched" This change lets ARC know whether an app should be launched in Touch Mode[1] or not. 1: http://android-developers.blogspot.com/2008/12/touch-mode.html BUG=669146 TEST=ARC app starts in touch mode when activated using mouse/trackpad/touch TEST=ARC app starts in focus mode when activated using keyboard. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b5898c35b2415dbf8b5d9aaa19a6b13b0510d1e7 Cr-Commit-Position: refs/heads/master@{#435115} ==========
Message was sent while issue was closed.
Committed patchset #9 (id:150001)
Message was sent while issue was closed.
Description was changed from ========== Reland "Propagate information about how ARC apps are launched" This change lets ARC know whether an app should be launched in Touch Mode[1] or not. 1: http://android-developers.blogspot.com/2008/12/touch-mode.html BUG=669146 TEST=ARC app starts in touch mode when activated using mouse/trackpad/touch TEST=ARC app starts in focus mode when activated using keyboard. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b5898c35b2415dbf8b5d9aaa19a6b13b0510d1e7 Cr-Commit-Position: refs/heads/master@{#435115} ========== to ========== Reland "Propagate information about how ARC apps are launched" This change lets ARC know whether an app should be launched in Touch Mode[1] or not. 1: http://android-developers.blogspot.com/2008/12/touch-mode.html BUG=669146 TEST=ARC app starts in touch mode when activated using mouse/trackpad/touch TEST=ARC app starts in focus mode when activated using keyboard. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b5898c35b2415dbf8b5d9aaa19a6b13b0510d1e7 Committed: https://crrev.com/fa0327a360d5e676680a4b5337a607958c0512d0 Cr-Original-Commit-Position: refs/heads/master@{#435115} Cr-Commit-Position: refs/heads/master@{#435327} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/fa0327a360d5e676680a4b5337a607958c0512d0 Cr-Commit-Position: refs/heads/master@{#435327} |
