Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2384773002:
Remember the orientation request without a window. (Closed)
DescriptionOrientation request may arrive before the window is created.
Instead of adding another map, I introduced AppWindowInfo which stores information about the arc windows w/o actual window.
Technically, we can move more from AppWindow, but we
can revisit it later if we want.
BUG=652863
, b/31836336
TEST=ChromeLauncherControllerOrientationTest.ArcOrientationLockBeforeWindowReady
Committed: https://crrev.com/f086a6dfa73928036f55434bd4ec4576478aaf90
Cr-Commit-Position: refs/heads/master@{#422635}
Patch Set 1 : . #
Total comments: 8
Patch Set 2 : addressed comments #
Messages
Total messages: 32 (26 generated)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by oshima@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...
Description was changed from ========== Orientation fix BUG=31836336 ========== to ========== Orientation fix BUG=31836336 TEST=ChromeLauncherControllerOrientationTest.ArcOrientationLockBeforeWindowReady ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== Orientation fix BUG=31836336 TEST=ChromeLauncherControllerOrientationTest.ArcOrientationLockBeforeWindowReady ========== to ========== Orientation request may arrive before the window is created. Instead of adding another map, I introduced AppWindowInfo which stores information about the arc windows w/o actual window. Technically, we can move more from AppWindow, but we can revisit it later if we want. BUG=31836336 TEST=ChromeLauncherControllerOrientationTest.ArcOrientationLockBeforeWindowReady ==========
oshima@chromium.org changed reviewers: + khmel@chromium.org
The CQ bit was checked by oshima@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: This issue passed the CQ dry run.
lgtm with nits https://codereview.chromium.org/2384773002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc (right): https://codereview.chromium.org/2384773002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:375: auto it = task_id_to_app_window_info_.find(task_id); nit: const auto...? https://codereview.chromium.org/2384773002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:411: LOG(ERROR) << "Could not find AppWindowInfo for task:" << task_id; This is not an error. There is a race when window created and new task message delivered. There is a unit_test for this. Please remove error logging. (::OnWindowVisibilityChanged may be called by new task id may not arrived yet, that is normal case) https://codereview.chromium.org/2384773002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:417: info->set_app_window(base::MakeUnique<AppWindow>(task_id, widget, this)); nit: Please add DCHECK_EQ(nullptr, info->get_app_window()); https://codereview.chromium.org/2384773002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:671: const std::string shelf_app_id = app_window_info->shelf_app_id(); const std::string&
The CQ bit was checked by oshima@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2384773002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc (right): https://codereview.chromium.org/2384773002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:375: auto it = task_id_to_app_window_info_.find(task_id); On 2016/10/03 15:24:13, khmel wrote: > nit: const auto...? Done. https://codereview.chromium.org/2384773002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:411: LOG(ERROR) << "Could not find AppWindowInfo for task:" << task_id; On 2016/10/03 15:24:13, khmel wrote: > This is not an error. There is a race when window created and new task message > delivered. There is a unit_test for this. Please remove error logging. > (::OnWindowVisibilityChanged may be called by new task id may not arrived yet, > that is normal case) changed to VLOG(1) https://codereview.chromium.org/2384773002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:417: info->set_app_window(base::MakeUnique<AppWindow>(task_id, widget, this)); On 2016/10/03 15:24:13, khmel wrote: > nit: Please add DCHECK_EQ(nullptr, info->get_app_window()); Done. https://codereview.chromium.org/2384773002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:671: const std::string shelf_app_id = app_window_info->shelf_app_id(); On 2016/10/03 15:24:13, khmel wrote: > const std::string& Done.
The CQ bit was checked by oshima@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/2384773002/#ps120001 (title: "addressed comments")
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 ========== Orientation request may arrive before the window is created. Instead of adding another map, I introduced AppWindowInfo which stores information about the arc windows w/o actual window. Technically, we can move more from AppWindow, but we can revisit it later if we want. BUG=31836336 TEST=ChromeLauncherControllerOrientationTest.ArcOrientationLockBeforeWindowReady ========== to ========== Orientation request may arrive before the window is created. Instead of adding another map, I introduced AppWindowInfo which stores information about the arc windows w/o actual window. Technically, we can move more from AppWindow, but we can revisit it later if we want. BUG=31836336 TEST=ChromeLauncherControllerOrientationTest.ArcOrientationLockBeforeWindowReady ==========
Message was sent while issue was closed.
Committed patchset #2 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Orientation request may arrive before the window is created. Instead of adding another map, I introduced AppWindowInfo which stores information about the arc windows w/o actual window. Technically, we can move more from AppWindow, but we can revisit it later if we want. BUG=31836336 TEST=ChromeLauncherControllerOrientationTest.ArcOrientationLockBeforeWindowReady ========== to ========== Orientation request may arrive before the window is created. Instead of adding another map, I introduced AppWindowInfo which stores information about the arc windows w/o actual window. Technically, we can move more from AppWindow, but we can revisit it later if we want. BUG=31836336 TEST=ChromeLauncherControllerOrientationTest.ArcOrientationLockBeforeWindowReady Committed: https://crrev.com/f086a6dfa73928036f55434bd4ec4576478aaf90 Cr-Commit-Position: refs/heads/master@{#422635} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f086a6dfa73928036f55434bd4ec4576478aaf90 Cr-Commit-Position: refs/heads/master@{#422635}
Message was sent while issue was closed.
Description was changed from ========== Orientation request may arrive before the window is created. Instead of adding another map, I introduced AppWindowInfo which stores information about the arc windows w/o actual window. Technically, we can move more from AppWindow, but we can revisit it later if we want. BUG=31836336 TEST=ChromeLauncherControllerOrientationTest.ArcOrientationLockBeforeWindowReady Committed: https://crrev.com/f086a6dfa73928036f55434bd4ec4576478aaf90 Cr-Commit-Position: refs/heads/master@{#422635} ========== to ========== Orientation request may arrive before the window is created. Instead of adding another map, I introduced AppWindowInfo which stores information about the arc windows w/o actual window. Technically, we can move more from AppWindow, but we can revisit it later if we want. BUG=b/31836336 TEST=ChromeLauncherControllerOrientationTest.ArcOrientationLockBeforeWindowReady Committed: https://crrev.com/f086a6dfa73928036f55434bd4ec4576478aaf90 Cr-Commit-Position: refs/heads/master@{#422635} ==========
Message was sent while issue was closed.
Description was changed from ========== Orientation request may arrive before the window is created. Instead of adding another map, I introduced AppWindowInfo which stores information about the arc windows w/o actual window. Technically, we can move more from AppWindow, but we can revisit it later if we want. BUG=b/31836336 TEST=ChromeLauncherControllerOrientationTest.ArcOrientationLockBeforeWindowReady Committed: https://crrev.com/f086a6dfa73928036f55434bd4ec4576478aaf90 Cr-Commit-Position: refs/heads/master@{#422635} ========== to ========== Orientation request may arrive before the window is created. Instead of adding another map, I introduced AppWindowInfo which stores information about the arc windows w/o actual window. Technically, we can move more from AppWindow, but we can revisit it later if we want. BUG=652863, b/31836336 TEST=ChromeLauncherControllerOrientationTest.ArcOrientationLockBeforeWindowReady Committed: https://crrev.com/f086a6dfa73928036f55434bd4ec4576478aaf90 Cr-Commit-Position: refs/heads/master@{#422635} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
