New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 755170 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Aug 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug
M61



Sign in to add a comment

Create a seperate class in AppListView to override WidgetObserver with no side effects

Project Member Reported by newcomer@chromium.org, Aug 14 2017

Issue description

Create a class to ensure that the state of AppListView is always set to CLOSED when the widget is closed.

A separate class needs to be created because observing a widget from AppListView calls functions in BubbleDialogDelegateView, which is causing a crash when the fullscreen flag is enabled.

Eventually we will be able to observe widgets the normal way with fullscreen enabled, but only once we remove the old launcher code (see bug 755169)
 
Labels: -Pri-3 NewLauncherUIV1 OS-Chrome Pri-1
Status: Started (was: Untriaged)
Project Member

Comment 2 by sheriffbot@chromium.org, Aug 14 2017

Labels: -Merge-Request-61 Merge-Review-61 Hotlist-Merge-Review
This bug requires manual review: M61 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a0ab0da7c46fdbc85496037c13a9ca7049d04409

commit a0ab0da7c46fdbc85496037c13a9ca7049d04409
Author: Alex Newcomer <newcomer@chromium.org>
Date: Mon Aug 14 22:13:47 2017

Added a widgetobserver class to AppListView

Added FullscreenWidgetObserver to AppListView which is only used when
the fullscreen flag is enabled. This is a workaround for the crash which
is caused when we add an observer to the AppListView. The crash is due
the AppListView being a child class of BubbleDialogDelegateView. This
class will be assimilated into AppListView once we get rid of the old
AppListView code path.

Bug:  755170 
Change-Id: I87a23b3ad0f73e143d9fc4502d392b1778b22f1b
Reviewed-on: https://chromium-review.googlesource.com/613576
Commit-Queue: Alex Newcomer <newcomer@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494207}
[modify] https://crrev.com/a0ab0da7c46fdbc85496037c13a9ca7049d04409/ui/app_list/views/app_list_view.cc
[modify] https://crrev.com/a0ab0da7c46fdbc85496037c13a9ca7049d04409/ui/app_list/views/app_list_view.h

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 21 2017

Labels: merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/943f24809596af59d7d519398144455534105aca

commit 943f24809596af59d7d519398144455534105aca
Author: Vadim Tryshev <vadimt@google.com>
Date: Mon Aug 21 21:20:40 2017

Added a widgetobserver class to AppListView

Added FullscreenWidgetObserver to AppListView which is only used when
the fullscreen flag is enabled. This is a workaround for the crash which
is caused when we add an observer to the AppListView. The crash is due
the AppListView being a child class of BubbleDialogDelegateView. This
class will be assimilated into AppListView once we get rid of the old
AppListView code path.

TBR=newcomer@chromium.org

(cherry picked from commit a0ab0da7c46fdbc85496037c13a9ca7049d04409)

Bug:  755170 
Change-Id: I87a23b3ad0f73e143d9fc4502d392b1778b22f1b
Reviewed-on: https://chromium-review.googlesource.com/613576
Commit-Queue: Alex Newcomer <newcomer@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#494207}
Reviewed-on: https://chromium-review.googlesource.com/624834
Reviewed-by: Vadim Tryshev <vadimt@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#717}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/943f24809596af59d7d519398144455534105aca/ui/app_list/views/app_list_view.cc
[modify] https://crrev.com/943f24809596af59d7d519398144455534105aca/ui/app_list/views/app_list_view.h

Status: Fixed (was: Started)
Labels: -Merge-Review-61 Merge-Approved-61
Approving merge for M61 and M62.
Project Member

Comment 7 by sheriffbot@chromium.org, Sep 11 2017

Cc: keta...@chromium.org
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 8 by sheriffbot@chromium.org, Sep 15 2017

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 9 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)
Labels: -Merge-Approved-61 -merge-merged-3163

Sign in to add a comment