Closed Bug 1275528 Opened 8 years ago Closed 8 years ago

[e10s] widget should be able to query contents with offset from start of composition string or current selection

Categories

(Core :: Widget, defect, P3)

Product:
Core
Component:
Widget
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
e10s + ---
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(5 files)

Bug 1275528 part.1 Support a way to query content relative to insertion point
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1275528 part.2 IMEContentObserver should use selection cache for computing absolute offset of WidgetQueryContentEvent if it's possible
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1275528 part.3 Support selection relative WidgetQueryContentEvent in e10s mode
58 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1275528 part.4 IMMHandler should use insertion point relative query for getting character rect and caret rect
58 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1275528 part.5 TSFTextStore should use insertion point relative query for getting character rect and caret rect only while there is a composition
58 bytes, text/x-review-board-request
m_kato
: review+
Details 
I think that we should add a new mode to each query events that can query contents with offset related to start of composition string (if there is a composition) or current selection (if there is no composition).

Then, we can make some native IME handler simpler and fix bug 1224994 easier.
tracking-e10s: --- → ?
tracking-e10s: ? → +
Priority: -- → P3
Native IME handler may want to query content relative to start of selection (or composition if there is it). Additionally, in e10s mode, insertion point in actual content may be different from the cache in parent.  Therefore, in some cases, it does make sense to query content with offset relative to start of selection or composition.

This patch implements it simply and only in non-e10s mode.

Additionally, this fixes a bug of nsQueryContentEventResult::GetOffset() which hasn't been accepted its calls even if the event message is valid (eQueryTextContent, eQueryTextRect and eQueryCaretRect).

Review commit: https://reviewboard.mozilla.org/r/59296/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59296/
Attachment #8762846 - Flags: review?(bugs)
Attachment #8762847 - Flags: review?(bugs)
Attachment #8762848 - Flags: review?(m_kato)
Attachment #8762849 - Flags: review?(m_kato)
Attachment #8762850 - Flags: review?(m_kato)
IMEContentObserver may have cache of normal selection.  If it's available, IMEContentObserver should use it for computing absolute offset of WidgetQueryContentEvent whose mInput::mOffset is relative offset to selection.

This patch just improves the performance of such query.

Review commit: https://reviewboard.mozilla.org/r/59298/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59298/
In e10s mode and during focus is in a remote process, ContentCache handles WidgetQueryContentEvent instead of ContentEventHandler.  Therefore, for supporting selection relative WidgetQueryContentEvent in e10s mode, we need to support it in ContentCache too.

Review commit: https://reviewboard.mozilla.org/r/59300/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59300/
IMM always queries character rect or caret rect relative to insertion point.  Therefore, it makes sense to use the new feature, insertion point relative query content events for them.  Then, IMMHandler doesn't need to care mismatch between its cache and actual content information.

Review commit: https://reviewboard.mozilla.org/r/59302/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59302/
TSFTextStore should use insertion point relative query for getting character rect and caret rect while there is a composition because in such case, TSF must want to query a part of or around the composition.  Therefore, it makes sense to use insertion point relative query since it adjusts the offset with the latest content information.

Review commit: https://reviewboard.mozilla.org/r/59304/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59304/
Comment on attachment 8762846 [details]
Bug 1275528 part.1 Support a way to query content relative to insertion point

https://reviewboard.mozilla.org/r/59296/#review56420

So, just requesting some comments for IsOffsetEnabled and IsNotFoundEnabled, so that I feel that I understand what the patch does.

::: dom/base/nsQueryContentEventResult.cpp:14
(Diff revision 1)
>  #include "nsPoint.h"
>  #include "mozilla/TextEvents.h"
>  
>  using namespace mozilla;
>  
> +static bool IsNotFoundEnabled(EventMessage aEventMessage)

Took a bit time to understand what this method is about. Just moved code sure, but a bit unclear what IsNotFound means in this context.

::: dom/base/nsQueryContentEventResult.cpp:73
(Diff revision 1)
> +      return NS_ERROR_NOT_AVAILABLE;
> +    }
> +  } else if (NS_WARN_IF(!IsOffsetEnabled(mEventMessage))) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +

I don't understand the setup here. IsNotFoundEnabled is called first, and in 'else' IsOffsetEnabled which internally calls IsNotFoundEnabled again.

And there needs to be some documentation somewhere why offset is enabled for certain messages but not others.

::: testing/mochitest/tests/SimpleTest/EventUtils.js:1591
(Diff revision 1)
> + *                      composition if there is, or start of selection.)
> + * @param aWindow  Optional (If null, current |window| will be used)
> + * @return         An nsIQueryContentEventResult object.  If this failed,
> + *                 the result might be null.
> + */
> +function synthesizeQueryTextContent(aOffset, aLength, aIsRelative, aWindow)

You went through all the current use of this function to ensure that 3rd param which used to be a window object is properly handled?
Attachment #8762846 - Flags: review?(bugs) → review-
Comment on attachment 8762847 [details]
Bug 1275528 part.2 IMEContentObserver should use selection cache for computing absolute offset of WidgetQueryContentEvent if it's possible

https://reviewboard.mozilla.org/r/59298/#review56422

sorry, I'm playing a bit dumb, but the setup isn't quite obvious from the patch.

::: dom/events/IMEContentObserver.cpp:710
(Diff revision 1)
>    // selection change notification but hasn't updated the cache yet (i.e.,
>    // not sending selection change notification to IME, don't use the cached
>    // value.  Note that don't update selection cache here since if you update
>    // selection cache here, IMENotificationSender won't notify IME of selection
>    // change because it looks like that the selection isn't actually changed.
> -  if (aEvent->mMessage == eQuerySelectedText && aEvent->mUseNativeLineBreak &&
> +  bool isSelectinCacheAvailable =

isSelectionCacheAvailable

::: dom/events/IMEContentObserver.cpp:733
(Diff revision 1)
>  
>    MOZ_LOG(sIMECOLog, LogLevel::Debug,
>      ("IMECO: 0x%p IMEContentObserver::HandleQueryContentEvent(aEvent={ "
>       "mMessage=%s })", this, ToChar(aEvent->mMessage)));
>  
> +  // If we can make the event's input offset absolute with TextComposition or

I'm lost here now. If we don't end up calling MakeOffsetAbsolute, where do we actually calculate the absolute offset?
Or in other words, how does calling MakeOffsetAbsolute prevent us to calculate the offset later.
Attachment #8762847 - Flags: review?(bugs) → review-
Comment on attachment 8762846 [details]
Bug 1275528 part.1 Support a way to query content relative to insertion point

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59296/diff/1-2/
Attachment #8762846 - Flags: review- → review?(bugs)
Attachment #8762847 - Flags: review- → review?(bugs)
Comment on attachment 8762847 [details]
Bug 1275528 part.2 IMEContentObserver should use selection cache for computing absolute offset of WidgetQueryContentEvent if it's possible

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59298/diff/1-2/
Comment on attachment 8762848 [details]
Bug 1275528 part.3 Support selection relative WidgetQueryContentEvent in e10s mode

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59300/diff/1-2/
Comment on attachment 8762849 [details]
Bug 1275528 part.4 IMMHandler should use insertion point relative query for getting character rect and caret rect

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59302/diff/1-2/
Comment on attachment 8762850 [details]
Bug 1275528 part.5 TSFTextStore should use insertion point relative query for getting character rect and caret rect only while there is a composition

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59304/diff/1-2/
https://reviewboard.mozilla.org/r/59296/#review56420

I renamed the methods to Is*PropertyAvailable(). This must be better than the old names. And of course, I added a few comment for explaining the methods what they do.

> Took a bit time to understand what this method is about. Just moved code sure, but a bit unclear what IsNotFound means in this context.

So, it's renamed to IsNotFoundProperyAvailable(). I hope this name helps you to understand that.

> I don't understand the setup here. IsNotFoundEnabled is called first, and in 'else' IsOffsetEnabled which internally calls IsNotFoundEnabled again.
> 
> And there needs to be some documentation somewhere why offset is enabled for certain messages but not others.

Okay, first, I changed the order. First, IsOffsetPropertyAvailable() is called and return if it's not available. Then, next, IsNotFoundPropertyAvailable() and GetNotFound() are called to to check if mOffset means "not found".  Finally, I added some comment to explain what and why the block does.

> You went through all the current use of this function to ensure that 3rd param which used to be a window object is properly handled?

Of course, I've already confirmed. All calleras don't specify |aWindow|. So, we don't need to change any other callers.
https://reviewboard.mozilla.org/r/59298/#review56422

> isSelectionCacheAvailable

Oh, thanks!

> I'm lost here now. If we don't end up calling MakeOffsetAbsolute, where do we actually calculate the absolute offset?
> Or in other words, how does calling MakeOffsetAbsolute prevent us to calculate the offset later.

Sure. I added some comments to IMEContentObserver::HandleQueryContentEvent().

If MakeOffsetAbsolute() was called already, WidgetQueryContentEvent::mInput is changed to absolute offset query. Therefore, ContentEventHandler won't try to recompute the offset as relative to insertion point query content event.  Additionally, if nobody computs the abosolute offset until reaching ContentEventHandler, ContentEventHandler::Init() computs selection start offset by itself but this may be expensive if the selection offset is too big (e.g., editing end of a long wikipedia page).
Comment on attachment 8762848 [details]
Bug 1275528 part.3 Support selection relative WidgetQueryContentEvent in e10s mode

https://reviewboard.mozilla.org/r/59300/#review56546
Attachment #8762848 - Flags: review?(m_kato) → review+
Attachment #8762849 - Flags: review?(m_kato) → review+
Comment on attachment 8762849 [details]
Bug 1275528 part.4 IMMHandler should use insertion point relative query for getting character rect and caret rect

https://reviewboard.mozilla.org/r/59302/#review56550
https://reviewboard.mozilla.org/r/59304/#review56554

::: widget/windows/TSFTextStore.cpp:3590
(Diff revision 2)
>    WidgetQueryContentEvent event(true, eQueryTextRect, mWidget);
>    mWidget->InitEvent(event);
> -  event.InitForQueryTextRect(acpStart, acpEnd - acpStart);
> +
> +  WidgetQueryContentEvent::Options options;
> +  int64_t startOffset = acpStart;
> +  if (mComposition.IsComposing()) {

Why int64_t instead of int32_t?

::: widget/windows/TSFTextStore.cpp:5113
(Diff revision 2)
> +  mWidget->InitEvent(queryCaretRect);
> +
> +  WidgetQueryContentEvent::Options options;
>    // XXX If this is called without composition and the selection isn't
>    //     collapsed, is it OK?
> -  uint32_t caretOffset = currentSel.MaxOffset();
> +  int64_t caretOffset = currentSel.MaxOffset();

Why int64_t instead of int32_t?
Comment on attachment 8762846 [details]
Bug 1275528 part.1 Support a way to query content relative to insertion point

https://reviewboard.mozilla.org/r/59296/#review56662
Attachment #8762846 - Flags: review?(bugs) → review+
Attachment #8762847 - Flags: review?(bugs) → review+
Comment on attachment 8762847 [details]
Bug 1275528 part.2 IMEContentObserver should use selection cache for computing absolute offset of WidgetQueryContentEvent if it's possible

https://reviewboard.mozilla.org/r/59298/#review56664

Those fixed, r+

::: dom/events/IMEContentObserver.cpp:739
(Diff revisions 1 - 2)
>    // mSelection, we should set it here for reducing the cost of computing
> -  // selection start offset.
> +  // selection start offset.  If ContentEventHandler receives a
> +  // WidgetQueryContentEvent whose input offset is relative to insertion point,
> +  // it computes current selection start offset (this may be expensive) and
> +  // make the offset absolute value itself.
> +  // Note that calling MakeOffsetAbsolute() makes the event is query content

Something odd with the sentence here.
"...makes the event is query..."


It is somewhat unclear to me why ContentEventHandler doesn't call MakeOffsetAbsolute if possible. Could you explain.
https://reviewboard.mozilla.org/r/59298/#review56664

> Something odd with the sentence here.
> "...makes the event is query..."
> 
> 
> It is somewhat unclear to me why ContentEventHandler doesn't call MakeOffsetAbsolute if possible. Could you explain.

As I said in the comment, ContentEventHandler does call MakeOffsetAbsolute() if coming event is relative to insertion point and with computing the insertion point offset. But the compuation may be expensive. Therefore, this patch uses IMEContentObserver's selection cache if it's available.
I see I see, silly question from me.
https://reviewboard.mozilla.org/r/59304/#review56554

> Why int64_t instead of int32_t?

Because WidgetQueryContent::mInput::mOffset is now int64_t (currently, uint32_t). The reason is, it should be able to represent all offset even if the base offset is 0 or UINT32_MAX.

> Why int64_t instead of int32_t?

Same as above. I think that it doesn't make senset to use smaller type in these cases.
Comment on attachment 8762846 [details]
Bug 1275528 part.1 Support a way to query content relative to insertion point

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59296/diff/2-3/
Comment on attachment 8762847 [details]
Bug 1275528 part.2 IMEContentObserver should use selection cache for computing absolute offset of WidgetQueryContentEvent if it's possible

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59298/diff/2-3/
Comment on attachment 8762848 [details]
Bug 1275528 part.3 Support selection relative WidgetQueryContentEvent in e10s mode

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59300/diff/2-3/
Comment on attachment 8762849 [details]
Bug 1275528 part.4 IMMHandler should use insertion point relative query for getting character rect and caret rect

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59302/diff/2-3/
Comment on attachment 8762850 [details]
Bug 1275528 part.5 TSFTextStore should use insertion point relative query for getting character rect and caret rect only while there is a composition

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59304/diff/2-3/
Comment on attachment 8762850 [details]
Bug 1275528 part.5 TSFTextStore should use insertion point relative query for getting character rect and caret rect only while there is a composition

https://reviewboard.mozilla.org/r/59304/#review57006
Attachment #8762850 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f727c7981ae
part.1 Support a way to query content relative to insertion point r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe82b725d93
part.2 IMEContentObserver should use selection cache for computing absolute offset of WidgetQueryContentEvent if it's possible r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/18676bf075ff
part.3 Support selection relative WidgetQueryContentEvent in e10s mode r=m_kato
https://hg.mozilla.org/integration/mozilla-inbound/rev/767e83fd263a
part.4 IMMHandler should use insertion point relative query for getting character rect and caret rect r=m_kato
https://hg.mozilla.org/integration/mozilla-inbound/rev/795180188ec3
part.5 TSFTextStore should use insertion point relative query for getting character rect and caret rect only while there is a composition r=m_kato
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: