Bug 1549661 - part 7: Make GetInternalCommand() take nsCommandParams instead of nsAString r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Tue, 21 May 2019 07:47:51 +0000
changeset 474698 476139ca9d4a5802cfd786fccb1fd87d088e78fa
parent 474697 bd77dd67175d25744b092e18f4a074bb15971fe4
child 474699 92a16e67af5e23e7dd5f6812bb264c48be6c8ad5
push id113168
push userrmaries@mozilla.com
push dateTue, 21 May 2019 16:39:23 +0000
treeherdermozilla-inbound@3c0f78074b72 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1549661
milestone69.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1549661 - part 7: Make GetInternalCommand() take nsCommandParams instead of nsAString r=m_kato `GetInternalCommand()` is currently used only by `EditorCommand` and it treats the additional parameter only when given command is `cmd_align`. However, the value is complicated since `AlignCommand` allows both `CString` value and `String` value. Therefore, `EditorCommand::DoCommandParams()` may fail to solve `cmd_align` to a `Command` value without checking both of them. Therefore, it must make sense that `GetInternalCommand()` take `nsCommandParams` as optional argument and check it only when given command matches `cmd_align`. Then, we don't need to waste unnecessary run-time cost. Note that this bug has been hidden since `AlignCommand` class does not refer the `Command` value but refers only `nsCommandParams`. However, the previous patch makes `EditorCommand::GetParamType()` not allow `Command::DoNothing`. Therefore, we need this follow-up fix now. Differential Revision: https://phabricator.services.mozilla.com/D30501
editor/libeditor/EditorCommands.cpp
widget/EventForwards.h
widget/WidgetEventImpl.cpp
--- a/editor/libeditor/EditorCommands.cpp
+++ b/editor/libeditor/EditorCommands.cpp
@@ -75,26 +75,18 @@ EditorCommand::DoCommandParams(const cha
                                nsISupports* aCommandRefCon) {
   if (NS_WARN_IF(!aCommandName) || NS_WARN_IF(!aCommandRefCon)) {
     return NS_ERROR_INVALID_ARG;
   }
   nsCOMPtr<nsIEditor> editor = do_QueryInterface(aCommandRefCon);
   if (NS_WARN_IF(!editor)) {
     return NS_ERROR_INVALID_ARG;
   }
-  Command command;
   nsCommandParams* params = aParams ? aParams->AsCommandParams() : nullptr;
-  if (params) {
-    nsAutoString value;
-    params->GetString(aCommandName, value);
-    command = GetInternalCommand(aCommandName, value);
-  } else {
-    command = GetInternalCommand(aCommandName);
-  }
-
+  Command command = GetInternalCommand(aCommandName, params);
   EditorCommandParamType paramType = EditorCommand::GetParamType(command);
   if (paramType == EditorCommandParamType::None) {
     nsresult rv =
         DoCommandParam(command, MOZ_KnownLive(*editor->AsTextEditor()));
     NS_WARNING_ASSERTION(
         NS_SUCCEEDED(rv),
         "Failed to do command from nsIControllerCommand::DoCommandParams()");
     return rv;
--- a/widget/EventForwards.h
+++ b/widget/EventForwards.h
@@ -6,16 +6,18 @@
 #ifndef mozilla_EventForwards_h__
 #define mozilla_EventForwards_h__
 
 #include <stdint.h>
 
 #include "nsStringFwd.h"
 #include "nsTArray.h"
 
+class nsCommandParams;
+
 /**
  * XXX Following enums should be in BasicEvents.h.  However, currently, it's
  *     impossible to use foward delearation for enum.
  */
 
 /**
  * Return status for event processors.
  */
@@ -215,31 +217,28 @@ enum class Command : CommandInt {
 #undef NS_DEFINE_COMMAND
 #undef NS_DEFINE_COMMAND_WITH_PARAM
 #undef NS_DEFINE_COMMAND_NO_EXEC_COMMAND
 
 const char* ToChar(Command aCommand);
 
 /**
  * Return a command value for aCommandName.
- * NOTE: We use overloads instead of default constructor with EmptyString()
- *       because using it here requires to include another header file but
- *       this header file shouldn't include other header files as far as
- *       possible to avoid making rebuild time of them longer.
  * XXX: Is there a better place to put `Command` related methods instead of
  *      global scope in `mozilla` namespace?
  *
- * @param aCommandName  Should be a XUL command name like "cmd_bold" (case
- *                      sensitive).
- * @param aValue        Additional parameter value of aCommandName.
- *                      It depends on the command whethere it's case sensitive
- *                      or not.
+ * @param aCommandName          Should be a XUL command name like "cmd_bold"
+ *                              (case sensitive).
+ * @param aCommandparams        Additional parameter value of aCommandName.
+ *                              Can be nullptr, but if aCommandName requires
+ *                              additional parameter and sets this to nullptr,
+ *                              will return Command::DoNothing with warning.
  */
-Command GetInternalCommand(const char* aCommandName);
-Command GetInternalCommand(const char* aCommandName, const nsAString& aValue);
+Command GetInternalCommand(const char* aCommandName,
+                           const nsCommandParams* aCommandParams = nullptr);
 
 }  // namespace mozilla
 
 /**
  * All header files should include this header instead of *Events.h.
  */
 
 namespace mozilla {
--- a/widget/WidgetEventImpl.cpp
+++ b/widget/WidgetEventImpl.cpp
@@ -9,16 +9,17 @@
 #include "mozilla/EventStateManager.h"
 #include "mozilla/InternalMutationEvent.h"
 #include "mozilla/MiscEvents.h"
 #include "mozilla/MouseEvents.h"
 #include "mozilla/Preferences.h"
 #include "mozilla/TextEvents.h"
 #include "mozilla/TouchEvents.h"
 #include "mozilla/dom/KeyboardEventBinding.h"
+#include "nsCommandParams.h"
 #include "nsContentUtils.h"
 #include "nsIContent.h"
 #include "nsPrintfCString.h"
 
 namespace mozilla {
 
 /******************************************************************************
  * Global helper methods
@@ -178,38 +179,48 @@ SelectionType ToSelectionType(TextRangeT
 }
 
 /******************************************************************************
  * non class method implementation
  ******************************************************************************/
 
 static nsDataHashtable<nsDepCharHashKey, Command>* sCommandHashtable = nullptr;
 
-Command GetInternalCommand(const char* aCommandName) {
-  return GetInternalCommand(aCommandName, EmptyString());
-}
-
-Command GetInternalCommand(const char* aCommandName, const nsAString& aParam) {
+Command GetInternalCommand(const char* aCommandName,
+                           const nsCommandParams* aCommandParams) {
   if (!aCommandName) {
     return Command::DoNothing;
   }
 
   // Special cases for "cmd_align".  It's mapped to multiple internal commands
   // with additional param.  Therefore, we cannot handle it with the hashtable.
   if (!strcmp(aCommandName, "cmd_align")) {
-    if (aParam.LowerCaseEqualsASCII("left")) {
+    if (NS_WARN_IF(!aCommandParams)) {
+      return Command::DoNothing;
+    }
+    nsAutoCString cValue;
+    nsresult rv = aCommandParams->GetCString("state_attribute", cValue);
+    if (NS_FAILED(rv)) {
+      nsString value;  // Avoid copying the string buffer with using nsString.
+      rv = aCommandParams->GetString("state_attribute", value);
+      if (NS_WARN_IF(NS_FAILED(rv))) {
+        return Command::DoNothing;
+      }
+      cValue = NS_ConvertUTF16toUTF8(value);
+    }
+    if (cValue.LowerCaseEqualsASCII("left")) {
       return Command::FormatJustifyLeft;
     }
-    if (aParam.LowerCaseEqualsASCII("right")) {
+    if (cValue.LowerCaseEqualsASCII("right")) {
       return Command::FormatJustifyRight;
     }
-    if (aParam.LowerCaseEqualsASCII("center")) {
+    if (cValue.LowerCaseEqualsASCII("center")) {
       return Command::FormatJustifyCenter;
     }
-    if (aParam.LowerCaseEqualsASCII("justify")) {
+    if (cValue.LowerCaseEqualsASCII("justify")) {
       return Command::FormatJustifyFull;
     }
     return Command::DoNothing;
   }
 
   if (!sCommandHashtable) {
     sCommandHashtable = new nsDataHashtable<nsDepCharHashKey, Command>();
 #define NS_DEFINE_COMMAND(aName, aCommandStr) \