Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Description • 6 years ago

There is non-virtual overload for HTMLEditor::GetSelectedElement(), however, it calls nsIHTMLEditor::GetSelectedElement() which is of course a virtual method.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 1 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce3a868a73b189397a6ad19764b3c1a848f32402

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 2 • 6 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 3 • 6 years ago

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().

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 4 • 6 years ago

Using nsAtom makes the method faster and simpler, although the caller needs
to atomize lower-cased string.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 5 • 6 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 6 • 6 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 7 • 6 years ago

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.

	Makoto Kato [:m_kato]
	Comment 8 • 6 years ago

Comment on attachment 8999591 [details]
Bug 1482019 - part 0: Add automated tests for nsIHTMLEditor.getSelectedElement()

Makoto Kato [:m_kato] has approved the revision.

	Makoto Kato [:m_kato]
	Comment 9 • 6 years ago

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.

	Makoto Kato [:m_kato]
	Comment 10 • 6 years ago

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.

	Makoto Kato [:m_kato]
	Comment 11 • 6 years ago

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.

	Makoto Kato [:m_kato]
	Comment 12 • 6 years ago

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.

	Makoto Kato [:m_kato]
	Comment 13 • 6 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 14 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3924883774bd631343dfb31a15c4170b8550e982

	Pulsebot
	Comment 15 • 6 years ago

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

	Eliza Balazs [:ebalazs_]
	Comment 16 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a61cdb1a654c
https://hg.mozilla.org/mozilla-central/rev/9b46ca68f3b0
https://hg.mozilla.org/mozilla-central/rev/40fbaeccbd89
https://hg.mozilla.org/mozilla-central/rev/f6053ad1d49a
https://hg.mozilla.org/mozilla-central/rev/66e9a30a6b1a
https://hg.mozilla.org/mozilla-central/rev/57e568a7f933