Closed Bug 1549661 Opened 4 months ago Closed 4 months ago

Split EditorCommand::DoCommandParams() for taking specific param type to avoid creating nsCommandParam instance temporarily

Categories

(Core :: Editor, task, P1)

task

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox68 --- wontfix
firefox69 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(7 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

Currently, when EditorCommand::DoCommandParmas() users call it, they need to create nsCommandParmas instance only for passing one type argument, e.g., a bool value, a string value, etc. This is really unnecessary (but expensive) run-time cost. So, EditorCommand should have an API to return which type of argument is required and nsIControllerCommand::DoCommandParams() should check the result and call proper new method.

Most EditorCommand classes don't require additional params for executing
a command. All of them just calls their DoCommand() or returns same result.
So, we can create new virtual method,
EditorCommand::DoCommandParam(Command aCommand, TextEditor& aTextEditor),
which just delegates to DoCommand().

This patch adds some undeclared commands but which are handled by
EditorCommand subclasses, and changes CommandInt type from int8_t to
uint8_t since the count of Command items becomes over 128.

We should use Maybe for bool because some command may treat the default
value when the parameter is omitted as true or `false. Although, current
implementation does not do that.

If nsIControllerCommand::DoCommandParams() is called with nullptr for its
aParams, this patch sets VoidCString() to DoCommandParam() for making
each implementation be able to consider whether the case is an error or
treat it as specific default value.

If nsIControllerCommand::DoCommandParams() is called with nullptr for its
aParams, this patch sets VoidString() to DoCommandParam() for making
each implementation be able to consider whether the case is an error or
treat it as specific default value.

Only MultiStateCommandBase::DoCommandParams() allows CString param and
String param (the former is preferred). This patch makes
EditorCommand::DoCommandParams() aware of this case.

If nsIControllerCommand::DoCommandParams() is called without aParams or
nsITransferable pointer, this patch sets nullptr to aTransferableParam for
DoCommandParam(). This allows each implementation to choose ignore or
return error.

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.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/cc344bcbf49d
part 1: Create `EditorCommand::DoCommandParam(Command aCommand, TextEditor& aTextEditor)` and make it just call `DoCommand()` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/b9fd628d913a
part 2: Create `EditorCommand::DoCommandParam(Command aCommand, const Maybe<bool>& aBoolParam, TextEditor& aTextEditor)` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/7c1521b96d93
part 3: Create `EditorCommand::DoCommandParam(Command aCommand, const nsACString& aCStringParam, TextEditor& aTextEditor)` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/c3c796dd6070
part 4: Create `EditorCommand::DoCommandParam(Command aCommand, const nsAString& aStringParam, TextEditor& aTextEditor)` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ed3667be6075
part 5: Make commands for `MultiStateCommandBase` take both `CString` and `String` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/bd77dd67175d
part 6: Create `EditorCommand::DoCommandParam(Command aCommand, nsITransferable* aTransferableParam, TextEditor& aTextEditor)` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/476139ca9d4a
part 7: Make GetInternalCommand() take nsCommandParams instead of nsAString r=m_kato
You need to log in before you can comment on or make changes to this bug.