Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2034623002:
Unify ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId. (Closed)
|
Created:
4 years, 6 months ago by karandeepb Modified:
4 years, 6 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, tdresser+watch_chromium.org, tfarina, shuchen+watch_chromium.org, nona+watch_chromium.org, James Su, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@refactor7_correct_text_edit_command_aura_typo Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUnify ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId.
This CL unifies ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId
by modifying TextEditCommandAuraLinux to use ui::TextEditCommand. Several new
enum values are added ui::TextEditCommand. Views::Textfield methods
IsEditCommandEnabled and ExecuteEditCommand are modified to correctly handle the
newly added enum values.
This is a follow-up to crrev.com/2064053002 and is sixth in series of CLs to
replace resource ids with a ui::TextEditCommand enum for text editing commands
in views::Textfield.
Link to complete patch - http://crrev.com/2029733003
BUG=586985
Committed: https://crrev.com/fbcc4e354fa9f3829638329d47b12849452be391
Cr-Commit-Position: refs/heads/master@{#401491}
Patch Set 1 : #Patch Set 2 : Rebase. #Patch Set 3 : Rebase. #Patch Set 4 : Rebase. #Patch Set 5 : Rebase. #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : Forward declare. #Patch Set 9 : #
Total comments: 18
Patch Set 10 : Rebase #Patch Set 11 : Address nits. #Patch Set 12 : Reorder enums in switch. #
Total comments: 8
Patch Set 13 : Rebase. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 42 (27 generated)
Description was changed from ========== Refactor8: Unify TextEditCommand and TextEditCommandAuraLinux. ========== to ========== Unify ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId. This CL unifies ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId by modifying TextEditCommandAuraLinux to use ui::TextEditCommand. Several new enum values are added ui::TextEditCommand. Views::Textfield method IsEditCommandEnabled and ExecuteEditCommand are modified to correctly handle the newly added enum values. This is fifth in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. BUG=586985 ==========
Description was changed from ========== Unify ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId. This CL unifies ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId by modifying TextEditCommandAuraLinux to use ui::TextEditCommand. Several new enum values are added ui::TextEditCommand. Views::Textfield method IsEditCommandEnabled and ExecuteEditCommand are modified to correctly handle the newly added enum values. This is fifth in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. BUG=586985 ========== to ========== Unify ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId. This CL unifies ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId by modifying TextEditCommandAuraLinux to use ui::TextEditCommand. Several new enum values are added ui::TextEditCommand. Views::Textfield method IsEditCommandEnabled and ExecuteEditCommand are modified to correctly handle the newly added enum values. This is fifth in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. BUG=586985 ==========
Description was changed from ========== Unify ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId. This CL unifies ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId by modifying TextEditCommandAuraLinux to use ui::TextEditCommand. Several new enum values are added ui::TextEditCommand. Views::Textfield method IsEditCommandEnabled and ExecuteEditCommand are modified to correctly handle the newly added enum values. This is fifth in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. BUG=586985 ========== to ========== Unify ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId. This CL unifies ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId by modifying TextEditCommandAuraLinux to use ui::TextEditCommand. Several new enum values are added ui::TextEditCommand. Views::Textfield methods IsEditCommandEnabled and ExecuteEditCommand are modified to correctly handle the newly added enum values. This is fifth in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. BUG=586985 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Patchset #2 (id:240001) has been deleted
karandeepb@chromium.org changed reviewers: + msw@chromium.org
PTAL msw@ for ui/views/controls/ and chrome/browser/ review. The patch is not completely done yet. For eg, I have not ordered enum values in switch statements so that the diff is clearer.
Description was changed from ========== Unify ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId. This CL unifies ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId by modifying TextEditCommandAuraLinux to use ui::TextEditCommand. Several new enum values are added ui::TextEditCommand. Views::Textfield methods IsEditCommandEnabled and ExecuteEditCommand are modified to correctly handle the newly added enum values. This is fifth in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. BUG=586985 ========== to ========== Unify ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId. This CL unifies ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId by modifying TextEditCommandAuraLinux to use ui::TextEditCommand. Several new enum values are added ui::TextEditCommand. Views::Textfield methods IsEditCommandEnabled and ExecuteEditCommand are modified to correctly handle the newly added enum values. This is fifth in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. Link to complete patch - http://crrev.com/2029733003 BUG=586985 ==========
mostly lg; just nits. https://codereview.chromium.org/2034623002/diff/400001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc (right): https://codereview.chromium.org/2034623002/diff/400001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc:286: if (str && *str) nit: add curlies https://codereview.chromium.org/2034623002/diff/400001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc:410: GetHandlerOwner(text_view)->EditCommandMatched(TextEditCommand::SELECT_ALL, optional nit: GetHandlerOwner(text_view)->EditCommandMatched(select? TextEditCommand::SELECT_ALL : TextEditCommand::UNSELECT, std::string()); https://codereview.chromium.org/2034623002/diff/400001/ui/events/linux/text_e... File ui/events/linux/text_edit_command_auralinux.cc (right): https://codereview.chromium.org/2034623002/diff/400001/ui/events/linux/text_e... ui/events/linux/text_edit_command_auralinux.cc:16: case TextEditCommand::COPY: nit: can you follow up with one more CL to reorder the switches to match the enum declaration order when it makes sense (ie. when there isn't some other rationale to ordering by type like read-only ops, etc.) (and maybe consider making the enum decl order match some other established order?) https://codereview.chromium.org/2034623002/diff/400001/ui/events/linux/text_e... ui/events/linux/text_edit_command_auralinux.cc:83: // Maybe return "Invalid" like in renderer. nit: remove/address this contemplative comment. https://codereview.chromium.org/2034623002/diff/400001/ui/events/linux/text_e... File ui/events/linux/text_edit_command_auralinux.h (right): https://codereview.chromium.org/2034623002/diff/400001/ui/events/linux/text_e... ui/events/linux/text_edit_command_auralinux.h:32: // The text to be inserted else an empty string. Only used for nit: "The text for TextEditCommand::INSERT_TEXT; otherwise empty and unused." https://codereview.chromium.org/2034623002/diff/400001/ui/events/text_edit_co... File ui/events/text_edit_commands.h (right): https://codereview.chromium.org/2034623002/diff/400001/ui/events/text_edit_co... ui/events/text_edit_commands.h:56: MOVE_UP_AND_MODIFY_SELECTION, nit: ordering... keep FOO and FOO_AND_MODIFY_SELECTION consistently adjacent or separated. (or follow some other established order...) https://codereview.chromium.org/2034623002/diff/400001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2034623002/diff/400001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:663: const ui::TextEditCommand command = commands[i].command(); optional nit: inline this. https://codereview.chromium.org/2034623002/diff/400001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1547: case ui::TextEditCommand::DELETE_WORD_BACKWARD: nit: reorder these to something a bit neater. https://codereview.chromium.org/2034623002/diff/400001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1675: case ui::TextEditCommand::MOVE_BACKWARD_AND_MODIFY_SELECTION: ditto nit: keep pairing ordering or follow some other pattern consistently.
Patchset #10 (id:420001) has been deleted
Patchset #10 (id:440001) has been deleted
Patchset #12 (id:500001) has been deleted
Patchset #12 (id:520001) has been deleted
Description was changed from ========== Unify ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId. This CL unifies ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId by modifying TextEditCommandAuraLinux to use ui::TextEditCommand. Several new enum values are added ui::TextEditCommand. Views::Textfield methods IsEditCommandEnabled and ExecuteEditCommand are modified to correctly handle the newly added enum values. This is fifth in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. Link to complete patch - http://crrev.com/2029733003 BUG=586985 ========== to ========== Unify ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId. This CL unifies ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId by modifying TextEditCommandAuraLinux to use ui::TextEditCommand. Several new enum values are added ui::TextEditCommand. Views::Textfield methods IsEditCommandEnabled and ExecuteEditCommand are modified to correctly handle the newly added enum values. This is a follow-up to crrev.com/2064053002 and is sixth in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. Link to complete patch - http://crrev.com/2029733003 BUG=586985 ==========
PTAL msw@. Thanks. https://codereview.chromium.org/2034623002/diff/400001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc (right): https://codereview.chromium.org/2034623002/diff/400001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc:286: if (str && *str) On 2016/06/10 01:33:23, msw wrote: > nit: add curlies Done. https://codereview.chromium.org/2034623002/diff/400001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc:410: GetHandlerOwner(text_view)->EditCommandMatched(TextEditCommand::SELECT_ALL, On 2016/06/10 01:33:23, msw wrote: > optional nit: GetHandlerOwner(text_view)->EditCommandMatched(select? > TextEditCommand::SELECT_ALL : TextEditCommand::UNSELECT, std::string()); Done. https://codereview.chromium.org/2034623002/diff/400001/ui/events/linux/text_e... File ui/events/linux/text_edit_command_auralinux.cc (right): https://codereview.chromium.org/2034623002/diff/400001/ui/events/linux/text_e... ui/events/linux/text_edit_command_auralinux.cc:16: case TextEditCommand::COPY: On 2016/06/10 01:33:23, msw wrote: > nit: can you follow up with one more CL to reorder the switches to match the > enum declaration order when it makes sense (ie. when there isn't some other > rationale to ordering by type like read-only ops, etc.) (and maybe consider > making the enum decl order match some other established order?) Done. https://codereview.chromium.org/2034623002/diff/400001/ui/events/linux/text_e... ui/events/linux/text_edit_command_auralinux.cc:83: // Maybe return "Invalid" like in renderer. On 2016/06/10 01:33:23, msw wrote: > nit: remove/address this contemplative comment. Done. https://codereview.chromium.org/2034623002/diff/400001/ui/events/linux/text_e... File ui/events/linux/text_edit_command_auralinux.h (right): https://codereview.chromium.org/2034623002/diff/400001/ui/events/linux/text_e... ui/events/linux/text_edit_command_auralinux.h:32: // The text to be inserted else an empty string. Only used for On 2016/06/10 01:33:23, msw wrote: > nit: "The text for TextEditCommand::INSERT_TEXT; otherwise empty and unused." Done. https://codereview.chromium.org/2034623002/diff/400001/ui/events/text_edit_co... File ui/events/text_edit_commands.h (right): https://codereview.chromium.org/2034623002/diff/400001/ui/events/text_edit_co... ui/events/text_edit_commands.h:56: MOVE_UP_AND_MODIFY_SELECTION, On 2016/06/10 01:33:23, msw wrote: > nit: ordering... keep FOO and FOO_AND_MODIFY_SELECTION consistently adjacent or > separated. (or follow some other established order...) Done. https://codereview.chromium.org/2034623002/diff/400001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2034623002/diff/400001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:663: const ui::TextEditCommand command = commands[i].command(); On 2016/06/10 01:33:23, msw wrote: > optional nit: inline this. Done. https://codereview.chromium.org/2034623002/diff/400001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1547: case ui::TextEditCommand::DELETE_WORD_BACKWARD: On 2016/06/10 01:33:23, msw wrote: > nit: reorder these to something a bit neater. Done. https://codereview.chromium.org/2034623002/diff/400001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1675: case ui::TextEditCommand::MOVE_BACKWARD_AND_MODIFY_SELECTION: On 2016/06/10 01:33:23, msw wrote: > ditto nit: keep pairing ordering or follow some other pattern consistently. Done.
lgtm
karandeepb@chromium.org changed reviewers: + sky@chromium.org
+sky for ui/base review.
https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... File ui/base/ime/linux/text_edit_command_auralinux.cc (right): https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... ui/base/ime/linux/text_edit_command_auralinux.cc:13: // third_party/WebKit/public/platform/WebEditingCommandType.h. Do we have a test that verifies two are in sync? https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... File ui/base/ime/linux/text_edit_command_auralinux.h (right): https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... ui/base/ime/linux/text_edit_command_auralinux.h:33: std::string argument_; nit: const https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/text_edit_... File ui/base/ime/text_edit_commands.h (right): https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/text_edit_... ui/base/ime/text_edit_commands.h:65: INVALID_COMMAND, Why do we need INVALID_COMMAND?
PTAL sky@. https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... File ui/base/ime/linux/text_edit_command_auralinux.cc (right): https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... ui/base/ime/linux/text_edit_command_auralinux.cc:13: // third_party/WebKit/public/platform/WebEditingCommandType.h. On 2016/06/16 16:54:09, sky wrote: > Do we have a test that verifies two are in sync? We don't currently. I can't think of a clean way to write a test. Any advice on how to test this? https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... File ui/base/ime/linux/text_edit_command_auralinux.h (right): https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... ui/base/ime/linux/text_edit_command_auralinux.h:33: std::string argument_; On 2016/06/16 16:54:09, sky wrote: > nit: const This needs to be added to a stl container here - https://cs.chromium.org/chromium/src/chrome/browser/ui/libgtk2ui/gtk2_key_bin.... It's data members cannot be made const, since the class will no longer remain assignable, which is required for it to be added to a stl container. https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/text_edit_... File ui/base/ime/text_edit_commands.h (right): https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/text_edit_... ui/base/ime/text_edit_commands.h:65: INVALID_COMMAND, On 2016/06/16 16:54:09, sky wrote: > Why do we need INVALID_COMMAND? We need a way to signal that a given TextEditCommand variable does not represent an actual command. For eg, Textfield has a TextEditCommand scheduled_text_edit_command_ variable, which refers to the scheduled command. It will be TextEditCommand::INVALID_COMMAND when no command is scheduled.
LGTM https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... File ui/base/ime/linux/text_edit_command_auralinux.cc (right): https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... ui/base/ime/linux/text_edit_command_auralinux.cc:13: // third_party/WebKit/public/platform/WebEditingCommandType.h. On 2016/06/17 07:54:14, karandeepb wrote: > On 2016/06/16 16:54:09, sky wrote: > > Do we have a test that verifies two are in sync? > > We don't currently. I can't think of a clean way to write a test. Any advice on > how to test this? I'm not quite sure what the comment refers to. I don't see a 'string representation' in WebEditingCommandType.h? Assuming there is a function that returns a string for a WebEditingCommandType then you could write a test that calls the blink version and this version and ensures the strings are equal.
https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... File ui/base/ime/linux/text_edit_command_auralinux.cc (right): https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... ui/base/ime/linux/text_edit_command_auralinux.cc:13: // third_party/WebKit/public/platform/WebEditingCommandType.h. On 2016/06/17 15:09:05, sky wrote: > On 2016/06/17 07:54:14, karandeepb wrote: > > On 2016/06/16 16:54:09, sky wrote: > > > Do we have a test that verifies two are in sync? > > > > We don't currently. I can't think of a clean way to write a test. Any advice > on > > how to test this? > > I'm not quite sure what the comment refers to. I don't see a 'string > representation' in WebEditingCommandType.h? Assuming there is a function that > returns a string for a WebEditingCommandType then you could write a test that > calls the blink version and this version and ensures the strings are equal. The string representation that we pass to the renderer is converted to the appropriate WebEditingCommandType at https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/c.... This utilises the file https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/c... which is used to generate a mapping between the string representation and its corresponding enum value. However, this is internal to Blink, and I can't add a dependency to it.
Not even for a test? Seems like there should be somewhere you could expose the function such that it could be used in a test. On Mon, Jun 20, 2016 at 12:16 AM, <karandeepb@chromium.org> wrote: > > https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... > File ui/base/ime/linux/text_edit_command_auralinux.cc (right): > > https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... > ui/base/ime/linux/text_edit_command_auralinux.cc:13: // > third_party/WebKit/public/platform/WebEditingCommandType.h. > On 2016/06/17 15:09:05, sky wrote: >> On 2016/06/17 07:54:14, karandeepb wrote: >> > On 2016/06/16 16:54:09, sky wrote: >> > > Do we have a test that verifies two are in sync? >> > >> > We don't currently. I can't think of a clean way to write a test. > Any advice >> on >> > how to test this? >> >> I'm not quite sure what the comment refers to. I don't see a 'string >> representation' in WebEditingCommandType.h? Assuming there is a > function that >> returns a string for a WebEditingCommandType then you could write a > test that >> calls the blink version and this version and ensures the strings are > equal. > > The string representation that we pass to the renderer is converted to > the > appropriate WebEditingCommandType at > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/c.... > > This utilises the file > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/c... > > which is used to generate a mapping between the string representation > and its > corresponding enum value. However, this is internal to Blink, and I > can't add a > dependency to it. > > https://codereview.chromium.org/2034623002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/20 15:07:26, sky wrote: > Not even for a test? Seems like there should be somewhere you could > expose the function such that it could be used in a test. > > On Mon, Jun 20, 2016 at 12:16 AM, <mailto:karandeepb@chromium.org> wrote: > > > > > https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... > > File ui/base/ime/linux/text_edit_command_auralinux.cc (right): > > > > > https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... > > ui/base/ime/linux/text_edit_command_auralinux.cc:13: // > > third_party/WebKit/public/platform/WebEditingCommandType.h. > > On 2016/06/17 15:09:05, sky wrote: > >> On 2016/06/17 07:54:14, karandeepb wrote: > >> > On 2016/06/16 16:54:09, sky wrote: > >> > > Do we have a test that verifies two are in sync? > >> > > >> > We don't currently. I can't think of a clean way to write a test. > > Any advice > >> on > >> > how to test this? > >> > >> I'm not quite sure what the comment refers to. I don't see a 'string > >> representation' in WebEditingCommandType.h? Assuming there is a > > function that > >> returns a string for a WebEditingCommandType then you could write a > > test that > >> calls the blink version and this version and ensures the strings are > > equal. > > > > The string representation that we pass to the renderer is converted to > > the > > appropriate WebEditingCommandType at > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/c.... > > > > This utilises the file > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/c... > > > > which is used to generate a mapping between the string representation > > and its > > corresponding enum value. However, this is internal to Blink, and I > > can't add a > > dependency to it. > > > > https://codereview.chromium.org/2034623002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > So I talked to yosin@ who owns third_party/WebKit/Source/core/editing regarding somehow exposing the mapping between the command string and WebEditingCommandType. He was not in favor of exposing the string to WebEditingCommandType mapping and instead suggesting refactoring the code such that instead of sending a string to the renderer, we send a WebEditingCommandType directly. However, this seems to be a big refactor and outside the scope of this CL. Is it ok if I land this without a test?
Certainly. Land this now. -Scott On Wed, Jun 22, 2016 at 12:14 AM, <karandeepb@chromium.org> wrote: > On 2016/06/20 15:07:26, sky wrote: >> Not even for a test? Seems like there should be somewhere you could >> expose the function such that it could be used in a test. >> >> On Mon, Jun 20, 2016 at 12:16 AM, <mailto:karandeepb@chromium.org> wrote: >> > >> > >> > https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... >> > File ui/base/ime/linux/text_edit_command_auralinux.cc (right): >> > >> > >> > https://codereview.chromium.org/2034623002/diff/540001/ui/base/ime/linux/text... >> > ui/base/ime/linux/text_edit_command_auralinux.cc:13: // >> > third_party/WebKit/public/platform/WebEditingCommandType.h. >> > On 2016/06/17 15:09:05, sky wrote: >> >> On 2016/06/17 07:54:14, karandeepb wrote: >> >> > On 2016/06/16 16:54:09, sky wrote: >> >> > > Do we have a test that verifies two are in sync? >> >> > >> >> > We don't currently. I can't think of a clean way to write a test. >> > Any advice >> >> on >> >> > how to test this? >> >> >> >> I'm not quite sure what the comment refers to. I don't see a 'string >> >> representation' in WebEditingCommandType.h? Assuming there is a >> > function that >> >> returns a string for a WebEditingCommandType then you could write a >> > test that >> >> calls the blink version and this version and ensures the strings are >> > equal. >> > >> > The string representation that we pass to the renderer is converted to >> > the >> > appropriate WebEditingCommandType at >> > >> > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/c.... >> > >> > This utilises the file >> > >> > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/c... >> > >> > which is used to generate a mapping between the string representation >> > and its >> > corresponding enum value. However, this is internal to Blink, and I >> > can't add a >> > dependency to it. >> > >> > https://codereview.chromium.org/2034623002/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an > email >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > So I talked to yosin@ who owns third_party/WebKit/Source/core/editing > regarding > somehow exposing the mapping between the command string and > WebEditingCommandType. He was not in favor of exposing the string to > WebEditingCommandType mapping and instead suggesting refactoring the code > such > that instead of sending a string to the renderer, we send a > WebEditingCommandType directly. However, this seems to be a big refactor and > outside the scope of this CL. Is it ok if I land this without a test? > > https://codereview.chromium.org/2034623002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2034623002/#ps560001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2034623002/560001
Message was sent while issue was closed.
Description was changed from ========== Unify ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId. This CL unifies ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId by modifying TextEditCommandAuraLinux to use ui::TextEditCommand. Several new enum values are added ui::TextEditCommand. Views::Textfield methods IsEditCommandEnabled and ExecuteEditCommand are modified to correctly handle the newly added enum values. This is a follow-up to crrev.com/2064053002 and is sixth in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. Link to complete patch - http://crrev.com/2029733003 BUG=586985 ========== to ========== Unify ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId. This CL unifies ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId by modifying TextEditCommandAuraLinux to use ui::TextEditCommand. Several new enum values are added ui::TextEditCommand. Views::Textfield methods IsEditCommandEnabled and ExecuteEditCommand are modified to correctly handle the newly added enum values. This is a follow-up to crrev.com/2064053002 and is sixth in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. Link to complete patch - http://crrev.com/2029733003 BUG=586985 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:560001)
Message was sent while issue was closed.
Description was changed from ========== Unify ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId. This CL unifies ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId by modifying TextEditCommandAuraLinux to use ui::TextEditCommand. Several new enum values are added ui::TextEditCommand. Views::Textfield methods IsEditCommandEnabled and ExecuteEditCommand are modified to correctly handle the newly added enum values. This is a follow-up to crrev.com/2064053002 and is sixth in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. Link to complete patch - http://crrev.com/2029733003 BUG=586985 ========== to ========== Unify ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId. This CL unifies ui::TextEditCommand and ui::TextEditCommandAuraLinux::CommandId by modifying TextEditCommandAuraLinux to use ui::TextEditCommand. Several new enum values are added ui::TextEditCommand. Views::Textfield methods IsEditCommandEnabled and ExecuteEditCommand are modified to correctly handle the newly added enum values. This is a follow-up to crrev.com/2064053002 and is sixth in series of CLs to replace resource ids with a ui::TextEditCommand enum for text editing commands in views::Textfield. Link to complete patch - http://crrev.com/2029733003 BUG=586985 Committed: https://crrev.com/fbcc4e354fa9f3829638329d47b12849452be391 Cr-Commit-Position: refs/heads/master@{#401491} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/fbcc4e354fa9f3829638329d47b12849452be391 Cr-Commit-Position: refs/heads/master@{#401491} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
