1203871 - Add eQueryCharRectArray event which retrieves each character's rect of specified range

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

In e10s mode, ContentCache caches each character rect of composition string. For caching them, ContentCacheInChild dispatches eQueryTextRect for each character, in other words, it's dispatched 20 times if composition string have 20 characters.

If the start offset is big value and the focused editor is contenteditable, computing the offset is expensive, nevertheless, it's computed many times.

So, if we implement eQueryCharRectArray as retrieving each character rect in the specified range, we can reduce the cost of preparing to retrieve each character rect except the first one.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If you'd like to steal this bug from me, feel free to do it ;-)

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

roc:

Do you have some idea to retrieve each character rect from nsTextFrames quickly? It seems that current approach of eQueryTextRect which retrieves union rect of characters in specified range isn't useful for the new eQueryCharRectArray: http://mxr.mozilla.org/mozilla-central/source/dom/events/ContentEventHandler.cpp?rev=e22b3043887e#1296

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

There's no direct API for this, but you could quite easily add a new nsTextFrame API similar to GetPointFromOffset that returns a set of rects.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thanks. I'll check current nsTextFrame.

Comment 6

[Makoto Kato [:m_kato]]

Using gfxTexRun, we can get width per character.  OK, I will take this

Comment 7

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66b36b32fd14

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> nsTArray<mozilla::LayoutDeviceIntRect> mRectArray;

How about make this a pointer to nsTArray? The array may be big and copied to another instance. If InitForQueryTextRectArray() takes a pointer to this, the caller can specify the dist or nsAutoTArray for avoiding memory fragmentation.

Comment 9

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #8)
> > nsTArray<mozilla::LayoutDeviceIntRect> mRectArray;
> 
> How about make this a pointer to nsTArray? The array may be big and copied
> to another instance. If InitForQueryTextRectArray() takes a pointer to this,
> the caller can specify the dist or nsAutoTArray for avoiding memory
> fragmentation.

sizeof(nsTArray<nsRect>) is always 8 bytes on 64bit platform and array data is allocated on heap.  Even if using pointer instead, allocated AutoTArray will be on heap.  So as memory fragmentation, it is same result. 

Can SetEventResult and WidgetQueryContentEvent use c++11 move?

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thanks. Looks like nsTArray_Impl:: SwapElements() is useful.

Comment 11

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #10)
> Thanks. Looks like nsTArray_Impl:: SwapElements() is useful.

We can use mozilla::Move instead.  Also, I should mark this event is invalid after using it.

Comment 12

[Makoto Kato [:m_kato]]

Attachment: Bug 1203871 - Part 1. Add nsIFrame::GetCharacterRectsInRange.

IME wants to need the width per character.  Now nsTextFrame/nsIFrmae has only API to get point of string.  So I want to add this method to calculate simply by comment #3.

If you cannot review this, please change reviewer.

Review commit: https://reviewboard.mozilla.org/r/40057/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40057/

Comment 13

[Makoto Kato [:m_kato]]

Attachment: Bug 1203871 - Part 2. Implement eQueryTextRectArray.

It will use on ContentCache.

Review commit: https://reviewboard.mozilla.org/r/40059/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40059/

Comment 14

[Makoto Kato [:m_kato]]

Attachment: Bug 1203871 - Part 3. Use eQueryTextRectArray on ContentCache for e10s.

Use new event to update cotent process's cache.

Review commit: https://reviewboard.mozilla.org/r/40061/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40061/

Comment 15

[Makoto Kato [:m_kato]]

Attachment: Bug 1203871 - Part 4. Implement QUERY_TEXT_RECT_ARRAY to nsIDOMWindowUtils for unit test.

To test eQueryCharRectArray, I would like to add it to nsIDOMWindowUtils.  Also this will require external keyboard support on Android.

Review commit: https://reviewboard.mozilla.org/r/40063/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40063/

Comment 16

[Makoto Kato [:m_kato]]

Attachment: Bug 1203871 - Part 5. Use mozilla::Move for rect array.

To optimize copy of text rect array, we should use mozilla::Move.  Also, after using it, we should mark is invalid result into SetEventResult

Review commit: https://reviewboard.mozilla.org/r/40065/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40065/

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8730639 [details]
Bug 1203871 - Part 2. Implement eQueryTextRectArray.

https://reviewboard.mozilla.org/r/40059/#review36543

Anyway, it's okay to fix the optimization later.

::: dom/events/ContentEventHandler.cpp:1309
(Diff revision 1)
> +    rv = SetRangeFromFlatTextOffset(range, offset, 1, lineBreakType, true,
> +                                    nullptr);

Hmm, SetRangeFromFlatTextOffset() is very expensive if offset is too large... If this can be optimized, I'd be happy though. But current code also wastes same amount about this, so, not problem for now.

(If we can retrieve character rects until the end of the frame, we can reduce this call, I guess.)

::: dom/events/ContentEventHandler.cpp:1330
(Diff revision 1)
> +      return rv;
> +    }
> +
> +    nsRect charRect;
> +    int32_t nodeOffset = range->StartOffset();
> +    rv = firstFrame->GetCharacterRectFromOffset(nodeOffset, &charRect);

So, I think that this should be GetCharacterRectsInRange(int32_t aOffset, int32_t aLenght, nsTArray<nsRect>& aRects). Then, offset can be increased with aRect.Length().

::: widget/EventMessageList.h:229
(Diff revision 1)
>  // Query for the bounding rect of a range of characters. This works on any
>  // valid character range given offset and length. Result is relative to top
>  // level widget coordinates
>  NS_EVENT_MESSAGE(eQueryTextRect)
> +// Query for the bounding rect array of a range of characters.
> +// Thiis similar event of qQueryTextRect.

nit: s/qQueryTextRect/eQueryTextRect

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8730638 [details]
Bug 1203871 - Part 1. Add nsIFrame::GetCharacterRectsInRange.

https://reviewboard.mozilla.org/r/40057/#review36547

It seems fine to add this API, but I think the implementation could be cleaned up so as to share code with GetPointFromOffset rather than duplicating it.

::: layout/generic/nsFrame.cpp:6242
(Diff revision 1)
> +nsFrame::GetCharacterRectFromOffset(int32_t inOffset, nsRect* aOutRect)
> +{
> +  NS_PRECONDITION(aOutRect, "Null parameter");
> +  *aOutRect = GetContentRectRelativeToSelf();
> +  aOutRect->width = 0;
> +  aOutRect->height = 0;

I guess returning a zero-sized rect is OK here, but wouldn't it make more sense to do the same thing as GetPointFromOffset to come up with a position? Maybe just call GetPointFromOffset to get the x,y coords, and then set width and height to zero. Or factor out a helper method?

::: layout/generic/nsTextFrame.cpp:7142
(Diff revision 1)
>  }
>  
>  nsresult
> +nsTextFrame::GetCharacterRectFromOffset(int32_t aInOffset,
> +                                        nsRect* aOutRect)
> +{

It seems like we should be able to share a lot of the code here with GetPointFromOffset. The two methods are basically doing the same thing, except that the CharacterRect version adds a lookahead to the next cluster so that it can get the advance.

Maybe factor out the body of GetPointFromOffset into a helper that GetCharacterRectFromOffset can also call, and then it just needs to do the extra work to get the extent of the rect. Or have a shared helper that returns a rect, and takes a flag telling it whether we want the full rect (position and size) or only its position, which would allow an early return.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8730640 [details]
Bug 1203871 - Part 3. Use eQueryTextRectArray on ContentCache for e10s.

https://reviewboard.mozilla.org/r/40061/#review36833

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8730641 [details]
Bug 1203871 - Part 4. Implement QUERY_TEXT_RECT_ARRAY to nsIDOMWindowUtils for unit test.

https://reviewboard.mozilla.org/r/40063/#review36841

I think that this also needs sr of smaug because you changes *.idl.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8730642 [details]
Bug 1203871 - Part 5. Use mozilla::Move for rect array.

https://reviewboard.mozilla.org/r/40065/#review36847

::: dom/base/nsQueryContentEventResult.cpp:11
(Diff revision 1)
> +#include "mozilla/Move.h"
>  

Move.h should be included before TextEvents.h (for a-z order). If you don't mind, could you move nsQueryContentEventResult.h after nsPoint.h?

Comment 22

[Makoto Kato [:m_kato]]

Comment on attachment 8730638 [details]
Bug 1203871 - Part 1. Add nsIFrame::GetCharacterRectsInRange.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40057/diff/1-2/

Comment 23

[Makoto Kato [:m_kato]]

Comment on attachment 8730639 [details]
Bug 1203871 - Part 2. Implement eQueryTextRectArray.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40059/diff/1-2/

Comment 24

[Makoto Kato [:m_kato]]

Comment on attachment 8730640 [details]
Bug 1203871 - Part 3. Use eQueryTextRectArray on ContentCache for e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40061/diff/1-2/

Comment 25

[Makoto Kato [:m_kato]]

Comment on attachment 8730641 [details]
Bug 1203871 - Part 4. Implement QUERY_TEXT_RECT_ARRAY to nsIDOMWindowUtils for unit test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40063/diff/1-2/

Comment 26

[Makoto Kato [:m_kato]]

Comment on attachment 8730642 [details]
Bug 1203871 - Part 5. Use mozilla::Move for rect array.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40065/diff/1-2/

Comment 27

[Makoto Kato [:m_kato]]

Comment on attachment 8730639 [details]
Bug 1203871 - Part 2. Implement eQueryTextRectArray.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40059/diff/2-3/

Comment 28

[Makoto Kato [:m_kato]]

Comment on attachment 8730640 [details]
Bug 1203871 - Part 3. Use eQueryTextRectArray on ContentCache for e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40061/diff/2-3/

Comment 29

[Makoto Kato [:m_kato]]

Comment on attachment 8730641 [details]
Bug 1203871 - Part 4. Implement QUERY_TEXT_RECT_ARRAY to nsIDOMWindowUtils for unit test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40063/diff/2-3/

Comment 30

[Makoto Kato [:m_kato]]

Comment on attachment 8730642 [details]
Bug 1203871 - Part 5. Use mozilla::Move for rect array.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40065/diff/2-3/

Comment 31

[Jonathan Kew [:jfkthame]]

Comment on attachment 8730638 [details]
Bug 1203871 - Part 1. Add nsIFrame::GetCharacterRectsInRange.

https://reviewboard.mozilla.org/r/40057/#review56566

::: layout/generic/nsIFrame.h:1549
(Diff revision 2)
> +   * Get a character rect given an offset into the content.
> +   * This is similar version of GetPointFromOffset.

The comment here needs to be updated now that you're getting a list of character rects in a range, not just a rect for a single offset.

::: layout/generic/nsTextFrame.h:799
(Diff revision 2)
> +  void GetSkipCharsIteratorFromOffset(int32_t& aInOffset,
> +                                      gfxSkipCharsIterator& aIter,
> +                                      PropertyProvider& aProperties);

Please add a comment to clarify why aInOffset is passed as a reference here (as it seems unexpected); looks like it's being clamped to the content range by this method.

It would be nice to pass aProperties as a const reference, so it's clearly an inparam only. I guess to do that, you'd need to pull the InitializeForDisplay() call out of this method and let the caller do it.

Also, the "Get..." name seems a bit wrong, as the iterator was already created, this method is just changing its position. Maybe something like "UpdateIteratorForOffset"? And reorder the params so that aIter (the outparam result) is last, please.

::: layout/generic/nsTextFrame.h:802
(Diff revision 2)
> +  void GetPointFromIterator(gfxSkipCharsIterator& aIter,
> +                            PropertyProvider& aProperties,
> +                            nsPoint& aPoint);

Here as well, I think aIter and aProperties should both be const references. And the point can be returned as the function result rather than through an outParam.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8730641 [details]
Bug 1203871 - Part 4. Implement QUERY_TEXT_RECT_ARRAY to nsIDOMWindowUtils for unit test.

https://reviewboard.mozilla.org/r/40063/#review56648

::: dom/base/nsQueryContentEventResult.cpp:160
(Diff revision 3)
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +nsQueryContentEventResult::GetCharacterRect(int32_t aOffset,
> +                                            int32_t * aLeft, int32_t* aTop,

nit, extra space before * in 'int32_t * aLeft'

Comment 33

[Makoto Kato [:m_kato]]

https://reviewboard.mozilla.org/r/40057/#review56566

> Here as well, I think aIter and aProperties should both be const references. And the point can be returned as the function result rather than through an outParam.

OK, but, although gfxTextRun::GetAdvanceWidth cannot be set as const function, we cannot use const aPropertyProvider on parameter.

Comment 34

[Makoto Kato [:m_kato]]

Comment on attachment 8730638 [details]
Bug 1203871 - Part 1. Add nsIFrame::GetCharacterRectsInRange.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40057/diff/2-3/

Comment 35

[Makoto Kato [:m_kato]]

Comment on attachment 8730639 [details]
Bug 1203871 - Part 2. Implement eQueryTextRectArray.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40059/diff/3-4/

Comment 36

[Makoto Kato [:m_kato]]

Comment on attachment 8730640 [details]
Bug 1203871 - Part 3. Use eQueryTextRectArray on ContentCache for e10s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40061/diff/3-4/

Comment 37

[Makoto Kato [:m_kato]]

Comment on attachment 8730641 [details]
Bug 1203871 - Part 4. Implement QUERY_TEXT_RECT_ARRAY to nsIDOMWindowUtils for unit test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40063/diff/3-4/

Comment 38

[Makoto Kato [:m_kato]]

Comment on attachment 8730642 [details]
Bug 1203871 - Part 5. Use mozilla::Move for rect array.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40065/diff/3-4/

Comment 39

[Jonathan Kew [:jfkthame]]

(In reply to Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) from comment #33)
> OK, but, although gfxTextRun::GetAdvanceWidth cannot be set as const
> function, we cannot use const aPropertyProvider on parameter.

Hmm, that's too bad. It really should be a const method, but it looks like doing that would involve pushing the const-ness down through various subsidiary methods, and eventually providing both const and non-const variants of the GetCharacterGlyphs() accessor.

I think that would be a good thing to do, but it turns into a big enough change that it's probably best done in a separate bug. I've filed bug 1282248 for this. With that, can we use more const here as well?

Comment 40

[Makoto Kato [:m_kato]]

(In reply to Jonathan Kew (:jfkthame) from comment #39)
> (In reply to Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) from comment #33)
> > OK, but, although gfxTextRun::GetAdvanceWidth cannot be set as const
> > function, we cannot use const aPropertyProvider on parameter.
> 
> Hmm, that's too bad. It really should be a const method, but it looks like
> doing that would involve pushing the const-ness down through various
> subsidiary methods, and eventually providing both const and non-const
> variants of the GetCharacterGlyphs() accessor.
> 
> I think that would be a good thing to do, but it turns into a big enough
> change that it's probably best done in a separate bug. I've filed bug
> 1282248 for this. With that, can we use more const here as well?

Humm..  It cannot...

To use const as parameter, PropertyProvider::GetSpacing(...) must be const function.  But PropertyProvider::GetSpacingInternal() wants to update mTabWidths on PropertyProvider::CalcTabWidths() and CalcTabWidths() adds tab width data to mFrame....

Comment 41

[Jonathan Kew [:jfkthame]]

OK, I guess we'll have to leave that -- thanks for looking into it.

Comment 42

[Jonathan Kew [:jfkthame]]

Comment on attachment 8730638 [details]
Bug 1203871 - Part 1. Add nsIFrame::GetCharacterRectsInRange.

https://reviewboard.mozilla.org/r/40057/#review57828

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b865bc98fb
Part 1. Add nsIFrame::GetCharacterRectsInRange. r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeadf50643fe
Part 2. Implement eQueryTextRectArray. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/894a2784d498
Part 3. Use eQueryTextRectArray on ContentCache for e10s. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/d654463ad5fa
Part 4. Implement QUERY_TEXT_RECT_ARRAY to nsIDOMWindowUtils for unit test. r=masayuki,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/99635fa90e92
Part 5. Use mozilla::Move for rect array. r=masayuki

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a1b865bc98fb
https://hg.mozilla.org/mozilla-central/rev/eeadf50643fe
https://hg.mozilla.org/mozilla-central/rev/894a2784d498
https://hg.mozilla.org/mozilla-central/rev/d654463ad5fa
https://hg.mozilla.org/mozilla-central/rev/99635fa90e92