1484131 - Split HTMLEditor::GetFirstSelectedCellInTable() for internal use

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P3)

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1484131 - part 0: Add automated tests for nsITableEditor::GetFirstSelectedCell()

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1484131 - part 1: Create HTMLEditor::GetFirstSelectedTableCellElement() for internal use of HTMLEditor::GetFirstSelectedCell()

HTMLEditor::GetFirstSelectedCell() is an XPCOM method, but used internally a
lot.  Therefore, we should create a non-virtual method for internal use.

This patch creates HTMLEditor::GetFirstSelectedTableCellElement(), and it
won't return NS_SUCCESS_EDITOR_ELEMENT_NOT_FOUND since nobody needs the
value.  It's enough to check whether the result is nullptr without error for
any callers.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1484131 - part 0: Add automated tests for nsITableEditor.getFirstSelectedCellInTable() and fixes a crash bug of it

This adds automated tests for nsITableEditor.getFirstSelectedCellInTable().
However, this test crashes due to regression of bug 1484128.  So, we need
to uplift only this fix into the Beta.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1484131 - part 1: Create HTMLEditor::CellAndIndexes struct to store cell element and its indexes in the <table>

If a cell element and its indexes in the <table> are stored in a struct,
it makes the user methods easier to read.  Therefore, this patch implement
such struct as HTMLEditor::CellAndIndexes and make it finds first selected
cell and indexes with its method.  Finally, this reimplement
HTMLEditor::GetFirstSelectedCellInTable() with it.

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 9008378 [details]
Bug 1484131 - part 0: Add automated tests for nsITableEditor.getFirstSelectedCellInTable() and fixes a crash bug of it

Makoto Kato [:m_kato] has approved the revision.

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 9008379 [details]
Bug 1484131 - part 1: Create HTMLEditor::CellAndIndexes struct to store cell element and its indexes in the <table>

Makoto Kato [:m_kato] has approved the revision.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/409d6dbcca46
part 0: Add automated tests for nsITableEditor.getFirstSelectedCellInTable() and fixes a crash bug of it r=m_kato
https://hg.mozilla.org/integration/autoland/rev/441e942242df
part 1: Create HTMLEditor::CellAndIndexes struct to store cell element and its indexes in the <table> r=m_kato

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9008378 [details]
Bug 1484131 - part 0: Add automated tests for nsITableEditor.getFirstSelectedCellInTable() and fixes a crash bug of it

Approval Request Comment
[Feature/Bug causing the regression]:
Although this is adding new automated test for legacy XPCOM API for editor, this detects and fixes a regression of bug 1484128.
[User impact if declined]:
Might meet the crash if user use inline-table-editing UI.
[Is this code covered by automated tests?]:
Yes, adding this is it.
[Has the fix been verified in Nightly?]:
Will be tested by this automated test (currently in autoland).
[Needs manual test from QE? If yes, steps to reproduce]: 
No. Without the fix, this automated test hits the crash.
[List of other uplifts needed for the feature/fix]:
No. (The other patch for this bug is also unnecessary, it's not good patch to uplift.)
[Is the change risky?]:
No.
[Why is the change risky/not risky?]:
Just fixing the mistake to access a raw pointer.
[String changes made/needed]:
No.

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/409d6dbcca46
https://hg.mozilla.org/mozilla-central/rev/441e942242df

Comment 14

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Comment on attachment 9008378 [details]
Bug 1484131 - part 0: Add automated tests for nsITableEditor.getFirstSelectedCellInTable() and fixes a crash bug of it

Approved for 63 beta 7, thanks.

Comment 15

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/releases/mozilla-beta/rev/504e97de9d59

Comment 16

[Marco Castelluccio [:marco]]

So this is a regression from bug 1484128? Or does this bug contain both a fix for the regression and a refactoring for GetFirstSelectedCellInTable? If this is the case, in the future it would be better to split things: a bug for the regression and a bug for the refactoring which is not directly fixing a regression.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Marco Castelluccio [:marco] from comment #16)
> So this is a regression from bug 1484128? Or does this bug contain both a
> fix for the regression and a refactoring for GetFirstSelectedCellInTable? If
> this is the case, in the future it would be better to split things: a bug
> for the regression and a bug for the refactoring which is not directly
> fixing a regression.

This bug is for refactoring, and I found the regression when I was writing the test for safe of refactoring. Additionally, I don't have much time. Therefore, I did it as what I planned.

Comment 18

[Marco Castelluccio [:marco]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (offline: 9/21-9/30) from comment #17)
> (In reply to Marco Castelluccio [:marco] from comment #16)
> > So this is a regression from bug 1484128? Or does this bug contain both a
> > fix for the regression and a refactoring for GetFirstSelectedCellInTable? If
> > this is the case, in the future it would be better to split things: a bug
> > for the regression and a bug for the refactoring which is not directly
> > fixing a regression.
> 
> This bug is for refactoring, and I found the regression when I was writing
> the test for safe of refactoring. Additionally, I don't have much time.
> Therefore, I did it as what I planned.

I understand that it takes a little more time, but there are several benefits in doing things in a self-contained way (both for tracking purposes, and for building automated tools and for analyzing Bugzilla/VCS data). It's not a rule, but it helps us, so you don't have to do it, but at least take it into account.