Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
765043002:
Bookmark pop-up doesn't open if Ctrl+D is set as keyboard shortcut for added extensions. (Closed)
|
Created:
6 years ago by Deepak Modified:
6 years ago Reviewers:
Mike Wittman, hajimehoshi, Avi (use Gerrit), Peter Kasting, sky, Finnur, Takashi Toyoshima, Nico, brettw, not at google - send to devlin CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionBookmark pop-up doesn't open if Ctrl+D is set as keyboard shortcut for added extensions.
Presently when we selects bookmark icon then it checks weather Ctrl+D has been set as overridden command for any extension if yes, then it executes extension popup for that extension.
Changes done so that when bookmarking a page is done via bookmark icon by mouse click then bookmark overridden commands should not be considered as overriding happened for keyboard shortcuts.
As user have overridden shortcut key for bookmark to launch extension pop up, not the bookmarking via selection of bookmark icon by mouse event.
TBR=sky@chromium.org
BUG=426791
Committed: https://crrev.com/54a7f399b0613cbae272fbe0ba97447be6517a99
Cr-Commit-Position: refs/heads/master@{#308312}
Patch Set 1 #Patch Set 2 : Addressing nit. #Patch Set 3 : Addressing nit. #
Total comments: 1
Patch Set 4 : Added comments for API's. #Patch Set 5 : Addressing nit. #Patch Set 6 : Addressing nit. #
Total comments: 2
Patch Set 7 : New Changes. #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #
Total comments: 7
Patch Set 12 : #
Total comments: 8
Patch Set 13 : Addressing nit. #Messages
Total messages: 79 (10 generated)
deepak.m1@samsung.com changed reviewers: + avi@chromium.org, pkasting@chromium.org
PTAL at my changes. Thanks.
Why don't you pay attention to out C++ coding stule? variables are named using unix_hacker style, not fooBar. Same for functions/methods. If it can be inlined in the header then it is my_simple_getter() or set_this_awesome_value(). Not someFuncName(). I have pointed out that to you in many other reviews. Having to review and point these style issues just wastes reviewers time. As they could have been focusing on checking the correctness of the logic. Please, address this and upload a new patch set. Thanks,
On 2014/11/28 12:44:46, tfarina wrote: > Why don't you pay attention to out C++ coding stule? > > variables are named using unix_hacker style, not fooBar. > > Same for functions/methods. If it can be inlined in the header then it is > my_simple_getter() or set_this_awesome_value(). > > Not someFuncName(). > > I have pointed out that to you in many other reviews. Having to review and point > these style issues just wastes reviewers time. As they could have been focusing > on checking the correctness of the logic. > > Please, address this and upload a new patch set. > > Thanks, I missed that, Done changes and uploaded again. PTAL.
In the description you write: > When we select the bookmark icon then it check overrided Accelerator > commands and execute, now for bookmark icon selection is handled > so that bookmark bubble will come instead of overridding bt extension > shortcut key. Please rewrite this in clear English; I currently can't make sense of it.
Bug 419601 was reported on the Mac, and you list it as being addressed by your patch, but your patch only touches Views code. How does it fix things on the Mac? https://codereview.chromium.org/765043002/diff/40001/chrome/browser/command_u... File chrome/browser/command_updater.h (right): https://codereview.chromium.org/765043002/diff/40001/chrome/browser/command_u... chrome/browser/command_updater.h:65: void set_bookmark_icon_selected(bool selection) { You need to have some comments explaining the meaning of this set of accessors and of the variable below.
On 2014/11/28 14:56:08, Avi wrote: > Bug 419601 was reported on the Mac, and you list it as being addressed by your > patch, but your patch only touches Views code. How does it fix things on the > Mac? > > https://codereview.chromium.org/765043002/diff/40001/chrome/browser/command_u... > File chrome/browser/command_updater.h (right): > > https://codereview.chromium.org/765043002/diff/40001/chrome/browser/command_u... > chrome/browser/command_updater.h:65: void set_bookmark_icon_selected(bool > selection) { > You need to have some comments explaining the meaning of this set of accessors > and of the variable below. @Avi, sorry for confusion, I have updated the comments for usage of API's, and have removed mac issue from this review. PTAL at my changes. Thanks
This seems wrong. I think this means that extensions that actually try to override the star button (e.g. the new bookmarking extension that was formerly called "Stars") can't do it. Instead I think we have to distinguish between when an extension is trying to override the bookmark system, versus when a user has set the keyboard shortcut of a random extension to ctrl-D. Please work with one of the folks who owns the bookmark extension override stuff to figure out the right way to do this, if you want to fix this bug.
On 2014/11/29 19:54:52, Peter Kasting wrote: > This seems wrong. I think this means that extensions that actually try to > override the star button (e.g. the new bookmarking extension that was formerly > called "Stars") can't do it. > > Instead I think we have to distinguish between when an extension is trying to > override the bookmark system, versus when a user has set the keyboard shortcut > of a random extension to ctrl-D. > > Please work with one of the folks who owns the bookmark extension override stuff > to figure out the right way to do this, if you want to fix this bug. Absolutely. Talk to the extension folks.
deepak.m1@samsung.com changed reviewers: + thakis@chromium.org, toyoshim@chromium.org
deepak.m1@samsung.com changed reviewers: + hajimehoshi@chromium.org
On 2014/11/30 21:48:45, Avi wrote: > On 2014/11/29 19:54:52, Peter Kasting wrote: > > This seems wrong. I think this means that extensions that actually try to > > override the star button (e.g. the new bookmarking extension that was formerly > > called "Stars") can't do it. > > > > Instead I think we have to distinguish between when an extension is trying to > > override the bookmark system, versus when a user has set the keyboard shortcut > > of a random extension to ctrl-D. > > > > Please work with one of the folks who owns the bookmark extension override > stuff > > to figure out the right way to do this, if you want to fix this bug. > > Absolutely. Talk to the extension folks. @Hajimehosi and @Toyoshim Can you please share your thoughts for this change. Thanks
On Mon, Dec 1, 2014 at 2:01 AM, <deepak.m1@samsung.com> wrote: > On 2014/11/30 21:48:45, Avi wrote: > >> On 2014/11/29 19:54:52, Peter Kasting wrote: >> > This seems wrong. I think this means that extensions that actually try >> to >> > override the star button (e.g. the new bookmarking extension that was >> > formerly > >> > called "Stars") can't do it. >> > >> > Instead I think we have to distinguish between when an extension is >> trying >> > to > >> > override the bookmark system, versus when a user has set the keyboard >> > shortcut > >> > of a random extension to ctrl-D. >> > >> > Please work with one of the folks who owns the bookmark extension >> override >> stuff >> > to figure out the right way to do this, if you want to fix this bug. >> > > Absolutely. Talk to the extension folks. >> > > @Hajimehosi and @Toyoshim > Can you please share your thoughts for this change. > How did you pick these reviewers? I think bookmark extensions reviewers are Benjamin Kalman and Mike Wittman. -- Thiago Farina To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/12/01 12:13:50, tfarina wrote: > On Mon, Dec 1, 2014 at 2:01 AM, <mailto:deepak.m1@samsung.com> wrote: > > > On 2014/11/30 21:48:45, Avi wrote: > > > >> On 2014/11/29 19:54:52, Peter Kasting wrote: > >> > This seems wrong. I think this means that extensions that actually try > >> to > >> > override the star button (e.g. the new bookmarking extension that was > >> > > formerly > > > >> > called "Stars") can't do it. > >> > > >> > Instead I think we have to distinguish between when an extension is > >> trying > >> > > to > > > >> > override the bookmark system, versus when a user has set the keyboard > >> > > shortcut > > > >> > of a random extension to ctrl-D. > >> > > >> > Please work with one of the folks who owns the bookmark extension > >> override > >> stuff > >> > to figure out the right way to do this, if you want to fix this bug. > >> > > > > Absolutely. Talk to the extension folks. > >> > > > > @Hajimehosi and @Toyoshim > > Can you please share your thoughts for this change. > > > How did you pick these reviewers? I think bookmark extensions reviewers are > Benjamin Kalman and Mike Wittman. > > -- > Thiago Farina > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I just saw git blame and people who have worked in bubble_icon_view.cc and put their name. I am sorry if I have chosen wrong people. I will add people mentioned by your also. Thanks.
deepak.m1@samsung.com changed reviewers: + kalman@chromium.org, wittman@chromium.org
@Kalman and @Wittman Can you please share your thoughts for this change. Thanks
See here: https://developer.chrome.com/extensions/ui_override one hopes that if an extension overrides the ctrl-d shortcut using remove_bookmark_shortcut then it would get first dibs at claiming it itself. I haven't actually read the code.
On 2014/12/01 23:22:02, kalman wrote: > See here: > > https://developer.chrome.com/extensions/ui_override > > one hopes that if an extension overrides the ctrl-d shortcut using > remove_bookmark_shortcut then it would get first dibs at claiming it itself. > > I haven't actually read the code. I think the relevant question is whether the patch as it stands would break some extensions, and how to design it so it doesn't, so I think you may have to read the code :) It looks to me as if the problem is that both ctrl-D and clicking the star simply call BookmarkCurrentPage(), so we don't know which type of event we're handling. That function in turn calls GetBookmarkOverrideCommand(), which doesn't distinguish between extensions trying to override the bookmarking system versus extensions which are simply trying to handle ctrl-D due to a user manually assigning that keypress (see bug). I don't even know whether the "manually assign shortcut to extension" functionality described in the bug report appears internally to be the same thing as if an extension is trying to register ctrl-D as a shortcut in its manifest. Can you work with the author (or on your own) to figure out what the right plumbing is to handle all combinations of (user action, manually-assigned shortcuts, manifest-registered shortcuts, "disable bookmark XYZ" prefs set)?
kalman@chromium.org changed reviewers: + finnur@chromium.org
Finnur is a better person to comment on this as he knows the commands API. Mike (Wittman) knows the bookmarking part. I'm just the poor person stuck in the middle :)
On 2014/12/02 00:02:18, kalman wrote: > Finnur is a better person to comment on this as he knows the commands API. Mike > (Wittman) knows the bookmarking part. I'm just the poor person stuck in the > middle :) @Finnur and @Wittman Can you please share your thoughts about this change. Thanks.
https://codereview.chromium.org/765043002/diff/100001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/765043002/diff/100001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:758: ->get_bookmark_icon_selected()) { Mike Wittman implemented the Ctrl+D override for extensions, so I think he should at least weigh in. My thinking is along the same lines as Peter's: This function doesn't know which type of event triggered it, which is key to determining whether to check the override or not. There are a few scenarios I can see: 1) A bookmarking override extension is registered for Ctrl+D. 2) A non-overriding extension has registered for Ctrl+D. 3) No extension has Ctrl+D registered. Then you have at least three ways the Star popup can be activated (all need to work). a) Press Ctrl+D. b) Mouse click on the Star. c) Shift+Alt+T, press Tab until Star has focus, press Space/Enter. Basically, scenarios b) and c) should lead to lines 748-772 not being executed. For Ctrl+D keypresses, the Star popup should show up for scenario 3. For scenarios 1) and 2), the extension should handle it as the extension chooses. https://codereview.chromium.org/765043002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right): https://codereview.chromium.org/765043002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/bubble_icon_view.cc:53: command_updater_) { This cannot possibly be correct. A cursory glance at how BubbleIconView is used in the code, shows that at least the ZoomView constructs it with a command_updater that is a nullptr. So, line 53 looks like it breaks the ZoomView (it won't execute the command anymore, as line 55 never gets called). But this whole patch feels like a big ugly hack that can be handled in a different way and more elegantly.
On 2014/12/03 10:49:18, Finnur wrote: > https://codereview.chromium.org/765043002/diff/100001/chrome/browser/ui/brows... > File chrome/browser/ui/browser_commands.cc (right): > > https://codereview.chromium.org/765043002/diff/100001/chrome/browser/ui/brows... > chrome/browser/ui/browser_commands.cc:758: ->get_bookmark_icon_selected()) { > Mike Wittman implemented the Ctrl+D override for extensions, so I think he > should at least weigh in. My thinking is along the same lines as Peter's: This > function doesn't know which type of event triggered it, which is key to > determining whether to check the override or not. > > There are a few scenarios I can see: > > 1) A bookmarking override extension is registered for Ctrl+D. > 2) A non-overriding extension has registered for Ctrl+D. > 3) No extension has Ctrl+D registered. > > Then you have at least three ways the Star popup can be activated (all need to > work). > > a) Press Ctrl+D. > b) Mouse click on the Star. > c) Shift+Alt+T, press Tab until Star has focus, press Space/Enter. > > Basically, scenarios b) and c) should lead to lines 748-772 not being executed. > > For Ctrl+D keypresses, the Star popup should show up for scenario 3. For > scenarios 1) and 2), the extension should handle it as the extension chooses. > > https://codereview.chromium.org/765043002/diff/100001/chrome/browser/ui/views... > File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right): > > https://codereview.chromium.org/765043002/diff/100001/chrome/browser/ui/views... > chrome/browser/ui/views/location_bar/bubble_icon_view.cc:53: command_updater_) { > This cannot possibly be correct. A cursory glance at how BubbleIconView is used > in the code, shows that at least the ZoomView constructs it with a > command_updater that is a nullptr. So, line 53 looks like it breaks the ZoomView > (it won't execute the command anymore, as line 55 never gets called). > > But this whole patch feels like a big ugly hack that can be handled in a > different way and more elegantly. Understood @Finnur concerns. Request @Mike Wittman to share his thoughts for same. Thanks
I agree with what Peter and Finnur have said; this is the wrong approach. I think the right thing to do is to update GetBookmarkOverrideCommand to check both that the key is assigned to the extension *and* that the extension requested the key. You can probably update extensions::CommandService to get the latter state. See the code in CommandService::UpdateExtensionSuggestedCommandPrefs which determines requested keys for an extension.
On 2014/12/04 03:04:25, Mike Wittman wrote:
> I agree with what Peter and Finnur have said; this is the wrong approach.
>
> I think the right thing to do is to update GetBookmarkOverrideCommand to check
> both that the key is assigned to the extension *and* that the extension
> requested the key. You can probably update extensions::CommandService to get
the
> latter state. See the code in
> CommandService::UpdateExtensionSuggestedCommandPrefs which determines
requested
> keys for an extension.
Thanks @Mike for sharing your thoughts.
It appears that the real issue is :
It is always checking that weather Ctrl+D is overridden by the extension.
ui::Accelerator bookmark_page_accelerator =
chrome::GetPrimaryChromeAcceleratorForCommandId(IDC_BOOKMARK_PAGE);
in GetBookmarkOverrideCommand().
independent of that how control comes to BookmarkCurrentPage().
As There are 3 possibility for call to come in BookmarkCurrentPage().
a) Press Ctrl+D.
b) Mouse click on the Star.
c) Shift+Alt+T, press Tab until Star has focus, press Space/Enter.
And when extension overrides Ctrl+D and user selects Ctrl+D then
control does not comes to BookmarkCurrentPage().
So my point is when extension override Ctrl+D key , and then even user do
selection by b) and c) method mentioned above, then also
it is checking that "Accelerator key for IDC_BOOKMARK_PAGE i.e Ctrl+D", is
overridden by extension or not. that is wrong.
It should check WHAT is causing BookmarkCurrentPage() call,
that is by mouse click, or selecting space/Enter while focus on 'star', is
overridden by extension or not.
NOT the fixed accelerator key for IDC_BOOKMARK_PAGE.
For doing this we need to bring the eventType info,so that
1) If it is mouse event then It can not be overridden by extension.
2) If it is keyboard event and then corresponding key we need to check if this
key is
overridden by the extension and do action accordingly.
instead of blindly checking 'Accelerator key of IDC_BOOKMARK_PAGE'.
Please correct me if I misunderstood something.
Thanks
I don't think you necessarily want to pass in the event details and try to make the determination that way. You don't necessarily _have_ those details at the caller side, since the caller is just the command handler, so I don't know how you'd implement this. It seems like instead you want to make the keyboard shortcut versus activating the start object follow different codepaths entirely, that do (or don't do) the relevant override checking. Only after they do this checking (or not) should they call into shared code that triggers the internal "bookmark this page" functionality. In other words, I think the override check needs to be hoisted to happen earlier in the callstack, for the keyboard shortcut only. To address Mike's comment about "update GetBookmarkOverrideCommand to check both that the key is assigned to the extension *and* that the extension requested the key." I'm not sure that solves the problem here. Let's say an extension asks to override ctrl-D, but it doesn't remove the star button. Then clicks on the star would still reach here, and they'd see that the extension requested ctrl-D, so they'd do the override, which isn't what we want. Conversely, if an extension didn't request ctrl-D but the user merely assigned it, then if the user hit ctrl-D, I'd think we'd reach here and _fail_ to trigger the extension correctly. Mike, can you tell me if my analysis is correct?
On 2014/12/04 20:51:29, Peter Kasting wrote: > I don't think you necessarily want to pass in the event details and try to make > the determination that way. You don't necessarily _have_ those details at the > caller side, since the caller is just the command handler, so I don't know how > you'd implement this. Agreed, there's not enough context to make the decision at that point. > It seems like instead you want to make the keyboard shortcut versus activating > the start object follow different codepaths entirely, that do (or don't do) the > relevant override checking. Only after they do this checking (or not) should > they call into shared code that triggers the internal "bookmark this page" > functionality. > > In other words, I think the override check needs to be hoisted to happen earlier > in the callstack, for the keyboard shortcut only. > > To address Mike's comment about "update GetBookmarkOverrideCommand to check both > that the key is assigned to the extension *and* that the extension requested the > key." I'm not sure that solves the problem here. Let's say an extension asks > to override ctrl-D, but it doesn't remove the star button. Then clicks on the > star would still reach here, and they'd see that the extension requested ctrl-D, > so they'd do the override, which isn't what we want. Conversely, if an > extension didn't request ctrl-D but the user merely assigned it, then if the > user hit ctrl-D, I'd think we'd reach here and _fail_ to trigger the extension > correctly. Mike, can you tell me if my analysis is correct? This is partially correct. In the latter case, the ctrl-D binding is handled directly by the extension system to raise the popup, so never invokes this codepath. (For similar reasons, this codepath is not invoked if the user clicks on the page action icon of an extension that replaces the star.) In the former case, I think you're correct and my proposed solution doesn't work. For clarity, the relevant cases where this codepath is triggered are: 1. no extension overrides the star, and the user triggers the built-in star 2. no extension overrides ctrl-D and the user hits ctrl-D 3. an extension overrides ctrl-D and the user hits ctrl-D This bug is about the failure to distinguish between case 1 and case 3. Case 3 needs to be handled via the command mechanism due to https://crbug.com/389340. So, perhaps the best way to deal with this is to define a separate command for star-triggered bookmarking (IDC_BOOKMARK_THIS_PAGE_FROM_STAR maybe?) to allow for different handling.
On 2014/12/05 00:04:59, Mike Wittman wrote: > For clarity, the relevant cases where this codepath is triggered are: > 1. no extension overrides the star, and the user triggers the built-in star > 2. no extension overrides ctrl-D and the user hits ctrl-D > 3. an extension overrides ctrl-D and the user hits ctrl-D > > This bug is about the failure to distinguish between case 1 and case 3. Case 3 > needs to be handled via the command mechanism due to https://crbug.com/389340. > So, perhaps the best way to deal with this is to define a separate command for > star-triggered bookmarking (IDC_BOOKMARK_THIS_PAGE_FROM_STAR maybe?) to allow > for different handling. I thought about defining a different command, but I'm not sure whether it's necessary. My hope was that in the case where the user hits ctrl-D, there's some stack frame above this point where we can move the override check that won't be reached by clicking the star, only by the keyboard shortcut. But I didn't investigate how the callstacks look for those cases. It may be that today no such point exists in the code. We could perhaps introduce one by changing the star code from "call the command handler" to "check whether the command is enabled on the handler, then call this method directly". That would mean that the existing command handler callsite for this function would only be reached by keyboard shortcuts, and we could then move the override check there. This still assumes that the star button can trust the command handler's "command enabled" state. Does the computation of this state ask the extension to make the determination, or is it always just an internal "is this page bookmarkable" check? If the former, then this is still a problem, because then an extension trying to muck with ctrl-D (but not star) could disable the star sometimes. Or maybe the star is already unclickable in the cases where the command ought to be disabled, so it doesn't even need to call the command handler at all, and can just unconditionally call this bookmarking function.
On 2014/12/05 00:13:29, Peter Kasting wrote: > On 2014/12/05 00:04:59, Mike Wittman wrote: > > For clarity, the relevant cases where this codepath is triggered are: > > 1. no extension overrides the star, and the user triggers the built-in star > > 2. no extension overrides ctrl-D and the user hits ctrl-D > > 3. an extension overrides ctrl-D and the user hits ctrl-D > > > > This bug is about the failure to distinguish between case 1 and case 3. Case 3 > > needs to be handled via the command mechanism due to https://crbug.com/389340. > > So, perhaps the best way to deal with this is to define a separate command for > > star-triggered bookmarking (IDC_BOOKMARK_THIS_PAGE_FROM_STAR maybe?) to allow > > for different handling. > > I thought about defining a different command, but I'm not sure whether it's > necessary. My hope was that in the case where the user hits ctrl-D, there's > some stack frame above this point where we can move the override check that > won't be reached by clicking the star, only by the keyboard shortcut. But I > didn't investigate how the callstacks look for those cases. > > It may be that today no such point exists in the code. We could perhaps > introduce one by changing the star code from "call the command handler" to > "check whether the command is enabled on the handler, then call this method > directly". That would mean that the existing command handler callsite for this > function would only be reached by keyboard shortcuts, and we could then move the > override check there. > > This still assumes that the star button can trust the command handler's "command > enabled" state. Does the computation of this state ask the extension to make > the determination, or is it always just an internal "is this page bookmarkable" > check? If the former, then this is still a problem, because then an extension > trying to muck with ctrl-D (but not star) could disable the star sometimes. > > Or maybe the star is already unclickable in the cases where the command ought to > be disabled, so it doesn't even need to call the command handler at all, and can > just unconditionally call this bookmarking function. Unfortunately, there's no star-specific code on the call stack above the command invocation for either case 2 or case 3. It's bog standard browser key handling all the way. Case 3 used to be routed through the extension key handlers and did pretty much what you're proposing, but this broke ctrl-D handling by web pages since extension key events get handled too early by the browser window. It's of course possible to update the browser window key handling to special case this, but I'm loathe to do that since that code is already quite complicated, with separate Views and Cocoa implementations. The enable/disable state would not be an issue since it's not set on a page-by-page basis nor observed by the star button; the only time it is set is if an extension requests to remove the bookmark shortcut. It wouldn't be difficult to have the star button handler route its invocation of bookmarking separately from the existing command, but I'm not sure that's any better than defining multiple command IDs.
On 2014/12/05 02:54:04, Mike Wittman wrote: > The enable/disable state would not be an issue since it's not set on a > page-by-page basis nor observed by the star button; Well, since currently the star button just asks the command handler to execute the IDC_BOOKMARK_PAGE command, I'm assuming it's checked by the command handler. Maybe not, though. > the only time it is set is > if an extension requests to remove the bookmark shortcut. If that's true then we probably _shouldn't_ have the star button check the command enable state, since it would couple those two incorrectly. (And if the command handler is checking it like I suggest above, that would be a bug today.) > It wouldn't be difficult to have the star button handler route its invocation of > bookmarking separately from the existing command, but I'm not sure that's any > better than defining multiple command IDs. I'm pretty sure it would be simpler to implement -- just move the check up to the command handler call, and then have the star button directly call the same helper the command handler will call. Shouldn't even require more functions than we have today, just moving some code. Adding more commands, by contrast, will touch a number of different places and you'll need to worry about correct UMA stats and the like. (That said, we should probably worry about UMA stats in the above proposal as well, to make sure we aren't breaking how some stat collecting is happening.) So I think what I'm proposing ought to be simpler; never know for sure until you try to actually implement it, though :) Deepak, does all this give you enough information to move forward?
On 2014/12/05 03:00:32, Peter Kasting wrote: > On 2014/12/05 02:54:04, Mike Wittman wrote: > > The enable/disable state would not be an issue since it's not set on a > > page-by-page basis nor observed by the star button; > > Well, since currently the star button just asks the command handler to execute > the IDC_BOOKMARK_PAGE command, I'm assuming it's checked by the command handler. > Maybe not, though. > > > the only time it is set is > > if an extension requests to remove the bookmark shortcut. > > If that's true then we probably _shouldn't_ have the star button check the > command enable state, since it would couple those two incorrectly. (And if the > command handler is checking it like I suggest above, that would be a bug today.) Right. And checking the command updater code, I suspect this is a bug today. > > It wouldn't be difficult to have the star button handler route its invocation > of > > bookmarking separately from the existing command, but I'm not sure that's any > > better than defining multiple command IDs. > > I'm pretty sure it would be simpler to implement -- just move the check up to > the command handler call, and then have the star button directly call the same > helper the command handler will call. Shouldn't even require more functions > than we have today, just moving some code. Adding more commands, by contrast, > will touch a number of different places and you'll need to worry about correct > UMA stats and the like. (That said, we should probably worry about UMA stats in > the above proposal as well, to make sure we aren't breaking how some stat > collecting is happening.) > > So I think what I'm proposing ought to be simpler; never know for sure until you > try to actually implement it, though :) SGTM, I have no strong opinion either way. And this also serendipitously avoids the bug above. :) > Deepak, does all this give you enough information to move forward?
On 2014/12/05 00:04:59, Mike Wittman wrote: > On 2014/12/04 20:51:29, Peter Kasting wrote: > > I don't think you necessarily want to pass in the event details and try to > make > > the determination that way. You don't necessarily _have_ those details at the > > caller side, since the caller is just the command handler, so I don't know how > > you'd implement this. > > Agreed, there's not enough context to make the decision at that point. > > > It seems like instead you want to make the keyboard shortcut versus activating > > the start object follow different codepaths entirely, that do (or don't do) > the > > relevant override checking. Only after they do this checking (or not) should > > they call into shared code that triggers the internal "bookmark this page" > > functionality. > > > > In other words, I think the override check needs to be hoisted to happen > earlier > > in the callstack, for the keyboard shortcut only. > > > > To address Mike's comment about "update GetBookmarkOverrideCommand to check > both > > that the key is assigned to the extension *and* that the extension requested > the > > key." I'm not sure that solves the problem here. Let's say an extension asks > > to override ctrl-D, but it doesn't remove the star button. Then clicks on the > > star would still reach here, and they'd see that the extension requested > ctrl-D, > > so they'd do the override, which isn't what we want. Conversely, if an > > extension didn't request ctrl-D but the user merely assigned it, then if the > > user hit ctrl-D, I'd think we'd reach here and _fail_ to trigger the extension > > correctly. Mike, can you tell me if my analysis is correct? > > This is partially correct. In the latter case, the ctrl-D binding is handled > directly by the extension system to raise the popup, so never invokes this > codepath. (For similar reasons, this codepath is not invoked if the user clicks > on the page action icon of an extension that replaces the star.) In the former > case, I think you're correct and my proposed solution doesn't work. > > For clarity, the relevant cases where this codepath is triggered are: > 1. no extension overrides the star, and the user triggers the built-in star > 2. no extension overrides ctrl-D and the user hits ctrl-D > 3. an extension overrides ctrl-D and the user hits ctrl-D > > This bug is about the failure to distinguish between case 1 and case 3. Case 3 > needs to be handled via the command mechanism due to https://crbug.com/389340. > So, perhaps the best way to deal with this is to define a separate command for > star-triggered bookmarking (IDC_BOOKMARK_THIS_PAGE_FROM_STAR maybe?) to allow > for different handling. Here for case 3, control does not come in BookmarkCurrentPage() in browser_commands.cc file. My opinion is failure to distinguish between the A. no extension overrides the star, and the user triggers the built-in star B. an extension overrides ctrl-D and the user triggers the built-in star so we want distinguish the selection by star and Ctrl-D when Ctrl-D is overridden. Please correct me if I misunderstood something.
On 2014/12/05 06:18:37, Deepak wrote: > On 2014/12/05 00:04:59, Mike Wittman wrote: > > On 2014/12/04 20:51:29, Peter Kasting wrote: > > > I don't think you necessarily want to pass in the event details and try to > > make > > > the determination that way. You don't necessarily _have_ those details at > the > > > caller side, since the caller is just the command handler, so I don't know > how > > > you'd implement this. > > > > Agreed, there's not enough context to make the decision at that point. > > > > > It seems like instead you want to make the keyboard shortcut versus > activating > > > the start object follow different codepaths entirely, that do (or don't do) > > the > > > relevant override checking. Only after they do this checking (or not) > should > > > they call into shared code that triggers the internal "bookmark this page" > > > functionality. > > > > > > In other words, I think the override check needs to be hoisted to happen > > earlier > > > in the callstack, for the keyboard shortcut only. > > > > > > To address Mike's comment about "update GetBookmarkOverrideCommand to check > > both > > > that the key is assigned to the extension *and* that the extension requested > > the > > > key." I'm not sure that solves the problem here. Let's say an extension > asks > > > to override ctrl-D, but it doesn't remove the star button. Then clicks on > the > > > star would still reach here, and they'd see that the extension requested > > ctrl-D, > > > so they'd do the override, which isn't what we want. Conversely, if an > > > extension didn't request ctrl-D but the user merely assigned it, then if the > > > user hit ctrl-D, I'd think we'd reach here and _fail_ to trigger the > extension > > > correctly. Mike, can you tell me if my analysis is correct? > > > > This is partially correct. In the latter case, the ctrl-D binding is handled > > directly by the extension system to raise the popup, so never invokes this > > codepath. (For similar reasons, this codepath is not invoked if the user > clicks > > on the page action icon of an extension that replaces the star.) In the former > > case, I think you're correct and my proposed solution doesn't work. > > > > For clarity, the relevant cases where this codepath is triggered are: > > 1. no extension overrides the star, and the user triggers the built-in star > > 2. no extension overrides ctrl-D and the user hits ctrl-D > > 3. an extension overrides ctrl-D and the user hits ctrl-D > > > > This bug is about the failure to distinguish between case 1 and case 3. Case 3 > > needs to be handled via the command mechanism due to https://crbug.com/389340. > > So, perhaps the best way to deal with this is to define a separate command for > > star-triggered bookmarking (IDC_BOOKMARK_THIS_PAGE_FROM_STAR maybe?) to allow > > for different handling. > > Here for case 3, control does not come in BookmarkCurrentPage() in > browser_commands.cc file. That directly contradicts what Mike just said. And why would we have this override-checking code here if it's not actually used for case 3, which is the only one we want to trigger it? > My opinion is failure to distinguish between the > A. no extension overrides the star, and the user triggers the built-in star > B. an extension overrides ctrl-D and the user triggers the built-in star > > so we want distinguish the selection by star and Ctrl-D when Ctrl-D is > overridden. We don't want to distinguish those cases. We want those cases to behave the same; both should trigger the built-in bookmarking functionality.
On 2014/12/05 06:35:58, Peter Kasting wrote: > On 2014/12/05 06:18:37, Deepak wrote: > > On 2014/12/05 00:04:59, Mike Wittman wrote: > > > On 2014/12/04 20:51:29, Peter Kasting wrote: > > > > I don't think you necessarily want to pass in the event details and try to > > > make > > > > the determination that way. You don't necessarily _have_ those details at > > the > > > > caller side, since the caller is just the command handler, so I don't know > > how > > > > you'd implement this. > > > > > > Agreed, there's not enough context to make the decision at that point. > > > > > > > It seems like instead you want to make the keyboard shortcut versus > > activating > > > > the start object follow different codepaths entirely, that do (or don't > do) > > > the > > > > relevant override checking. Only after they do this checking (or not) > > should > > > > they call into shared code that triggers the internal "bookmark this page" > > > > functionality. > > > > > > > > In other words, I think the override check needs to be hoisted to happen > > > earlier > > > > in the callstack, for the keyboard shortcut only. > > > > > > > > To address Mike's comment about "update GetBookmarkOverrideCommand to > check > > > both > > > > that the key is assigned to the extension *and* that the extension > requested > > > the > > > > key." I'm not sure that solves the problem here. Let's say an extension > > asks > > > > to override ctrl-D, but it doesn't remove the star button. Then clicks on > > the > > > > star would still reach here, and they'd see that the extension requested > > > ctrl-D, > > > > so they'd do the override, which isn't what we want. Conversely, if an > > > > extension didn't request ctrl-D but the user merely assigned it, then if > the > > > > user hit ctrl-D, I'd think we'd reach here and _fail_ to trigger the > > extension > > > > correctly. Mike, can you tell me if my analysis is correct? > > > > > > This is partially correct. In the latter case, the ctrl-D binding is handled > > > directly by the extension system to raise the popup, so never invokes this > > > codepath. (For similar reasons, this codepath is not invoked if the user > > clicks > > > on the page action icon of an extension that replaces the star.) In the > former > > > case, I think you're correct and my proposed solution doesn't work. > > > > > > For clarity, the relevant cases where this codepath is triggered are: > > > 1. no extension overrides the star, and the user triggers the built-in star > > > 2. no extension overrides ctrl-D and the user hits ctrl-D > > > 3. an extension overrides ctrl-D and the user hits ctrl-D > > > > > > This bug is about the failure to distinguish between case 1 and case 3. Case > 3 > > > needs to be handled via the command mechanism due to > https://crbug.com/389340. > > > So, perhaps the best way to deal with this is to define a separate command > for > > > star-triggered bookmarking (IDC_BOOKMARK_THIS_PAGE_FROM_STAR maybe?) to > allow > > > for different handling. > > > > Here for case 3, control does not come in BookmarkCurrentPage() in > > browser_commands.cc file. > > That directly contradicts what Mike just said. And why would we have this > override-checking code here if it's not actually used for case 3, which is the > only one we want to trigger it? > > > My opinion is failure to distinguish between the > > A. no extension overrides the star, and the user triggers the built-in star > > B. an extension overrides ctrl-D and the user triggers the built-in star > > > > so we want distinguish the selection by star and Ctrl-D when Ctrl-D is > > overridden. > > We don't want to distinguish those cases. We want those cases to behave the > same; both should trigger the built-in bookmarking functionality. Below is my understanding till now. We have following cases for this issue: 1) Ctrl-D is not overridden by extension : A) User selects Ctrl+D. Then control will come in BookmarkCurrentPage() that starts from BrowserView::AcceleratorPressed() , and extension overridden check get failed and BookmarkCurrentPageInternal() get called and bookmark omnibox came. B) User Selects star icon by mouse/keyboard: Here control comes in the BookmarkCurrentPage() that starts from BubbleIconView::ExecuteCommand() and extension overridden check get failed and BookmarkCurrentPageInternal() get called and bookmark omnibox came. 2) Ctrl-D is overridden by the extension, then we have again 2 cases : A) User selects Ctrl+D. Here control does not come in the BookmarkCurrentPage(), as it goes in ExtensionActionPlatformDelegateViews::AcceleratorPressed() and extension popup will get launched. B) User Selects star icon by mouse/keyboard: Here control comes in the BookmarkCurrentPage() and then check for the overridden command that is CTRL-D every time. This is our ISSUE. One way is we can have some info like setting the eventtype(Mouse/Keyboard) and keycode(Keyboard selection) in 'command updater' that selection is by 'star' then i) If it is key event, We can send keycode of key instead of currently fixed 'IDC_BOOKMARK_PAGE' so that if key is not overloaded then GetBookmarkOverrideCommand() get failed and proper bookmark omnibox will come. ii) if it is mouse event then as mouse event can not overridden then no need to do GetBookmarkOverrideCommand() check. so we don't have to add new command id, that can disturb UMA states. And our 2)B) case also get addressed. Second way is introducing new command id as Wittman suggested. Above is my understanding till now, Please correct me If I misunderstood something. Thanks
I feel like you haven't read the last several posts in our conversation. You're simply repeating what you said earlier. We claimed your description of control flow in 2A is wrong, and you haven't clarified how you came to the conclusion that we're mistaken. We said that passing in event details was the wrong way to solve this. I suggested a refactor of how the star handling works that you seem to have ignored entirely. Mike, can you comment on Deepak's assertion that when the extension overrides ctrl-d we don't actually handle it through our normal command-handling flow like you said we currently do? Is he just looking at an old build?
On 2014/12/05 13:48:59, Deepak wrote: > One way is we can have some info like setting the eventtype(Mouse/Keyboard) > and keycode(Keyboard selection) in 'command updater' that selection is by 'star' > then > > i) If it is key event, We can send keycode of key instead of currently fixed > 'IDC_BOOKMARK_PAGE' > so that if key is not overloaded then GetBookmarkOverrideCommand() get failed > and proper bookmark omnibox will come. > ii) if it is mouse event then as mouse event can not overridden then no need to > do > GetBookmarkOverrideCommand() check. > > so we don't have to add new command id, that can disturb UMA states. > And our 2)B) case also get addressed. Trying to determine the source of the event after the fact is not a good solution. It's fragile and spreads knowledge of how events are handled to CommandUpdater, where it doesn't belong. As Peter said, we should move the logic to distinguish these two cases up the stack to code that has the context to make the distinction between a star action and keyboard action. We've determined that the best way to do this is to change the code that handles the star action to bypass the key override check, rather than executing the IDC_BOOKMARK_THIS_PAGE command. In Views this occurs in BubbleIconView, and there should be similar code for Cocoa. > Second way is introducing new command id as Wittman suggested. And the third and recommended way is what I've just described. Please follow this guidance.
On 2014/12/05 19:30:04, Peter Kasting wrote: > Mike, can you comment on Deepak's assertion that when the extension overrides > ctrl-d we don't actually handle it through our normal command-handling flow like > you said we currently do? Is he just looking at an old build? Unless he's working off an ancient build (pre-38) I don't how this could happen. I've just verified the behavior in the debugger under Windows against a recent trunk. Deepak, what revision are you working from? Have you tried running this under the debugger with a breakpoint in BookmarkCurrentPage and verified that it is not hit?
On 2014/12/05 20:19:47, Mike Wittman wrote:
> On 2014/12/05 19:30:04, Peter Kasting wrote:
> > Mike, can you comment on Deepak's assertion that when the extension
overrides
> > ctrl-d we don't actually handle it through our normal command-handling flow
> like
> > you said we currently do? Is he just looking at an old build?
>
> Unless he's working off an ancient build (pre-38) I don't how this could
happen.
> I've just verified the behavior in the debugger under Windows against a recent
> trunk. Deepak, what revision are you working from? Have you tried running this
> under the debugger with a breakpoint in BookmarkCurrentPage and verified that
it
> is not hit?
@Wittman
I have using
Chromium 41.0.2241.0 (Developer Build)
Revision 50b3fb8dc460461cee676ddf9923c5a1d499f4fc-refs/heads/master@{#307152}
OS Linux
Blink 537.36 (@186634)
JavaScript V8 3.31.42
Flash (Disabled)
and when extension overrides Ctrl-D, and I do Ctrl-D, then control does not come
in BookmarkCurrentPage()
in browser_commands.cc file.
I have verified this by putting breakpoint in BookmarkCurrentPage() as well as
putting logs.
neither break point get hit nor logs are coming in this function.
On 2014/12/05 19:30:04, Peter Kasting wrote: > I feel like you haven't read the last several posts in our conversation. You're > simply repeating what you said earlier. We claimed your description of control > flow in 2A is wrong, and you haven't clarified how you came to the conclusion > that we're mistaken. We said that passing in event details was the wrong way to > solve this. I suggested a refactor of how the star handling works that you seem > to have ignored entirely. > > Mike, can you comment on Deepak's assertion that when the extension overrides > ctrl-d we don't actually handle it through our normal command-handling flow like > you said we currently do? Is he just looking at an old build? Sorry @Peter, I am not ignoring any of your suggestions/Advice. But still not getting control in BookmarkCurrentPage() in browser_commands.cc file. when extension overrides Ctrl-D and I selected Ctrl-D, I have tested this on linux box with latest pulled code. I informed Wittman about this, so that we come on same page. Tried your suggestion of refactoring code. so that we will do GetBookmarkOverrideCommand() check before common code for Ctrl-D and 'star' selection starts. I tried and checked, this solves our problem. Just wanted to know your opinion in below case: - Extension overrided Ctrl-D, then what would happen if user selects wrenchmenu->Bookmarks->Bookmark this page..
On 2014/12/06 06:04:29, Deepak wrote: > Sorry @Peter, > I am not ignoring any of your suggestions/Advice. > But still not getting control in BookmarkCurrentPage() in browser_commands.cc > file. > when extension overrides Ctrl-D and I selected Ctrl-D, I have tested this on > linux box > with latest pulled code. > I informed Wittman about this, so that we come on same page. Yeah, you two will have to work this out. > Just wanted to know your opinion in below case: > - Extension overrided Ctrl-D, then what would happen if user selects > wrenchmenu->Bookmarks->Bookmark this page.. Ugh, I don't know. My instinct is that either this should do what ctrl-D does, or we shouldn't show the "ctrl-D" shortcut next to it. The former would be better for extensions trying to override ctrl-D in order to replace the bookmarking system. The latter would be better for extensions that are just overriding ctrl-D for some other reason or that the user has assigned the shortcut too. This is probably another one where Finnur/Mike should weigh in.
On 2014/12/08 01:59:59, Peter Kasting wrote: > On 2014/12/06 06:04:29, Deepak wrote: > > Sorry @Peter, > > I am not ignoring any of your suggestions/Advice. > > But still not getting control in BookmarkCurrentPage() in browser_commands.cc > > file. > > when extension overrides Ctrl-D and I selected Ctrl-D, I have tested this on > > linux box > > with latest pulled code. > > I informed Wittman about this, so that we come on same page. > > Yeah, you two will have to work this out. > > > Just wanted to know your opinion in below case: > > - Extension overrided Ctrl-D, then what would happen if user selects > > wrenchmenu->Bookmarks->Bookmark this page.. > > Ugh, I don't know. My instinct is that either this should do what ctrl-D does, > or we shouldn't show the "ctrl-D" shortcut next to it. The former would be > better for extensions trying to override ctrl-D in order to replace the > bookmarking system. The latter would be better for extensions that are just > overriding ctrl-D for some other reason or that the user has assigned the > shortcut too. > > This is probably another one where Finnur/Mike should weigh in. @Peter I have checked when extension overrides Ctrl+D then we do selection by wrenchmenu->Bookmarks->Bookmark this page.. Then bookmark popup should come instead of extension popup. As is happening for other's like. I have overrided Ctrl-T that opening new tab, So selecting Ctrl-T opens extension popup. and selecting wrenchmenu->New Tab opens New tab, instead of opening extension popup. I have done change. PTAL.
On 2014/12/08 05:39:44, Deepak wrote: > As is happening for other's like. > I have overrided Ctrl-T that opening new tab, > So selecting Ctrl-T opens extension popup. > and selecting > wrenchmenu->New Tab > opens New tab, instead of opening extension popup. > I have done change. I don't think looking at that behavior is an indicator about whether it's a desirable behavior. As I said, I would like Finnur's and Mike's comments on that behavior.
On 2014/12/08 08:33:49, Peter Kasting wrote: > On 2014/12/08 05:39:44, Deepak wrote: > > As is happening for other's like. > > I have overrided Ctrl-T that opening new tab, > > So selecting Ctrl-T opens extension popup. > > and selecting > > wrenchmenu->New Tab > > opens New tab, instead of opening extension popup. > > I have done change. > > I don't think looking at that behavior is an indicator about whether it's a > desirable behavior. > > As I said, I would like Finnur's and Mike's comments on that behavior. I understand, I just mentioned my observation. I am also waiting for Finnur's and Mike's comments for expected behavior. Thanks
On 2014/12/06 05:51:43, Deepak wrote:
> On 2014/12/05 20:19:47, Mike Wittman wrote:
> > Unless he's working off an ancient build (pre-38) I don't how this could
> happen.
> > I've just verified the behavior in the debugger under Windows against a
recent
> > trunk. Deepak, what revision are you working from? Have you tried running
this
> > under the debugger with a breakpoint in BookmarkCurrentPage and verified
that
> it
> > is not hit?
>
> @Wittman
> I have using
> Chromium 41.0.2241.0 (Developer Build)
> Revision 50b3fb8dc460461cee676ddf9923c5a1d499f4fc-refs/heads/master@{#307152}
> OS Linux
> Blink 537.36 (@186634)
> JavaScript V8 3.31.42
> Flash (Disabled)
>
> and when extension overrides Ctrl-D, and I do Ctrl-D, then control does not
come
> in BookmarkCurrentPage()
> in browser_commands.cc file.
> I have verified this by putting breakpoint in BookmarkCurrentPage() as well as
> putting logs.
> neither break point get hit nor logs are coming in this function.
Ah, I suspect the problem is you're not testing with an extension that requests
to override ctrl-D. The behavior is different if the extension requests the
shortcut than if the user assigns it manually. The easiest way to test this case
is likely to install the new Bookmark Manager extension:
https://chrome.google.com/webstore/detail/bookmark-manager/gmlllbghnfkpflemih...
On 2014/12/08 01:59:59, Peter Kasting wrote: > On 2014/12/06 06:04:29, Deepak wrote: > > Just wanted to know your opinion in below case: > > - Extension overrided Ctrl-D, then what would happen if user selects > > wrenchmenu->Bookmarks->Bookmark this page.. > > Ugh, I don't know. My instinct is that either this should do what ctrl-D does, > or we shouldn't show the "ctrl-D" shortcut next to it. The former would be > better for extensions trying to override ctrl-D in order to replace the > bookmarking system. The latter would be better for extensions that are just > overriding ctrl-D for some other reason or that the user has assigned the > shortcut too. The keybinding works as you expect in both these cases. The convention is that, if an extension is permitted to do so, and requests to remove the bookmark shortcut, and also requests to override ctrl-D, it's treated as overriding the "bookmark this page" wherever it appears in the UI. General extensions are not allowed to request overriding ctrl-D since it's a Chrome-reserved shortcut key.
On 2014/12/08 17:47:08, Mike Wittman wrote: > On 2014/12/08 01:59:59, Peter Kasting wrote: > > On 2014/12/06 06:04:29, Deepak wrote: > > > Just wanted to know your opinion in below case: > > > - Extension overrided Ctrl-D, then what would happen if user selects > > > wrenchmenu->Bookmarks->Bookmark this page.. > > > > Ugh, I don't know. My instinct is that either this should do what ctrl-D > does, > > or we shouldn't show the "ctrl-D" shortcut next to it. The former would be > > better for extensions trying to override ctrl-D in order to replace the > > bookmarking system. The latter would be better for extensions that are just > > overriding ctrl-D for some other reason or that the user has assigned the > > shortcut too. > > The keybinding works as you expect in both these cases. The convention is that, > if an extension is permitted to do so, and requests to remove the bookmark > shortcut, and also requests to override ctrl-D, it's treated as overriding the > "bookmark this page" wherever it appears in the UI. General extensions are not > allowed to request overriding ctrl-D since it's a Chrome-reserved shortcut key. Do you mean "the menu item works as you expect"? I'm confused by your use of the word "keybinding". Here's what seems like logical behavior based on your comments. (We may already do much of this.) * Extension removes bookmark shortcut and overrides ctrl-D: Menu item appears as normal and triggers extension * Extension removes bookmark shortcut and does not override ctrl-D: Menu item does not appear * Extension does not remove bookmark shortcut and overrides ctrl-D: Extension is not allowed to do this, ignore override attempt * Extension does not remove bookmark shortcut and user assigns ctrl-D to extension: Menu item appears and triggers Chrome's internal bookmarking functionality, not extension. Menu item doesn't show "ctrl-D" shortcut. We implement similar behavior for all other menus with keyboard shortcuts that the user has assigned to particular extensions.
On 2014/12/08 20:27:29, Peter Kasting wrote: > On 2014/12/08 17:47:08, Mike Wittman wrote: > > The keybinding works as you expect in both these cases. The convention is > that, > > if an extension is permitted to do so, and requests to remove the bookmark > > shortcut, and also requests to override ctrl-D, it's treated as overriding the > > "bookmark this page" wherever it appears in the UI. General extensions are not > > allowed to request overriding ctrl-D since it's a Chrome-reserved shortcut > key. > > Do you mean "the menu item works as you expect"? I'm confused by your use of > the word "keybinding". > > Here's what seems like logical behavior based on your comments. (We may already > do much of this.) > > * Extension removes bookmark shortcut and overrides ctrl-D: Menu item appears as > normal and triggers extension > * Extension removes bookmark shortcut and does not override ctrl-D: Menu item > does not appear > * Extension does not remove bookmark shortcut and overrides ctrl-D: Extension is > not allowed to do this, ignore override attempt > * Extension does not remove bookmark shortcut and user assigns ctrl-D to > extension: Menu item appears and triggers Chrome's internal bookmarking > functionality, not extension. Menu item doesn't show "ctrl-D" shortcut. We > implement similar behavior for all other menus with keyboard shortcuts that the > user has assigned to particular extensions. Yes, the menu item works as you expect. :) Your assessment of the behavior is correct and consistent with the existing implementation, except there appears to be a bug in the last case in that the menu item is still labeled with ctrl-D, at least on Windows. This bug is not specific to this action; I reproduced it for ctrl+T also. I filed a bug: https://crbug.com/440108.
On 2014/12/08 21:28:46, Mike Wittman wrote: > On 2014/12/08 20:27:29, Peter Kasting wrote: > > On 2014/12/08 17:47:08, Mike Wittman wrote: > > > The keybinding works as you expect in both these cases. The convention is > > that, > > > if an extension is permitted to do so, and requests to remove the bookmark > > > shortcut, and also requests to override ctrl-D, it's treated as overriding > the > > > "bookmark this page" wherever it appears in the UI. General extensions are > not > > > allowed to request overriding ctrl-D since it's a Chrome-reserved shortcut > > key. > > > > Do you mean "the menu item works as you expect"? I'm confused by your use of > > the word "keybinding". > > > > Here's what seems like logical behavior based on your comments. (We may > already > > do much of this.) > > > > * Extension removes bookmark shortcut and overrides ctrl-D: Menu item appears > as > > normal and triggers extension > > * Extension removes bookmark shortcut and does not override ctrl-D: Menu item > > does not appear > > * Extension does not remove bookmark shortcut and overrides ctrl-D: Extension > is > > not allowed to do this, ignore override attempt > > * Extension does not remove bookmark shortcut and user assigns ctrl-D to > > extension: Menu item appears and triggers Chrome's internal bookmarking > > functionality, not extension. Menu item doesn't show "ctrl-D" shortcut. We > > implement similar behavior for all other menus with keyboard shortcuts that > the > > user has assigned to particular extensions. > > Yes, the menu item works as you expect. :) Thanks @Wittman for confirming & sharing your thoughts. > Your assessment of the behavior is correct and consistent with the existing > implementation, except there appears to be a bug in the last case in that the > menu item is still labeled with ctrl-D, at least on Windows. This bug is not > specific to this action; I reproduced it for ctrl+T also. I filed a bug: > https://crbug.com/440108. @Peter PTAL for these changes. As a part of incremental changes,I will work for not showing 'Shortcuts' when that key is overridden by extension in followup CL as Wittman already raised separate issue for this. Thanks.
This isn't the implementation I asked for. Instead of making the star button call directly to the lower-level bookmarking code, you've hoisted knowledge of bookmark override commands up to the accelerator handling code. That's wrong because: * That code shouldn't know about bookmark override issues at all * It's platform-specific code and you haven't implemented the equivalent on non-views * I'm concerned this might make clicking the Chrome menu item not trigger the extension popup if the extension is overriding this command Please implement this as we described earlier, by changing which function the star code calls.
On 2014/12/09 21:45:39, Peter Kasting wrote: > This isn't the implementation I asked for. Instead of making the star button > call directly to the lower-level bookmarking code, you've hoisted knowledge of > bookmark override commands up to the accelerator handling code. That's wrong > because: > * That code shouldn't know about bookmark override issues at all > * It's platform-specific code and you haven't implemented the equivalent on > non-views > * I'm concerned this might make clicking the Chrome menu item not trigger the extension popup if the extension is overriding this command. This Wittman told that it should not call the extension popup when extension overrides Ctrl-D and we do selection by wrenchmenu->Bookmarks->Bookmark this page.. But we need not show Ctr-D in the menu, for that he filed new bug. > > Please implement this as we described earlier, by changing which function the > star code calls. @Peter & Wittman I am sorry, I does not understand your way to implementation. AFAIK when we do we select Ctr-D then flow goes like : -BrowserView::AcceleratorPressed() -BrowserCommands ExecuteCommand() -CommandUpdater::ExecuteCommand() -CommandUpdater::ExecuteCommandWithDisposition() -BrowserCommandController::ExecuteCommandWithDisposition() -BrowserCommands BookmarkCurrentPage(Browser* browser) But when we select 'Star' icon then it is like -BubbleIconView::ExecuteCommand -CommandUpdater::ExecuteCommand() -CommandUpdater::ExecuteCommandWithDisposition() -BrowserCommandController::ExecuteCommandWithDisposition() -BrowserCommands BookmarkCurrentPage(Browser* browser) we don't have access to Browser* in BubbleIconView, for direct call to BookmarkCurrentPageInternal(). and we need to expose BookmarkCurrentPageInternal() in BrowserCommands also. Last you mentioned that we can achieve just by restructuring code and without introducing new functions. Please correct me if I understanding is wrong.
On 2014/12/10 08:33:43, Deepak wrote: > On 2014/12/09 21:45:39, Peter Kasting wrote: > > This isn't the implementation I asked for. Instead of making the star button > > call directly to the lower-level bookmarking code, you've hoisted knowledge of > > bookmark override commands up to the accelerator handling code. That's wrong > > because: > > * That code shouldn't know about bookmark override issues at all > > * It's platform-specific code and you haven't implemented the equivalent on > > non-views > > * I'm concerned this might make clicking the Chrome menu item not trigger the > extension popup if the extension is overriding this command. > > This Wittman told that it should not call the extension popup when extension > overrides > Ctrl-D and we do selection by wrenchmenu->Bookmarks->Bookmark this page.. > But we need not show Ctr-D in the menu, for that he filed new bug. Argh. No. If the extension overrides ctrl-D, selecting the menu entry should trigger the extension popup. And that's what should already be happening today. I think you're confusing the extension overriding ctrl-D with the user assigning ctrl-D to the extension, which should _not_ make the menu entry trigger the extension. > we don't have access to Browser* in BubbleIconView, for direct call to > BookmarkCurrentPageInternal(). > and we need to expose BookmarkCurrentPageInternal() in BrowserCommands also. No, just go get the Browser. It shouldn't be hard to access. The LocationBarView has it so it can either provide it to the star at creation time or expose it via an accessor.
On 2014/12/10 18:00:03, Peter Kasting wrote: > On 2014/12/10 08:33:43, Deepak wrote: > > On 2014/12/09 21:45:39, Peter Kasting wrote: > > > This isn't the implementation I asked for. Instead of making the star > button > > > call directly to the lower-level bookmarking code, you've hoisted knowledge > of > > > bookmark override commands up to the accelerator handling code. That's > wrong > > > because: > > > * That code shouldn't know about bookmark override issues at all > > > * It's platform-specific code and you haven't implemented the equivalent on > > > non-views > > > * I'm concerned this might make clicking the Chrome menu item not trigger > the > > extension popup if the extension is overriding this command. > > > > This Wittman told that it should not call the extension popup when extension > > overrides > > Ctrl-D and we do selection by wrenchmenu->Bookmarks->Bookmark this page.. > > But we need not show Ctr-D in the menu, for that he filed new bug. > > Argh. No. If the extension overrides ctrl-D, selecting the menu entry should > trigger the extension popup. And that's what should already be happening today. yes, You are right. I understood. > I think you're confusing the extension overriding ctrl-D with the user assigning > ctrl-D to the extension, which should _not_ make the menu entry trigger the > extension. I said about user assigning one.It triggered extension popup to come when selecting via wrench menu. > > we don't have access to Browser* in BubbleIconView, for direct call to > > BookmarkCurrentPageInternal(). > > and we need to expose BookmarkCurrentPageInternal() in BrowserCommands also. > > No, just go get the Browser. It shouldn't be hard to access. The > LocationBarView has it so it can either provide it to the star at creation time > or expose it via an accessor. I understood, Thanks for guidance. Changes done. PTAL.
On 2014/12/11 07:22:47, Deepak wrote: > I said about user assigning one.It triggered extension popup to come > when selecting via wrench menu. Assigning ctrl-D to a non-bookmarking extension today causes selecting the wrench menu item to trigger the extension popup instead of the internal bookmarking system? Mike, can you confirm this and file a bug if necessary? https://codereview.chromium.org/765043002/diff/200001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.h (right): https://codereview.chromium.org/765043002/diff/200001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.h:30: void BookmarkCurrentPageInternal(Browser* browser); Nit: List this just below the other bookmarking-related functions below (in the .cc file too). I'm not a fan of using "Internal" in a public API as it's confusing and not very descriptive. It would be nice to distinguish these two functions, a la: BookmarkCurrentPageAllowingExtensionOverrides() BookmarkCurrentPageIgnoringExtensionOverrides() https://codereview.chromium.org/765043002/diff/200001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right): https://codereview.chromium.org/765043002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/bubble_icon_view.cc:79: chrome::BookmarkCurrentPageInternal(browser_); This change isn't appropriate for a generic base class of several different icons. Not all icons trigger bookmarking, only the star does. You should be implementing this behavior in the star subclass, not here. This also means the base class has no reason to be provided with the Browser object, and subclasses other than the star don't need it either. https://codereview.chromium.org/765043002/diff/200001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right): https://codereview.chromium.org/765043002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/bubble_icon_view.h:24: Browser* browser); Nit: While here: This shouldn't be explicit as it's not single-arg. https://codereview.chromium.org/765043002/diff/200001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/star_view.h (right): https://codereview.chromium.org/765043002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/star_view.h:15: explicit StarView(CommandUpdater* command_updater, Browser* browser); The star doesn't need the command updater if it has the browser, it can compute it from the browser pointer.
On 2014/12/12 01:20:57, Peter Kasting wrote: > On 2014/12/11 07:22:47, Deepak wrote: > > I said about user assigning one.It triggered extension popup to come > > when selecting via wrench menu. > > Assigning ctrl-D to a non-bookmarking extension today causes selecting the > wrench menu item to trigger the extension popup instead of the internal > bookmarking system? Mike, can you confirm this and file a bug if necessary? Yes, I can reproduce. Looks like the check for override is not specific enough to distinguish between user-assigned keybindings and extension-requested keybindings. I'll address this separately. Filed https://crbug.com/441597.
Thanks @Peter for review and sharing your thoughts. Changes done as suggested. PTAL. https://codereview.chromium.org/765043002/diff/200001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.h (right): https://codereview.chromium.org/765043002/diff/200001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.h:30: void BookmarkCurrentPageInternal(Browser* browser); On 2014/12/12 01:20:56, Peter Kasting wrote: > Nit: List this just below the other bookmarking-related functions below (in the > .cc file too). > > I'm not a fan of using "Internal" in a public API as it's confusing and not very > descriptive. It would be nice to distinguish these two functions, a la: > > BookmarkCurrentPageAllowingExtensionOverrides() > BookmarkCurrentPageIgnoringExtensionOverrides() Done. https://codereview.chromium.org/765043002/diff/200001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right): https://codereview.chromium.org/765043002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/bubble_icon_view.h:24: Browser* browser); On 2014/12/12 01:20:57, Peter Kasting wrote: > Nit: While here: This shouldn't be explicit as it's not single-arg. Done. https://codereview.chromium.org/765043002/diff/200001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/star_view.h (right): https://codereview.chromium.org/765043002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/star_view.h:15: explicit StarView(CommandUpdater* command_updater, Browser* browser); On 2014/12/12 01:20:57, Peter Kasting wrote: > The star doesn't need the command updater if it has the browser, it can compute > it from the browser pointer. Done.
LGTM; is there any way to automatically test this? Perhaps we already have some tests on file for the bookmark override functionality, and we can extend them, or something.
On 2014/12/12 19:02:52, Peter Kasting wrote: > LGTM; is there any way to automatically test this? Perhaps we already have some > tests on file for the bookmark override functionality, and we can extend them, > or something. LGTM as well, with nits. A test for this functionality would be very useful. Between the code in StarViewTest.HideOnSecondClick, CommandsApiTest.Basic, and CommandsApiTest.OverwriteBookmarkShortcut, I think we should be able to put together an automated test for this. Unless you want to add the test here Deepak, I'm fine with addressing it while working on https://crbug.com/441597.
https://codereview.chromium.org/765043002/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/star_view.cc (right): https://codereview.chromium.org/765043002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/star_view.cc:24: browser_ = browser; Nit: move to initializer list. https://codereview.chromium.org/765043002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/star_view.cc:63: if (browser_) Nit: you're already depending on browser to be non-null in the constructor, so remove this check.
https://codereview.chromium.org/765043002/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/star_view.cc (right): https://codereview.chromium.org/765043002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/star_view.cc:63: if (browser_) On 2014/12/12 20:41:57, Mike Wittman wrote: > Nit: you're already depending on browser to be non-null in the constructor, so > remove this check. Actually, it turns out I was wrong about it always being safe to compute the command updater from the browser. Instead, the constructor should take both Browser* and CommandUpdater* arguments, pass the latter to the parent class, and then in this function do: if (browser_) { OnExecuting(source); chrome::BookmarkCurrentPageIgnoringExtensionOverrides(browser_); } else { BubbleIconView::ExecuteCommand(source); }
https://codereview.chromium.org/765043002/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/star_view.h (right): https://codereview.chromium.org/765043002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/star_view.h:28: Browser* browser_; please, add a blank line after this.
Thanks @Peter, @Wittman, @Tfarina for giving time for review. I have addressed all nit's. So committing this change. https://codereview.chromium.org/765043002/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/star_view.cc (right): https://codereview.chromium.org/765043002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/star_view.cc:24: browser_ = browser; On 2014/12/12 20:41:57, Mike Wittman wrote: > Nit: move to initializer list. Done. https://codereview.chromium.org/765043002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/star_view.cc:63: if (browser_) On 2014/12/12 21:49:05, Peter Kasting wrote: > On 2014/12/12 20:41:57, Mike Wittman wrote: > > Nit: you're already depending on browser to be non-null in the constructor, so > > remove this check. > > Actually, it turns out I was wrong about it always being safe to compute the > command updater from the browser. Instead, the constructor should take both > Browser* and CommandUpdater* arguments, pass the latter to the parent class, and > then in this function do: > > if (browser_) { > OnExecuting(source); > chrome::BookmarkCurrentPageIgnoringExtensionOverrides(browser_); > } else { > BubbleIconView::ExecuteCommand(source); > } Done. https://codereview.chromium.org/765043002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/star_view.cc:63: if (browser_) On 2014/12/12 20:41:57, Mike Wittman wrote: > Nit: you're already depending on browser to be non-null in the constructor, so > remove this check. For considering Peter's advice, I need this if check. https://codereview.chromium.org/765043002/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/star_view.h (right): https://codereview.chromium.org/765043002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/star_view.h:28: Browser* browser_; On 2014/12/12 21:54:23, tfarina wrote: > please, add a blank line after this. Done.
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/765043002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2014/12/13 03:44:19, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) @Avi @Tfaina I am having Presubmit error, as ** Presubmit ERRORS ** Missing LGTM from an OWNER for these files: chrome/browser/browser_commands_unittest.cc PTAL. Thanks
On 2014/12/13 04:09:40, Deepak wrote: > I am having Presubmit error That error tells you all you need to know. You need an OWNER approval for every file. You don't have a LG for that one file. Find the OWNER and get them to give it to you. http://www.chromium.org/developers/owners-files
On 2014/12/13 04:34:20, Avi wrote: > On 2014/12/13 04:09:40, Deepak wrote: > > I am having Presubmit error > > That error tells you all you need to know. You need an OWNER approval for every > file. You don't have a LG for that one file. > > Find the OWNER and get them to give it to you. > > http://www.chromium.org/developers/owners-files I understand, I need to have approval from all file owners, that I have touched. And I don't have approval only for browser_commands_unittest.cc file. In /src/chrome/browser/OWNERS I am not sure which owner should I add for browser_commands_unittest.cc Request you to please suggest owner. Thanks
On 2014/12/13 04:45:00, Deepak wrote: > On 2014/12/13 04:34:20, Avi wrote: > > On 2014/12/13 04:09:40, Deepak wrote: > > > I am having Presubmit error > > > > That error tells you all you need to know. You need an OWNER approval for > every > > file. You don't have a LG for that one file. > > > > Find the OWNER and get them to give it to you. > > > > http://www.chromium.org/developers/owners-files > > I understand, I need to have approval from all file owners, that I have touched. > And I don't have approval only for browser_commands_unittest.cc file. > > In /src/chrome/browser/OWNERS > I am not sure which owner should I add for browser_commands_unittest.cc > Request you to please suggest owner. > Thanks Use git log and git blame to see who made recent changes to the file, and find the owner who made the most recent or relevant change. Or just randomly pick one from the OWNER file. Either way.
deepak.m1@samsung.com changed reviewers: + brettw@chromium.org
@brettw Need your approval for browser_commands_unittests.cc file for submitting these changes. PTAL.
On Saturday, December 13, 2014, <deepak.m1@samsung.com> wrote: > On 2014/12/13 04:34:20, Avi wrote: > >> On 2014/12/13 04:09:40, Deepak wrote: >> > I am having Presubmit error >> > > That error tells you all you need to know. You need an OWNER approval for >> > every > >> file. You don't have a LG for that one file. >> > > Find the OWNER and get them to give it to you. >> > > http://www.chromium.org/developers/owners-files >> > > I understand, I need to have approval from all file owners, that I have > touched. > And I don't have approval only for browser_commands_unittest.cc file. > > In /src/chrome/browser/OWNERS > I am not sure which owner should I add for browser_commands_unittest.cc I would suggest getting either Lei or Scott for for approving this. I think Scott is more familiar with this file though. > Request you to please suggest owner. > Thanks > > https://codereview.chromium.org/765043002/ > -- Thiago Farina To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I just need rubber stamping for browser_commands_unittest.cc as function name get changed in browser_commands.cc file. And that function is also getting used here. Their is no change in functionality. So I request owner to approve, so that patch can be landed. Thanks
On 2014/12/13 15:54:14, Deepak wrote: > I just need rubber stamping for browser_commands_unittest.cc as function name > get changed in browser_commands.cc file. > > And that function is also getting used here. > Their is no change in functionality. > So I request owner to approve, so that patch can be landed. You can TBR that sort of purely-mechanical change. Add "TBR=<owner>" to the change description and the CQ should allow you to land. (TBR still expects review, it just allow commit to proceed anyway; so make sure you still send mail as normal when you do this sort of thing.)
deepak.m1@samsung.com changed reviewers: + sky@chromium.org
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/765043002/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/54a7f399b0613cbae272fbe0ba97447be6517a99 Cr-Commit-Position: refs/heads/master@{#308312} |
