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

RESOLVED FIXED in Firefox 50

Status

()

defect
P3
normal
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: masayuki, Assigned: m_kato)

Tracking

(Blocks 1 bug, {inputmethod, perf})

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox43 affected, firefox50 fixed)

Details

Attachments

(5 attachments)

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.
tracking-e10s: --- → +
Priority: -- → P3
If you'd like to steal this bug from me, feel free to do it ;-)
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
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
Duplicate of this bug: 1067230
Assignee: masayuki → m_kato
Using gfxTexRun, we can get width per character.  OK, I will take this
> 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.
(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?
Thanks. Looks like nsTArray_Impl:: SwapElements() is useful.
(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.
Blocks: 1094729
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/
Attachment #8730638 - Flags: review?(jfkthame)
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/
Attachment #8730640 - Flags: review?(masayuki)
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/
Attachment #8730641 - Flags: review?(masayuki)
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/
Attachment #8730642 - Flags: review?(masayuki)
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
Attachment #8730639 - Flags: review?(masayuki) → review+
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.
Attachment #8730638 - Flags: review?(jfkthame)
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.
Attachment #8730641 - Flags: review?(masayuki) → review+
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 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/
Attachment #8730638 - Attachment description: MozReview Request: Bug 1203871 - Part 1. Add nsIFrame::GetCharacterRectFromOffset. r?jfkthame → Bug 1203871 - Part 1. Add nsIFrame::GetCharacterRectsInRange.
Attachment #8730639 - Attachment description: MozReview Request: Bug 1203871 - Part 2. Implement eQueryTextRectArray. r?masayuki → Bug 1203871 - Part 2. Implement eQueryTextRectArray.
Attachment #8730640 - Attachment description: MozReview Request: Bug 1203871 - Part 3. Use eQueryTextRectArray on ContentCache for e10s. r?masayuki → Bug 1203871 - Part 3. Use eQueryTextRectArray on ContentCache for e10s.
Attachment #8730641 - Attachment description: MozReview Request: Bug 1203871 - Part 4. Implement QUERY_TEXT_RECT_ARRAY to nsIDOMWindowUtils. r?masayuki → Bug 1203871 - Part 4. Implement QUERY_TEXT_RECT_ARRAY to nsIDOMWindowUtils for unit test.
Attachment #8730642 - Attachment description: MozReview Request: Bug 1203871 - Part 5. Use mozilla::Move for rect array. r?masayuki → Bug 1203871 - Part 5. Use mozilla::Move for rect array.
Attachment #8730638 - Flags: review?(jfkthame)
Attachment #8730641 - Flags: review?(bugs)
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 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 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 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 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 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 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 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/
Attachment #8730638 - Flags: review?(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 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'
Attachment #8730641 - Flags: review?(bugs) → review+
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 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/
Attachment #8730638 - Flags: review?(jfkthame)
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 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 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 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/
(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?
(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....
OK, I guess we'll have to leave that -- thanks for looking into it.
Comment on attachment 8730638 [details]
Bug 1203871 - Part 1. Add nsIFrame::GetCharacterRectsInRange.

https://reviewboard.mozilla.org/r/40057/#review57828
Attachment #8730638 - Flags: review?(jfkthame) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
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
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.