Closed Bug 1393348 Opened 7 years ago Closed 7 years ago

Optimize retrieving selection from nsTextEditorState

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1393348 - part1: nsTextEditorState should use nsTextInputSelectionImpl::GetSelection(SelectionType) instead of nsTextInputSelectionImpl::GetSelection(RawSelectionType, nsISelection**)
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1393348 - part2: nsISelectionController::SELECTION_* should be sequential integer values
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1393348 - part3: Converting SelectionType to index of Selection array in nsFrameSelection should use array instead of switch statement
59 bytes, text/x-review-board-request
smaug
: review+
Details 
I see nsTextInputSelectionImpl::GetSelection(RawSelectionType, nsISelection**) and nsFrameSelection::GetSelection() in the profile at running attachment 8848015 [details].
Comment on attachment 8900970 [details]
Bug 1393348 - part1: nsTextEditorState should use nsTextInputSelectionImpl::GetSelection(SelectionType) instead of nsTextInputSelectionImpl::GetSelection(RawSelectionType, nsISelection**)

https://reviewboard.mozilla.org/r/172194/#review177708
Attachment #8900970 - Flags: review?(m_kato) → review+
Comment on attachment 8900972 [details]
Bug 1393348 - part3: Converting SelectionType to index of Selection array in nsFrameSelection should use array instead of switch statement

https://reviewboard.mozilla.org/r/172198/#review177888

::: layout/generic/nsFrameSelection.cpp:165
(Diff revision 1)
> +};
> +
> +inline int8_t
>  GetIndexFromSelectionType(SelectionType aSelectionType)
>  {
> -  switch (aSelectionType) {
> +  return kIndexOfSelections[static_cast<int8_t>(aSelectionType) + 1];

I don't understand this. SelectionType NORMAL is 1, so we get index 2 here, no?
Were you going to use - 1?
Attachment #8900972 - Flags: review?(bugs) → review-
Comment on attachment 8900971 [details]
Bug 1393348 - part2: nsISelectionController::SELECTION_* should be sequential integer values

https://reviewboard.mozilla.org/r/172196/#review177890

::: dom/base/nsISelectionController.idl:309
(Diff revision 1)
> +
> +// SelectionType should be used in internal handling because of type safe.
>  enum class SelectionType : RawSelectionType
>  {
>    eInvalid = -1,
> -  eNone = nsISelectionController::SELECTION_NONE,
> +  // nsISelectionController::SELECTION_NONE

why you don't assign the value explicitly anymore? SELECTION_NONE is 0 and so is eNone.

::: layout/generic/nsTextFrame.cpp:7744
(Diff revision 1)
>  
>    UniquePtr<SelectionDetails> details = GetSelectionDetails();
>    for (SelectionDetails* sd = details.get(); sd; sd = sd->mNext.get()) {
>      if (sd->mStart == sd->mEnd ||
> -        !(sd->mSelectionType & kRawSelectionTypesWithDecorations) ||
> +        sd->mSelectionType == SelectionType::eInvalid ||
> +        !(sd->mSelectionType & kSelectionTypesWithDecorations) ||

Why is this right. mSelectionType isn't in bitmask form.

::: layout/generic/nsTextFrame.cpp:7822
(Diff revision 1)
>    nsPresContext* presContext = PresContext();
>    while (f && f->GetContentOffset() < int32_t(aEnd)) {
>      // We may need to reflow to recompute the overflow area for
>      // spellchecking or IME underline if their underline is thicker than
>      // the normal decoration line.
> -    if (aSelectionType & kRawSelectionTypesWithDecorations) {
> +    if (aSelectionType & kSelectionTypesWithDecorations) {

Why is this right? aSelection type isn't in bitmask form.
Attachment #8900971 - Flags: review?(bugs) → review-
Comment on attachment 8900972 [details]
Bug 1393348 - part3: Converting SelectionType to index of Selection array in nsFrameSelection should use array instead of switch statement

https://reviewboard.mozilla.org/r/172198/#review177888

> I don't understand this. SelectionType NORMAL is 1, so we get index 2 here, no?
> Were you going to use - 1?

The array is storing from index of eInvalid (-1). Therefore, index for eNormal is stored at 2. That the reason why I added +1.

I'd like to get rid of if/switch from the method.
Comment on attachment 8900971 [details]
Bug 1393348 - part2: nsISelectionController::SELECTION_* should be sequential integer values

https://reviewboard.mozilla.org/r/172196/#review177890

> why you don't assign the value explicitly anymore? SELECTION_NONE is 0 and so is eNone.

Okay. I'll revert the changes.

> Why is this right. mSelectionType isn't in bitmask form.

Because of SelectionType has custom & operator override. See Selection.h.

Or, should I create SelectionTypeIsIncluded(SelectionType, SelectionTypeMask)? But it may make other developers confused which is SelectionType and SelectionTypeMask. I we could write it as SelectionTypeMask::Includes(SelectionType)...

> Why is this right? aSelection type isn't in bitmask form.

See above. The & operator is overridden in Selection.h (declared in nsISelectionController.idl).
setting ni? because I need reply for the above before I rewrote the patches.
Flags: needinfo?(bugs)
Comment on attachment 8900972 [details]
Bug 1393348 - part3: Converting SelectionType to index of Selection array in nsFrameSelection should use array instead of switch statement

https://reviewboard.mozilla.org/r/172198/#review178108

oh, I see. Could you add some comment.
Attachment #8900972 - Flags: review- → review+
Comment on attachment 8900971 [details]
Bug 1393348 - part2: nsISelectionController::SELECTION_* should be sequential integer values

https://reviewboard.mozilla.org/r/172196/#review178114

::: dom/base/Selection.h:582
(Diff revision 1)
> +  MOZ_ASSERT(aSelectionType != SelectionType::eInvalid);
> +  return aSelectionType == SelectionType::eNone ? 0 :
> +           (1 << (static_cast<uint8_t>(aSelectionType) - 1));
> +}
> +
>  inline bool operator &(SelectionType aSelectionType,

Could we get rid of this operator& and just explicitly call ToSelectionTypeMask when needed.
Comment on attachment 8900971 [details]
Bug 1393348 - part2: nsISelectionController::SELECTION_* should be sequential integer values

https://reviewboard.mozilla.org/r/172196/#review178114

> Could we get rid of this operator& and just explicitly call ToSelectionTypeMask when needed.

Ah, I should've gone to the bed for refreshing my brain :-(
Comment on attachment 8900971 [details]
Bug 1393348 - part2: nsISelectionController::SELECTION_* should be sequential integer values

https://reviewboard.mozilla.org/r/172196/#review178310
Attachment #8900971 - Flags: review?(bugs) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/fc9c5f963abf
part1: nsTextEditorState should use nsTextInputSelectionImpl::GetSelection(SelectionType) instead of nsTextInputSelectionImpl::GetSelection(RawSelectionType, nsISelection**) r=m_kato
https://hg.mozilla.org/integration/autoland/rev/a69d5ede21ba
part2: nsISelectionController::SELECTION_* should be sequential integer values r=smaug
https://hg.mozilla.org/integration/autoland/rev/156c3f4e0b16
part3: Converting SelectionType to index of Selection array in nsFrameSelection should use array instead of switch statement r=smaug
Flags: needinfo?(bugs)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: