Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2693803002:
Fix position of permission bubbles anchored off the padlock. (Closed)
|
Created:
3 years, 10 months ago by tapted Modified:
3 years, 10 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix position of permission bubbles anchored off the padlock.
Allows permission request bubbles to share some of the anchoring logic
used by the site settings bubble.
Under non-MD/Harmony, this will shift the permissions request popup up
slightly so that the arrow points to the icon.
Under MD/Harmony, the bubble will be anchored to the bottom of the
location bar itself, with edges aligned.
BUG=682073
Review-Url: https://codereview.chromium.org/2693803002
Cr-Commit-Position: refs/heads/master@{#450268}
Committed: https://chromium.googlesource.com/chromium/src/+/6ef9d93c9b63ff3169f2c3c13e1ef2b4113ae1a1
Patch Set 1 #Patch Set 2 : Share some logic #
Total comments: 12
Patch Set 3 : respond to comments #Patch Set 4 : fix ternary if-else (+respond to comments) #
Messages
Total messages: 26 (18 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...
Description was changed from ========== Fix position of bubbles anchored off the padlock BUG=682073 ========== to ========== Fix position of permission bubbles anchored off the padlock. Allows permission request bubbles to share some of the anchoring logic used by the site settings bubble. Under non-MD/Harmony, this will shift the permissions request popup up slightly so that the arrow points to the icon. Under MD/Harmony, the bubble will be anchored to the bottom of the location bar itself, with edges aligned. BUG=682073 ==========
tapted@chromium.org changed reviewers: + pkasting@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Peter, please take a look
LGTM https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:1278: GetLocationBarView()->GetSecurityBubbleAnchorView(); Nit: Shorter to either use auto: auto* anchor_view = GetLocationBarView()->GetSecurityBubbleAnchorView(); Or inline: WebsiteSettingsPopupView::ShowPopup( GetLocationBarView()->GetSecurityBubbleAnchorView(), gfx::Rect(), profile, web_contents, virtual_url, security_info); https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:376: if (ui::MaterialDesignController::IsSecondaryUiMaterial()) Nit: Should we be using LayoutDelegate::Get()->IsHarmonyMode()? I feel like we should standardize on one or the other, I'm not sure if we have a pattern to follow. Since this is views code, using the views LayoutDelegate seems correct. OTOH, I was trying to kill that function. So I don't know what to do. https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:378: return location_icon_view()->GetImageView(); Nit: Fractionally simpler: return ui::MaterialDesignController::IsSecondaryUiMaterial() ? this : location_icon_view()->GetImageView(); https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:205: // the left side of the Omnibox, under the padlock. Nit: left side -> leading edge (so the comment is still correct for RTL) https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_prompt_impl_views.cc (right): https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_prompt_impl_views.cc:29: BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser_); Nit: This is only used if the conditional succeeds, and should probably be inside that conditional, or inlined into the statement within it.
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...
https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:1278: GetLocationBarView()->GetSecurityBubbleAnchorView(); On 2017/02/13 21:36:31, Peter Kasting wrote: > Nit: Shorter to either use auto: > auto* anchor_view = GetLocationBarView()->GetSecurityBubbleAnchorView(); > > Or inline: > WebsiteSettingsPopupView::ShowPopup( > GetLocationBarView()->GetSecurityBubbleAnchorView(), gfx::Rect(), profile, > web_contents, virtual_url, security_info); Done. https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:376: if (ui::MaterialDesignController::IsSecondaryUiMaterial()) On 2017/02/13 21:36:31, Peter Kasting wrote: > Nit: Should we be using LayoutDelegate::Get()->IsHarmonyMode()? > > I feel like we should standardize on one or the other, I'm not sure if we have a > pattern to follow. Since this is views code, using the views LayoutDelegate > seems correct. OTOH, I was trying to kill that function. So I don't know what > to do. hrm. I could go either way really, but I'm leaning towards ui::MaterialDesignController -- it's verbose, but can be used in more places (e.g. if we _had_ to pick one - that would be it [unless we move LayoutDelegate..]). And there is ui/views code in BubbleBorder that influences positioning that is already using ui::MaterialDesignController (or anon::UseMd() for brevity), so using the same thing here on the lead up to that positioning logic gives better consistency guarantees. https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:378: return location_icon_view()->GetImageView(); On 2017/02/13 21:36:31, Peter Kasting wrote: > Nit: Fractionally simpler: > > return ui::MaterialDesignController::IsSecondaryUiMaterial() > ? this > : location_icon_view()->GetImageView(); Done. https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.h (right): https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.h:205: // the left side of the Omnibox, under the padlock. On 2017/02/13 21:36:31, Peter Kasting wrote: > Nit: left side -> leading edge > > (so the comment is still correct for RTL) Done. https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permission_prompt_impl_views.cc (right): https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permission_prompt_impl_views.cc:29: BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser_); On 2017/02/13 21:36:31, Peter Kasting wrote: > Nit: This is only used if the conditional succeeds, and should probably be > inside that conditional, or inlined into the statement within it. Done. (inverted condition - added an early exit)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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...
https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:378: return location_icon_view()->GetImageView(); On 2017/02/14 04:00:27, tapted wrote: > On 2017/02/13 21:36:31, Peter Kasting wrote: > > Nit: Fractionally simpler: > > > > return ui::MaterialDesignController::IsSecondaryUiMaterial() > > ? this > > : location_icon_view()->GetImageView(); > > Done. Un-Done :) - compiler wouldn't let me do this -- the types either side of the `:` need to be the same (i.e. even though they both inherit from View*, the compiler doesn't know at that point that they will be upcast to View* and refuses to pick one of LocationBarView* or ImageView* arbitrarily to give type to the expression). Went back to `if` rather than introducing a local var or static_cast<>ing
https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:378: return location_icon_view()->GetImageView(); On 2017/02/14 04:27:08, tapted wrote: > On 2017/02/14 04:00:27, tapted wrote: > > On 2017/02/13 21:36:31, Peter Kasting wrote: > > > Nit: Fractionally simpler: > > > > > > return ui::MaterialDesignController::IsSecondaryUiMaterial() > > > ? this > > > : location_icon_view()->GetImageView(); > > > > Done. > > Un-Done :) - compiler wouldn't let me do this -- the types either side of the > `:` need to be the same (i.e. even though they both inherit from View*, the > compiler doesn't know at that point that they will be upcast to View* and > refuses to pick one of LocationBarView* or ImageView* arbitrarily to give type > to the expression). > > Went back to `if` rather than introducing a local var or static_cast<>ing I probably would have "static_cast<views::View*>(this)" the first arm, which should have made the second arm work without a cast, but if you're more comfortable with a conditional, that's OK too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/14 04:30:00, Peter Kasting wrote: > https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... > File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): > > https://codereview.chromium.org/2693803002/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/location_bar/location_bar_view.cc:378: return > location_icon_view()->GetImageView(); > On 2017/02/14 04:27:08, tapted wrote: > > On 2017/02/14 04:00:27, tapted wrote: > > > On 2017/02/13 21:36:31, Peter Kasting wrote: > > > > Nit: Fractionally simpler: > > > > > > > > return ui::MaterialDesignController::IsSecondaryUiMaterial() > > > > ? this > > > > : location_icon_view()->GetImageView(); > > > > > > Done. > > > > Un-Done :) - compiler wouldn't let me do this -- the types either side of the > > `:` need to be the same (i.e. even though they both inherit from View*, the > > compiler doesn't know at that point that they will be upcast to View* and > > refuses to pick one of LocationBarView* or ImageView* arbitrarily to give type > > to the expression). > > > > Went back to `if` rather than introducing a local var or static_cast<>ing > > I probably would have "static_cast<views::View*>(this)" the first arm, which > should have made the second arm work without a cast, but if you're more > comfortable with a conditional, that's OK too. yeah - for me static_casts carry an added suspicion that a downcast is happening (I still miss base::implicit_cast<>..)
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2693803002/#ps60001 (title: "fix ternary if-else (+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": 60001, "attempt_start_ts": 1487050077047900,
"parent_rev": "82dafff44d048b03c236469ed4f5d2c260a4e95c", "commit_rev":
"6ef9d93c9b63ff3169f2c3c13e1ef2b4113ae1a1"}
Message was sent while issue was closed.
Description was changed from ========== Fix position of permission bubbles anchored off the padlock. Allows permission request bubbles to share some of the anchoring logic used by the site settings bubble. Under non-MD/Harmony, this will shift the permissions request popup up slightly so that the arrow points to the icon. Under MD/Harmony, the bubble will be anchored to the bottom of the location bar itself, with edges aligned. BUG=682073 ========== to ========== Fix position of permission bubbles anchored off the padlock. Allows permission request bubbles to share some of the anchoring logic used by the site settings bubble. Under non-MD/Harmony, this will shift the permissions request popup up slightly so that the arrow points to the icon. Under MD/Harmony, the bubble will be anchored to the bottom of the location bar itself, with edges aligned. BUG=682073 Review-Url: https://codereview.chromium.org/2693803002 Cr-Commit-Position: refs/heads/master@{#450268} Committed: https://chromium.googlesource.com/chromium/src/+/6ef9d93c9b63ff3169f2c3c13e1e... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6ef9d93c9b63ff3169f2c3c13e1e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
