Closed Bug 1423097 Opened 2 years ago Closed 2 years ago

Make Selection and nsRange more friendly with RawRangeBoundary

Categories

(Core :: DOM: Serializers, enhancement)

56 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

Details

Attachments

(3 files)

No description provided.
Comment on attachment 8935650 [details]
Bug 1423097 - part 1: Implement Selection::AnchorRef() and Selection::FocusRef()

https://reviewboard.mozilla.org/r/206542/#review212286

Oddly hard to review change. Hopefully I didn't miss anything.

::: dom/base/Selection.cpp:824
(Diff revision 1)
> +const RangeBoundary&
> +Selection::AnchorRef()
> +{
> +  if (!mAnchorFocusRange) {
> +    static RangeBoundary sEmpty;
> +    return sEmpty;

Ah, this is a bit ugly, but I guess it is fine, since the return value is after all const.
Attachment #8935650 - Flags: review?(bugs) → review+
Comment on attachment 8935651 [details]
Bug 1423097 - part 2: Add overloads of nsRange::SetStart(), nsRange::SetEnd(), nsRange::IsPointInRange() and nsRange::ComparePoint() to use them with RawRangeBoundary

https://reviewboard.mozilla.org/r/206544/#review212308

::: editor/libeditor/HTMLEditRules.cpp:6619
(Diff revision 1)
>  {
>    if (NS_WARN_IF(!aPoint.IsSet())) {
>      return NS_ERROR_INVALID_ARG;
>    }
>    RefPtr<nsRange> range = new nsRange(aPoint.Container());
> -  DebugOnly<nsresult> rv = range->SetStart(aPoint.Container(), aPoint.Offset());
> +  ErrorResult error;

Just use IgnoredErrorResult ?

::: editor/libeditor/HTMLEditRules.cpp:8652
(Diff revision 1)
>  HTMLEditRules::UpdateDocChangeRange(nsRange* aRange)
>  {
> +  if (NS_WARN_IF(!mHTMLEditor)) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +  RefPtr<HTMLEditor> htmlEditor(mHTMLEditor);

I don't understand this change. Please don't add extra strong member variables unless needed.
So, remove this or explain why this is needed.
Attachment #8935651 - Flags: review?(bugs) → review+
Comment on attachment 8935650 [details]
Bug 1423097 - part 1: Implement Selection::AnchorRef() and Selection::FocusRef()

https://reviewboard.mozilla.org/r/206542/#review212286

> Ah, this is a bit ugly, but I guess it is fine, since the return value is after all const.

Yeah. It might be better to create EmptyString() like stuff, though.
Comment on attachment 8935651 [details]
Bug 1423097 - part 2: Add overloads of nsRange::SetStart(), nsRange::SetEnd(), nsRange::IsPointInRange() and nsRange::ComparePoint() to use them with RawRangeBoundary

https://reviewboard.mozilla.org/r/206544/#review212308

> Just use IgnoredErrorResult ?

Sure.

> I don't understand this change. Please don't add extra strong member variables unless needed.
> So, remove this or explain why this is needed.

I'm avoiding accessing raw mHTMLEditor in HTMLEditRules for reducing security risk when I touch each method. However, this method indeed doesn't call risky methods of the editor. So, yes, we can get rid of using local RefPtr<HTMLEditor>.
Comment on attachment 8935652 [details]
Bug 1423097 - part 3: Fix new orange caused by an existing bug of EditorBase::DeleteSelectionAndCreateElement()

https://reviewboard.mozilla.org/r/206546/#review212650
Attachment #8935652 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/2c1ad12ab12b
part 1: Implement Selection::AnchorRef() and Selection::FocusRef() r=smaug
https://hg.mozilla.org/integration/autoland/rev/b9b64291ad94
part 2: Add overloads of nsRange::SetStart(), nsRange::SetEnd(), nsRange::IsPointInRange() and nsRange::ComparePoint() to use them with RawRangeBoundary r=smaug
https://hg.mozilla.org/integration/autoland/rev/ec4df7b61cce
part 3: Fix new orange caused by an existing bug of EditorBase::DeleteSelectionAndCreateElement() r=m_kato
You need to log in before you can comment on or make changes to this bug.