Move implementation of nsIHTMLEditor::GetSelectedElement() to HTMLEditor::GetSelectedElement()

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
10 months ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks 1 bug)

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(6 attachments)

46 bytes, text/x-phabricator-request
m_kato
: review+
Details | Review
46 bytes, text/x-phabricator-request
m_kato
: review+
Details | Review
46 bytes, text/x-phabricator-request
m_kato
: review+
Details | Review
46 bytes, text/x-phabricator-request
m_kato
: review+
Details | Review
46 bytes, text/x-phabricator-request
m_kato
: review+
Details | Review
46 bytes, text/x-phabricator-request
m_kato
: review+
Details | Review
There is non-virtual overload for HTMLEditor::GetSelectedElement(), however, it calls nsIHTMLEditor::GetSelectedElement() which is of course a virtual method.
The HTMLEditor::GetSelectedElement() is ugly.  Probably, it's checked only with
expected simple cases since the result does not make sense in some cases.

For example, when Selection is collapsed, it returns an element only when
"href" is specified with the argument.  When Selection selects only one
element node (including its children), it quickly returns the node, however,
in the slow path, it returns second element node if first element node matches
with the argument.  Or returns first element ndoe if it does not match with
the argument.

For preventing regressions, new test is designed to keep current odd behavior.
For making each diff compact, this bug needs some patches.

First of all, this patch moves implementation of
nsIHTMLEditor::GetSelectedElement() to new non-virtual method
HTMLEditor::GetSelectedNode().
Using nsAtom makes the method faster and simpler, although the caller needs
to atomize lower-cased string.
Some variables in HTMLEditor::GetSelectedNode() are declared very large block
they are not used and/or referred.  So, we can get rid of some variables or
move smaller block.
The first check of the last block of HTMLEditor::GetSelectedNode() can use
early-return style.  Additionally, |currange| is not necessary since Selection
is not changed since the method retrieved first range.  So, we can get rid of
|currange| and the |nullptr| case of it.
If HTMLEditor::GetSelectedNode() is called with nullptr for aTagName,
the first block may return non-element node.  In such case, we should return
nullptr without error for now (since I have no idea which element node is
a good node to return).

Then, we can rename it to GetSelectedElement() and can replace existing
GetSelectedElement() with the new one.
Comment on attachment 8999591 [details]
Bug 1482019 - part 0: Add automated tests for nsIHTMLEditor.getSelectedElement()

Makoto Kato [:m_kato] has approved the revision.
Attachment #8999591 - Flags: review+
Comment on attachment 8999594 [details]
Bug 1482019 - part 1: Create non-virtual method HTMLEditor::GetSelectedNode() for implementing nsIHTMLEditor::GetSelectedElement()

Makoto Kato [:m_kato] has approved the revision.
Attachment #8999594 - Flags: review+
Comment on attachment 8999601 [details]
Bug 1482019 - part 2: Make HTMLEditor::GetSelectedNode() take nsAtom* for element name

Makoto Kato [:m_kato] has approved the revision.
Attachment #8999601 - Flags: review+
Comment on attachment 8999605 [details]
Bug 1482019 - part 3: Minimize some scope of auto varaiables in HTMLEditor::GetSelectedNode()

Makoto Kato [:m_kato] has approved the revision.
Attachment #8999605 - Flags: review+
Comment on attachment 8999608 [details]
Bug 1482019 - part 4: Reduce the indent level of the last block in HTMLEditor::GetSelectedNode()

Makoto Kato [:m_kato] has approved the revision.
Attachment #8999608 - Flags: review+
Comment on attachment 8999610 [details]
Bug 1482019 - part 5: Make HTMLEditor::GetSelectedNode() never return non-element node and change its name to GetSelectedElement()

Makoto Kato [:m_kato] has approved the revision.
Attachment #8999610 - Flags: review+
Priority: -- → P3
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a61cdb1a654c
part 0: Add automated tests for nsIHTMLEditor.getSelectedElement() r=m_kato
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b46ca68f3b0
part 1: Create non-virtual method HTMLEditor::GetSelectedNode() for implementing nsIHTMLEditor::GetSelectedElement() r=m_kato
https://hg.mozilla.org/integration/mozilla-inbound/rev/40fbaeccbd89
part 2: Make HTMLEditor::GetSelectedNode() take nsAtom* for element name r=m_kato
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6053ad1d49a
part 3: Minimize some scope of auto varaiables in HTMLEditor::GetSelectedNode() r=m_kato
https://hg.mozilla.org/integration/mozilla-inbound/rev/66e9a30a6b1a
part 4: Reduce the indent level of the last block in HTMLEditor::GetSelectedNode() r=m_kato
https://hg.mozilla.org/integration/mozilla-inbound/rev/57e568a7f933
part 5: Make HTMLEditor::GetSelectedNode() never return non-element node and change its name to GetSelectedElement() r=m_kato
You need to log in before you can comment on or make changes to this bug.