1444847 - Implement AbstractRange and StaticRange

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Anne (:annevk)]

See https://github.com/whatwg/dom/pull/589 and https://github.com/w3c/web-platform-tests/pull/9967.

I suppose we might want to wait with this until we also implement https://w3c.github.io/input-events/index.html (could not find a tracking bug), unless bug 1438635 becomes high enough of a priority somehow.

Comment 1

[Makoto Kato [:m_kato]]

- Edge 18 will support both (Insider build supports both)
- Safari 10.1 supports StaticRange only
- Chrome 60 supports StaticRange only with old IDL

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

What's the status of this bug? Perhaps, I can fix bug 998941 soon. Then, I'll work on implementing beforeinput behind a flag. (Perhaps, I need to rewrite a lot especially around keypress event dispatcher, so, still not so urgent though.)

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Part 1. Implement AbstractRange

Comment 4

[Makoto Kato [:m_kato]]

Attachment: Part 2. Implement StaticRange

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Part 3. Update in-tree tests

Comment 6

[Makoto Kato [:m_kato]]

Comment on attachment 9048090 [details] [diff] [review]
Part 2. Implement StaticRange

Actually, there no value setter in StaticRange spec. So we will need C++ method or chrome only method to use StaticRange. Since I add chrome only method for test, what method that do you want for input type?

getTargetRanges will be necessary to use undo/redo implementation by JavaScript's editor.  But it may be better way to implement undo/redo without getTargetRanges.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Comment on attachment 9048090 [details] [diff] [review]
Part 2. Implement StaticRange

In most cases, we need to create StaticRange instance with an nsRange instance. So, nsRange::CloneAsStaticRange() or something is useful. However, I have no idea for some specific cases like "deleteWordBackward" inputType. Probably, necessary API should be added when we need actually to save the binary size.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1444847 - part 1: Create `mozilla::dom::AbstractRange`

This patch is based on the patch created by Makoto Kato-san.

Range and StaticRange have common base interface, AbstractRange.
https://dom.spec.whatwg.org/#abstractrange

This interface has simply returns startContainer, endContainer,
startOffset, endOffset and collapsed.

Different from the original patch's approach, this patch moves related
members in nsRange to AbstractRange since this approach avoids
virtual call cost. Additionally, this patch makes them not throw as
declared by the spec. As far as I know, the destruction cost of
ErrorResult may appear in profile so that we should avoid creating
the instance if we can avoid it.

Unfortunately, the instance size of nsRange becomes larger with this
patch. The size is changed from 176 to 184. I.e., now, nsRange
requires bigger chunk.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1444847 - part 2: Sort out basic API of `nsRange` for consistency with coming `StaticRange`

For avoiding confusion between API of nsRange and StaticRange, I'd like to
rename nsRange::CreateRange() to nsRange::MaybeCreate() because
StaticRange::CreateStaticRange() is too long name and
StaticRange::CreateRange() sounds odd. And sometimes, it won't return new
instance. This patch renames it and changes related methods to template
methods for avoiding runtime cost of temporary RawRangeBoundary
instance creation.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1444847 - part 3: Create `RangeUtils` to place public static methods of `nsRange`

Some nsRange static methods are useful in StaticRange and some of them
are used in a lot of places but not related to nsRange directly. This
patch moves them into new static method only class, mozilla::RangeUtils.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1444847 - part 4: Implement `mozilla::dom::StaticRange` and static factory methods

This patch is based on Makoto Kato-san's patch.

This patch implements mozilla::dom::StaticRange class and creating some
static factory methods.

Then, makes AbstractRange has a utility method of SetStartAndEnd()
method of nsRange and StaticRange for sharing same logic in one place.
However, there are some additional work is required only in nsRange, e.g.,
nsRange needs to start observing mutation of the range, but StaticRange
does not it. Therefore, it's implemented as a template method which takes
nsRange* or StaticRange* as a parameter. Then, each DoSetRange()
method of them can do different things without virtual calls.

Note that StaticRange does not have any properties, methods nor constructor.
Therefore, we need additional API to test it.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1444847 - part 5: Make `HTMLEditRules::ExpandSelectionForDeletion()` use `StaticRange` instead of `nsRange` for temporary use

This patch makes HTMLEditRules::ExpandSelectionForDeletion() use StaticRange
instead of nsRange for comparing a point and a range (i.e., the DOM tree
won't be changed during it's alive). Unfortunately, we still have allocation
cost, but we can save the cost of registering and unregistering mutation
observer and computing common ancestor of the range.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1444847 - part 6: Make `HTMLEditRules::PinSelectionToNewBlock()` use `StaticRange` instead of `nsRange` for temporary use

This patch makes HTMLEditRules::PinSelectionToNewBlock() use StaticRange
instead of nsRange for comparing a point and a range (i.e., the DOM tree
won't be changed during it's alive). Unfortunately, we still have allocation
cost, but we can save the cost of registering and unregistering mutation
observer and computing common ancestor of the range.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1444847 - part 7: Make `EditorSpellCheck` use `StaticRange` instead of `nsRange` to initialize itself

EditorSpellCheck clones nsRange instance only for temporary use during
initialization even though the DOM tree won't be changed during it. In this
case, using StaticRange is better since it does not need to observe the
DOM tree mutation. I.e., we can save the cost of registering and
unregistering the mutation observer.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

I think that we should land part.1 ~ part.5 first. Then, part.6 and part.7 should be landed separately for making it easier to look for regression-window.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

https://groups.google.com/forum/#!topic/mozilla.dev.platform/S8WEan1uLGo

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/feb8bd2e7973
part 1: Create `mozilla::dom::AbstractRange` r=smaug
https://hg.mozilla.org/integration/autoland/rev/610d62589b4e
part 2: Sort out basic API of `nsRange` for consistency with coming `StaticRange` r=smaug
https://hg.mozilla.org/integration/autoland/rev/bcfd386d2585
part 3: Create `RangeUtils` to place public static methods of `nsRange` r=smaug
https://hg.mozilla.org/integration/autoland/rev/762824a037ce
part 4: Implement `mozilla::dom::StaticRange` and static factory methods r=smaug

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/feb8bd2e7973
https://hg.mozilla.org/mozilla-central/rev/610d62589b4e
https://hg.mozilla.org/mozilla-central/rev/bcfd386d2585
https://hg.mozilla.org/mozilla-central/rev/762824a037ce

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f81be4a7864d
part 5: Make `HTMLEditRules::ExpandSelectionForDeletion()` use `StaticRange` instead of `nsRange` for temporary use r=smaug

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/446927a94617
part 6: Make `HTMLEditRules::PinSelectionToNewBlock()` use `StaticRange` instead of `nsRange` for temporary use r=smaug

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f81be4a7864d
https://hg.mozilla.org/mozilla-central/rev/446927a94617

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c8756efe2b0d
part 7: Make `EditorSpellCheck` use `StaticRange` instead of `nsRange` to initialize itself r=smaug

Comment 23

[Daniel Varga [:dvarga]]

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