Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2630473003:
MacViews: Harmony for toolbar actions bubbles. (Closed)
|
Created:
3 years, 11 months ago by tapted Modified:
3 years, 9 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, mac-reviews_chromium.org, tfarina, extensions-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Harmony for toolbar actions bubbles.
These are used for
- DevModeBubbleDelegate
- NtpOverriddenBubbleDelegate
- ProxyOverriddenBubbleDelegate
- SettingsApiBubbleDelegate
- SuspiciousExtensionBubbleDelegate
- BlockedActionBubbleDelegate
Adds a ToolbarActionsBarBubbleViewsPresenter for showing the
toolkit-views bubble on a Cocoa browser. It observes the Views bubble
and notifies the Cocoa BrowserActionsController when it closes.
One issue: the Cocoa bubble uses a `BOOL anchoredToAction;` data member
whereas the Views bubble inspects the anchor_view's ID. The latter
doesn't work for a Cocoa browser, so make these consistent by adding the
same data member to the views bubble.
Adds a TestBrowserDialog case for showing the DevModeBubble with Views
on all desktop platforms. These need to be added for the other bubble
types.
Brings up all the views-based toolbar action bubble tests on Mac by
factoring out two functions that depend on whether the Browser window is
Cocoa or toolkit-views.
Screenshot at http://crbug.com/654128#c5 (it's currently too wide under
Harmony on Mac, but there are framework fixes coming for that).
The interactive bubble test can be invoked with `browser_tests
--gtest_filter=BrowserDialogTest.Invoke --interactive
--dialog=ExtensionMessageBubbleViewBrowserTest.InvokeDialog_devmode_warning`
BUG=654128, 654126, 654121
Review-Url: https://codereview.chromium.org/2630473003
Cr-Commit-Position: refs/heads/master@{#456329}
Committed: https://chromium.googlesource.com/chromium/src/+/0400d0a14218e82b41e2be64fb3174e70352b4e7
Patch Set 1 : nits #Patch Set 2 : Rebase, but some diff can be culled #Patch Set 3 : Harmony anchoring #Patch Set 4 : Cleanups #Patch Set 5 : selfnits #Patch Set 6 : Fix DialogBrowserTest #
Total comments: 14
Patch Set 7 : respond to comments #Patch Set 8 : base::EraseIf #
Total comments: 9
Patch Set 9 : respond to comments #
Total comments: 9
Patch Set 10 : respond to comments #Messages
Total messages: 84 (74 generated)
The CQ bit was checked by tapted@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 checked by tapted@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by tapted@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.
Description was changed from ========== MacViews: Harmony for toolbar actions bubbles. BUG= ========== to ========== MacViews: Harmony for toolbar actions bubbles. BUG=654128 ==========
Description was changed from ========== MacViews: Harmony for toolbar actions bubbles. BUG=654128 ========== to ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate BUG=654128 ==========
The CQ bit was checked by tapted@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 ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate BUG=654128 ========== to ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a TestBrowserDialog case for showing the DevModeBubble with Views on Mac. These need to be added for the other bubble types, and for the Views browser on on other platforms. BUG=654128, 654126, ==========
tapted@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Description was changed from ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a TestBrowserDialog case for showing the DevModeBubble with Views on Mac. These need to be added for the other bubble types, and for the Views browser on on other platforms. BUG=654128, 654126, ========== to ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a TestBrowserDialog case for showing the DevModeBubble with Views on Mac. These need to be added for the other bubble types, and for the Views browser on on other platforms. BUG=654128, 654126, 654121 ==========
Description was changed from ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a TestBrowserDialog case for showing the DevModeBubble with Views on Mac. These need to be added for the other bubble types, and for the Views browser on on other platforms. BUG=654128, 654126, 654121 ========== to ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a ToolbarActionsBarBubbleViewsPresenter for showing the toolkit-views bubble on a Cocoa browser. It observes the bubble and notifies the BrowserActionsController when it closes. One issue: the Cocoa bubble uses a `BOOL anchoredToAction;` data member whereas the Views bubble inspects the anchor_view's ID. The latter doesn't work for a Cocoa browser, so make these consistent by adding the same data member to the views bubble. Adds a TestBrowserDialog case for showing the DevModeBubble with Views on Mac. These need to be added for the other bubble types, and for the Views browser on on other platforms. Screenshot at http://crbug.com/654128#c5 (it's currently too wide under Harmony, but there are framework fixes coming for that). BUG=654128, 654126, 654121 ==========
The CQ bit was checked by tapted@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: linux_chromium_chromeos_rel_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 tapted@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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #9 (id:160001) has been deleted
The CQ bit was checked by tapted@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: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a ToolbarActionsBarBubbleViewsPresenter for showing the toolkit-views bubble on a Cocoa browser. It observes the bubble and notifies the BrowserActionsController when it closes. One issue: the Cocoa bubble uses a `BOOL anchoredToAction;` data member whereas the Views bubble inspects the anchor_view's ID. The latter doesn't work for a Cocoa browser, so make these consistent by adding the same data member to the views bubble. Adds a TestBrowserDialog case for showing the DevModeBubble with Views on Mac. These need to be added for the other bubble types, and for the Views browser on on other platforms. Screenshot at http://crbug.com/654128#c5 (it's currently too wide under Harmony, but there are framework fixes coming for that). BUG=654128, 654126, 654121 ========== to ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a ToolbarActionsBarBubbleViewsPresenter for showing the toolkit-views bubble on a Cocoa browser. It observes the bubble and notifies the BrowserActionsController when it closes. One issue: the Cocoa bubble uses a `BOOL anchoredToAction;` data member whereas the Views bubble inspects the anchor_view's ID. The latter doesn't work for a Cocoa browser, so make these consistent by adding the same data member to the views bubble. Adds a TestBrowserDialog case for showing the DevModeBubble with Views on all desktop platforms. These need to be added for the other bubble types. Brings up all the views-based toolbar action bubble tests on Mac by factoring out two functions that depend on whether the Browser window is Cocoa or toolkit-views. Screenshot at http://crbug.com/654128#c5 (it's currently too wide under Harmony, but there are framework fixes coming for that). BUG=654128, 654126, 654121 ==========
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:80001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
The CQ bit was checked by tapted@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 ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a ToolbarActionsBarBubbleViewsPresenter for showing the toolkit-views bubble on a Cocoa browser. It observes the bubble and notifies the BrowserActionsController when it closes. One issue: the Cocoa bubble uses a `BOOL anchoredToAction;` data member whereas the Views bubble inspects the anchor_view's ID. The latter doesn't work for a Cocoa browser, so make these consistent by adding the same data member to the views bubble. Adds a TestBrowserDialog case for showing the DevModeBubble with Views on all desktop platforms. These need to be added for the other bubble types. Brings up all the views-based toolbar action bubble tests on Mac by factoring out two functions that depend on whether the Browser window is Cocoa or toolkit-views. Screenshot at http://crbug.com/654128#c5 (it's currently too wide under Harmony, but there are framework fixes coming for that). BUG=654128, 654126, 654121 ========== to ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a ToolbarActionsBarBubbleViewsPresenter for showing the toolkit-views bubble on a Cocoa browser. It observes the Views bubble and notifies the Cocoa BrowserActionsController when it closes. One issue: the Cocoa bubble uses a `BOOL anchoredToAction;` data member whereas the Views bubble inspects the anchor_view's ID. The latter doesn't work for a Cocoa browser, so make these consistent by adding the same data member to the views bubble. Adds a TestBrowserDialog case for showing the DevModeBubble with Views on all desktop platforms. These need to be added for the other bubble types. Brings up all the views-based toolbar action bubble tests on Mac by factoring out two functions that depend on whether the Browser window is Cocoa or toolkit-views. Screenshot at http://crbug.com/654128#c5 (it's currently too wide under Harmony, but there are framework fixes coming for that). BUG=654128, 654126, 654121 ==========
Description was changed from ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a ToolbarActionsBarBubbleViewsPresenter for showing the toolkit-views bubble on a Cocoa browser. It observes the Views bubble and notifies the Cocoa BrowserActionsController when it closes. One issue: the Cocoa bubble uses a `BOOL anchoredToAction;` data member whereas the Views bubble inspects the anchor_view's ID. The latter doesn't work for a Cocoa browser, so make these consistent by adding the same data member to the views bubble. Adds a TestBrowserDialog case for showing the DevModeBubble with Views on all desktop platforms. These need to be added for the other bubble types. Brings up all the views-based toolbar action bubble tests on Mac by factoring out two functions that depend on whether the Browser window is Cocoa or toolkit-views. Screenshot at http://crbug.com/654128#c5 (it's currently too wide under Harmony, but there are framework fixes coming for that). BUG=654128, 654126, 654121 ========== to ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a ToolbarActionsBarBubbleViewsPresenter for showing the toolkit-views bubble on a Cocoa browser. It observes the Views bubble and notifies the Cocoa BrowserActionsController when it closes. One issue: the Cocoa bubble uses a `BOOL anchoredToAction;` data member whereas the Views bubble inspects the anchor_view's ID. The latter doesn't work for a Cocoa browser, so make these consistent by adding the same data member to the views bubble. Adds a TestBrowserDialog case for showing the DevModeBubble with Views on all desktop platforms. These need to be added for the other bubble types. Brings up all the views-based toolbar action bubble tests on Mac by factoring out two functions that depend on whether the Browser window is Cocoa or toolkit-views. Screenshot at http://crbug.com/654128#c5 (it's currently too wide under Harmony, but there are framework fixes coming for that). The interactive bubble test can be invoked with `browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=ExtensionMessageBubbleViewBrowserTest.InvokeDialog_devmode` BUG=654128, 654126, 654121 ==========
Description was changed from ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a ToolbarActionsBarBubbleViewsPresenter for showing the toolkit-views bubble on a Cocoa browser. It observes the Views bubble and notifies the Cocoa BrowserActionsController when it closes. One issue: the Cocoa bubble uses a `BOOL anchoredToAction;` data member whereas the Views bubble inspects the anchor_view's ID. The latter doesn't work for a Cocoa browser, so make these consistent by adding the same data member to the views bubble. Adds a TestBrowserDialog case for showing the DevModeBubble with Views on all desktop platforms. These need to be added for the other bubble types. Brings up all the views-based toolbar action bubble tests on Mac by factoring out two functions that depend on whether the Browser window is Cocoa or toolkit-views. Screenshot at http://crbug.com/654128#c5 (it's currently too wide under Harmony, but there are framework fixes coming for that). The interactive bubble test can be invoked with `browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=ExtensionMessageBubbleViewBrowserTest.InvokeDialog_devmode` BUG=654128, 654126, 654121 ========== to ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a ToolbarActionsBarBubbleViewsPresenter for showing the toolkit-views bubble on a Cocoa browser. It observes the Views bubble and notifies the Cocoa BrowserActionsController when it closes. One issue: the Cocoa bubble uses a `BOOL anchoredToAction;` data member whereas the Views bubble inspects the anchor_view's ID. The latter doesn't work for a Cocoa browser, so make these consistent by adding the same data member to the views bubble. Adds a TestBrowserDialog case for showing the DevModeBubble with Views on all desktop platforms. These need to be added for the other bubble types. Brings up all the views-based toolbar action bubble tests on Mac by factoring out two functions that depend on whether the Browser window is Cocoa or toolkit-views. Screenshot at http://crbug.com/654128#c5 (it's currently too wide under Harmony, but there are framework fixes coming for that). The interactive bubble test can be invoked with `browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=ExtensionMessageBubbleViewBrowserTest.InvokeDialog_devmode` BUG=654128, 654126, 654121 ==========
Description was changed from ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a ToolbarActionsBarBubbleViewsPresenter for showing the toolkit-views bubble on a Cocoa browser. It observes the Views bubble and notifies the Cocoa BrowserActionsController when it closes. One issue: the Cocoa bubble uses a `BOOL anchoredToAction;` data member whereas the Views bubble inspects the anchor_view's ID. The latter doesn't work for a Cocoa browser, so make these consistent by adding the same data member to the views bubble. Adds a TestBrowserDialog case for showing the DevModeBubble with Views on all desktop platforms. These need to be added for the other bubble types. Brings up all the views-based toolbar action bubble tests on Mac by factoring out two functions that depend on whether the Browser window is Cocoa or toolkit-views. Screenshot at http://crbug.com/654128#c5 (it's currently too wide under Harmony, but there are framework fixes coming for that). The interactive bubble test can be invoked with `browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=ExtensionMessageBubbleViewBrowserTest.InvokeDialog_devmode` BUG=654128, 654126, 654121 ========== to ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a ToolbarActionsBarBubbleViewsPresenter for showing the toolkit-views bubble on a Cocoa browser. It observes the Views bubble and notifies the Cocoa BrowserActionsController when it closes. One issue: the Cocoa bubble uses a `BOOL anchoredToAction;` data member whereas the Views bubble inspects the anchor_view's ID. The latter doesn't work for a Cocoa browser, so make these consistent by adding the same data member to the views bubble. Adds a TestBrowserDialog case for showing the DevModeBubble with Views on all desktop platforms. These need to be added for the other bubble types. Brings up all the views-based toolbar action bubble tests on Mac by factoring out two functions that depend on whether the Browser window is Cocoa or toolkit-views. Screenshot at http://crbug.com/654128#c5 (it's currently too wide under Harmony on Mac, but there are framework fixes coming for that). The interactive bubble test can be invoked with `browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=ExtensionMessageBubbleViewBrowserTest.InvokeDialog_devmode` BUG=654128, 654126, 654121 ==========
tapted@chromium.org changed reviewers: + ellyjones@chromium.org
Hi Devlin, please take a look overall, for all the toolbar-y stuff +ellyjones: please take a look at the Cocoa stuff
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_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 tapted@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm :) https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_views_presenter.h (right): https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_views_presenter.h:37: bool anchored_to_action_view); what is an action view in this context? https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_views_presenter.h:46: BrowserActionsController* owner_; who owns these? the views hierarchy?
Nifty! A few small nits + questions for my own edification. https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right): https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:826: // No need to set |activeBubble_| as it's only used for Cocoa tests. Maybe also comment that bubbleWindowClosing will be called by the presenter, since we don't register for a notification of it anymore? https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_views_presenter.mm (right): https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_views_presenter.mm:22: if (active_bubble_) When can this happen? https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc (right): https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc:43: : public SupportsTestDialog<ExtensionMessageBubbleBrowserTest> { Are bubbles typically considered dialogs? https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc:86: command_line->AppendSwitch(switches::kExtendMdToSecondaryUi); Does this have any affect on non-mac platforms? https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest_nonmac.cc (right): https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest_nonmac.cc:6: #include "chrome/browser/ui/toolbar/toolbar_actions_bar.h" Do we need all these includes?
The CQ bit was checked by tapted@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 checked by tapted@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/2630473003/diff/260001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right): https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:826: // No need to set |activeBubble_| as it's only used for Cocoa tests. On 2017/03/09 02:49:48, Devlin wrote: > Maybe also comment that bubbleWindowClosing will be called by the presenter, > since we don't register for a notification of it anymore? Done. https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_views_presenter.h (right): https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_views_presenter.h:37: bool anchored_to_action_view); On 2017/03/08 16:33:44, Elly Fong-Jones wrote: > what is an action view in this context? Updated comment: // Presents |bubble| attached to the provided browser |parent_window| at // |point_in_screen|. |anchored_to_action_view| indicates that the anchor is // a specific browser action view, rather than something more general. https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_views_presenter.h:46: BrowserActionsController* owner_; On 2017/03/08 16:33:45, Elly Fong-Jones wrote: > who owns these? the views hierarchy? Added comments: BrowserActionsController* owner_; // Weak. Owns |this|. // Weak. Owns by its Widget (observed by |this|). ToolbarActionsBarBubbleViews* active_bubble_; The BrowserActionsController is actually owned by the toolbar or the app menu (they both have one) -- NSViewControllers. ToolbarActionsBarBubbleViews is a WidgetDelegate, so it drives its own lifetime. https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_views_presenter.mm (right): https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_views_presenter.mm:22: if (active_bubble_) On 2017/03/09 02:49:48, Devlin wrote: > When can this happen? Hm. good question. I don't think it can. Made a dcheck. https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc (right): https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc:43: : public SupportsTestDialog<ExtensionMessageBubbleBrowserTest> { On 2017/03/09 02:49:48, Devlin wrote: > Are bubbles typically considered dialogs? Yup - they all inherit from views::DialogDelegate. https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc:86: command_line->AppendSwitch(switches::kExtendMdToSecondaryUi); On 2017/03/09 02:49:48, Devlin wrote: > Does this have any affect on non-mac platforms? It does. It will affect layout, fonts, button styles and a few other appearance things, but shouldn't impact the tests really (also it's The Future :) https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest_nonmac.cc (right): https://codereview.chromium.org/2630473003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest_nonmac.cc:6: #include "chrome/browser/ui/toolbar/toolbar_actions_bar.h" On 2017/03/09 02:49:49, Devlin wrote: > Do we need all these includes? heh this one I'd assumed yes, since I moved it out of the other file. Looks safe to get rid of though. The others are needed. https://codereview.chromium.org/2630473003/diff/300001/chrome/browser/ui/test... File chrome/browser/ui/test/test_browser_dialog.cc (right): https://codereview.chromium.org/2630473003/diff/300001/chrome/browser/ui/test... chrome/browser/ui/test/test_browser_dialog.cc:118: base::EraseIf(added, [](views::Widget* widget) { (base::EraseIf just got added to base/stl_util.h, might as well use it)
lgtm https://codereview.chromium.org/2630473003/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_views_presenter.mm (right): https://codereview.chromium.org/2630473003/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_views_presenter.mm:22: DCHECK(!active_bubble_); nit: maybe DCHECK(!active_bubble_) << "|active_bubble_| should have been cleared by OnWidgetDestroying()"; ? https://codereview.chromium.org/2630473003/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_views_presenter.mm:32: nullptr, views_point, anchored_to_action_view, std::move(delegate)); nit: document what nullptr is https://codereview.chromium.org/2630473003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc (right): https://codereview.chromium.org/2630473003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc:85: // MD is required on Mac to get a Views bubble. Enable for all platforms. nit: maybe expand to // This shouldn't affect the behavior of the bubble, so enable for all platforms. https://codereview.chromium.org/2630473003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc:91: EXPECT_EQ("devmode", name); // TODO(tapted): Add cases for all bubbles types. how descriptive are these names supposed to be? "devmode" is pretty vague, "developer mode extensions warning bubble" is probably too specific, "dev mode extensions bubble" is somewhere in between. You know the naming convention best, so leaving it up to you. :)
The CQ bit was checked by tapted@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 ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a ToolbarActionsBarBubbleViewsPresenter for showing the toolkit-views bubble on a Cocoa browser. It observes the Views bubble and notifies the Cocoa BrowserActionsController when it closes. One issue: the Cocoa bubble uses a `BOOL anchoredToAction;` data member whereas the Views bubble inspects the anchor_view's ID. The latter doesn't work for a Cocoa browser, so make these consistent by adding the same data member to the views bubble. Adds a TestBrowserDialog case for showing the DevModeBubble with Views on all desktop platforms. These need to be added for the other bubble types. Brings up all the views-based toolbar action bubble tests on Mac by factoring out two functions that depend on whether the Browser window is Cocoa or toolkit-views. Screenshot at http://crbug.com/654128#c5 (it's currently too wide under Harmony on Mac, but there are framework fixes coming for that). The interactive bubble test can be invoked with `browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=ExtensionMessageBubbleViewBrowserTest.InvokeDialog_devmode` BUG=654128, 654126, 654121 ========== to ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a ToolbarActionsBarBubbleViewsPresenter for showing the toolkit-views bubble on a Cocoa browser. It observes the Views bubble and notifies the Cocoa BrowserActionsController when it closes. One issue: the Cocoa bubble uses a `BOOL anchoredToAction;` data member whereas the Views bubble inspects the anchor_view's ID. The latter doesn't work for a Cocoa browser, so make these consistent by adding the same data member to the views bubble. Adds a TestBrowserDialog case for showing the DevModeBubble with Views on all desktop platforms. These need to be added for the other bubble types. Brings up all the views-based toolbar action bubble tests on Mac by factoring out two functions that depend on whether the Browser window is Cocoa or toolkit-views. Screenshot at http://crbug.com/654128#c5 (it's currently too wide under Harmony on Mac, but there are framework fixes coming for that). The interactive bubble test can be invoked with `browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=ExtensionMessageBubbleViewBrowserTest.InvokeDialog_devmode_warning` BUG=654128, 654126, 654121 ==========
tapted@chromium.org changed reviewers: + pkasting@chromium.org
+pkasting for chrome/browser/ui OWNERS
Missing OWNER reviewers for these files:
chrome/browser/ui/test/test_browser_dialog.cc
chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc
chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest_nonmac.cc
Please take a look - thanks!
https://codereview.chromium.org/2630473003/diff/300001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_views_presenter.mm
(right):
https://codereview.chromium.org/2630473003/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_views_presenter.mm:22:
DCHECK(!active_bubble_);
On 2017/03/09 16:18:51, Devlin wrote:
> nit: maybe
> DCHECK(!active_bubble_) << "|active_bubble_| should have been cleared by
> OnWidgetDestroying()";
> ?
Done.
https://codereview.chromium.org/2630473003/diff/300001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/extensions/toolbar_actions_bar_bubble_views_presenter.mm:32:
nullptr, views_point, anchored_to_action_view, std::move(delegate));
On 2017/03/09 16:18:51, Devlin wrote:
> nit: document what nullptr is
Done.
https://codereview.chromium.org/2630473003/diff/300001/chrome/browser/ui/view...
File
chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc
(right):
https://codereview.chromium.org/2630473003/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc:85:
// MD is required on Mac to get a Views bubble. Enable for all platforms.
On 2017/03/09 16:18:51, Devlin wrote:
> nit: maybe expand to
> // This shouldn't affect the behavior of the bubble, so enable for all
> platforms.
Done.
https://codereview.chromium.org/2630473003/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc:91:
EXPECT_EQ("devmode", name); // TODO(tapted): Add cases for all bubbles types.
On 2017/03/09 16:18:51, Devlin wrote:
> how descriptive are these names supposed to be? "devmode" is pretty vague,
> "developer mode extensions warning bubble" is probably too specific, "dev mode
> extensions bubble" is somewhere in between. You know the naming convention
> best, so leaving it up to you. :)
I went with "devmode_warning" and added a comment above the test case:
// BrowserDialogTest for the warning bubble that appears at startup when there
// are extensions installed in developer mode. Can be invoked interactively with
// --gtest_filter=BrowserDialogTest.Invoke.
IN_PROC_BROWSER_TEST_F(ExtensionMessageBubbleView
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2630473003/diff/320001/chrome/browser/ui/test... File chrome/browser/ui/test/test_browser_dialog.cc (right): https://codereview.chromium.org/2630473003/diff/320001/chrome/browser/ui/test... chrome/browser/ui/test/test_browser_dialog.cc:118: base::EraseIf(added, [](views::Widget* widget) { Nice use of this API that just went in! I think you may be the first user. https://codereview.chromium.org/2630473003/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc (right): https://codereview.chromium.org/2630473003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc:56: void CloseBubble(Browser* browser) override; Nit: Goes after CheckBubbleNative() (match base class declaration order) https://codereview.chromium.org/2630473003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc:97: block_close_ = true; Nit: Use base::AutoReset<> rather than manually toggling on and then back off https://codereview.chromium.org/2630473003/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest_nonmac.cc (right): https://codereview.chromium.org/2630473003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest_nonmac.cc:35: views::BubbleDialogDelegateView* bubble = container->active_bubble(); Nit: Can just inline below https://codereview.chromium.org/2630473003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest_nonmac.cc:39: DCHECK_GT(container->num_toolbar_actions(), 0u); Nit: Seems like this should be ASSERT_GT.
The CQ bit was checked by tapted@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/2630473003/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc (right): https://codereview.chromium.org/2630473003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc:56: void CloseBubble(Browser* browser) override; On 2017/03/10 08:29:54, Peter Kasting wrote: > Nit: Goes after CheckBubbleNative() (match base class declaration order) Done. https://codereview.chromium.org/2630473003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc:97: block_close_ = true; On 2017/03/10 08:29:54, Peter Kasting wrote: > Nit: Use base::AutoReset<> rather than manually toggling on and then back off Done. https://codereview.chromium.org/2630473003/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest_nonmac.cc (right): https://codereview.chromium.org/2630473003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest_nonmac.cc:35: views::BubbleDialogDelegateView* bubble = container->active_bubble(); On 2017/03/10 08:29:55, Peter Kasting wrote: > Nit: Can just inline below Done. https://codereview.chromium.org/2630473003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest_nonmac.cc:39: DCHECK_GT(container->num_toolbar_actions(), 0u); On 2017/03/10 08:29:55, Peter Kasting wrote: > Nit: Seems like this should be ASSERT_GT. Changed to EXPECT + a return (ASSERT requires the method to return void)
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ellyjones@chromium.org, rdevlin.cronin@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2630473003/#ps340001 (title: "respond to comments")
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": 340001, "attempt_start_ts": 1489385118903770,
"parent_rev": "dedb4c1fcf7a1f1b9d43c3ca4f5b73c10d04c5a5", "commit_rev":
"0400d0a14218e82b41e2be64fb3174e70352b4e7"}
Message was sent while issue was closed.
Description was changed from ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a ToolbarActionsBarBubbleViewsPresenter for showing the toolkit-views bubble on a Cocoa browser. It observes the Views bubble and notifies the Cocoa BrowserActionsController when it closes. One issue: the Cocoa bubble uses a `BOOL anchoredToAction;` data member whereas the Views bubble inspects the anchor_view's ID. The latter doesn't work for a Cocoa browser, so make these consistent by adding the same data member to the views bubble. Adds a TestBrowserDialog case for showing the DevModeBubble with Views on all desktop platforms. These need to be added for the other bubble types. Brings up all the views-based toolbar action bubble tests on Mac by factoring out two functions that depend on whether the Browser window is Cocoa or toolkit-views. Screenshot at http://crbug.com/654128#c5 (it's currently too wide under Harmony on Mac, but there are framework fixes coming for that). The interactive bubble test can be invoked with `browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=ExtensionMessageBubbleViewBrowserTest.InvokeDialog_devmode_warning` BUG=654128, 654126, 654121 ========== to ========== MacViews: Harmony for toolbar actions bubbles. These are used for - DevModeBubbleDelegate - NtpOverriddenBubbleDelegate - ProxyOverriddenBubbleDelegate - SettingsApiBubbleDelegate - SuspiciousExtensionBubbleDelegate - BlockedActionBubbleDelegate Adds a ToolbarActionsBarBubbleViewsPresenter for showing the toolkit-views bubble on a Cocoa browser. It observes the Views bubble and notifies the Cocoa BrowserActionsController when it closes. One issue: the Cocoa bubble uses a `BOOL anchoredToAction;` data member whereas the Views bubble inspects the anchor_view's ID. The latter doesn't work for a Cocoa browser, so make these consistent by adding the same data member to the views bubble. Adds a TestBrowserDialog case for showing the DevModeBubble with Views on all desktop platforms. These need to be added for the other bubble types. Brings up all the views-based toolbar action bubble tests on Mac by factoring out two functions that depend on whether the Browser window is Cocoa or toolkit-views. Screenshot at http://crbug.com/654128#c5 (it's currently too wide under Harmony on Mac, but there are framework fixes coming for that). The interactive bubble test can be invoked with `browser_tests --gtest_filter=BrowserDialogTest.Invoke --interactive --dialog=ExtensionMessageBubbleViewBrowserTest.InvokeDialog_devmode_warning` BUG=654128, 654126, 654121 Review-Url: https://codereview.chromium.org/2630473003 Cr-Commit-Position: refs/heads/master@{#456329} Committed: https://chromium.googlesource.com/chromium/src/+/0400d0a14218e82b41e2be64fb31... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:340001) as https://chromium.googlesource.com/chromium/src/+/0400d0a14218e82b41e2be64fb31... |
