Audit uses of BrowserView::toolbar() with DesktopPWAWindowing |
||||||||
Issue descriptionThere are parts of the UI that expect the Chrome's main toolbar UI to be present (example issue 825554 ). With HostedAppButtonContainer reusing toolbar button related code we added BrowserViewButtonProvider as a generic button getter that defers to the appropriate "toolbar" view (either HostedAppButtonContainer or ToolbarView). We need to audit uses of ToolbarView* BrowserView::toolbar() that may need to use BrowserView::button_provider() instead.
,
Mar 28 2018
I'll take this given that it ties into my refactoring on Issue 791399 . Pri-1 because these might be crashes. Will drop the priority once confirmed.
,
Apr 12 2018
Audit results: - incognito_window_tracker.cc: Found weird existing issue 811982 ; added one more null check and removed assumption that it's non-null. (Note: This file wasn't being compiled in on my local build; it's behind a build flag.) - extension_action_platform_delegate_views.cc: Already null check. - extension_installed_bubble_view.cc: Added null check. - browser_non_client_frame_view_ash.cc: Not accessing property that can be null. - browser_view_layout.cc: Not accessing property that can be null. - ime_warning_bubble_view.cc: **outstanding**. Can't be null-checked as the whole thing relies on browser_actions. Should be fixed by using toolbar_button_provider(). - media_router_action_platform_delegate_views.cc: Added null check. - relaunch_recommended_bubble_view.cc: Not compiled on Chrome OS. **outstanding** Can't be null-checked as it needs a point. Should be fixed by using toolbar_button_provider(). - opaque_browser_frame_view.cc: Not compiled on Chrome OS. Not accessing property that can be null. This covers all of the files mentioned in #1. Note that even the ones where a null check was added should be audited again and converted to use toolbar_button_provider. The "**outstanding**" ones above are the ones that might still crash, and we should figure out if that's a possibility. Also note that this audit really has nothing to do with the Desktop PWAs launch; if any of these are crashy they will have been for some time, in Hosted and Bookmark app windows. So I don't think we should block launch on fixing these, but they should be addressed. CL to add null checks: https://chromium-review.googlesource.com/#/c/chromium/src/+/1010002
,
Apr 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8c852c8db3273cf728bdeb36b5ecd152aea8271c commit 8c852c8db3273cf728bdeb36b5ecd152aea8271c Author: Matt Giuca <mgiuca@chromium.org> Date: Thu Apr 12 22:07:54 2018 Added null checks for properties on ToolbarView that can be null. BrowserView's toolbar() will have null app_menu_button() and browser_actions() in hosted app windows, which could result in null dereferences in certain corner cases. This adds null checks where it's obviously safe to do so. A better fix would be to use toolbar_button_provider() to access the hosted app window's real app menu button and actions container, but that requires more careful consideration. Bug: 826596, 811982 Change-Id: Iea0a0ea3acffe5ed1016e61bd8ae2fe36971c79d Reviewed-on: https://chromium-review.googlesource.com/1010002 Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: Matt Giuca <mgiuca@chromium.org> Cr-Commit-Position: refs/heads/master@{#550381} [modify] https://crrev.com/8c852c8db3273cf728bdeb36b5ecd152aea8271c/chrome/browser/feature_engagement/incognito_window/incognito_window_tracker.cc [modify] https://crrev.com/8c852c8db3273cf728bdeb36b5ecd152aea8271c/chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc [modify] https://crrev.com/8c852c8db3273cf728bdeb36b5ecd152aea8271c/chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc
,
Apr 12 2018
Now the audit is done and null checks landed, dropping pri to 2. Still keeping M67, because we need to look into ime_warning_bubble_view and see if it can be triggered for hosted app windows (in which case it would be a crash and needs to use toolbar_button_provider).
,
Apr 13 2018
67 has branched, moving bugs over to 68.
,
Apr 13 2018
Per #5, I want to keep this on 67 in case there's a problem that needs to be merged.
,
Apr 13 2018
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8c852c8db3273cf728bdeb36b5ecd152aea8271c commit 8c852c8db3273cf728bdeb36b5ecd152aea8271c Author: Matt Giuca <mgiuca@chromium.org> Date: Thu Apr 12 22:07:54 2018 Added null checks for properties on ToolbarView that can be null. BrowserView's toolbar() will have null app_menu_button() and browser_actions() in hosted app windows, which could result in null dereferences in certain corner cases. This adds null checks where it's obviously safe to do so. A better fix would be to use toolbar_button_provider() to access the hosted app window's real app menu button and actions container, but that requires more careful consideration. Bug: 826596, 811982 Change-Id: Iea0a0ea3acffe5ed1016e61bd8ae2fe36971c79d Reviewed-on: https://chromium-review.googlesource.com/1010002 Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: Matt Giuca <mgiuca@chromium.org> Cr-Commit-Position: refs/heads/master@{#550381} [modify] https://crrev.com/8c852c8db3273cf728bdeb36b5ecd152aea8271c/chrome/browser/feature_engagement/incognito_window/incognito_window_tracker.cc [modify] https://crrev.com/8c852c8db3273cf728bdeb36b5ecd152aea8271c/chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc [modify] https://crrev.com/8c852c8db3273cf728bdeb36b5ecd152aea8271c/chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc
,
May 14 2018
,
Jun 18 2018
Bulk punting M68 PWA bugs to M69. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by alancutter@chromium.org
, Mar 28 2018