Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1915123005:
Use normal bubble header components in ExtensionInstalledBubbleView. (Closed)
|
Created:
4 years, 8 months ago by Evan Stade Modified:
4 years, 7 months ago Reviewers:
msw CC:
chromium-reviews, chromium-apps-reviews_chromium.org, tfarina, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse normal bubble header components in ExtensionInstalledBubbleView.
BUG=601633
Committed: https://crrev.com/6f523398a2537664751d9dbb676b28ad1a081a82
Cr-Commit-Position: refs/heads/master@{#390262}
Patch Set 1 #
Total comments: 20
Patch Set 2 : msw review #
Total comments: 2
Patch Set 3 : remove a couple more includes #Messages
Total messages: 16 (6 generated)
estade@chromium.org changed reviewers: + msw@chromium.org
https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (left): https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:349: close_ = views::BubbleFrameView::CreateCloseButton(this); fyi: AppListDialogContainer will be the last external CreateCloseButton user. https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (right): https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:12: #include "chrome/browser/profiles/profile.h" nit: remove https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:14: #include "chrome/browser/ui/browser_window.h" nit: remove? https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:21: #include "chrome/browser/ui/views/location_bar/location_icon_view.h" nit: remove https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:37: #include "ui/gfx/render_text.h" nit: remove https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:38: #include "ui/gfx/text_elider.h" nit: remove https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:41: #include "ui/views/bubble/bubble_frame_view.h" nit: remove https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:42: #include "ui/views/controls/button/button.h" nit: remove https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:43: #include "ui/views/controls/button/image_button.h" nit: remove https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:44: #include "ui/views/controls/image_view.h" nit: remove https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:233: set_margins(gfx::Insets(0, insets.left(), insets.bottom(), insets.right())); nit: add a explanatory comment (why is this needed?) https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:241: // | | Heading [X] | optional nit: s/Heading/Title/, s/[X]/(x)/ https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:242: // | Icon | Info | q: Is this representation of the vertical layout of the icon still accurate? https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:266: layout->set_inside_border_insets(gfx::Insets( nit: add a explanatory comment (why is this needed?) https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:272: const gfx::FontList& font_list = rb.GetFontList(ui::ResourceBundle::BaseFont); nit: specifying the base font is not necessary for views::Label.
https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (right): https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:21: #include "chrome/browser/ui/views/location_bar/location_icon_view.h" On 2016/04/26 23:44:31, msw wrote: > nit: remove This one is needed. The rest have been removed. https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:233: set_margins(gfx::Insets(0, insets.left(), insets.bottom(), insets.right())); On 2016/04/26 23:44:32, msw wrote: > nit: add a explanatory comment (why is this needed?) Done. https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:241: // | | Heading [X] | On 2016/04/26 23:44:31, msw wrote: > optional nit: s/Heading/Title/, s/[X]/(x)/ Done. https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:242: // | Icon | Info | On 2016/04/26 23:44:32, msw wrote: > q: Is this representation of the vertical layout of the icon still accurate? It already wasn't accurate in the vertical dimension (icon is top-aligned, not centered). See LEADING alignment on L317. Updated the graphic. https://codereview.chromium.org/1915123005/diff/1/chrome/browser/ui/views/ext... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:266: layout->set_inside_border_insets(gfx::Insets( On 2016/04/26 23:44:31, msw wrote: > nit: add a explanatory comment (why is this needed?) Done.
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/1915123005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1915123005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with minor nit https://codereview.chromium.org/1915123005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (right): https://codereview.chromium.org/1915123005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:33: #include "ui/base/resource/resource_bundle.h" nit: remove
https://codereview.chromium.org/1915123005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc (right): https://codereview.chromium.org/1915123005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:33: #include "ui/base/resource/resource_bundle.h" On 2016/04/27 17:29:19, 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 msw@chromium.org Link to the patchset: https://codereview.chromium.org/1915123005/#ps40001 (title: "remove a couple more includes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1915123005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1915123005/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6f523398a2537664751d9dbb676b28ad1a081a82 Cr-Commit-Position: refs/heads/master@{#390262}
Message was sent while issue was closed.
Description was changed from ========== Use normal bubble header components in ExtensionInstalledBubbleView. BUG=601633 ========== to ========== Use normal bubble header components in ExtensionInstalledBubbleView. BUG=601633 Committed: https://crrev.com/6f523398a2537664751d9dbb676b28ad1a081a82 Cr-Commit-Position: refs/heads/master@{#390262} ========== |
