Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2233293003:
Position Mac permission bubbles on the left when in fullscreen. (Closed)
DescriptionPosition Mac permission bubbles on the left when in fullscreen.
This means they will not obscure the fullscreen bubble, if it is
visible.
As part of this change, some code which forced the location bar
to be shown when a permission bubble was visible was removed.
This code isn't necessary anymore, as permission bubbles are
positioned appropriately when in fullscreen.
BUG=624296
Committed: https://crrev.com/cf7360dbc5ab679e20c5f03d2b7cf52b8cd7ee65
Cr-Commit-Position: refs/heads/master@{#411565}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix test #Patch Set 3 : Fix MacViews bubbles #
Total comments: 11
Patch Set 4 : Nits #
Messages
Total messages: 28 (16 generated)
Description was changed from ========== Position Mac permission bubbles on the left when in fullscreen. This means they will not obscure the fullscreen bubble, if it is visible. BUG=624496 ========== to ========== Position Mac permission bubbles on the left when in fullscreen. This means they will not obscure the fullscreen bubble, if it is visible. As part of this change, some code which forced the location bar to be shown when a permission bubble was visible was removed. This code isn't necessary anymore, as permission bubbles are positioned appropriately when in fullscreen. BUG=624496 ==========
karandeepb@chromium.org changed reviewers: + karandeepb@chromium.org, tapted@chromium.org
Oops, mailed out as Karan accidentally (was on the shared Mac). Karan, feel free to review, or not, up to you.
The CQ bit was checked by benwells@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...
lgtm, but I think your BUG= is the wrong bug. Also, without locking the toolbar, it might look weird when the toolbar drops down in response to moving the mouse to the top edge of the screen - but I don't think it's worth adding code to do something interesting when that happens. https://codereview.chromium.org/2233293003/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/2233293003/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:491: return info_bubble::kTopLeft; does this mean that there will be an arrow in fullscreen (pointing at.. something? I guess just the top of the screen). That's probably fine.
(oh, and it looks like there's a test to update :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Position Mac permission bubbles on the left when in fullscreen. This means they will not obscure the fullscreen bubble, if it is visible. As part of this change, some code which forced the location bar to be shown when a permission bubble was visible was removed. This code isn't necessary anymore, as permission bubbles are positioned appropriately when in fullscreen. BUG=624496 ========== to ========== Position Mac permission bubbles on the left when in fullscreen. This means they will not obscure the fullscreen bubble, if it is visible. As part of this change, some code which forced the location bar to be shown when a permission bubble was visible was removed. This code isn't necessary anymore, as permission bubbles are positioned appropriately when in fullscreen. BUG=624296 ==========
I tried the patch and the bubble did not position correctly in fullscreen for me. Also, nit: wrap the CL description.
Description was changed from ========== Position Mac permission bubbles on the left when in fullscreen. This means they will not obscure the fullscreen bubble, if it is visible. As part of this change, some code which forced the location bar to be shown when a permission bubble was visible was removed. This code isn't necessary anymore, as permission bubbles are positioned appropriately when in fullscreen. BUG=624296 ========== to ========== Position Mac permission bubbles on the left when in fullscreen. This means they will not obscure the fullscreen bubble, if it is visible. As part of this change, some code which forced the location bar to be shown when a permission bubble was visible was removed. This code isn't necessary anymore, as permission bubbles are positioned appropriately when in fullscreen. BUG=624296 ==========
The CQ bit was checked by benwells@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.
OK, I think this is needed after all. The fullscreen bubble can come up under permission bubbles, the z order of these seems to be based on when they are shown. I think Matt looked into it in depth and decided this is the best way forward, so I won't second guess that. This patch should have the mac views behavior fixed. https://codereview.chromium.org/2233293003/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/2233293003/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:491: return info_bubble::kTopLeft; On 2016/08/11 23:51:20, tapted wrote: > does this mean that there will be an arrow in fullscreen (pointing at.. > something? I guess just the top of the screen). That's probably fine. Yep, that's what it means. This matches the views behavior.
lgtm % nits https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h (right): https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h:47: + (NSInteger)getFullscreenLeftOffset; you can move this to `@interface PermissionBubbleController (ExposedForTesting)` in permission_bubble_controller_unittest.mm (and duplicate it in `PermissionBubbleController ()`, e.g., below onClose) https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:212: (j.e. here) https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:293: + (NSInteger)getFullscreenLeftOffset { nit: move to after onClose https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm (right): https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm:39: - (void)onCustomize:(id)sender; (and here). Although the obvious flaw is that `onCustomize` and `onCheckboxChanged` don't actually exist any more - I think they can be deleted https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm:377: // Expected anchor location will be top center when there's no location bar. nit: update comment https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm:382: frame.size.height); nit: `frame.size.height` should probably be NSMaxY.. (the height works, but in theory it wouldn't always be so in a dual-screen setup)
LGTM.
https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h (right): https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h:47: + (NSInteger)getFullscreenLeftOffset; On 2016/08/12 04:25:39, tapted (OOO soon) wrote: > you can move this to `@interface PermissionBubbleController (ExposedForTesting)` > in permission_bubble_controller_unittest.mm > > (and duplicate it in `PermissionBubbleController ()`, e.g., below onClose) Done. https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:293: + (NSInteger)getFullscreenLeftOffset { On 2016/08/12 04:25:39, tapted (OOO soon) wrote: > nit: move to after onClose Done. https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm (right): https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm:377: // Expected anchor location will be top center when there's no location bar. On 2016/08/12 04:25:39, tapted (OOO soon) wrote: > nit: update comment Done. https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm:377: // Expected anchor location will be top center when there's no location bar. On 2016/08/12 04:25:39, tapted (OOO soon) wrote: > nit: update comment Done. https://codereview.chromium.org/2233293003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller_unittest.mm:382: frame.size.height); On 2016/08/12 04:25:39, tapted (OOO soon) wrote: > nit: `frame.size.height` should probably be NSMaxY.. (the height works, but in > theory it wouldn't always be so in a dual-screen setup) Done.
The CQ bit was checked by benwells@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from karandeepb@chromium.org, tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2233293003/#ps60001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Position Mac permission bubbles on the left when in fullscreen. This means they will not obscure the fullscreen bubble, if it is visible. As part of this change, some code which forced the location bar to be shown when a permission bubble was visible was removed. This code isn't necessary anymore, as permission bubbles are positioned appropriately when in fullscreen. BUG=624296 ========== to ========== Position Mac permission bubbles on the left when in fullscreen. This means they will not obscure the fullscreen bubble, if it is visible. As part of this change, some code which forced the location bar to be shown when a permission bubble was visible was removed. This code isn't necessary anymore, as permission bubbles are positioned appropriately when in fullscreen. BUG=624296 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Position Mac permission bubbles on the left when in fullscreen. This means they will not obscure the fullscreen bubble, if it is visible. As part of this change, some code which forced the location bar to be shown when a permission bubble was visible was removed. This code isn't necessary anymore, as permission bubbles are positioned appropriately when in fullscreen. BUG=624296 ========== to ========== Position Mac permission bubbles on the left when in fullscreen. This means they will not obscure the fullscreen bubble, if it is visible. As part of this change, some code which forced the location bar to be shown when a permission bubble was visible was removed. This code isn't necessary anymore, as permission bubbles are positioned appropriately when in fullscreen. BUG=624296 Committed: https://crrev.com/cf7360dbc5ab679e20c5f03d2b7cf52b8cd7ee65 Cr-Commit-Position: refs/heads/master@{#411565} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/cf7360dbc5ab679e20c5f03d2b7cf52b8cd7ee65 Cr-Commit-Position: refs/heads/master@{#411565} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
