1484126 - Split HTMLEditor::GetCellDataAt() for internal use

Status: RESOLVED FIXED

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

Comment 1

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

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

Comment 2

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

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

Comment 3

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

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

Comment 4

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

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

Comment 5

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

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

Comment 6

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

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

Comment 7

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

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

Comment 8

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

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

Comment 9

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

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

Comment 10

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

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

Comment 11

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

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

Comment 12

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

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

Comment 13

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

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

Comment 14

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

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

Comment 15

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

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

Comment 16

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

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

Comment 17

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

Attachment: Bug 1484126 - part 0: Add automated tests for nsITableEditor.getCellDataAt()

Comment 18

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

Attachment: Bug 1484126 - part 1: Create CellData struct which implements nsITableEditor::GetCellDataAt()

nsITableEditor::GetCellDataAt() is an XPCOM method but used internally a lot.
Additionally, it scatters a lot of variables (including unused) since it takes
a lot of out arguments to return.  Therefore, this should be implemented as
struct and users should refer each member as result.

This does not replaces the callers yet since it's too risky if the caller is
not tested by automated test.  Following patches will replace the method call
and each variable step by step.

Comment 19

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

Attachment: Bug 1484126 - part 2: Make all nsITableEditor::GetCellDataAt() use CellData

This patch makes all nsITableEditor::GetCellDataAt() use CellData, and if
each caller checks the result of GetCellDataAt() with NS_FAILED(), this
replaces it with CellData::FailedOrNotFound() since GetCellDataAt() returns
error even when it does not find a cell element.  Finally, copies each
CellData member to the variable which received corresponding value from
GetCellDataAt() for making this change safe.  Note that for easier to review,
the copying blocks have odd indent.  Those variables will be removed or
corrected the indent by the following patches.

Comment 20

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

Attachment: Bug 1484126 - part 3: Make all CellData users use CellData::mCurrent.mRow

Comment 21

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

Attachment: Bug 1484126 - part 4: Make all CellData users use CellData::mCurrent.mColumn

Comment 22

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

Attachment: Bug 1484126 - part 5: Make all CellData users refer CellData::mElement directly

Comment 23

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

Attachment: Bug 1484126 - part 6: Make all CellData users refer CellData::mFirst.mRow directly

Comment 24

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

Attachment: Bug 1484126 - part 7: Replace all comparisons of CellData::mFirst.mRow and CellData::mCurrent.mRow with CellData::IsSpannedFromOtherRow()

Comment 25

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

Attachment: Bug 1484126 - part 8: Replace |CellData::mCurrent.mRow - CellData::mFirst.mRow| with CellData::NumberOfPrecedingRows()

Comment 26

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

Attachment: Bug 1484126 - part 9: Make all CellData users refer CellData::mFirst.mColumn directly

Comment 27

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

Attachment: Bug 1484126 - part 10: Replace all comparisons of CellData::mFirst.mColumn and CellData::mCurrent.mColumn with CellData::IsSpannedFromOtherColumn() or CellData::IsSpannedFromOtherRowOrColumn()

Comment 28

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

Attachment: Bug 1484126 - part 11: Make all CellData users refer CellData.mRowSpan directly

Comment 29

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

Attachment: Bug 1484126 - part 12: Make all CellData users refer CellData::mColSpan directly

Comment 30

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

Attachment: Bug 1484126 - part 13: Make all CellData users refer CellData::mEffectiveRowSpan directly

Comment 31

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

Attachment: Bug 1484126 - part 14: Replace |CellData::mEffectiveRowSpan - 1| with CellData::NumberOfFollowingRows()

Comment 32

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

Attachment: Bug 1484126 - part 15: Make all CellData users refer CellData::mEffectiveColspan directly

Comment 33

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

Attachment: Bug 1484126 - part 16: Replace |CellData::mEffectiveColSpan - 1| with CellData::NumberOfFollowingColumns()

Comment 34

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

Attachment: Bug 1484126 - part 17: Make all CellData users refer CellData::mIsSelected directly

Comment 35

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

Attachment: Bug 1484126 - part 18: Make HTMLEditor::InsertTableCellsWithTransaction() use CellData::NextColumnIndex() instead of |CellData::mCurrent::mColumn + CellData::mColSpan|

Comment 36

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

Attachment: Bug 1484126 - part 19: Replace |CellData::mCurrent.mColumn + CellData::NumberOfFollowingColumns()| with CellData::LastColumnIndex()

Comment 37

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

Attachment: Bug 1484126 - part 20: Replace |CellData::mFirst.mColumn + CellData::mEffectiveColSpan| with CellData::NextColumnIndex()

Comment 38

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

Attachment: Bug 1484126 - part 21: Rewrite some loops in HTMLTableEditor.cpp with CellData::NextColumnIndex() or CellData::NextRowIndex()

A lot of loops in HTMLTableEditor.cpp scans cells along column-axis or
row-axis.  In those cases, they need to skip columns/rows which are spanned
from prior column/row.  So, we can rewrite them with CellData::NextColumnIndex()
or CellData::NextRowColumn().

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7f0492bf262
part 0: Add automated tests for nsITableEditor.getCellDataAt() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/bf74fc63e816
part 1: Create CellData struct which implements nsITableEditor::GetCellDataAt() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/d3609fcd3c88
part 2: Make all nsITableEditor::GetCellDataAt() use CellData r=m_kato
https://hg.mozilla.org/integration/autoland/rev/24e31e2070fa
part 3: Make all CellData users use CellData::mCurrent.mRow r=m_kato
https://hg.mozilla.org/integration/autoland/rev/539a4c3e1946
part 4: Make all CellData users use CellData::mCurrent.mColumn r=m_kato
https://hg.mozilla.org/integration/autoland/rev/181c07ae07c5
part 5: Make all CellData users refer CellData::mElement directly r=m_kato
https://hg.mozilla.org/integration/autoland/rev/0f9a168cb7c8
part 6: Make all CellData users refer CellData::mFirst.mRow directly r=m_kato
https://hg.mozilla.org/integration/autoland/rev/448d887560d4
part 7: Replace all comparisons of CellData::mFirst.mRow and CellData::mCurrent.mRow with CellData::IsSpannedFromOtherRow() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/b23363aa593d
part 8: Replace |CellData::mCurrent.mRow - CellData::mFirst.mRow| with CellData::NumberOfPrecedingRows() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/8f7afde0a6f6
part 9: Make all CellData users refer CellData::mFirst.mColumn directly r=m_kato
https://hg.mozilla.org/integration/autoland/rev/3e9c08f4f705
part 10: Replace all comparisons of CellData::mFirst.mColumn and CellData::mCurrent.mColumn with CellData::IsSpannedFromOtherColumn() or CellData::IsSpannedFromOtherRowOrColumn() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/6ab5dd75fbf4
part 11: Make all CellData users refer CellData.mRowSpan directly r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e0cea47221b8
part 12: Make all CellData users refer CellData::mColSpan directly r=m_kato
https://hg.mozilla.org/integration/autoland/rev/3e1b1e01e6a4
part 13: Make all CellData users refer CellData::mEffectiveRowSpan directly r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ed17432df28c
part 14: Replace |CellData::mEffectiveRowSpan - 1| with CellData::NumberOfFollowingRows() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e5c8e5cbaf3d
part 15: Make all CellData users refer CellData::mEffectiveColspan directly r=m_kato
https://hg.mozilla.org/integration/autoland/rev/4c70964d90fd
part 16: Replace |CellData::mEffectiveColSpan - 1| with CellData::NumberOfFollowingColumns() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/cbb895793f89
part 17: Make all CellData users refer CellData::mIsSelected directly r=m_kato
https://hg.mozilla.org/integration/autoland/rev/8a2722c90ac9
part 18: Make HTMLEditor::InsertTableCellsWithTransaction() use CellData::NextColumnIndex() instead of |CellData::mCurrent::mColumn + CellData::mColSpan| r=m_kato
https://hg.mozilla.org/integration/autoland/rev/35c3aff251f0
part 19: Replace |CellData::mCurrent.mColumn + CellData::NumberOfFollowingColumns()| with CellData::LastColumnIndex() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/19605351b361
part 20: Replace |CellData::mFirst.mColumn + CellData::mEffectiveColSpan| with CellData::NextColumnIndex() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/92c8efea7714
part 21: Rewrite some loops in HTMLTableEditor.cpp with CellData::NextColumnIndex() or CellData::NextRowIndex() r=m_kato

Comment 40

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/d7f0492bf262
https://hg.mozilla.org/mozilla-central/rev/bf74fc63e816
https://hg.mozilla.org/mozilla-central/rev/d3609fcd3c88
https://hg.mozilla.org/mozilla-central/rev/24e31e2070fa
https://hg.mozilla.org/mozilla-central/rev/539a4c3e1946
https://hg.mozilla.org/mozilla-central/rev/181c07ae07c5
https://hg.mozilla.org/mozilla-central/rev/0f9a168cb7c8
https://hg.mozilla.org/mozilla-central/rev/448d887560d4
https://hg.mozilla.org/mozilla-central/rev/b23363aa593d
https://hg.mozilla.org/mozilla-central/rev/8f7afde0a6f6
https://hg.mozilla.org/mozilla-central/rev/3e9c08f4f705
https://hg.mozilla.org/mozilla-central/rev/6ab5dd75fbf4
https://hg.mozilla.org/mozilla-central/rev/e0cea47221b8
https://hg.mozilla.org/mozilla-central/rev/3e1b1e01e6a4
https://hg.mozilla.org/mozilla-central/rev/ed17432df28c
https://hg.mozilla.org/mozilla-central/rev/e5c8e5cbaf3d
https://hg.mozilla.org/mozilla-central/rev/4c70964d90fd
https://hg.mozilla.org/mozilla-central/rev/cbb895793f89
https://hg.mozilla.org/mozilla-central/rev/8a2722c90ac9
https://hg.mozilla.org/mozilla-central/rev/35c3aff251f0
https://hg.mozilla.org/mozilla-central/rev/19605351b361
https://hg.mozilla.org/mozilla-central/rev/92c8efea7714