Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1841143002:
Add enum class |WebEditingCommandType| for EditorCommand (Closed)
|
Created:
4 years, 8 months ago by chongz Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd enum class |WebEditingCommandType| for EditorCommand
Motivation: Using strings for actions is error prone (see
http://crbug.com/251275) and ideally we want to replace
string names with enum (e.g. replace strings in
https://goo.gl/VeUZFX). Also with enum we could eliminate
the huge |EditorCommand::CommandMap| with switch statements
to improve readability.
This CL is the first step and adds |WebEditingCommandType| enum class in public/platform/ and
provides conversion between string command name and enum type in
EditorCommand.cpp.
Also |EditorInternalCommand.idForUserMetrics| is replaced by
|EditorInternalCommand.commandType| but remains the same enum value, so
the user matrics won't be affected.
BUG=596064
Committed: https://crrev.com/2dcc34c1e14d6847a6340da3d03fbe5e04801bfb
Cr-Commit-Position: refs/heads/master@{#386702}
Patch Set 1 #
Total comments: 2
Patch Set 2 : dtapuska's review #Patch Set 3 : Fix windows |strcasecmp| compile issue #
Total comments: 11
Patch Set 4 : yosin's review #
Total comments: 6
Patch Set 5 : yosin's review 2 #
Total comments: 6
Patch Set 6 : tkent's review #
Total comments: 4
Patch Set 7 : Introducing |codePointCompareIgnoringASCIICase()| #Messages
Total messages: 37 (12 generated)
Description was changed from ========== Add enum class for EditorCommand This CL adds |WebEditingCommandType| enum class in public/platform/ and provides conversion between string command name and enum type in EditorCommand.cpp. Also |EditorInternalCommand.idForUserMetrics| is replaced by |EditorInternalCommand.commandType| but remains the same enum value, so the user matrics won't be affected. BUG=596064 ========== to ========== Add enum class WebEditingCommandType for EditorCommand This CL adds |WebEditingCommandType| enum class in public/platform/ and provides conversion between string command name and enum type in EditorCommand.cpp. Also |EditorInternalCommand.idForUserMetrics| is replaced by |EditorInternalCommand.commandType| but remains the same enum value, so the user matrics won't be affected. BUG=596064 ==========
Description was changed from ========== Add enum class WebEditingCommandType for EditorCommand This CL adds |WebEditingCommandType| enum class in public/platform/ and provides conversion between string command name and enum type in EditorCommand.cpp. Also |EditorInternalCommand.idForUserMetrics| is replaced by |EditorInternalCommand.commandType| but remains the same enum value, so the user matrics won't be affected. BUG=596064 ========== to ========== Add enum class |WebEditingCommandType| for EditorCommand This CL adds |WebEditingCommandType| enum class in public/platform/ and provides conversion between string command name and enum type in EditorCommand.cpp. Also |EditorInternalCommand.idForUserMetrics| is replaced by |EditorInternalCommand.commandType| but remains the same enum value, so the user matrics won't be affected. BUG=596064 ==========
chongz@chromium.org changed reviewers: + dtapuska@chromium.org
Hi dtapuska, can you take a look at this CL please? Thanks!
https://codereview.chromium.org/1841143002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/EditingCommandTest.cpp (right): https://codereview.chromium.org/1841143002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EditingCommandTest.cpp:13: const struct CommandNameEntry { This table is duplicated.. Perhaps you can place it in some header file and include it into your implementations so the code isn't duplicated? https://codereview.chromium.org/1841143002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1841143002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1663: { WebEditingCommandType::AlignLeft /* 2 */, executeJustifyLeft, supportedFromMenuOrKeyBinding, enabledInRichlyEditableText, stateNone, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled }, I'm not sure you need the /* 1 */ etc...
Hi dtapuska, I've updated CL as per your comments, PTAL, thanks!
chongz@chromium.org changed reviewers: + yosin@chromium.org
Hi yosin, can you take a look at this CL please? Thanks!
https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1526: { WebEditingCommandType::AlignJustified, executeJustifyFull, supportedFromMenuOrKeyBinding, enabledInRichlyEditableText, stateNone, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled }, In a follow-up CL this table can be eliminated and switch statements instead of function ptrs.
On 2016/03/29 20:46:05, dtapuska wrote: > https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): > > https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1526: { > WebEditingCommandType::AlignJustified, executeJustifyFull, > supportedFromMenuOrKeyBinding, enabledInRichlyEditableText, stateNone, > valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled }, > In a follow-up CL this table can be eliminated and switch statements instead of > function ptrs. lgtm
https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingCommandTest.cpp (right): https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingCommandTest.cpp:25: static_assert(arraysize(kCommandNameEntries) + 1 == static_cast<int>(WebEditingCommandType::NumberOfCommandTypes), "must test all valid WebEditingCommandType"); s/<int>/<size_t>/ https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingCommandTest.cpp:35: EXPECT_GT(0, strcasecmp(kCommandNameEntries[i-1].name, kCommandNameEntries[i].name)) << "EDITOR_COMMAND_MAP must be case-folding ordered."; s/i-1/i - 1/ We may want to have "<< i" to know where we break the rule. BTW, once we use HashMap<K,V>, we don't need to have this restriction, don't you? https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:95: reinterpret_cast<const CommandNameEntry*>(entry1)->name, I think |static_cast<T>| can work for |void*| https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:104: bsearch(&keyEntry, kCommandNameEntries, arraysize(kCommandNameEntries), Let's use WTF::HashMap<String, CommandNameEntry*> here. To support case insensitivity of command name, we can convert command name to down case for HashMap key and search.
https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:104: bsearch(&keyEntry, kCommandNameEntries, arraysize(kCommandNameEntries), On 2016/03/30 01:50:51, Yosi_UTC9 wrote: > Let's use WTF::HashMap<String, CommandNameEntry*> here. To support case > insensitivity of command name, we can convert command name to down case for > HashMap key and search. Isn't it better to not allocate data especially when the data is well sorted? The CommandNameEntry objects will essentially go away because it can all be done with switch statements now. And really there will just be a table mapping from Name->ID; yes that can be a WTF:HashMap<String, id> but with bsearch you can do it with 0 allocations and very similar performance.
https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:104: bsearch(&keyEntry, kCommandNameEntries, arraysize(kCommandNameEntries), On 2016/03/30 at 02:04:09, dtapuska wrote: > On 2016/03/30 01:50:51, Yosi_UTC9 wrote: > > Let's use WTF::HashMap<String, CommandNameEntry*> here. To support case > > insensitivity of command name, we can convert command name to down case for > > HashMap key and search. > > Isn't it better to not allocate data especially when the data is well sorted? The CommandNameEntry objects will essentially go away because it can all be done with switch statements now. And really there will just be a table mapping from Name->ID; yes that can be a WTF:HashMap<String, id> but with bsearch you can do it with 0 allocations and very similar performance. I see. How about using std::binary_search() with std::begin()/std::end() to make code C++ish? These are allowed C++ library, http://chromium-cpp.appspot.com/#library-whitelist and there are usage: https://code.google.com/p/chromium/codesearch#search/&q=std::binary_search%20...
Hi yosin, I've updated CL as per your comments, PTAL, thanks! https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingCommandTest.cpp (right): https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingCommandTest.cpp:25: static_assert(arraysize(kCommandNameEntries) + 1 == static_cast<int>(WebEditingCommandType::NumberOfCommandTypes), "must test all valid WebEditingCommandType"); On 2016/03/30 01:50:51, Yosi_UTC9 wrote: > s/<int>/<size_t>/ Done. https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingCommandTest.cpp:35: EXPECT_GT(0, strcasecmp(kCommandNameEntries[i-1].name, kCommandNameEntries[i].name)) << "EDITOR_COMMAND_MAP must be case-folding ordered."; On 2016/03/30 01:50:51, Yosi_UTC9 wrote: > s/i-1/i - 1/ > > We may want to have "<< i" to know where we break the rule. > > BTW, once we use HashMap<K,V>, we don't need to have this restriction, don't > you? Done. https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:95: reinterpret_cast<const CommandNameEntry*>(entry1)->name, On 2016/03/30 01:50:51, Yosi_UTC9 wrote: > I think |static_cast<T>| can work for |void*| Done. https://codereview.chromium.org/1841143002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:104: bsearch(&keyEntry, kCommandNameEntries, arraysize(kCommandNameEntries), On 2016/03/30 03:48:11, Yosi_UTC9 wrote: > On 2016/03/30 at 02:04:09, dtapuska wrote: > > On 2016/03/30 01:50:51, Yosi_UTC9 wrote: > > > Let's use WTF::HashMap<String, CommandNameEntry*> here. To support case > > > insensitivity of command name, we can convert command name to down case for > > > HashMap key and search. > > > > Isn't it better to not allocate data especially when the data is well sorted? > The CommandNameEntry objects will essentially go away because it can all be done > with switch statements now. And really there will just be a table mapping from > Name->ID; yes that can be a WTF:HashMap<String, id> but with bsearch you can do > it with 0 allocations and very similar performance. > > I see. How about using std::binary_search() with std::begin()/std::end() to make > code C++ish? > These are allowed C++ library, > http://chromium-cpp.appspot.com/#library-whitelist and there are usage: > https://code.google.com/p/chromium/codesearch#search/&q=std::binary_search%20... Cannot use std::binary_search() since it only returns true/false, use std::lower_bound() and do a compare instead.
https://codereview.chromium.org/1841143002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1841143002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:82: #define EDITOR_COMMAND_MAP_DECLARATION const CommandNameEntry kCommandNameEntries[] = V8 team invents good technique to define names. If we use it, we could write: "core/editing/commands/EditorCommandNames.h" #define FOR_EACH_BLINK_EDITING_COMMAND_NAME(V) \ V(Bold) V(Delete) V(Insert) ... const CommandEntry kCommandNameEntries[] = { #define V(name) { #name, WebEditingCommandType::name }, FOR_EACH_BLINK_EDITING_COMMAND_NAME(V) #undef V }; In this way, we don't need to name file ".inc" and reserve macro names |EDITOR_COMMAND_MAP| and |EDITOR_COMMAND_MAP_DECLARATION| Example: https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/objects.h&q... https://codereview.chromium.org/1841143002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:99: CString latin1String = commandName.latin1(); s/CString/const CString&/ to avoid copy ctor. https://codereview.chromium.org/1841143002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:101: const CommandNameEntry* result = std::lower_bound(std::begin(kCommandNameEntries), std::end(kCommandNameEntries), needle, CommandNameEntryCmp); Could you add |#include <iterator>|? Note: You can use lambda here: const CommandNameEntry* result = std::lower_bound(std::begin(kCommandNameEntries), std::end(kCommandNameEntries), needle, [](const CommandNameEntry& entry1, const CommandNameEntry& entry2) { return strcasecmp(entry1.name, entry2.name) < 0; }); Example: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
Hi yosin@, I've updated CL as per your comments, PTAL, thanks! https://codereview.chromium.org/1841143002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1841143002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:82: #define EDITOR_COMMAND_MAP_DECLARATION const CommandNameEntry kCommandNameEntries[] = On 2016/03/31 01:29:36, Yosi_UTC9 wrote: > V8 team invents good technique to define names. If we use it, we could write: > > "core/editing/commands/EditorCommandNames.h" > #define FOR_EACH_BLINK_EDITING_COMMAND_NAME(V) \ > V(Bold) V(Delete) V(Insert) ... > > const CommandEntry kCommandNameEntries[] = { > #define V(name) { #name, WebEditingCommandType::name }, > FOR_EACH_BLINK_EDITING_COMMAND_NAME(V) > #undef V > }; > > In this way, we don't need to name file ".inc" and reserve macro names > |EDITOR_COMMAND_MAP| and |EDITOR_COMMAND_MAP_DECLARATION| > > Example: > https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/objects.h&q... Done. Nice technique. https://codereview.chromium.org/1841143002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:99: CString latin1String = commandName.latin1(); On 2016/03/31 01:29:37, Yosi_UTC9 wrote: > s/CString/const CString&/ to avoid copy ctor. Done. https://codereview.chromium.org/1841143002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:101: const CommandNameEntry* result = std::lower_bound(std::begin(kCommandNameEntries), std::end(kCommandNameEntries), needle, CommandNameEntryCmp); On 2016/03/31 01:29:37, Yosi_UTC9 wrote: > Could you add |#include <iterator>|? > > Note: You can use lambda here: > > const CommandNameEntry* result = > std::lower_bound(std::begin(kCommandNameEntries), std::end(kCommandNameEntries), > needle, > [](const CommandNameEntry& entry1, const CommandNameEntry& entry2) > { > return strcasecmp(entry1.name, entry2.name) < 0; > }); > > Example: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Done.
yosin@chromium.org changed reviewers: + tkent@chromium.org
lgtm +tkent@ for public/
https://codereview.chromium.org/1841143002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1841143002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1673: ASSERT(commandIndex >= 0 && commandIndex < static_cast<int>(arraysize(kEditorCommands))); Please do not add ASSERT in new code. https://codereview.chromium.org/1841143002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebEditingCommandType.h (right): https://codereview.chromium.org/1841143002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebEditingCommandType.h:12: // in chrome/trunk/src/tools/metrics/histograms/histograms.xml, and add the new I think "chrome/trunk/src/" is unnecessary. https://codereview.chromium.org/1841143002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebEditingCommandType.h:14: enum class WebEditingCommandType { Why do you want to expose WebEditingCommandType from Blink? This is not used from outside of Blink. This CL doesn't look necessary to me because this CL doesn't explain the motivation.
Description was changed from ========== Add enum class |WebEditingCommandType| for EditorCommand This CL adds |WebEditingCommandType| enum class in public/platform/ and provides conversion between string command name and enum type in EditorCommand.cpp. Also |EditorInternalCommand.idForUserMetrics| is replaced by |EditorInternalCommand.commandType| but remains the same enum value, so the user matrics won't be affected. BUG=596064 ========== to ========== Add enum class |WebEditingCommandType| for EditorCommand Motivation: Using strings for actions is error prone (see http://crbug.com/251275) and ideally we want to replace string names with enum (e.g. replace strings in https://goo.gl/VeUZFX). Also with enum we could eliminate the huge |EditorCommand::CommandMap| with switch statements to improve readability. This CL is the first step and adds |WebEditingCommandType| enum class in public/platform/ and provides conversion between string command name and enum type in EditorCommand.cpp. Also |EditorInternalCommand.idForUserMetrics| is replaced by |EditorInternalCommand.commandType| but remains the same enum value, so the user matrics won't be affected. BUG=596064 ==========
Description was changed from ========== Add enum class |WebEditingCommandType| for EditorCommand Motivation: Using strings for actions is error prone (see http://crbug.com/251275) and ideally we want to replace string names with enum (e.g. replace strings in https://goo.gl/VeUZFX). Also with enum we could eliminate the huge |EditorCommand::CommandMap| with switch statements to improve readability. This CL is the first step and adds |WebEditingCommandType| enum class in public/platform/ and provides conversion between string command name and enum type in EditorCommand.cpp. Also |EditorInternalCommand.idForUserMetrics| is replaced by |EditorInternalCommand.commandType| but remains the same enum value, so the user matrics won't be affected. BUG=596064 ========== to ========== Add enum class |WebEditingCommandType| for EditorCommand Motivation: Using strings for actions is error prone (see http://crbug.com/251275) and ideally we want to replace string names with enum (e.g. replace strings in https://goo.gl/VeUZFX). Also with enum we could eliminate the huge |EditorCommand::CommandMap| with switch statements to improve readability. This CL is the first step and adds |WebEditingCommandType| enum class in public/platform/ and provides conversion between string command name and enum type in EditorCommand.cpp. Also |EditorInternalCommand.idForUserMetrics| is replaced by |EditorInternalCommand.commandType| but remains the same enum value, so the user matrics won't be affected. BUG=596064 ==========
Hi tkent@, PTAL, thanks! https://codereview.chromium.org/1841143002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1841143002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1673: ASSERT(commandIndex >= 0 && commandIndex < static_cast<int>(arraysize(kEditorCommands))); On 2016/04/01 04:43:42, tkent wrote: > Please do not add ASSERT in new code. Done. https://codereview.chromium.org/1841143002/diff/80001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebEditingCommandType.h (right): https://codereview.chromium.org/1841143002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebEditingCommandType.h:12: // in chrome/trunk/src/tools/metrics/histograms/histograms.xml, and add the new On 2016/04/01 04:43:42, tkent wrote: > I think "chrome/trunk/src/" is unnecessary. Done. https://codereview.chromium.org/1841143002/diff/80001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebEditingCommandType.h:14: enum class WebEditingCommandType { On 2016/04/01 04:43:42, tkent wrote: > Why do you want to expose WebEditingCommandType from Blink? This is not used > from outside of Blink. > > This CL doesn't look necessary to me because this CL doesn't explain the > motivation. > Thanks for the review and sorry for the unclear CL description, I've updated motivation: ``` Motivation: Using strings for actions is error prone (see http://crbug.com/251275) and ideally we want to replace string names with enum (e.g. replace strings in https://goo.gl/VeUZFX). Also with enum we could eliminate the huge |EditorCommand::CommandMap| with switch statements to improve readability. ``` And since the original problem is the editing commands being passed into the APIs as WebStrings, we want to be able to pass enum instead. Or is there another place you want me to put this file? Thanks!
Thank you for the explanation. Introducing WebEditingCommandType.h is ok. https://codereview.chromium.org/1841143002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1841143002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:102: return strcasecmp(entry1.name, entry2.name) < 0; We should avoid to use C string functions such as strcasecmp.
https://codereview.chromium.org/1841143002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1841143002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:102: return strcasecmp(entry1.name, entry2.name) < 0; On 2016/04/04 00:35:56, unavailable_until_Apr_7 wrote: > We should avoid to use C string functions such as strcasecmp. > If we want to use a codePointCompare then we'd need to turn the CommandNameEntry into a table of non ro-data. (ie; an AtomicString or a WTFString). I think not doing the allocations is actually better here than forcing allocations and using a C++ API. There are other places in WebKit that already use strcasecmp... (and it exists in StringExtras.h). I do agree that with unicode using the C compare APIs can get ugly.. Ideally it would be nice if we could depend on base/strings/string_utils.h here is that a possibility?
Hi yosin@, can you take a look again please? Thanks! https://codereview.chromium.org/1841143002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1841143002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:102: return strcasecmp(entry1.name, entry2.name) < 0; On 2016/04/04 15:42:46, dtapuska wrote: > On 2016/04/04 00:35:56, unavailable_until_Apr_7 wrote: > > We should avoid to use C string functions such as strcasecmp. > > > > If we want to use a codePointCompare then we'd need to turn the CommandNameEntry > into a table of non ro-data. (ie; an AtomicString or a WTFString). I think not > doing the allocations is actually better here than forcing allocations and using > a C++ API. There are other places in WebKit that already use strcasecmp... (and > it exists in StringExtras.h). > > I do agree that with unicode using the C compare APIs can get ugly.. Ideally it > would be nice if we could depend on base/strings/string_utils.h here is that a > possibility? > > > > For example to get rid of |strcasecmp| I can turn |CommandNameEntry| into an AtomicString table and do the comparison |codePointCompareLessThan(s1.foldCase(), s2.foldCase())|, but as dtapuska@ mentioned I'm not sure if it worth the extra allocation costs... yosin@ what do you think? (It seems that tkent@ is not available)
On 2016/04/04 at 15:56:12, chongz wrote: > Hi yosin@, can you take a look again please? Thanks! > > https://codereview.chromium.org/1841143002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): > > https://codereview.chromium.org/1841143002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:102: return strcasecmp(entry1.name, entry2.name) < 0; > On 2016/04/04 15:42:46, dtapuska wrote: > > On 2016/04/04 00:35:56, unavailable_until_Apr_7 wrote: > > > We should avoid to use C string functions such as strcasecmp. > > > > > > > If we want to use a codePointCompare then we'd need to turn the CommandNameEntry > > into a table of non ro-data. (ie; an AtomicString or a WTFString). I think not > > doing the allocations is actually better here than forcing allocations and using > > a C++ API. There are other places in WebKit that already use strcasecmp... (and > > it exists in StringExtras.h). > > > > I do agree that with unicode using the C compare APIs can get ugly.. Ideally it > > would be nice if we could depend on base/strings/string_utils.h here is that a > > possibility? > > > > > > > > > > For example to get rid of |strcasecmp| I can turn |CommandNameEntry| into an AtomicString table and do the comparison |codePointCompareLessThan(s1.foldCase(), s2.foldCase())|, but as dtapuska@ mentioned I'm not sure if it worth the extra allocation costs... > > yosin@ what do you think? (It seems that tkent@ is not available) It seems Blink uses strcasecmp(), defined in "wtf/StringExtras.h". https://code.google.com/p/chromium/codesearch#search/&q=strcasecmp%20file:web... I think it is OK to use strcasecmp() now and it should be replaced with base::CompareCaseInsensitiveASCII() once it allowed. tkent@ will be back on April 8, he is in front of my spot. ;-) Anyway, please wait for tkent@ response.
https://codereview.chromium.org/1841143002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1841143002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:102: return strcasecmp(entry1.name, entry2.name) < 0; On 2016/04/04 at 15:56:12, chongz wrote: > On 2016/04/04 15:42:46, dtapuska wrote: > > On 2016/04/04 00:35:56, unavailable_until_Apr_7 wrote: > > > We should avoid to use C string functions such as strcasecmp. > > > > > > > If we want to use a codePointCompare then we'd need to turn the CommandNameEntry > > into a table of non ro-data. (ie; an AtomicString or a WTFString). I think not > > doing the allocations is actually better here than forcing allocations and using > > a C++ API. There are other places in WebKit that already use strcasecmp... (and > > it exists in StringExtras.h). > > > > I do agree that with unicode using the C compare APIs can get ugly.. Ideally it > > would be nice if we could depend on base/strings/string_utils.h here is that a > > possibility? > > > > > > > > > > For example to get rid of |strcasecmp| I can turn |CommandNameEntry| into an AtomicString table and do the comparison |codePointCompareLessThan(s1.foldCase(), s2.foldCase())|, but as dtapuska@ mentioned I'm not sure if it worth the extra allocation costs... > > yosin@ what do you think? (It seems that tkent@ is not available) String -> CString conversion needs to allocate and copy the string. I don't think Patch Set 6 is the best way. I recommend: * Introducing codePointCompareIgnoringASCIICase(const String&, const char*), or * Building a HashMap to map String to WebEditingCommandType.
I've added method |codePointCompareIgnoringASCIICase()|, PTAL, thanks!
Patchset #7 (id:120001) has been deleted
lgtm
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/1841143002/#ps140001 (title: "Introducing |codePointCompareIgnoringASCIICase()|")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1841143002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1841143002/140001
Message was sent while issue was closed.
Description was changed from ========== Add enum class |WebEditingCommandType| for EditorCommand Motivation: Using strings for actions is error prone (see http://crbug.com/251275) and ideally we want to replace string names with enum (e.g. replace strings in https://goo.gl/VeUZFX). Also with enum we could eliminate the huge |EditorCommand::CommandMap| with switch statements to improve readability. This CL is the first step and adds |WebEditingCommandType| enum class in public/platform/ and provides conversion between string command name and enum type in EditorCommand.cpp. Also |EditorInternalCommand.idForUserMetrics| is replaced by |EditorInternalCommand.commandType| but remains the same enum value, so the user matrics won't be affected. BUG=596064 ========== to ========== Add enum class |WebEditingCommandType| for EditorCommand Motivation: Using strings for actions is error prone (see http://crbug.com/251275) and ideally we want to replace string names with enum (e.g. replace strings in https://goo.gl/VeUZFX). Also with enum we could eliminate the huge |EditorCommand::CommandMap| with switch statements to improve readability. This CL is the first step and adds |WebEditingCommandType| enum class in public/platform/ and provides conversion between string command name and enum type in EditorCommand.cpp. Also |EditorInternalCommand.idForUserMetrics| is replaced by |EditorInternalCommand.commandType| but remains the same enum value, so the user matrics won't be affected. BUG=596064 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add enum class |WebEditingCommandType| for EditorCommand Motivation: Using strings for actions is error prone (see http://crbug.com/251275) and ideally we want to replace string names with enum (e.g. replace strings in https://goo.gl/VeUZFX). Also with enum we could eliminate the huge |EditorCommand::CommandMap| with switch statements to improve readability. This CL is the first step and adds |WebEditingCommandType| enum class in public/platform/ and provides conversion between string command name and enum type in EditorCommand.cpp. Also |EditorInternalCommand.idForUserMetrics| is replaced by |EditorInternalCommand.commandType| but remains the same enum value, so the user matrics won't be affected. BUG=596064 ========== to ========== Add enum class |WebEditingCommandType| for EditorCommand Motivation: Using strings for actions is error prone (see http://crbug.com/251275) and ideally we want to replace string names with enum (e.g. replace strings in https://goo.gl/VeUZFX). Also with enum we could eliminate the huge |EditorCommand::CommandMap| with switch statements to improve readability. This CL is the first step and adds |WebEditingCommandType| enum class in public/platform/ and provides conversion between string command name and enum type in EditorCommand.cpp. Also |EditorInternalCommand.idForUserMetrics| is replaced by |EditorInternalCommand.commandType| but remains the same enum value, so the user matrics won't be affected. BUG=596064 Committed: https://crrev.com/2dcc34c1e14d6847a6340da3d03fbe5e04801bfb Cr-Commit-Position: refs/heads/master@{#386702} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/2dcc34c1e14d6847a6340da3d03fbe5e04801bfb Cr-Commit-Position: refs/heads/master@{#386702} |
