Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2041723002:
[Mac] Dismiss an open page info dialog when the location icon is clicked. (Closed)
Description[Mac] Dismiss an open page info dialog when the location icon is clicked.
On views platforms, clicking the location (lock) icon opens the page
info dialog. Clicking the icon again dismisses the dialog. Currently,
the same actions on Mac "refreshes" the dialog in place, with a strange
animation caused by the original dialog disappearing and a new one
taking its place immediately.
This CL makes the Cocoa and toolkit-views page info dialogs on Mac
behave consistently with views platforms, so that clicking the location
icon when the dialog is open dismisses the dialog.
BUG=59289
TEST=Open Chrome on Mac, and navigate to a page. Click on the lock
icon to open the page info dialog. Click on the icon again and the
dialog should dismiss, rather than refresh in place. The same
behaviour should apply to the toolkit-views and MacViews-WebUI
browsers.
Committed: https://crrev.com/77e33ef8f3837e7bad8794501cafd0d0268b988f
Cr-Commit-Position: refs/heads/master@{#398721}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address reviewer comments #
Total comments: 2
Patch Set 3 : Address nit #
Messages
Total messages: 21 (10 generated)
Description was changed from ========== [Mac] Hide the page info dialog when it is open and the location icon is clicked. On views platforms, clicking the location (lock) icon opens the page info dialog. Clicking the icon again dismisses the dialog. Currently, the same actions on Mac "refreshes" the dialog in place, with a strange animation caused by the original dialog disappearing and a new one taking its place immediately. This CL makes the Cocoa and toolkit-views page info dialogs behave consistently with views platforms, so that clicking the location icon when the dialog is open dismisses the dialog. BUG=609608 ========== to ========== [Mac] Dismiss an open page info dialog when the location icon is clicked. On views platforms, clicking the location (lock) icon opens the page info dialog. Clicking the icon again dismisses the dialog. Currently, the same actions on Mac "refreshes" the dialog in place, with a strange animation caused by the original dialog disappearing and a new one taking its place immediately. This CL makes the Cocoa and toolkit-views page info dialogs behave consistently with views platforms, so that clicking the location icon when the dialog is open dismisses the dialog. BUG=609608 ==========
dominickn@chromium.org changed reviewers: + tapted@chromium.org
tapted, WDYT? This patch is very similar to the existing way that views works. I'd like to follow up with a similar patch for content settings page action bubbles. It would be nice to have a consistent implementation of |is_popup_showing| between platforms, rather than a separate one in WebsiteSettingsPopupView (views) and WebsiteSettingsUIBridge (Mac) Unfortunately I think that would require a lot more plumbing; it would need to live in BrowserWindow since the platform-specific implementations are called in BrowserView (views) and BrowserWindowCocoa (Mac).
Can we have a test for Cocoa? Something like LocationIconViewTest.HideOnSecondClick I guess. In fact, you *should* be able to make that test cross-platform just by updating ui_test_utils::MoveMouseToCenterAndPress to use ClickOnView(browser, ViewID::VIEW_ID_LOCATION_ICON) But you'll probably need to force-enable toolkit-views dialogs so that the call to WebsiteSettingsPopupView::IsPopupShowing() works https://codereview.chromium.org/2041723002/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm (right): https://codereview.chromium.org/2041723002/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:65: bool is_popup_showing = false; g_is_popup_showing. Also move down to the end of the namespace - constants come first in a scope. https://codereview.chromium.org/2041723002/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:1108: is_popup_showing = false; why is this needed twice? Can we just tie the lifetime to WebsiteSettingsUIBridge? and only set = true in the constructor? (after DCHECKing that it is currently false?) https://codereview.chromium.org/2041723002/diff/1/chrome/browser/ui/views/bro... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/2041723002/diff/1/chrome/browser/ui/views/bro... chrome/browser/ui/views/browser_dialogs_views_mac.cc:31: // with the non-Mac views implementation. toolkit-views does something weird around this in LocationIconView - can you reference that here and explain why we can't just check this in ShowPopup()? (if we can just do it in ShowPopup, we should do that, so things actually are consistent :).
On 2016/06/06 05:06:37, tapted wrote: > Can we have a test for Cocoa? > > Something like LocationIconViewTest.HideOnSecondClick I guess. > > In fact, you *should* be able to make that test cross-platform just by updating > ui_test_utils::MoveMouseToCenterAndPress to use ClickOnView(browser, > ViewID::VIEW_ID_LOCATION_ICON) > > But you'll probably need to force-enable toolkit-views dialogs so that the call > to WebsiteSettingsPopupView::IsPopupShowing() works As mentioned offline, I don't think we can have a cross platform test here because the location icon is still Cocoa when toolkit-views is active. Hence VIEW_ID_LOCATION_ICON doesn't refer to anything when I try testing it, and we get a CHECK failure. Additionally, having a Cocoa test is very challenging because the location bar decorations aren't real views. They're very slippery to get a hold of for click testing - I tried a number of approaches (use the PageInfoBubblePoint(), manually triggering OnMousePress), but either they didn't work or didn't correctly trigger the close behaviour that we want to test. https://codereview.chromium.org/2041723002/diff/1/chrome/browser/ui/cocoa/web... File chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm (right): https://codereview.chromium.org/2041723002/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:65: bool is_popup_showing = false; On 2016/06/06 05:06:37, tapted wrote: > g_is_popup_showing. Also move down to the end of the namespace - constants come > first in a scope. Done. https://codereview.chromium.org/2041723002/diff/1/chrome/browser/ui/cocoa/web... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:1108: is_popup_showing = false; On 2016/06/06 05:06:37, tapted wrote: > why is this needed twice? Can we just tie the lifetime to > WebsiteSettingsUIBridge? and only set = true in the constructor? (after > DCHECKing that it is currently false?) Done. https://codereview.chromium.org/2041723002/diff/1/chrome/browser/ui/views/bro... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/2041723002/diff/1/chrome/browser/ui/views/bro... chrome/browser/ui/views/browser_dialogs_views_mac.cc:31: // with the non-Mac views implementation. On 2016/06/06 05:06:37, tapted wrote: > toolkit-views does something weird around this in LocationIconView - can you > reference that here and explain why we can't just check this in ShowPopup()? (if > we can just do it in ShowPopup, we should do that, so things actually are > consistent :). Done.
lgtm https://codereview.chromium.org/2041723002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/2041723002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:32: // This must be checked explicitly on views platforms because the bubble is 'views platforms' is a confusing concept now :). Perhaps something like Note that when the browser is toolkit-views, IsPopupShowing() is checked earlier because the popup is shown on mouse release (but dismissed on mouse pressed). A Cocoa browser does both on mouse pressed, so a check when showing is sufficient.
Description was changed from ========== [Mac] Dismiss an open page info dialog when the location icon is clicked. On views platforms, clicking the location (lock) icon opens the page info dialog. Clicking the icon again dismisses the dialog. Currently, the same actions on Mac "refreshes" the dialog in place, with a strange animation caused by the original dialog disappearing and a new one taking its place immediately. This CL makes the Cocoa and toolkit-views page info dialogs behave consistently with views platforms, so that clicking the location icon when the dialog is open dismisses the dialog. BUG=609608 ========== to ========== [Mac] Dismiss an open page info dialog when the location icon is clicked. On views platforms, clicking the location (lock) icon opens the page info dialog. Clicking the icon again dismisses the dialog. Currently, the same actions on Mac "refreshes" the dialog in place, with a strange animation caused by the original dialog disappearing and a new one taking its place immediately. This CL makes the Cocoa and toolkit-views page info dialogs behave consistently with views platforms, so that clicking the location icon when the dialog is open dismisses the dialog. BUG=609608 TEST=Open Chrome on Mac, and navigate to a page. Click on the lock icon to open the page info dialog. Click on the icon again and the dialog should dismiss, rather than refresh in place. The same behaviour should apply to the toolkit-views and MacViews-WebUI browsers. ==========
Thanks! +msw: please review views/browser_dialogs_views_mac.cc https://codereview.chromium.org/2041723002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right): https://codereview.chromium.org/2041723002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/browser_dialogs_views_mac.cc:32: // This must be checked explicitly on views platforms because the bubble is On 2016/06/08 06:54:57, tapted wrote: > 'views platforms' is a confusing concept now :). Perhaps something like > > Note that when the browser is toolkit-views, IsPopupShowing() is checked earlier > because the popup is shown on mouse release (but dismissed on mouse pressed). A > Cocoa browser does both on mouse pressed, so a check when showing is sufficient. Done.
dominickn@chromium.org changed reviewers: + msw@chromium.org
Actually +msw this time. PTAL at views/, thanks!
Description was changed from ========== [Mac] Dismiss an open page info dialog when the location icon is clicked. On views platforms, clicking the location (lock) icon opens the page info dialog. Clicking the icon again dismisses the dialog. Currently, the same actions on Mac "refreshes" the dialog in place, with a strange animation caused by the original dialog disappearing and a new one taking its place immediately. This CL makes the Cocoa and toolkit-views page info dialogs behave consistently with views platforms, so that clicking the location icon when the dialog is open dismisses the dialog. BUG=609608 TEST=Open Chrome on Mac, and navigate to a page. Click on the lock icon to open the page info dialog. Click on the icon again and the dialog should dismiss, rather than refresh in place. The same behaviour should apply to the toolkit-views and MacViews-WebUI browsers. ========== to ========== [Mac] Dismiss an open page info dialog when the location icon is clicked. On views platforms, clicking the location (lock) icon opens the page info dialog. Clicking the icon again dismisses the dialog. Currently, the same actions on Mac "refreshes" the dialog in place, with a strange animation caused by the original dialog disappearing and a new one taking its place immediately. This CL makes the Cocoa and toolkit-views page info dialogs on Mac behave consistently with views platforms, so that clicking the location icon when the dialog is open dismisses the dialog. BUG=609608 TEST=Open Chrome on Mac, and navigate to a page. Click on the lock icon to open the page info dialog. Click on the icon again and the dialog should dismiss, rather than refresh in place. The same behaviour should apply to the toolkit-views and MacViews-WebUI browsers. ==========
Description was changed from ========== [Mac] Dismiss an open page info dialog when the location icon is clicked. On views platforms, clicking the location (lock) icon opens the page info dialog. Clicking the icon again dismisses the dialog. Currently, the same actions on Mac "refreshes" the dialog in place, with a strange animation caused by the original dialog disappearing and a new one taking its place immediately. This CL makes the Cocoa and toolkit-views page info dialogs on Mac behave consistently with views platforms, so that clicking the location icon when the dialog is open dismisses the dialog. BUG=609608 TEST=Open Chrome on Mac, and navigate to a page. Click on the lock icon to open the page info dialog. Click on the icon again and the dialog should dismiss, rather than refresh in place. The same behaviour should apply to the toolkit-views and MacViews-WebUI browsers. ========== to ========== [Mac] Dismiss an open page info dialog when the location icon is clicked. On views platforms, clicking the location (lock) icon opens the page info dialog. Clicking the icon again dismisses the dialog. Currently, the same actions on Mac "refreshes" the dialog in place, with a strange animation caused by the original dialog disappearing and a new one taking its place immediately. This CL makes the Cocoa and toolkit-views page info dialogs on Mac behave consistently with views platforms, so that clicking the location icon when the dialog is open dismisses the dialog. BUG=59289 TEST=Open Chrome on Mac, and navigate to a page. Click on the lock icon to open the page info dialog. Click on the icon again and the dialog should dismiss, rather than refresh in place. The same behaviour should apply to the toolkit-views and MacViews-WebUI browsers. ==========
lgtm, but :( at globals and hackyness...
On 2016/06/08 20:02:02, msw wrote: > lgtm, but :( at globals and hackyness... Acknowledged. Perhaps at some point this should be redone on Mac and views with a WeakPtr or the like... :(
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2041723002/#ps40001 (title: "Address nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2041723002/40001
Message was sent while issue was closed.
Description was changed from ========== [Mac] Dismiss an open page info dialog when the location icon is clicked. On views platforms, clicking the location (lock) icon opens the page info dialog. Clicking the icon again dismisses the dialog. Currently, the same actions on Mac "refreshes" the dialog in place, with a strange animation caused by the original dialog disappearing and a new one taking its place immediately. This CL makes the Cocoa and toolkit-views page info dialogs on Mac behave consistently with views platforms, so that clicking the location icon when the dialog is open dismisses the dialog. BUG=59289 TEST=Open Chrome on Mac, and navigate to a page. Click on the lock icon to open the page info dialog. Click on the icon again and the dialog should dismiss, rather than refresh in place. The same behaviour should apply to the toolkit-views and MacViews-WebUI browsers. ========== to ========== [Mac] Dismiss an open page info dialog when the location icon is clicked. On views platforms, clicking the location (lock) icon opens the page info dialog. Clicking the icon again dismisses the dialog. Currently, the same actions on Mac "refreshes" the dialog in place, with a strange animation caused by the original dialog disappearing and a new one taking its place immediately. This CL makes the Cocoa and toolkit-views page info dialogs on Mac behave consistently with views platforms, so that clicking the location icon when the dialog is open dismisses the dialog. BUG=59289 TEST=Open Chrome on Mac, and navigate to a page. Click on the lock icon to open the page info dialog. Click on the icon again and the dialog should dismiss, rather than refresh in place. The same behaviour should apply to the toolkit-views and MacViews-WebUI browsers. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Mac] Dismiss an open page info dialog when the location icon is clicked. On views platforms, clicking the location (lock) icon opens the page info dialog. Clicking the icon again dismisses the dialog. Currently, the same actions on Mac "refreshes" the dialog in place, with a strange animation caused by the original dialog disappearing and a new one taking its place immediately. This CL makes the Cocoa and toolkit-views page info dialogs on Mac behave consistently with views platforms, so that clicking the location icon when the dialog is open dismisses the dialog. BUG=59289 TEST=Open Chrome on Mac, and navigate to a page. Click on the lock icon to open the page info dialog. Click on the icon again and the dialog should dismiss, rather than refresh in place. The same behaviour should apply to the toolkit-views and MacViews-WebUI browsers. ========== to ========== [Mac] Dismiss an open page info dialog when the location icon is clicked. On views platforms, clicking the location (lock) icon opens the page info dialog. Clicking the icon again dismisses the dialog. Currently, the same actions on Mac "refreshes" the dialog in place, with a strange animation caused by the original dialog disappearing and a new one taking its place immediately. This CL makes the Cocoa and toolkit-views page info dialogs on Mac behave consistently with views platforms, so that clicking the location icon when the dialog is open dismisses the dialog. BUG=59289 TEST=Open Chrome on Mac, and navigate to a page. Click on the lock icon to open the page info dialog. Click on the icon again and the dialog should dismiss, rather than refresh in place. The same behaviour should apply to the toolkit-views and MacViews-WebUI browsers. Committed: https://crrev.com/77e33ef8f3837e7bad8794501cafd0d0268b988f Cr-Commit-Position: refs/heads/master@{#398721} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/77e33ef8f3837e7bad8794501cafd0d0268b988f Cr-Commit-Position: refs/heads/master@{#398721} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
