1635709 - Clean up code around `nsFrameSelection::HandleDrag`

Status: RESOLVED FIXED

(Core :: DOM: Selection, enhancement)

Comment 1

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 1) Rename `nsFrameSelection::SetCaretBidiLevel` to `SetCaretBidiLevelAndMaybeSchedulePaint`. r=hsivonen

The old name was misleading.

Comment 2

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 2) Add unit to `nsITimer`'s `init` methods. r=hsivonen

Selection's nsAutoScrollTimer uses it and it's clearer when the unit
is known.

Depends on D74051

Comment 3

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 3) Add unit to `Selection::StartAutoScrollTimer`'s delay argument. r=hsivonen

Depends on D74052

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2af59ac0778c
part 1) Rename `nsFrameSelection::SetCaretBidiLevel` to `SetCaretBidiLevelAndMaybeSchedulePaint`. r=hsivonen
https://hg.mozilla.org/integration/autoland/rev/b8e38d56a744
part 2) Add unit to `nsITimer`'s `init` methods. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/8a2c548a7d28
part 3) Add unit to `Selection::StartAutoScrollTimer`'s delay argument. r=hsivonen

Comment 5

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2af59ac0778c
https://hg.mozilla.org/mozilla-central/rev/b8e38d56a744
https://hg.mozilla.org/mozilla-central/rev/8a2c548a7d28

Comment 6

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 4) Transform `Selection::DoAutoScroll` to `AutoScroller::DoAutoScroll`. r=masayuki

Allows defriending AutoScroller from Selection and removes the
direct dependency of AutoScroller to Selection.

Comment 7

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 5) Annotate `Selection::ReplaceAnchorFocusRange` with `MOZ_CAN_RUN_SCRIPT`. r=masayuki

Depends on D74382

Comment 8

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 6) Rename `nsFrameSelection::*DesiredPos` to `*DesiredCaretPos`. r=masayuki

nsFrameSelection has (too) many responsibilites, this adds some
clarification.

Depends on D74383

Comment 9

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 7) Move `nsFrameSelection::SetDesiredCaretPos` and `FetchDesiredCaretPos` to `DesiredCaretPos`. r=masayuki

Depends on D74615

Comment 10

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 8) Remove superfluously setting `mIsSet` in `nsFrameSelection`'s constructor. r=masayuki

Depends on D74616

Comment 11

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 9) Add `DesiredCaretPos::Invalidate`. r=masayuki

Depends on D74617

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/626d32b5cebe
part 4) Transform `Selection::DoAutoScroll` to `AutoScroller::DoAutoScroll`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/8faefd45845b
part 5) Annotate `Selection::ReplaceAnchorFocusRange` with `MOZ_CAN_RUN_SCRIPT`. r=masayuki

Comment 13

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/626d32b5cebe
https://hg.mozilla.org/mozilla-central/rev/8faefd45845b

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf41cf75d4ae
part 6) Rename `nsFrameSelection::*DesiredPos` to `*DesiredCaretPos`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/1142ad80e5cc
part 7) Move `nsFrameSelection::SetDesiredCaretPos` and `FetchDesiredCaretPos` to `DesiredCaretPos`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/98a183ae21fe
part 8) Remove superfluously setting `mIsSet` in `nsFrameSelection`'s constructor. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/1ca9c1b2ad31
part 9) Add `DesiredCaretPos::Invalidate`. r=masayuki

Comment 15

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 10) Factor `IsAnchorRelativeOperation` out from `AutoPrepareFocusRange`. r=masayuki

Comment 16

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 11) Factor removing generated ranges out from `AutoPrepareFocusRange`. r=masayuki

Depends on D74856

Comment 17

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 12) Factor determining range most distant from anchor out from `AutoPrepareFocusRange`. r=masayuki

Depends on D74857

Comment 18

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 13) Call `SetAnchorFocusRange` in `AutoPrepareFocusRange`. r=masayuki

Depends on D74858

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/bf41cf75d4ae
https://hg.mozilla.org/mozilla-central/rev/1142ad80e5cc
https://hg.mozilla.org/mozilla-central/rev/98a183ae21fe
https://hg.mozilla.org/mozilla-central/rev/1ca9c1b2ad31

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c86d82544e54
part 10) Factor `IsAnchorRelativeOperation` out from `AutoPrepareFocusRange`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/17c10a7fa75a
part 11) Factor removing generated ranges out from `AutoPrepareFocusRange`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/84d70bc1f5c3
part 12) Factor determining range most distant from anchor out from `AutoPrepareFocusRange`. r=masayuki

Comment 21

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 13) Remove stack variable for `Selection::mUserInitiated`. r=masayuki

Simplifies reasoning about the code when using searchfox.

Comment 22

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 14) Change `nsRange` argument from `const` pointer to `const` reference around table selection. r=masayuki

Helps to get rid of unnecessary nsresults later.

Depends on D75057

Comment 23

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 15) Rename `GetTableSelectionType` to `GetTableSelectionMode`. r=masayuki

Depends on D75058

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88e2374f9c43
part 13) Remove stack variable for `Selection::mUserInitiated`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/520e6ddadf14
part 14) Change `nsRange` argument from `const` pointer to `const` reference around table selection. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/5ba7377eca32
part 15) Rename `GetTableSelectionType` to `GetTableSelectionMode`. r=masayuki

Comment 25

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 16) Replace `GetTableCellLocationFromRange` with `GetTableSelectionMode`. r=masayuki

The location wasn't used from the caller of
GetTableCellLocationFromRange.

However, GetTableCellLocationFromRange
included flushing frames, this is now done in
HTMLEditor::CellIndexes::Update.

Comment 26

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 17) Annotate `CellIndexes`'s methods with `MOZ_CAN_RUN_SCRIPT`. r=masayuki

Depends on D75098

Comment 27

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 18) Correct assignment to `aDidAddRange` in `Selection::MaybeAddTableCellRange`. r=masayuki

Depends on D75099

Comment 28

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c86d82544e54
https://hg.mozilla.org/mozilla-central/rev/17c10a7fa75a
https://hg.mozilla.org/mozilla-central/rev/84d70bc1f5c3

Comment 29

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/88e2374f9c43
https://hg.mozilla.org/mozilla-central/rev/520e6ddadf14
https://hg.mozilla.org/mozilla-central/rev/5ba7377eca32

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4951563fd101
part 16) Replace `GetTableCellLocationFromRange` with `GetTableSelectionMode`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/b933f7ded872
part 17) Annotate `CellIndexes`'s methods with `MOZ_CAN_RUN_SCRIPT`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/6e994c07eca1
part 18) Correct assignment to `aDidAddRange` in `Selection::MaybeAddTableCellRange`. r=masayuki

Comment 31

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4951563fd101
https://hg.mozilla.org/mozilla-central/rev/b933f7ded872
https://hg.mozilla.org/mozilla-central/rev/6e994c07eca1

Comment 32

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 19) Clean up a little in `nsFrameSelection::MoveCaret`. r=masayuki

Comment 33

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 20) Rename `GetCellParent` to `GetClosestInclusiveTableCellAncestor`. r=masayuki

It's more accurate.

Depends on D75562

Comment 34

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 21) Rename `TableSelection::mCellParent` to `mClosestInclusiveTableCellAncestor`. r=masayuki

Depends on D75563

Comment 35

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 22) Rename local `cellparent` variables to `inclusiveTableCellAncestor`. r=masayuki

Depends on D75564

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25ef52043c93
part 19) Clean up a little in `nsFrameSelection::MoveCaret`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/2824405e6eb7
part 20) Rename `GetCellParent` to `GetClosestInclusiveTableCellAncestor`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/14275b5ea171
part 21) Rename `TableSelection::mCellParent` to `mClosestInclusiveTableCellAncestor`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/9c52c330dd88
part 22) Rename local `cellparent` variables to `inclusiveTableCellAncestor`. r=masayuki

Comment 37

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/25ef52043c93
https://hg.mozilla.org/mozilla-central/rev/2824405e6eb7
https://hg.mozilla.org/mozilla-central/rev/14275b5ea171
https://hg.mozilla.org/mozilla-central/rev/9c52c330dd88

Comment 38

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 23) Factor code out to `IsContentInActivelyEditableTableCell`. r=masayuki

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1d29a18bc0b
part 23) Factor code out to `IsContentInActivelyEditableTableCell`. r=masayuki

Comment 40

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/d1d29a18bc0b

Comment 41

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 24) Directly call `TableSelection::HandleSelection` in `nsFrameSelection::TakeFocus`. r=masayuki

TableSelection has fewer responsibilites than nsFrameSelection. This
simplifies reasoning about TakeFocus.

Comment 42

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 24.1) Factor `HandleDragSelecting` and `HandleMouseUpOrDown` out from `TableSelection::HandleSelection`. r=masayuki

HandleSelection was too complex.

The arguments of the new methods will be renamed in a separate commit to
simplify reviewing and avoid mistakes.

Depends on D77252

Comment 43

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 24.2) Rename `childContent` to `aChildContent`. r=masayuki

Depends on D77253

Comment 44

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 25) `const` qualify arguments of `GetCellLayout` and `GetCellIndexes`. r=masayuki

Comment 45

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 26) Remove some code duplication in `HandleDragSelecting`. r=masayuki

Depends on D77423

Comment 46

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 27) clang-format previous changes in nsFrameSelection.cpp. r=masayuki

Simple application of ./mach clang-format -p layout/generic/nsFrameSelection.cpp. Separating this from previous
changes simplified reviewing and decreases the probability for mistakes.

Depends on D77424

Comment 47

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 28) Declare some function arguments in `nsFrameSelection` `const`. r=masayuki

Depends on D77459

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70cb19f1ad24
part 24.1) Factor `HandleDragSelecting` and `HandleMouseUpOrDown` out from `TableSelection::HandleSelection`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/423000b06503
part 24.2) Rename `childContent` to `aChildContent`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/df78142de12b
part 25) `const` qualify  arguments of `GetCellLayout` and `GetCellIndexes`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/8568aae413f7
part 26) Remove some code duplication in `HandleDragSelecting`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/cb312ffd7223
part 27) clang-format previous changes in nsFrameSelection.cpp. r=masayuki

Comment 49

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1635709: part 29) Factor finding first and last cell of row or column out. r=masayuki

Depends on D77460

Comment 50

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/70cb19f1ad24
https://hg.mozilla.org/mozilla-central/rev/423000b06503
https://hg.mozilla.org/mozilla-central/rev/df78142de12b
https://hg.mozilla.org/mozilla-central/rev/8568aae413f7
https://hg.mozilla.org/mozilla-central/rev/cb312ffd7223

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ceadf0ed0f1
part 28) Declare some function arguments in `nsFrameSelection` `const`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/3acd33c72d73
part 29) Factor finding first and last cell of row or column out. r=masayuki

Comment 52

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/0ceadf0ed0f1
https://hg.mozilla.org/mozilla-central/rev/3acd33c72d73