Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2705323004:
Anchor the first run bubble off the location bar icon's ImageView. (Closed)
|
Created:
3 years, 10 months ago by tapted Modified:
3 years, 10 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAnchor the first run bubble off the location bar icon's ImageView.
Currently anchors off the whole icon, which might be the centre of a
rectangular security chip.
Use LocationBarView::GetSecurityBubbleAnchorView(), same as other
bubbles. The FirstRunBubble constructor already offsets the anchor by
LOCATION_BAR_BUBBLE_ANCHOR_VERTICAL_INSET, like other bubbles anchoring
off the ImageView.
BUG=690361
Review-Url: https://codereview.chromium.org/2705323004
Cr-Commit-Position: refs/heads/master@{#452728}
Committed: https://chromium.googlesource.com/chromium/src/+/96752f23762f0ad4320bb4aa0737857b5fe582f3
Patch Set 1 #
Total comments: 2
Patch Set 2 : GetSecurityBubbleAnchorView #
Total comments: 3
Messages
Total messages: 24 (15 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 ========== Anchor the first run bubble off the location bar icon's ImageView. Currently anchors off the whole icon, which might be the centre of a rectangular security chip. Using the ImageView matches what happens in LocationBarView::GetSecurityBubbleAnchorView(). BUG=690361 ========== to ========== Anchor the first run bubble off the location bar icon's ImageView. Currently anchors off the whole icon, which might be the centre of a rectangular security chip. Using the ImageView matches what happens in LocationBarView::GetSecurityBubbleAnchorView() and the FirstRunBubble constructor already offsets the anchor by LOCATION_BAR_BUBBLE_ANCHOR_VERTICAL_INSET like other bubbles anchoring off the ImageView. BUG=690361 ==========
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.
Description was changed from ========== Anchor the first run bubble off the location bar icon's ImageView. Currently anchors off the whole icon, which might be the centre of a rectangular security chip. Using the ImageView matches what happens in LocationBarView::GetSecurityBubbleAnchorView() and the FirstRunBubble constructor already offsets the anchor by LOCATION_BAR_BUBBLE_ANCHOR_VERTICAL_INSET like other bubbles anchoring off the ImageView. BUG=690361 ========== to ========== Anchor the first run bubble off the location bar icon's ImageView. Currently anchors off the whole icon, which might be the centre of a rectangular security chip. Using the ImageView matches what happens in LocationBarView::GetSecurityBubbleAnchorView(). The FirstRunBubble constructor already offsets the anchor by LOCATION_BAR_BUBBLE_ANCHOR_VERTICAL_INSET, like other bubbles anchoring off the ImageView. BUG=690361 ==========
https://codereview.chromium.org/2705323004/diff/1/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2705323004/diff/1/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:856: FirstRunBubble::ShowBubble(browser, location_icon_view_->GetImageView()); It seems like this should just call GetSecurityBubbleAnchorView(). (If the name is now too specific, we could rename this, although I don't have any great name suggestions.) Otherwise it seems like we will need to fix this again for Harmony, to just duplicate that code.
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 ========== Anchor the first run bubble off the location bar icon's ImageView. Currently anchors off the whole icon, which might be the centre of a rectangular security chip. Using the ImageView matches what happens in LocationBarView::GetSecurityBubbleAnchorView(). The FirstRunBubble constructor already offsets the anchor by LOCATION_BAR_BUBBLE_ANCHOR_VERTICAL_INSET, like other bubbles anchoring off the ImageView. BUG=690361 ========== to ========== Anchor the first run bubble off the location bar icon's ImageView. Currently anchors off the whole icon, which might be the centre of a rectangular security chip. Use LocationBarView::GetSecurityBubbleAnchorView(), same as other bubbles. The FirstRunBubble constructor already offsets the anchor by LOCATION_BAR_BUBBLE_ANCHOR_VERTICAL_INSET, like other bubbles anchoring off the ImageView. BUG=690361 ==========
https://codereview.chromium.org/2705323004/diff/1/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2705323004/diff/1/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:856: FirstRunBubble::ShowBubble(browser, location_icon_view_->GetImageView()); On 2017/02/23 22:06:03, Peter Kasting wrote: > It seems like this should just call GetSecurityBubbleAnchorView(). (If the name > is now too specific, we could rename this, although I don't have any great name > suggestions.) > > Otherwise it seems like we will need to fix this again for Harmony, to just > duplicate that code. Done. I had debated this and couldn't decide :). We don't have mocks for the first run bubble. The bubble says "you can search from here with Google" and points to a magnifying glass. Seems weird to have a bubble that says "look here" and doesn't have an arrow on it, but the way BubbleFrameView is set up under harmony, the arrow is removed regardless of where we anchor it (and I guess it doesn't look too bad).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
https://codereview.chromium.org/2705323004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2705323004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:856: FirstRunBubble::ShowBubble(browser, GetSecurityBubbleAnchorView()); (Still wondering if maybe this should be renamed to GetLeadingBubbleAnchorView() or something?)
https://codereview.chromium.org/2705323004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2705323004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:856: FirstRunBubble::ShowBubble(browser, GetSecurityBubbleAnchorView()); On 2017/02/24 01:55:54, Peter Kasting wrote: > (Still wondering if maybe this should be renamed to GetLeadingBubbleAnchorView() > or something?) If it's OK, I might defer the rename.. Someone might want this merged to m57 (same as r449119). Also our designer might decide the first run bubble needs an arrow.. (Plus, how would we pronounce "leading"? :)
https://codereview.chromium.org/2705323004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/2705323004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:856: FirstRunBubble::ShowBubble(browser, GetSecurityBubbleAnchorView()); On 2017/02/24 02:01:24, tapted wrote: > (Plus, how would we pronounce "leading"? :) Ha! I'm pretty sure THIS one, at least, I know how to pronounce.
The CQ bit was checked by tapted@chromium.org
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": 20001, "attempt_start_ts": 1487905179910860,
"parent_rev": "8c3875d433e85fc3868d5aa1f0350c0127175973", "commit_rev":
"96752f23762f0ad4320bb4aa0737857b5fe582f3"}
Message was sent while issue was closed.
Description was changed from ========== Anchor the first run bubble off the location bar icon's ImageView. Currently anchors off the whole icon, which might be the centre of a rectangular security chip. Use LocationBarView::GetSecurityBubbleAnchorView(), same as other bubbles. The FirstRunBubble constructor already offsets the anchor by LOCATION_BAR_BUBBLE_ANCHOR_VERTICAL_INSET, like other bubbles anchoring off the ImageView. BUG=690361 ========== to ========== Anchor the first run bubble off the location bar icon's ImageView. Currently anchors off the whole icon, which might be the centre of a rectangular security chip. Use LocationBarView::GetSecurityBubbleAnchorView(), same as other bubbles. The FirstRunBubble constructor already offsets the anchor by LOCATION_BAR_BUBBLE_ANCHOR_VERTICAL_INSET, like other bubbles anchoring off the ImageView. BUG=690361 Review-Url: https://codereview.chromium.org/2705323004 Cr-Commit-Position: refs/heads/master@{#452728} Committed: https://chromium.googlesource.com/chromium/src/+/96752f23762f0ad4320bb4aa0737... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/96752f23762f0ad4320bb4aa0737... |
