Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2024893003:
Views: Modify Textfield::ExecuteCommand to use Textfield::IsCommandIdEnabled instead of a virtual ca (Closed)
|
Created:
4 years, 6 months ago by karandeepb Modified:
4 years, 6 months ago Reviewers:
tapted CC:
chromium-reviews, tfarina, James Su, shuchen+watch_chromium.org, yusukes+watch_chromium.org, nona+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@refactor1_IsEditCommandEnabled Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionViews: Modify Textfield::ExecuteCommand to use Textfield::IsCommandIdEnabled instead of a virtual call.
Textfield::ExecuteCommand is currently using a virtual IsCommandIdEnabled call.
It doesn't make sense to use a virtual call since Textfield::ExecuteCommand
should not be concerned with commands enabled in subclasses. Hence this CL
modifies Textfield::ExecuteCommand to use Textfield::IsCommandIdEnabled instead
of a virtual call
BUG=586985
Patch Set 1 #
Total comments: 3
Patch Set 2 : #Patch Set 3 : Address review comments. #Patch Set 4 : Address review comments. #
Total comments: 1
Patch Set 5 : Address review comments. #Messages
Total messages: 13 (5 generated)
Description was changed from ========== Refactor2: virtual calls. ========== to ========== Views: Modify Textfield::ExecuteCommand to return early in case edit command is not enabled. BUG=None ==========
tapted@chromium.org changed reviewers: + tapted@chromium.org
https://codereview.chromium.org/2024893003/diff/1/ui/views/controls/textfield... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2024893003/diff/1/ui/views/controls/textfield... ui/views/controls/textfield/textfield.cc:1316: command_id = IDS_DELETE_BACKWARD; The command_id can change down here, so this doesn't look right in isolation
Description was changed from ========== Views: Modify Textfield::ExecuteCommand to return early in case edit command is not enabled. BUG=None ========== to ========== Views: Modify Textfield::ExecuteCommand to return early in case edit command is not enabled. BUG=586985 ==========
PTAL Thanks. https://codereview.chromium.org/2024893003/diff/1/ui/views/controls/textfield... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2024893003/diff/1/ui/views/controls/textfield... ui/views/controls/textfield/textfield.cc:1316: command_id = IDS_DELETE_BACKWARD; On 2016/06/06 07:26:16, tapted wrote: > The command_id can change down here, so this doesn't look right in isolation Yeah I added this code in https://codereview.chromium.org/1923793002. It should not matter since all these delete commands are enabled under the same conditions.
https://codereview.chromium.org/2024893003/diff/1/ui/views/controls/textfield... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2024893003/diff/1/ui/views/controls/textfield... ui/views/controls/textfield/textfield.cc:1316: command_id = IDS_DELETE_BACKWARD; On 2016/06/08 03:05:49, karandeepb wrote: > On 2016/06/06 07:26:16, tapted wrote: > > The command_id can change down here, so this doesn't look right in isolation > > Yeah I added this code in https://codereview.chromium.org/1923793002. It should > not matter since all these delete commands are enabled under the same > conditions. Perhaps the CL needs a clearer rationale then, or this should be merged onto another. It's not landable like this. You might need a comment above Textfield::IsCommandIdEnabled(command_id). Something like // Even though |command_id| changes below, return early here so that something something. Also, since something something, ensure Textfield's version of IsCommandIdEnabled() is invoked rather than an override. If a subclass overrides IsCommandIdEnabled, it should handle altered commands in an ExecuteCommand override.
Description was changed from ========== Views: Modify Textfield::ExecuteCommand to return early in case edit command is not enabled. BUG=586985 ========== to ========== Views: Modify Textfield::ExecuteCommand to return early in case edit command is not enabled. This CL modifies Textfield::ExecuteCommand to return early in case edit command is not enabled. Also, instead of making a virtual call, Textfield::IsCommandIdEnabled is used now. BUG=586985 ==========
On 2016/06/08 04:02:40, tapted wrote: > https://codereview.chromium.org/2024893003/diff/1/ui/views/controls/textfield... > File ui/views/controls/textfield/textfield.cc (right): > > https://codereview.chromium.org/2024893003/diff/1/ui/views/controls/textfield... > ui/views/controls/textfield/textfield.cc:1316: command_id = IDS_DELETE_BACKWARD; > On 2016/06/08 03:05:49, karandeepb wrote: > > On 2016/06/06 07:26:16, tapted wrote: > > > The command_id can change down here, so this doesn't look right in isolation > > > > Yeah I added this code in https://codereview.chromium.org/1923793002. It > should > > not matter since all these delete commands are enabled under the same > > conditions. > > Perhaps the CL needs a clearer rationale then, or this should be merged onto > another. It's not landable like this. I did this as a separate CL, because in the next CL (https://codereview.chromium.org/2031433002/), I am moving the code from ExecuteCommand to ExecuteEditCommand verbatim. > You might need a comment above Textfield::IsCommandIdEnabled(command_id). > Something like > > // Even though |command_id| changes below, return early here so that something > something. Also, since something something, ensure Textfield's version of > IsCommandIdEnabled() is invoked rather than an override. If a subclass overrides > IsCommandIdEnabled, it should handle altered commands in an ExecuteCommand > override. Both ExecuteCommand(..) and IsCommandIdEnabled are virtual methods. IsCommandIdEnabled is a precondition for a command to be executed. Its normal to expect a subclass to override both methods or none. Also, consider this, OmniboxViewViews* omnibox = new ...; omnibox->Textfield::ExecuteCommand(IDS_PASTE_AND_GO); This would lead to a NOTREACHED currently but would be a no-op with this CL.
On 2016/06/09 01:37:56, karandeepb wrote: > On 2016/06/08 04:02:40, tapted wrote: > > > https://codereview.chromium.org/2024893003/diff/1/ui/views/controls/textfield... > > File ui/views/controls/textfield/textfield.cc (right): > > > > > https://codereview.chromium.org/2024893003/diff/1/ui/views/controls/textfield... > > ui/views/controls/textfield/textfield.cc:1316: command_id = > IDS_DELETE_BACKWARD; > > On 2016/06/08 03:05:49, karandeepb wrote: > > > On 2016/06/06 07:26:16, tapted wrote: > > > > The command_id can change down here, so this doesn't look right in > isolation > > > > > > Yeah I added this code in https://codereview.chromium.org/1923793002. It > > should > > > not matter since all these delete commands are enabled under the same > > > conditions. > > > > Perhaps the CL needs a clearer rationale then, or this should be merged onto > > another. It's not landable like this. > > I did this as a separate CL, because in the next CL > (https://codereview.chromium.org/2031433002/), I am moving the code from > ExecuteCommand to ExecuteEditCommand verbatim. Yah, so this is why we split CLs - so it's easier to spot stuff like this :) The same comment applies to the follow-up at https://codereview.chromium.org/2031433002/diff/80001/ui/views/controls/textf... It doesn't look right to validate one |command_id|, and then change it. I'm not saying it IS wrong, just that it looks wrong. Stuff that looks wrong needs a comment to say why it's right. > > You might need a comment above Textfield::IsCommandIdEnabled(command_id). > > Something like > > > > // Even though |command_id| changes below, return early here so that something > > something. Also, since something something, ensure Textfield's version of > > IsCommandIdEnabled() is invoked rather than an override. If a subclass > overrides > > IsCommandIdEnabled, it should handle altered commands in an ExecuteCommand > > override. > > Both ExecuteCommand(..) and IsCommandIdEnabled are virtual methods. > IsCommandIdEnabled is a precondition for a command to be executed. Its normal to > > expect a subclass to override both methods or none. > > Also, consider this, > > OmniboxViewViews* omnibox = new ...; > omnibox->Textfield::ExecuteCommand(IDS_PASTE_AND_GO); > > This would lead to a NOTREACHED currently but would be a no-op with this CL. Yup - this is all fine. We just need to comment in Textfield::ExecuteCommand() to explain it.
On 2016/06/09 02:10:05, tapted wrote: > On 2016/06/09 01:37:56, karandeepb wrote: > > On 2016/06/08 04:02:40, tapted wrote: > > > > > > https://codereview.chromium.org/2024893003/diff/1/ui/views/controls/textfield... > > > File ui/views/controls/textfield/textfield.cc (right): > > > > > > > > > https://codereview.chromium.org/2024893003/diff/1/ui/views/controls/textfield... > > > ui/views/controls/textfield/textfield.cc:1316: command_id = > > IDS_DELETE_BACKWARD; > > > On 2016/06/08 03:05:49, karandeepb wrote: > > > > On 2016/06/06 07:26:16, tapted wrote: > > > > > The command_id can change down here, so this doesn't look right in > > isolation > > > > > > > > Yeah I added this code in https://codereview.chromium.org/1923793002. It > > > should > > > > not matter since all these delete commands are enabled under the same > > > > conditions. > > > > > > Perhaps the CL needs a clearer rationale then, or this should be merged onto > > > another. It's not landable like this. > > > > I did this as a separate CL, because in the next CL > > (https://codereview.chromium.org/2031433002/), I am moving the code from > > ExecuteCommand to ExecuteEditCommand verbatim. > > Yah, so this is why we split CLs - so it's easier to spot stuff like this :) > > The same comment applies to the follow-up at > https://codereview.chromium.org/2031433002/diff/80001/ui/views/controls/textf... > > It doesn't look right to validate one |command_id|, and then change it. I'm not > saying it IS wrong, just that it looks wrong. Stuff that looks wrong needs a > comment to say why it's right. Yeah you are correct, it's not correct to depend on the command_id modifications below. Have changed it now, so that we do the check after modifying the command_id. > > > > You might need a comment above Textfield::IsCommandIdEnabled(command_id). > > > Something like > > > > > > // Even though |command_id| changes below, return early here so that > something > > > something. Also, since something something, ensure Textfield's version of > > > IsCommandIdEnabled() is invoked rather than an override. If a subclass > > overrides > > > IsCommandIdEnabled, it should handle altered commands in an ExecuteCommand > > > override. > > > > Both ExecuteCommand(..) and IsCommandIdEnabled are virtual methods. > > IsCommandIdEnabled is a precondition for a command to be executed. Its normal > to > > > > expect a subclass to override both methods or none. > > > > Also, consider this, > > > > OmniboxViewViews* omnibox = new ...; > > omnibox->Textfield::ExecuteCommand(IDS_PASTE_AND_GO); > > > > This would lead to a NOTREACHED currently but would be a no-op with this CL. > > Yup - this is all fine. We just need to comment in Textfield::ExecuteCommand() > to explain it. Done.
make sure you update the CL description too https://codereview.chromium.org/2024893003/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2024893003/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1325: DestroyTouchSelection(); The comment above is good, but now the remaining goal is to convince the code reviewer that moving `DestroyTouchSelection()` to come after the check is the right thing to do. The description for http://crrev.com/267813 says "Whenever a mouse or keyboard event happens anywhere in any display, touch text seleciton should be deactivated.". That suggests to me that it might belong above the check regardless of whether the command is enabled, but also that we might want to duplicate the call to DestroyTouchSelection() inside ExecuteEditCommand (also before checking whether the command_id is enabled) since the codepaths for menus and edit commands will have less overlap.
On 2016/06/09 05:10:09, tapted wrote: > make sure you update the CL description too > > https://codereview.chromium.org/2024893003/diff/60001/ui/views/controls/textf... > File ui/views/controls/textfield/textfield.cc (right): > > https://codereview.chromium.org/2024893003/diff/60001/ui/views/controls/textf... > ui/views/controls/textfield/textfield.cc:1325: DestroyTouchSelection(); > The comment above is good, but now the remaining goal is to convince the code > reviewer that moving `DestroyTouchSelection()` to come after the check is the > right thing to do. > > The description for http://crrev.com/267813 says "Whenever a mouse or keyboard > event happens anywhere in any display, touch text seleciton should be > deactivated.". That suggests to me that it might belong above the check > regardless of whether the command is enabled, but also that we might want to > duplicate the call to DestroyTouchSelection() inside ExecuteEditCommand (also > before checking whether the command_id is enabled) since the codepaths for menus > and edit commands will have less overlap. Yeah I am also not sure. Will see what the owners have to say.
Description was changed from ========== Views: Modify Textfield::ExecuteCommand to return early in case edit command is not enabled. This CL modifies Textfield::ExecuteCommand to return early in case edit command is not enabled. Also, instead of making a virtual call, Textfield::IsCommandIdEnabled is used now. BUG=586985 ========== to ========== Views: Modify Textfield::ExecuteCommand to use Textfield::IsCommandIdEnabled instead of a virtual call. Textfield::ExecuteCommand is currently using a virtual IsCommandIdEnabled call. It doesn't make sense to use a virtual call since Textfield::ExecuteCommand should not be concerned with commands enabled in subclasses. Hence this CL modifies Textfield::ExecuteCommand to use Textfield::IsCommandIdEnabled instead of a virtual call BUG=586985 ========== |
