Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
291006:
Control Click to all bookmarks in folder in background tabs. This currently w... (Closed)
|
Created:
11 years, 2 months ago by pierre.lafayette Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com, ben+cc_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionOpen all bookmarks in a bookmark menu folder according to the window disposition derived from the event flags. This is currently only possible with a middle click on the top level bookmark bar button.
BUG=19597
TEST=Control click a folder on your bookmark bar.
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 7
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 3
Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 5
Patch Set 8 : '' #
Total comments: 2
Patch Set 9 : '' #
Messages
Total messages: 21 (0 generated)
Does this work for nested folders too? For example, can I click on a folder on the bookmark bar to open a folder, then control click on another folder in that menu to open all it's bookmarks?
http://codereview.chromium.org/291006/diff/5001/5002 File chrome/browser/views/bookmark_bar_view.cc (right): http://codereview.chromium.org/291006/diff/5001/5002#newcode233 Line 233: (e.IsOnlyLeftMouseButton() && e.IsControlDown()); Please use event_utils.cc:DispositionFromEventFlags() if possible. http://codereview.chromium.org/291006/diff/5001/5003 File views/controls/button/menu_button.cc (right): http://codereview.chromium.org/291006/diff/5001/5003#newcode195 Line 195: HitTest(e.location())) { Please use event_utils.cc:DispositionFromEventFlags() if possible.
Is there a bug for this work to be done on Mac? I assume we'd want this for command-click, no?
>Does this work for nested folders too? For example, can I click on a folder on the bookmark bar to open a folder, then control click on another folder in that menu to open all it's bookmarks? I've updated the patch to handle ctrl+click/middleclick on nested folders. http://codereview.chromium.org/291006/diff/5001/5002 File chrome/browser/views/bookmark_bar_view.cc (right): http://codereview.chromium.org/291006/diff/5001/5002#newcode233 Line 233: (e.IsOnlyLeftMouseButton() && e.IsControlDown()); If IsTriggerableEvent returns true it will allow CustomButton::OnMouseReleased to call NotifyClick() which in turn allows for ButtonPressed to be called on the listener, in this case BookmarkBarView::ButtonPressed. In that method, event_utils::DispositionFromEventFlags() is used to determine how to open the tabs. On 2009/10/19 18:52:49, Peter Kasting wrote: > Please use event_utils.cc:DispositionFromEventFlags() if possible. http://codereview.chromium.org/291006/diff/5001/5003 File views/controls/button/menu_button.cc (right): http://codereview.chromium.org/291006/diff/5001/5003#newcode195 Line 195: HitTest(e.location())) { We are not trying to open the tabs here, this is just to stop from showing the contents of the bookmark folder when control is down. In the else block, CustomButton::OnMouseReleased will notify the BookmarkBarView which will handle opening the tabs in BookmarkBarView::ButtonPressed; which makes use of DispositionFromEventFlags. On 2009/10/19 18:52:49, Peter Kasting wrote: > Please use event_utils.cc:DispositionFromEventFlags() if possible.
On 2009/10/20 15:55:55, pink wrote: > Is there a bug for this work to be done on Mac? I assume we'd want this for > command-click, no? I don't know if there is a bug on mac. I don't have access to a mac so I can't help on this one.
UI changes need to be kept in sync on all platforms. Please find someone to help and file associated bugs for Mac (and Linux if appropriate). On Tue, Oct 20, 2009 at 12:39 PM, <pierre.lafayette@gmail.com> wrote: > On 2009/10/20 15:55:55, pink wrote: >> >> Is there a bug for this work to be done on Mac? I assume we'd want this >> for >> command-click, no? > > I don't know if there is a bug on mac. I don't have access to a mac so I > can't > help on this one. > > http://codereview.chromium.org/291006 > -- Mike Pinkerton Mac Weenie pinkerton@google.com
http://codereview.chromium.org/291006/diff/5001/5003 File views/controls/button/menu_button.cc (right): http://codereview.chromium.org/291006/diff/5001/5003#newcode195 Line 195: HitTest(e.location())) { On 2009/10/20 16:14:38, pierre.lafayette wrote: > We are not trying to open the tabs here, this is just to stop from showing the > contents of the bookmark folder when control is down. I know, but the two places should use the same logic. For example, if one place takes action when the calculated disposition is A, B, or C, then the other place should block action on A, B, or C as needed. Your current code is brittle if in the future we change what all the disposition modifiers mean.
http://codereview.chromium.org/291006/diff/5001/5003 File views/controls/button/menu_button.cc (right): http://codereview.chromium.org/291006/diff/5001/5003#newcode195 Line 195: HitTest(e.location())) { Okay, what if we extract the the logic in the event_utils::DispositionFromEventFlags into another function in that namespace not unlike the IsPossibleDispositionEvent? Not sure what to call it though. On 2009/10/20 17:02:21, Peter Kasting wrote: > On 2009/10/20 16:14:38, pierre.lafayette wrote: > > We are not trying to open the tabs here, this is just to stop from showing the > > contents of the bookmark folder when control is down. > > I know, but the two places should use the same logic. For example, if one place > takes action when the calculated disposition is A, B, or C, then the other place > should block action on A, B, or C as needed. Your current code is brittle if in > the future we change what all the disposition modifiers mean.
Actually this bug was filled as OS=all so I guess I can try and fix it in the Mac code and get someone to test it. On 2009/10/20 16:45:16, pink wrote: > UI changes need to be kept in sync on all platforms. Please find > someone to help and file associated bugs for Mac (and Linux if > appropriate). > > On Tue, Oct 20, 2009 at 12:39 PM, <mailto:pierre.lafayette@gmail.com> wrote: > > On 2009/10/20 15:55:55, pink wrote: > >> > >> Is there a bug for this work to be done on Mac? I assume we'd want this > >> for > >> command-click, no? > > > > I don't know if there is a bug on mac. I don't have access to a mac so I > > can't > > help on this one. > > > > http://codereview.chromium.org/291006 > > > > > > -- > Mike Pinkerton > Mac Weenie > mailto:pinkerton@google.com
http://codereview.chromium.org/291006/diff/5001/5003 File views/controls/button/menu_button.cc (right): http://codereview.chromium.org/291006/diff/5001/5003#newcode195 Line 195: HitTest(e.location())) { On 2009/10/20 19:12:39, pierre.lafayette wrote: > Okay, what if we extract the the logic in the > event_utils::DispositionFromEventFlags into another function in that namespace > not unlike the IsPossibleDispositionEvent? Not sure what to call it though. I don't know that that's a lot better than WindowOpenDisposition disposition(DispositionFromEventFlags(e.flags); if ((disposition == CURRENT_TAB) || (disposition == NEW_BACKGROUND_TAB)) ...; (or whatever the right syntax would be) Besides, you still need the granularity of DispositionFromEventFlags(), so you couldn't extract its guts to somewhere.
Good point. I've updated the patch to use WindowOpenDisposition. I'll file a separate bug for Mac. On 2009/10/20 19:57:41, Peter Kasting wrote: > http://codereview.chromium.org/291006/diff/5001/5003 > File views/controls/button/menu_button.cc (right): > > http://codereview.chromium.org/291006/diff/5001/5003#newcode195 > Line 195: HitTest(e.location())) { > On 2009/10/20 19:12:39, pierre.lafayette wrote: > > Okay, what if we extract the the logic in the > > event_utils::DispositionFromEventFlags into another function in that namespace > > not unlike the IsPossibleDispositionEvent? Not sure what to call it though. > > I don't know that that's a lot better than > > WindowOpenDisposition disposition(DispositionFromEventFlags(e.flags); > if ((disposition == CURRENT_TAB) || (disposition == NEW_BACKGROUND_TAB)) > ...; > > (or whatever the right syntax would be) > > Besides, you still need the granularity of DispositionFromEventFlags(), so you > couldn't extract its guts to somewhere.
http://codereview.chromium.org/291006/diff/11009/15002 File views/controls/button/menu_button.cc (right): http://codereview.chromium.org/291006/diff/11009/15002#newcode10 Line 10: #include "chrome/browser/views/event_utils.h" views should not depend upon chrome. http://codereview.chromium.org/291006/diff/11009/15002#newcode195 Line 195: event_utils::DispositionFromEventFlags(e.GetFlags()); Use IsTriggerableEvent here and move the code into the subclass. http://codereview.chromium.org/291006/diff/11009/15003 File views/controls/menu/menu_controller.cc (right): http://codereview.chromium.org/291006/diff/11009/15003#newcode448 Line 448: event_utils::DispositionFromEventFlags(event.GetFlags())); This code should be moved into IsTriggerableEvent and not in views code.
Updated. On 2009/10/21 14:15:07, sky wrote: > http://codereview.chromium.org/291006/diff/11009/15002 > File views/controls/button/menu_button.cc (right): > > http://codereview.chromium.org/291006/diff/11009/15002#newcode10 > Line 10: #include "chrome/browser/views/event_utils.h" > views should not depend upon chrome. > > http://codereview.chromium.org/291006/diff/11009/15002#newcode195 > Line 195: event_utils::DispositionFromEventFlags(e.GetFlags()); > Use IsTriggerableEvent here and move the code into the subclass. > > http://codereview.chromium.org/291006/diff/11009/15003 > File views/controls/menu/menu_controller.cc (right): > > http://codereview.chromium.org/291006/diff/11009/15003#newcode448 > Line 448: event_utils::DispositionFromEventFlags(event.GetFlags())); > This code should be moved into IsTriggerableEvent and not in views code.
http://codereview.chromium.org/291006/diff/2005/2009 File chrome/browser/views/bookmark_bar_view.cc (right): http://codereview.chromium.org/291006/diff/2005/2009#newcode230 Line 230: WindowOpenDisposition disposition( I'm worried that if for some reason event_utils suddenly wanted something to happen on left/right click, this code would break. This should explicitly return false for left/right clicks then fall through to event_utils if not left/right. http://codereview.chromium.org/291006/diff/2005/2007 File views/controls/button/menu_button.cc (right): http://codereview.chromium.org/291006/diff/2005/2007#newcode193 Line 193: state() != BS_DISABLED && !canceled && !InDrag() && !IsTriggerableEvent(e) Did you verify this doesn't break any other overrides of IsTriggerableEvent? http://codereview.chromium.org/291006/diff/2005/2006 File views/controls/menu/menu_controller.cc (right): http://codereview.chromium.org/291006/diff/2005/2006#newcode451 Line 451: if (part.menu->GetDelegate()->IsTriggerableEvent(event) && Check !should_showFolder first. Please update BookmarkMenuController to ignore left/right clicks too.
Updated. http://codereview.chromium.org/291006/diff/2005/2007 File views/controls/button/menu_button.cc (right): http://codereview.chromium.org/291006/diff/2005/2007#newcode193 Line 193: state() != BS_DISABLED && !canceled && !InDrag() && !IsTriggerableEvent(e) On 2009/10/23 16:38:49, sky wrote: > Did you verify this doesn't break any other overrides of IsTriggerableEvent? I've tested for the MenuButtons in BookmarkManager, BlockedPopupContainer, BrowserActionContainer and ToolbarView. I haven't seen any issues. http://codereview.chromium.org/291006/diff/2005/2006 File views/controls/menu/menu_controller.cc (right): http://codereview.chromium.org/291006/diff/2005/2006#newcode451 Line 451: if (part.menu->GetDelegate()->IsTriggerableEvent(event) && On 2009/10/23 16:38:49, sky wrote: > Check !should_showFolder first. > Please update BookmarkMenuController to ignore left/right clicks too. Well BookmarkMenuController already ignores right clicks, but it shouldn't ignore left clicks as a left click on a bookmark should open the link. We need to ignore two cases: right clicks and left clicks on a bookmark folder.
http://codereview.chromium.org/291006/diff/20001/21004 File chrome/browser/views/bookmark_bar_view.cc (right): http://codereview.chromium.org/291006/diff/20001/21004#newcode232 Line 232: return false; Please add a comment here, and fix indenting. http://codereview.chromium.org/291006/diff/20001/21001 File views/controls/menu/menu_controller.cc (right): http://codereview.chromium.org/291006/diff/20001/21001#newcode455 Line 455: } This doesn't open the submenu immediately where as we previously did. We previously did as part.menu->HasSubmenu returned false and we fell through to the else case. You can see this with your patch by clicking on the 'other bookmarks' menu, dragging down, and releasing over a menu. With your patch it opens after a delay, without your patch it opens immediately.
Updated. Thanks. On 2009/10/26 23:03:58, sky wrote: > http://codereview.chromium.org/291006/diff/20001/21004 > File chrome/browser/views/bookmark_bar_view.cc (right): > > http://codereview.chromium.org/291006/diff/20001/21004#newcode232 > Line 232: return false; > Please add a comment here, and fix indenting. > > http://codereview.chromium.org/291006/diff/20001/21001 > File views/controls/menu/menu_controller.cc (right): > > http://codereview.chromium.org/291006/diff/20001/21001#newcode455 > Line 455: } > This doesn't open the submenu immediately where as we previously did. We > previously did as part.menu->HasSubmenu returned false and we fell through to > the else case. You can see this with your patch by clicking on the 'other > bookmarks' menu, dragging down, and releasing over a menu. With your patch it > opens after a delay, without your patch it opens immediately.
LGTM Thanks for your persistence!
On 2009/10/27 15:41:58, sky wrote: > LGTM > > Thanks for your persistence! No problem. I'll need you to land this one for me. This should be my 10th committed patch, if you would be so kind as to nominate me for commit access I would be greatly appreciative. Thanks.
Landed in r30281. -Scott On Tue, Oct 27, 2009 at 9:09 AM, <pierre.lafayette@gmail.com> wrote: > On 2009/10/27 15:41:58, sky wrote: >> >> LGTM > >> Thanks for your persistence! > > No problem. I'll need you to land this one for me. This should be my 10th > committed patch, if you would be so kind as to nominate me for commit access > I > would be greatly appreciative. > > Thanks. > > > http://codereview.chromium.org/291006 > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
