Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1807793003:
Convert ExtensionInstalledBubble to BubbleDialogDelegateView (Closed)
|
Created:
4 years, 9 months ago by Evan Stade Modified:
4 years, 8 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, tfarina, extensions-reviews_chromium.org, groby-ooo-7-16 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert ExtensionInstalledBubble to BubbleDialogDelegateView
Also do a bit of simplification. There are no longer omnibox-anchored bubbles AFAICT because extensions always get browser actions, which will serve as the anchor.
BUG=585312
Committed: https://crrev.com/09535511d38d67d3b28e3e79ef0bb9e26cb04b92
Cr-Commit-Position: refs/heads/master@{#386848}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : iwyu #
Total comments: 4
Patch Set 4 : restore omnibox anchor support #
Total comments: 14
Patch Set 5 : collapse header file into cc file #Patch Set 6 : circumvent crash #Patch Set 7 : . #Patch Set 8 : resolve unique_ptr conflict #Patch Set 9 : destruction #Patch Set 10 : proper fix #Patch Set 11 : . #Patch Set 12 : . #
Messages
Total messages: 43 (20 generated)
estade@chromium.org changed reviewers: + groby@chromium.org
+groby ping
Overall LG - but I thought this is WIP? On Wed, Apr 6, 2016 at 4:15 PM, <estade@chromium.org> wrote: > Reviewers: groby > CL: https://codereview.chromium.org/1807793003/ > > Message: > +groby ping > > Description: > WIP > > BUG= > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+51, -79 lines): > M chrome/browser/ui/views/extensions/extension_installed_bubble_view.h > M chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
yes, it's a WIP, but we discussed that it might be a good idea for you to see where this was going before I proceeded.
Ah, OK. It seemed like a good idea, so I simply said nothing :) (I'd still like to record the close reason at some point, but we might be able to just move that to the base class. So yeah, let's go ahead with this) On Thu, Apr 7, 2016 at 9:46 AM, <estade@chromium.org> wrote: > yes, it's a WIP, but we discussed that it might be a good idea for you to > see > where this was going before I proceeded. > > https://codereview.chromium.org/1807793003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== WIP BUG= ========== to ========== Convert ExtensionInstalledBubble to BubbleDialogDelegateView Also do a bit of simplification. There are no longer omnibox-anchored bubbles AFAICT because extensions always get browser actions, which will serve as the anchor. BUG=585312 ==========
estade@chromium.org changed reviewers: + rdevlin.cronin@chromium.org - groby@chromium.org
I don't think this line will ever be hit: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807793003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807793003/20001
On 2016/04/07 17:59:57, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1807793003/20001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1807793003/20001 I'd prefer to wait on the cleanup to remove support for anchor_omnibox for a little bit longer. The change to make all extensions have a browser action is relatively recent, and I don't think we're quite ready to get rid of the legacy implementation just yet. When we are, I'll go ahead and remove this (along with a *bunch* of other stuff). (Feel free to add TODO(devlin) if you like, but it's already on my radar.)
Otherwise, lgtm. https://codereview.chromium.org/1807793003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (right): https://codereview.chromium.org/1807793003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:338: // NB: This bubble is using the temporarily-deprecated bubble manager interface This is deprecated now? I thought it was just added. https://codereview.chromium.org/1807793003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:370: void ExtensionInstalledBubbleUi::Show(BubbleReference /*bubble_reference*/) { nit: is this part of the styleguide? I don't see it often.
estade@chromium.org changed reviewers: + msw@chromium.org
ok, restored the support for omnibox anchoring (still simpler than it used to be). +msw for OWNERS. https://codereview.chromium.org/1807793003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (right): https://codereview.chromium.org/1807793003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:338: // NB: This bubble is using the temporarily-deprecated bubble manager interface On 2016/04/07 19:02:51, Devlin wrote: > This is deprecated now? I thought it was just added. Yes, but it's already been de-staffed (at least temporarily) and it's not ready for wider usage. I discussed this at length with groby and hcarmona. https://codereview.chromium.org/1807793003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:370: void ExtensionInstalledBubbleUi::Show(BubbleReference /*bubble_reference*/) { On 2016/04/07 19:02:50, Devlin wrote: > nit: is this part of the styleguide? I don't see it often. yes, it's in the style guide but not commonly done in Chrome. I made this change to underline that the BubbleReference is not used in case people try to copy paste this code.
https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (left): https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:201: if (type_ == ExtensionInstalledBubble::OMNIBOX_KEYWORD) { These bubbles shouldn't point to the left omnibox edge anymore? https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:231: bool did_close = bubble_reference_->CloseBubble(BUBBLE_CLOSE_USER_DISMISSED); Was this implementing the close on esc behavior? Does it still work? I just realized that BubbleDialogDelegateView doesn't have close_on_esc, and wonder if some of the converted bubbles aren't closing on escape now... https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (right): https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:129: void ExtensionInstalledBubbleView::UpdateAnchorView() { nit q: Should this function also update the arrow? (may not really matter) https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.h (right): https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:11: #include "components/bubble/bubble_reference.h" nit: remove https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:20: class Extension; nit: remove
no changes uploaded yet, just responding to comments. https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (left): https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:201: if (type_ == ExtensionInstalledBubble::OMNIBOX_KEYWORD) { On 2016/04/07 21:48:57, msw wrote: > These bubbles shouldn't point to the left omnibox edge anymore? I accomplished this in a simpler way, see L163 https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:231: bool did_close = bubble_reference_->CloseBubble(BUBBLE_CLOSE_USER_DISMISSED); On 2016/04/07 21:48:57, msw wrote: > Was this implementing the close on esc behavior? Does it still work? I just > realized that BubbleDialogDelegateView doesn't have close_on_esc, and wonder if > some of the converted bubbles aren't closing on escape now... DCV handles close on escape. https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (right): https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:129: void ExtensionInstalledBubbleView::UpdateAnchorView() { On 2016/04/07 21:48:57, msw wrote: > nit q: Should this function also update the arrow? (may not really matter) I don't think that can change (because the controller_->type()/anchor_position() can't change) and it wasn't doing so before.
lgtm https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (left): https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:201: if (type_ == ExtensionInstalledBubble::OMNIBOX_KEYWORD) { On 2016/04/07 22:19:15, Evan Stade wrote: > On 2016/04/07 21:48:57, msw wrote: > > These bubbles shouldn't point to the left omnibox edge anymore? > > I accomplished this in a simpler way, see L163 Ah, I figured this really wanted the left edge, not the location icon on the left side; but it probably doesn't matter. https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:231: bool did_close = bubble_reference_->CloseBubble(BUBBLE_CLOSE_USER_DISMISSED); On 2016/04/07 22:19:15, Evan Stade wrote: > On 2016/04/07 21:48:57, msw wrote: > > Was this implementing the close on esc behavior? Does it still work? I just > > realized that BubbleDialogDelegateView doesn't have close_on_esc, and wonder > if > > some of the converted bubbles aren't closing on escape now... > > DCV handles close on escape. Acknowledged. https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (right): https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:129: void ExtensionInstalledBubbleView::UpdateAnchorView() { On 2016/04/07 22:19:15, Evan Stade wrote: > On 2016/04/07 21:48:57, msw wrote: > > nit q: Should this function also update the arrow? (may not really matter) > > I don't think that can change (because the controller_->type()/anchor_position() > can't change) and it wasn't doing so before. Acknowledged.
https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.h (right): https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. moved this whole file into the .cc https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:11: #include "components/bubble/bubble_reference.h" On 2016/04/07 21:48:58, msw wrote: > nit: remove Done. https://codereview.chromium.org/1807793003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.h:20: class Extension; On 2016/04/07 21:48:58, msw wrote: > nit: remove Done.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/1807793003/#ps80001 (title: "collapse header file into cc file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807793003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807793003/80001
The CQ bit was unchecked by estade@chromium.org
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807793003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807793003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/1807793003/#ps120001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807793003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807793003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/1807793003/#ps140001 (title: "resolve unique_ptr conflict")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807793003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807793003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/1807793003/#ps220001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1807793003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1807793003/220001
Message was sent while issue was closed.
Description was changed from ========== Convert ExtensionInstalledBubble to BubbleDialogDelegateView Also do a bit of simplification. There are no longer omnibox-anchored bubbles AFAICT because extensions always get browser actions, which will serve as the anchor. BUG=585312 ========== to ========== Convert ExtensionInstalledBubble to BubbleDialogDelegateView Also do a bit of simplification. There are no longer omnibox-anchored bubbles AFAICT because extensions always get browser actions, which will serve as the anchor. BUG=585312 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Convert ExtensionInstalledBubble to BubbleDialogDelegateView Also do a bit of simplification. There are no longer omnibox-anchored bubbles AFAICT because extensions always get browser actions, which will serve as the anchor. BUG=585312 ========== to ========== Convert ExtensionInstalledBubble to BubbleDialogDelegateView Also do a bit of simplification. There are no longer omnibox-anchored bubbles AFAICT because extensions always get browser actions, which will serve as the anchor. BUG=585312 Committed: https://crrev.com/09535511d38d67d3b28e3e79ef0bb9e26cb04b92 Cr-Commit-Position: refs/heads/master@{#386848} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/09535511d38d67d3b28e3e79ef0bb9e26cb04b92 Cr-Commit-Position: refs/heads/master@{#386848} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
