Closed Bug 1391978 Opened 7 years ago Closed 7 years ago

Don't use nsISelection's method if possible in Editor

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
enhancement

Tracking

()

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

People

(Reporter: m_kato, Assigned: m_kato)

References

Details

Attachments

(9 files)

Bug 1391978 - Part 1. Replace nsISelection::GetRangeCount with Selection::RangeCount.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1391978 - Part 2. Replace nsISelection::SelectAllChildren with Selection::SelectAllChildren.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1391978 - Part 3. Replace nsISelection::GetIsCollapsed with Selection::IsCollapsed.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1391978 - Part 4. Replace nsISelection::GetFocusNode with Selection::GetFocusNode.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1391978 - Part 5. Replace nsISelection::GetFocusOffset/GetAnchroOffset with Selection::FocusOffset/AnchorOffset.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1391978 - Part 6. Replace nsISelection::Extend with Selection::Extend.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1391978 - Part 7. HTMLEditRules::NormalizeSelection should use nsINode instead of nsIDOMNode.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1391978 - Part 8. Replace nsISelection::GetAnchorOffset with Selection::AnchorOffset.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1391978 - Part 9. Replace nsISelection::GetAnchorNode with Selection::GetAnchorNode.
59 bytes, text/x-review-board-request
masayuki
: review+
Details 
Except to nsISelection::Collapse(nsINode*, ...), we can replace nsISelection's method with Selection's method.  Collapse is too hard to replace various code in Editor, so this isn't target for this bug.
Priority: -- → P3
nsISelection's method is virtual, but Selection's method isn't virtual and uses nsINode instead of nsIDOMNode.
Assignee: nobody → m_kato
Blocks: 1387143
Comment on attachment 8901088 [details]
Bug 1391978 - Part 1. Replace nsISelection::GetRangeCount with Selection::RangeCount.

https://reviewboard.mozilla.org/r/172554/#review178344

::: editor/composer/nsEditorSpellCheck.cpp:363
(Diff revision 1)
>      if (NS_WARN_IF(!domSelection)) {
>        return NS_ERROR_FAILURE;
>      }
>      RefPtr<Selection> selection = domSelection->AsSelection();
>  
> -    int32_t count = 0;
> +    if (selection->RangeCount() > 0) {

nit: you can write this as |if (selection->RangeCount())|. However, I guess that such change could cause some trouble with merge. So, keeping this style is okay.

::: editor/libeditor/EditorEventListener.cpp:1026
(Diff revision 1)
> -  int32_t rangeCount;
> -  rv = selection->GetRangeCount(&rangeCount);
> +  uint32_t rangeCount = selection->RangeCount();
> +  for (uint32_t i = 0; i < rangeCount; i++) {

I wonder, Selection should be range-based-for aware?

::: editor/libeditor/HTMLTableEditor.cpp:767
(Diff revision 1)
> -  int32_t rangeCount;
> +  uint32_t rangeCount = selection->RangeCount();
> -  rv = selection->GetRangeCount(&rangeCount);
> -  NS_ENSURE_SUCCESS(rv, rv);
>  
>    if (firstCell && rangeCount > 1) {

if |rangeCount| is referred only by this if condition, it's better to refer selection->RangeCount() directly though.

::: editor/libeditor/HTMLTableEditor.cpp:1039
(Diff revision 1)
> -  int32_t rangeCount;
> +  uint32_t rangeCount = selection->RangeCount();
> -  rv = selection->GetRangeCount(&rangeCount);
> -  NS_ENSURE_SUCCESS(rv, rv);
>  
>    if (firstCell && rangeCount > 1) {

Ditto.

::: editor/libeditor/HTMLTableEditor.cpp:1202
(Diff revision 1)
> -  int32_t rangeCount;
> +  uint32_t rangeCount = selection->RangeCount();
> -  rv = selection->GetRangeCount(&rangeCount);
> -  NS_ENSURE_SUCCESS(rv, rv);
>  
>    if (firstCell && rangeCount > 1) {

Ditto.

::: editor/libeditor/HTMLTableEditor.cpp:2213
(Diff revision 1)
> -    int32_t rangeCount;
> +    uint32_t rangeCount = selection->RangeCount();
> -    rv = selection->GetRangeCount(&rangeCount);
> -    NS_ENSURE_SUCCESS(rv, rv);
>  
>      RefPtr<nsRange> range;
> -    for (int32_t i = 0; i < rangeCount; i++) {
> +    for (uint32_t i = 0; i < rangeCount; i++) {

Ditto.

::: editor/txtsvc/nsTextServicesDocument.cpp:2588
(Diff revision 1)
> -  rv = selection->GetRangeCount(&rangeCount);
> +  uint32_t rangeCount = selection->RangeCount();
>  
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // Find the first range in the selection that intersects
>    // the current text block.
>  
> -  for (int32_t i = 0; i < rangeCount; i++) {
> +  for (uint32_t i = 0; i < rangeCount; i++) {

You should remove the NS_ENSURE_SUCCESS(rv, rv);. And you should move the RangeCount() calling line to immediately before the for loop.
Attachment #8901088 - Flags: review?(masayuki) → review+
Comment on attachment 8901089 [details]
Bug 1391978 - Part 2. Replace nsISelection::SelectAllChildren with Selection::SelectAllChildren.

https://reviewboard.mozilla.org/r/172556/#review178356
Attachment #8901089 - Flags: review?(masayuki) → review+
Comment on attachment 8901090 [details]
Bug 1391978 - Part 3. Replace nsISelection::GetIsCollapsed with Selection::IsCollapsed.

https://reviewboard.mozilla.org/r/172558/#review178358

::: editor/txtsvc/nsTextServicesDocument.cpp:498
(Diff revision 1)
> -  nsCOMPtr<nsISelection> domSelection;
> -  nsresult rv = mSelCon->GetSelection(nsISelectionController::SELECTION_NORMAL,
> +  RefPtr<Selection> selection =
> +    mSelCon->GetDOMSelection(nsISelectionController::SELECTION_NORMAL);

I really want to create SelectionControllerBase class to creating such method as non-virtual!

::: editor/txtsvc/nsTextServicesDocument.cpp:603
(Diff revision 1)
> +      bool isCollapsed;
>        rv = range->GetCollapsed(&isCollapsed);

Sigh, there are still some nsIDOMRange interface users...

::: editor/txtsvc/nsTextServicesDocument.cpp:713
(Diff revision 1)
>        return NS_OK; // XXX Really?
>      }
>  
>      // Create an iterator for the range.
>  
> -    rv = CreateContentIterator(range, getter_AddRefs(iter));
> +    nsresult rv = CreateContentIterator(range, getter_AddRefs(iter));

And also I'd like to create AutoContentIterator...
Attachment #8901090 - Flags: review?(masayuki) → review+
Comment on attachment 8901091 [details]
Bug 1391978 - Part 4. Replace nsISelection::GetFocusNode with Selection::GetFocusNode.

https://reviewboard.mozilla.org/r/172560/#review178362
Attachment #8901091 - Flags: review?(masayuki) → review+
Comment on attachment 8901092 [details]
Bug 1391978 - Part 5. Replace nsISelection::GetFocusOffset/GetAnchroOffset with Selection::FocusOffset/AnchorOffset.

https://reviewboard.mozilla.org/r/172562/#review178364

::: commit-message-1335e:1
(Diff revision 1)
> +Bug 1391978 - Part 5. Replace nsISelection::GetFocusOffset with Selection::FocusOffset. r?masayuki

"Replace nsISelection::GetFocusOffset/GetAnchorOffset with Selection::FocusOffset/AnchorOffset"?
Attachment #8901092 - Flags: review?(masayuki) → review+
Comment on attachment 8901093 [details]
Bug 1391978 - Part 6. Replace nsISelection::Extend with Selection::Extend.

https://reviewboard.mozilla.org/r/172564/#review178366

::: commit-message-a9dc9:3
(Diff revision 1)
> +Bug 1391978 - Part 6. Replace nsISelection::Extend with Selection::Extend. r?masayuki
> +
> +Except to HTMLEditRules::NormalizeSelection, I replace with Selection::Extend.

s/replace with/replace nsISelection::Extend with

::: editor/libeditor/HTMLEditor.cpp:1703
(Diff revision 1)
> -  nsCOMPtr<nsIDOMNode>parent;
> -  nsresult rv = aElement->GetParentNode(getter_AddRefs(parent));
> -  if (NS_SUCCEEDED(rv) && parent) {
> -    int32_t offsetInParent = GetChildOffset(aElement, parent);
> +  nsINode* parent = element->GetParentNode();
> +  if (NS_WARN_IF(!parent)) {
> +    return NS_ERROR_FAILURE;
> +  }

Hmm, current code returns NS_OK if parent is nullptr:
https://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/dom/base/nsINode.cpp#511

However, according to the method name, returning error in this case makes sense. So, I don't mind you to change it here. But if you don't like to change any behavior in this bug, you should return NS_OK here.

::: editor/libeditor/HTMLEditor.cpp:1703
(Diff revision 1)
> -  nsCOMPtr<nsIDOMNode>parent;
> -  nsresult rv = aElement->GetParentNode(getter_AddRefs(parent));
> -  if (NS_SUCCEEDED(rv) && parent) {
> -    int32_t offsetInParent = GetChildOffset(aElement, parent);
> +  nsINode* parent = element->GetParentNode();
> +  if (NS_WARN_IF(!parent)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  int32_t offsetInParent = parent->IndexOf(element);
>  
> -    // Collapse selection to just before desired element,
> +  // Collapse selection to just before desired element,
> -    rv = selection->Collapse(parent, offsetInParent);
> +  nsresult rv = selection->Collapse(parent, offsetInParent);
> -    if (NS_SUCCEEDED(rv)) {
> +  if (NS_SUCCEEDED(rv)) {
> -      // then extend it to just after
> +    // then extend it to just after
> -      rv = selection->Extend(parent, offsetInParent + 1);
> +    rv = selection->Extend(parent, offsetInParent + 1);
> -    }
> +  }

Oh, here is doing it redundantly... Selection::SetBaseAndExtent() (or we should have a method like Selection::SelectNode() and) should be used because Selection::Extend is too slow. I'll file a follow up bug for this after investigating other users.

::: editor/txtsvc/nsTextServicesDocument.cpp:2242
(Diff revision 1)
>    int32_t endOffset = aOffset + aLength;
> -  for (int32_t i = mOffsetTable.Length() - 1; !eNode && i >= 0; i--) {
> +  for (int32_t i = mOffsetTable.Length() - 1; !endNode && i >= 0; i--) {

Looks like that you should move endNode and endNodeOffset declaration here.
Attachment #8901093 - Flags: review?(masayuki) → review+
Comment on attachment 8901094 [details]
Bug 1391978 - Part 7. HTMLEditRules::NormalizeSelection should use nsINode instead of nsIDOMNode.

https://reviewboard.mozilla.org/r/172566/#review178378

::: editor/libeditor/HTMLEditRules.cpp:5325
(Diff revision 1)
> +  if (NS_WARN_IF(!mHTMLEditor)) {
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +  RefPtr<HTMLEditor> htmlEditor = mHTMLEditor;
> +
>    RefPtr<nsRange> range = inSelection->GetRangeAt(0);

Oh, I'm surprised at here. This means that we modifying the range which may be grabbed by JS. However, in any Selection API, we create another Range instance for disconnection from JS.
Attachment #8901094 - Flags: review?(masayuki) → review+
Comment on attachment 8901095 [details]
Bug 1391978 - Part 8. Replace nsISelection::GetAnchorOffset with Selection::AnchorOffset.

https://reviewboard.mozilla.org/r/172568/#review178382
Attachment #8901095 - Flags: review?(masayuki) → review+
Comment on attachment 8901096 [details]
Bug 1391978 - Part 9. Replace nsISelection::GetAnchorNode with Selection::GetAnchorNode.

https://reviewboard.mozilla.org/r/172570/#review178384

::: editor/libeditor/HTMLEditor.cpp:1586
(Diff revision 1)
>        } else {
>          selection->CollapseToEnd();
>        }
>      }
>  
> -    nsCOMPtr<nsIDOMNode> parentSelectedNode;
> +    nsINode* parentSelectedNodeNode = selection->GetAnchorNode();

NodeNode is too odd. Please drop one of them.

::: editor/libeditor/HTMLEditor.cpp:1590
(Diff revision 1)
> -    nsCOMPtr<nsIDOMNode> parentSelectedNode;
> -    rv = selection->GetAnchorNode(getter_AddRefs(parentSelectedNode));
> +    nsINode* parentSelectedNodeNode = selection->GetAnchorNode();
> +    if (parentSelectedNodeNode) {
> -    if (parentSelectedNode) {
>        int32_t offsetForInsert = selection->AnchorOffset();
>        // Adjust position based on the node we are going to insert.
> +      nsCOMPtr<nsIDOMNode> parentSelectedNode =

Then, this should be parentSelectedDOMNode.
Attachment #8901096 - Flags: review?(masayuki) → review+
Comment on attachment 8901088 [details]
Bug 1391978 - Part 1. Replace nsISelection::GetRangeCount with Selection::RangeCount.

https://reviewboard.mozilla.org/r/172554/#review178344

> I wonder, Selection should be range-based-for aware?

What's do you mean?

There is no iterator for range of Selection.  nsRange::IsPointInRange won't modify range and node, so if we can use "for (auto& range : selection->GeRanges())", we should replace this with it.
Comment on attachment 8901090 [details]
Bug 1391978 - Part 3. Replace nsISelection::GetIsCollapsed with Selection::IsCollapsed.

https://reviewboard.mozilla.org/r/172558/#review178358

> Sigh, there are still some nsIDOMRange interface users...

I have a plan that I file a new bug for nsIDOMRange -> nsRange...  This bug is for Selection.
Comment on attachment 8901088 [details]
Bug 1391978 - Part 1. Replace nsISelection::GetRangeCount with Selection::RangeCount.

https://reviewboard.mozilla.org/r/172554/#review178344

> What's do you mean?
> 
> There is no iterator for range of Selection.  nsRange::IsPointInRange won't modify range and node, so if we can use "for (auto& range : selection->GeRanges())", we should replace this with it.

Yeah, like that, I'd be happy if this kind of loops would be rewritten with range-based-for loops. Of course, not the scope of this bug.
Comment on attachment 8901090 [details]
Bug 1391978 - Part 3. Replace nsISelection::GetIsCollapsed with Selection::IsCollapsed.

https://reviewboard.mozilla.org/r/172558/#review178358

> I have a plan that I file a new bug for nsIDOMRange -> nsRange...  This bug is for Selection.

Yeah, I know. I just commented without opening issues.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 34c358cffdac -d 1bdcee585e10: rebasing 416457:34c358cffdac "Bug 1391978 - Part 1. Replace nsISelection::GetRangeCount with Selection::RangeCount. r=masayuki"
merging editor/libeditor/EditorBase.cpp
rebasing 416458:f53734ba4983 "Bug 1391978 - Part 2. Replace nsISelection::SelectAllChildren with Selection::SelectAllChildren. r=masayuki"
merging editor/libeditor/EditorBase.cpp
rebasing 416459:fbcde1355bcd "Bug 1391978 - Part 3. Replace nsISelection::GetIsCollapsed with Selection::IsCollapsed. r=masayuki"
rebasing 416460:cd68502bacd9 "Bug 1391978 - Part 4. Replace nsISelection::GetFocusNode with Selection::GetFocusNode. r=masayuki"
rebasing 416461:8e043877aa9f "Bug 1391978 - Part 5. Replace nsISelection::GetFocusOffset/GetAnchroOffset with Selection::FocusOffset/AnchorOffset. r=masayuki"
rebasing 416462:9b3f92c55943 "Bug 1391978 - Part 6. Replace nsISelection::Extend with Selection::Extend. r=masayuki"
merging editor/libeditor/TextEditorTest.cpp
warning: conflicts while merging editor/libeditor/TextEditorTest.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe569af2097b
Part 1. Replace nsISelection::GetRangeCount with Selection::RangeCount. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/876b85ec5e06
Part 2. Replace nsISelection::SelectAllChildren with Selection::SelectAllChildren. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf7248d94cf9
Part 3. Replace nsISelection::GetIsCollapsed with Selection::IsCollapsed. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/e26c94e30014
Part 4. Replace nsISelection::GetFocusNode with Selection::GetFocusNode. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/072a338ad2bd
Part 5. Replace nsISelection::GetFocusOffset/GetAnchroOffset with Selection::FocusOffset/AnchorOffset. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/e756156d8300
Part 6. Replace nsISelection::Extend with Selection::Extend. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9afcfcf935a
Part 7. HTMLEditRules::NormalizeSelection should use nsINode instead of nsIDOMNode. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/5725f87e84ab
Part 8. Replace nsISelection::GetAnchorOffset with Selection::AnchorOffset. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7a0ae9b5ad3
Part 9. Replace nsISelection::GetAnchorNode with Selection::GetAnchorNode. r=masayuki
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: