Enable jsplugin to retrieve composition clauses information from a composition dom event

RESOLVED FIXED in Firefox 50

Status

()

Core
Plug-ins
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Ya-Chieh Wu (away Dec 7 - 9, Dec 16 - 2018), Assigned: Ya-Chieh Wu (away Dec 7 - 9, Dec 16 - 2018))

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 10 obsolete attachments)

11.45 KB, patch
Details | Diff | Splinter Review
In some cases, a plugin might need to set IMEStatus as disable or update the position of the candiateWindow. However, we don't have these for a plugin. 

This bug is for 
(1)
Rewrite the IMEStatus in nsIDOMWindowUtils.idl
https://dxr.mozilla.org/mozilla-central/rev/c4449eab07d39e20ea315603f1b1863eeed7dcfe/dom/interfaces/base/nsIDOMWindowUtils.idl#1017
(2)
Add a function to set the candiateWindow for a plugin
Blocks: 1269575
According to Bug 1269575 Comment 12, I would like to put all I need for a jsplugin into a new interface.
Summary: Let plugin be able to set IMEStatus and set candiateWindow → A new xpcom class for scripted application to handle IME
This bug is for scripted application to have more control on IME.

Here is the list of what app might need to handle IME:
1.Set IMEStatus 
2.Set candiateWindow
3.Clause information
4.Cancel Composition Text
If this is first time to create a scriptable XPCOM class, the patches for bug 917322 must help you. Especially part.11 (attachment 8550143 [details] [diff] [review]).

JS should be able to create nsITextInputContext with createInstance() like this:
https://dxr.mozilla.org/mozilla-central/rev/5511d54a3f172c1d68f98cc55dce4de1d0ba1b51/dom/base/test/chrome/window_nsITextInputProcessor.xul#72-73

Then, the instance should be initialized with a DOM window, e.g., attachTo(DOMWindow aDOMWindow). Then, the class can access proper nsIWidget instance:
https://dxr.mozilla.org/mozilla-central/rev/5511d54a3f172c1d68f98cc55dce4de1d0ba1b51/dom/base/TextInputProcessor.cpp#129,138,142,147,154
Thanks Masayuki, Comment 3 is really helpful. I will take look at those file.
Rough sketch of nsITextInputContext.idl:

 
interface nsITextInputContext : nsISupports {

    //set IME status    
    attribute unsigned long IMEStatus;
    
    //size of TextRangeArray
    readonly attribute unsigned long NumberOfClauses;
    
    //target clause length
    readonly attribute unsigned long ClauseLength;

    //target clause offset
    readonly attribute unsigned long ClauseOffset;

    //Set candiate window    
    void SetCandiateWindow(unsigned long mPoint_x, unsigned long mPoint_y, float aRect_x, float aRect_y, float aRect_width, float aRect_height);

    //Cancel composition
    void RequestToCommit(); 

}

Hey Masayuki, how do you think about this rough nsITextInputContext.idl?
oops! hey Masayuki, could you take a look at Comment 5?  thanks!
Flags: needinfo?(masayuki)
(In reply to Ya-Chieh Wu from comment #5)
> Rough sketch of nsITextInputContext.idl:
> 
>  
> interface nsITextInputContext : nsISupports {
> 
>     //set IME status    
>     attribute unsigned long IMEStatus;

Okay, but of course, you need to define const values for this.

>     //size of TextRangeArray
>     readonly attribute unsigned long NumberOfClauses;
>     
>     //target clause length
>     readonly attribute unsigned long ClauseLength;
> 
>     //target clause offset
>     readonly attribute unsigned long ClauseOffset;

I don't think these attributes are convenient. I think that you should define another interface e.g., nsITextComposition and it should wrap TextComposition class. Then, the clauses should be exposed with nsIArray whose items are defined by a new interface, e.g., nsITextCompositionClause or nsITextCompositionRange. (I think that all of them can be defined in an idl file.)

>     //Set candiate window    
>     void SetCandiateWindow(unsigned long mPoint_x, unsigned long mPoint_y,
> float aRect_x, float aRect_y, float aRect_width, float aRect_height);

Shouldn't mPoint_* be long rather than unsigned long? Do they always positive value?

Why are aRect_* float rather than long?

Additionally, don't you need to make it vertical-writing aware? If so, candidate window should be positioned left of right of first character of the target clause instead of bottom of it.

>     //Cancel composition
>     void RequestToCommit(); 

Don't you need an option to discard composition string?
Flags: needinfo?(masayuki)
(In reply to Ya-Chieh Wu from comment #5)
> interface nsITextInputContext : nsISupports {
> 
>     //set IME status    
>     attribute unsigned long IMEStatus;
Some nits, attributes and methods in idl should start with lowercase letter

>     //Set candiate window    
>     void SetCandiateWindow(unsigned long mPoint_x, unsigned long mPoint_y,
> float aRect_x, float aRect_y, float aRect_width, float aRect_height);
It needs to be document in which coordinates these all are.
A parameter shouldn't start with m.
And we don't usually use _ in the param names. aCamelCase is the normal style for params.
Created attachment 8757210 [details] [diff] [review]
Add two idl in order to expose more control on IME to scripted app

Comment 7 and Comment 8 help a lot. Thanks for masayuki and smaug. 

(In reply to smaug from Comment #8)
Could the IMEStatus start in upper case like?
https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#1017
Attachment #8757210 - Flags: feedback?(masayuki)
Assignee: nobody → ywu
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) from comment #7)
> (In reply to Ya-Chieh Wu from comment #5)
> >     //Set candiate window    
> >     void SetCandiateWindow(unsigned long mPoint_x, unsigned long mPoint_y,
> > float aRect_x, float aRect_y, float aRect_width, float aRect_height);
> 
> Shouldn't mPoint_* be long rather than unsigned long? Do they always
> positive value?
> 
> Why are aRect_* float rather than long?
> 
> Additionally, don't you need to make it vertical-writing aware? If so,
> candidate window should be positioned left of right of first character of
> the target clause instead of bottom of it.
 
By the way, I don't really get how to make vertical-writing aware available to this api. Should we give it one more parameter to indicate the orientation of the input box? or we make changes in gecko to check the rectangle box's orientation?

thanks for helping
Comment on attachment 8757210 [details] [diff] [review]
Add two idl in order to expose more control on IME to scripted app

>+++ b/dom/interfaces/base/nsITextComposition.idl
>@@ -0,0 +1,23 @@
>+#include "nsISupports.idl"
>+
>+interface nsIArray;
>+
>+interface nsITextComposition : nsISupports {

nit: puts |{| to the next line.

>+  
>+  /**
>+   * The latest clauses range of the conposition string.

s/conposition/composition

>+   * The items of nsIArray are defined by nsITextCompositionClause.
>+   */
>+  readonly attribute nsIArray textRangeArray;
>+
>+  /**
>+   * The offset of first selected clause or start of compositon.
>+   */
>+  readonly attribute unsigned long offsetOfTargetClause;

Looks good. And perhaps, like TextComposition, requestToCommit() should be here. (But it's okay nsITextInputContext to have same method too for making callers simpler.)

>+  
>+}
>+
>+interface nsITextCompositionClause : nsISupports {  
>+  readonly attribute unsigned long startOffset;
>+  readonly attribute unsigned long endOffset;

Don't you need clause type?

And don't you need to expose caret position which is stored as a range whose type is "caret".

Anyway, could you add comments to explain what they mean.

>+}
>diff --git a/dom/interfaces/base/nsITextInputContext.idl b/dom/interfaces/base/nsITextInputContext.idl
>new file mode 100644
>--- /dev/null
>+++ b/dom/interfaces/base/nsITextInputContext.idl
>@@ -0,0 +1,54 @@
>+#include "nsISupports.idl"
>+
>+interface mozIDOMWindow;
>+interface nsITextComposition;
>+
>+interface nsITextInputContext : nsISupports {

nit: put |{| to the next line.

>+
>+  /**
>+   * DISABLED means users cannot use IME completely.
>+   * Note that this state is *not* same as |ime-mode: disabled;|.
>+   */
>+  const unsigned long IME_STATUS_DISABLED = 0;
>+
>+  /**
>+   * ENABLED means users can use all functions of IME. This state is same as
>+   * |ime-mode: normal;|.
>+   */
>+  const unsigned long IME_STATUS_ENABLED  = 1;
>+
>+  /**
>+   * PASSWORD means users cannot use most functions of IME. But on GTK2,
>+   * users can use "Simple IM" which only supports dead key inputting.
>+   * The behavior is same as the behavior of the native password field.
>+   * This state is same as |ime-mode: disabled;|.
>+   */
>+  const unsigned long IME_STATUS_PASSWORD = 2;
>+
>+  /**
>+   * PLUGIN means a plug-in has focus. At this time we should not touch to
>+   * controlling the IME state.
>+   */
>+  const unsigned long IME_STATUS_PLUGIN   = 3;

Looks good. For easier to maintain, nsITextInputContext.h should be included by IMEData.h and IMEData.h should use these constants to define Enabled enum.

>+
>+  /* 
>+   * Set or get IME status, see above IME_STATUS_* definitions.
>+   */
>+  attribute unsigned long IMEStatus;

I think that using uppercase at the first character of attribute name is okay in this case but I'm not sure the smaug's thought. (Basically, his previous comment is right, but writing acronyms with lowercase letters is not easy to read/understand for me.)

>+  /* 
>+   * Set candiate window. 
>+   */
>+  void SetCandiateWindow(in nsIArray layoutDeviceIntPoint, in nsIArray layoutDeviceIntRect);
>+    
>+  /*
>+   * Get TextComposition from window
>+   */
>+  nsITextComposition getTextComposition(in mozIDOMWindow aWindow);

I don't think that this is right. This interface should have |Init(in nsIDOMWindow aWindow)| and which should be called before calling the others. For example, on Mac and Linux, native IME context is created per top level window. Therefore, IMEStatus cannot return proper value without initialized already with a DOM window.

>+ 
>+  /** 
>+   * Request to commit (or cancel) the composition to IME.
>+   */
>+  void requestToCommit(in mozIDOMWindow aWindow, in boolean aDiscard);

So, aWindow isn't necessary, instead, there should be |init(in nsIDOMWindow aWindow)|.

>+
>+}
Attachment #8757210 - Flags: feedback?(masayuki) → feedback?(bugs)
yeah, ok, IMEStatus as an attribute name is fine, but functions and attributes should start with lowercase in general.
Attachment #8757210 - Flags: feedback?(bugs)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) from comment #11)
> Comment on attachment 8757210 [details] [diff] [review]
> Add two idl in order to expose more control on IME to scripted app

Masayuki, your comments help a lot. thanks.
And I have one question to ask since this is my first time to create a xpcom. 

> >+  /* 
> >+   * Set candiate window. 
> >+   */
> >+  void SetCandiateWindow(in nsIArray layoutDeviceIntPoint, in nsIArray layoutDeviceIntRect);
> >+    
> >+  /*
> >+   * Get TextComposition from window
> >+   */
> >+  nsITextComposition getTextComposition(in mozIDOMWindow aWindow);
> 
> I don't think that this is right. This interface should have |Init(in
> nsIDOMWindow aWindow)| and which should be called before calling the others.
> For example, on Mac and Linux, native IME context is created per top level
> window. Therefore, IMEStatus cannot return proper value without initialized
> already with a DOM window.
> 
> >+ 
> >+  /** 
> >+   * Request to commit (or cancel) the composition to IME.
> >+   */
> >+  void requestToCommit(in mozIDOMWindow aWindow, in boolean aDiscard);
> 
> So, aWindow isn't necessary, instead, there should be |init(in nsIDOMWindow
> aWindow)|.
> 
> >+
> >+}

I don't really get to this point. I wonder the reason we have one more API, |Boolean Init(in nsIDOMWindow aWindow)|, is because we won't have duplicate code in |getTextComposition(in nsIDOMWindow aWindow)| and |requestToCommit(in mozIDOMWindow aWindow, in boolean aDiscard);| ?   Because, for example, like |nsITextComposition getTextComposition(in mozIDOMWindow aWindow)|, if we don't have "nsITextComposition" already, we can just give the value of "nsITextComposition" as null? Am I misunderstanding anything?
Flags: needinfo?(masayuki)
Basically, you're right. And you define IMEStatus attribute. This means that you need to specify DOM window *before* accessing this. So, you need init(DOMWindow) for them.

If there is no TextComposition, getTextComposition() should return null, I think. But another possible implementation is, it returns always non-null but has isComposing or something and linked with active composition. I guess the former is easier to understand, though.

>+  /* 
>+   * Set candiate window. 
>+   */
>+  void SetCandiateWindow(in nsIArray layoutDeviceIntPoint, in nsIArray layoutDeviceIntRect);

And I realized that you shouldn't use nsIArray here because it's unclear that which item is for x/y/top/left/width/height. Please define a lot of int arguments.

Additionally, I'm not sure this is right API. If this API is available only with IMM on Windows and GTK, this is okay. But with TSF on Windows and Cocoa, this does not make sense because native IME on them tries to query a rect of caret, first character of composition string or first character of target clause and decides the position with the query result. So, I think that ideally, JS plugin should have a listener interface for listening to such query events.
Flags: needinfo?(masayuki)
Created attachment 8758582 [details] [diff] [review]
rewrite my two idl base on the Comment above.

Masayuki, could you help to take a look at this? I hope this one make more sense to you. thanks for your patience and time. It's really helpful.

Beside this, as Comment 14, I originally thought I can somehow setup my candidate window by using [1]. And the reason why I might need to setup candidate window at the very begining is when my js-plugin gets a mouse event on a input box, don't I need to notify the widget where exactly is my inputbox in a jsplugin?  thanks again for your time.
  
[1]https://dxr.mozilla.org/mozilla-central/source/widget/IMEData.h#444
Attachment #8757210 - Attachment is obsolete: true
Attachment #8758582 - Flags: feedback?(masayuki)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) from comment #14)
 
> Additionally, I'm not sure this is right API. If this API is available only
> with IMM on Windows and GTK, this is okay. But with TSF on Windows and
> Cocoa, this does not make sense because native IME on them tries to query a
> rect of caret, first character of composition string or first character of
> target clause and decides the position with the query result. So, I think
> that ideally, JS plugin should have a listener interface for listening to
> such query events.

Hey Masayuki, 
now I get your point. Yes, you're right. This "SetCandiateWindow" might not make sense. I think the candidate window should dynamically change position like you said. I think at this point I will skip putting "SetCandiateWindow" in this xpcom until I figure out how to make it more sense.  thanks for your  explanation.
Comment on attachment 8758582 [details] [diff] [review]
rewrite my two idl base on the Comment above.

># HG changeset patch
># User mozilla <mozilla>
># Date 1464762784 -28800
>#      Wed Jun 01 14:33:04 2016 +0800
># Node ID 243ce1beb71bbc37ba85a569d21043bcb1615cf2
># Parent  8d0aadfe7da782d415363880008b4ca027686137
>add nsITextComposition.idl and nsITextInputContext.idl
>
>diff --git a/dom/interfaces/base/nsITextComposition.idl b/dom/interfaces/base/nsITextComposition.idl
>new file mode 100644
>--- /dev/null
>+++ b/dom/interfaces/base/nsITextComposition.idl
>@@ -0,0 +1,69 @@
>+#include "nsISupports.idl"
>+
>+interface nsIArray;
>+
>+interface nsITextComposition : nsISupports
>+{
>+  /**
>+   * The latest clauses range of the composition string.
>+   * This is the clause and caret range information.
>+   * The items of nsIArray are defined by nsITextCompositionClause.
>+   */
>+  readonly attribute nsIArray textClauseArray;

Oh, I realized that *when* you need to access this interface? I mean, if this interface's implementation just returns the result of similar name methods of TextComposition, you need to be careful about the timing.

The clauses are modified when editor receives eCompositionChange event which is performed by an "text" event bubbling phase listener in the system event group. So, if you won't use <input>, <textarea> nor <foo contenteditable> for handling composition events, nobody will update this in current code. If so, you need to call EditorWillHandleCompositionChangeEvent() via this interface.

(FYI: See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIEventListenerService#addSystemEventListener() for the detail of "the system event group")

>+  /**
>+   * The offset of first selected clause or start of compositon.
>+   */
>+  readonly attribute unsigned long offsetOfTargetClause;
>+
>+  /**
>+   * The offset of first composition string
>+   */
>+  readonly attribute unsigned long nativeOffsetOfStartComposition;

Similarly, these offsets of TextComposition are modified after compositionstart and text events are fired on all event listeners in the DOM tree. In other words, never updated if you try to retrieve them from a DOM event listener. Perhaps, we can change the timing, though.

And I think that "native" isn't necessary here. TextComposition stores/returns offsets in text which has native line breakers. I.e., the offset is different between Windows (CR+LF) and the others (LF).  When you need to notify IME of offsets in the future, native offset is better than XP offset, perhaps.

If you'd need XP offsets for them, we'd be able to add new attributes for them.

>+/*
>+ * This interface is related to "TextRange" in Gecko.
>+ * It has the clause and caret range information.
>+ */
>+interface nsITextCompositionClause : nsISupports
>+{
>+  /*
>+   * There are six types of the textType which are
>+   *
>+   * NS_TEXTRANGE_UNDEFINED = 0x00,
>+   * NS_TEXTRANGE_CARETPOSITION = 0x01,
>+   * NS_TEXTRANGE_RAWINPUT = nsITextInputProcessor::ATTR_RAW_CLAUSE,
>+   * NS_TEXTRANGE_SELECTEDRAWTEXT = nsITextInputProcessor::ATTR_SELECTED_RAW_CLAUSE,
>+   * NS_TEXTRANGE_CONVERTEDTEXT = nsITextInputProcessor::ATTR_CONVERTED_CLAUSE,
>+   * NS_TEXTRANGE_SELECTEDCONVERTEDTEXT = nsITextInputProcessor::ATTR_SELECTED_CLAUSE
>+   *
>+   * The first two ATTR_UNDEFINED and ATTR_CARETPOSITION are defined here.
>+   * The other are defined in nsITextInputProcessor.
>+   */
>+  const unsigned long ATTR_UNDEFINED = 0x00;
>+  const unsigned long ATTR_CARETPOSITION = 0x01;

I think that you can create a new XPCOM interface which is common base interface of nsITextInputProcessor and this interface and defines only the constants.

>+  /*
>+   * There are six types of the textType. Please see the comments above.
>+   */
>+  readonly attribute unsigned long textType;

sounds odd. |type| is should be the best name, but I'm not sure if it's available. If it's not available, perhaps, |rangeType| or |clauseType|?

>+  //The start position of this clause or caret.
>+  readonly attribute unsigned long startOffset;

"from the start of composition"?

And just |offset| is okay.

Additionally, if you need to support native look and feel, you need API to access TextRangeStyle but it must be put off to another bug.

>+interface nsITextInputContext : nsISupports
>+{
>+  /*
>+   * When you create an instance, you must call initTextContext() first
>+   */
>+  boolean initTextContext(in mozIDOMWindow aWindow);

Ah, |attachTo(aWindow)| could be better name.

>+  /*
>+   * Get TextComposition from window
>+   */
>+  nsITextComposition getTextComposition();

Why don't you make this just an readonly attribute? And then, the name could be |composition|. The reason why I named the class "TextComposition" is that "composition" is also used by gfx. So, the name is too generic. However, as a member of an interface or class, we can use shorter name since its interface/class name gives enough hint. So, nsITextInputContext.composition sounds enough clear to me.
Attachment #8758582 - Attachment is patch: true
Attachment #8758582 - Flags: feedback?(masayuki)
And if you need to retrieve clause information from a composition event handler, you can add |textClauseArray| attribute to CompositionEvent too. Perhaps, this must be simplest approach if you need to access the new clauses.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) from comment #18)
> And if you need to retrieve clause information from a composition event
> handler, you can add |textClauseArray| attribute to CompositionEvent too.
> Perhaps, this must be simplest approach if you need to access the new
> clauses.

If it is possible to put one more attribute into CompositionEvent, I guess it will be simpler than getting all Textcomposition expose.
So the tasks, for a js-plugin to handle IME, could be divided into two parts.
(1) Be able to "set" IME widget. This is bug 1269575
(2) Get the clauses information from a composition dom event. This is bug 1271932

For the first part, I won't handle it in this bug because there might be easy ways to handle it for a jsplugin and I still need time to figure it out. So I would like to put this bug as a dedicated one for the second part. I hope by doing so, it will be clearer for this bug.
Blocks: 1271932
No longer blocks: 1269575
Summary: A new xpcom class for scripted application to handle IME → Enable jsplugin to retrieve composition clauses information from a composition dom event
Created attachment 8765364 [details] [diff] [review]
put TextRangeArray information into composition dom event

Hey Masayuki,
this patch is trying to get clauses information expose with composition dom event. could you give me any advises? 

Thanks!
Attachment #8758582 - Attachment is obsolete: true
Attachment #8765364 - Flags: feedback?(masayuki)
Comment on attachment 8765364 [details] [diff] [review]
put TextRangeArray information into composition dom event

Looks good.

> interface CompositionEvent : UIEvent
> {
>   readonly attribute DOMString? data;
>   readonly attribute DOMString  locale;
>+
>+  /*
>+   * mClauses is trying to expose TextRangeArray in Gecko so a js-plugin couble able to know the
>+   * clauses infomation
>+   */
>+  [ChromeOnly]
>+  sequence<TextClauseArray> mClauses();

Shouldn't be remove "Array" from the interface name? because this means that array of array of clauses.

And mClauses() is odd because it may include caret information and shouldn't it be a readonly attribute? So, I guess that it should be:

readonly attribute sequence<TextRange> ranges;

# Note that I've not used/implemented sequence, so, I might be wrong.

> };
> 
> partial interface CompositionEvent
> {
>   void initCompositionEvent(DOMString typeArg,
>                             boolean canBubbleArg,
>                             boolean cancelableArg,
>                             Window? viewArg,
>                             DOMString? dataArg,
>                             DOMString localeArg);
> };
>+

Why do you add this line?

>diff --git a/dom/webidl/TextClauseArray.webidl b/dom/webidl/TextClauseArray.webidl
>new file mode 100644
>--- /dev/null
>+++ b/dom/webidl/TextClauseArray.webidl
>@@ -0,0 +1,23 @@
>+
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+
>+interface TextClauseArray {

I think this should be just "TextRange". And please put |{| to the next line. 

>+
>+    //The Start Offset of TextRange
>+	readonly attribute long mStartOffset;

startOffset

>+
>+    //The End Offset of TextRange
>+	readonly attribute long mEndOffset;

endOffset

>+
>+    //If the TextRange is Caret or not
>+	readonly attribute boolean mCaret;

isCaret

>+
>+    //If the TextRange is TargetClause or not
>+	readonly attribute boolean mTargetClause;

isTargetClause
Attachment #8765364 - Flags: feedback?(masayuki)
Created attachment 8767018 [details] [diff] [review]
Enhance "text" composition event's infomation

Masayuki, 

This patch is trying to expose clauses information with text composition event. 
I put a new api in CompositionEvent.webidl to access "TextRange array" and I add TextClause.webidl file to define the data struct of the "TextRange array". 

Thank you.
Attachment #8765364 - Attachment is obsolete: true
Attachment #8767018 - Flags: review?(masayuki)
Comment on attachment 8767018 [details] [diff] [review]
Enhance "text" composition event's infomation

>Bug 1275473 - Enhance text composition event's info

Perhaps, "Implement CompositionEvent.ranges" is better.

>diff --git a/dom/events/CompositionEvent.cpp b/dom/events/CompositionEvent.cpp
>--- a/dom/events/CompositionEvent.cpp
>+++ b/dom/events/CompositionEvent.cpp
>@@ -31,16 +31,35 @@ CompositionEvent::CompositionEvent(Event
>     //     However, it doesn't make sence for us, we cannot cancel composition
>     //     when we sends compositionstart event.
>     mEvent->mFlags.mCancelable = false;
>   }
> 
>   // XXX Do we really need to duplicate the data value?
>   mData = mEvent->AsCompositionEvent()->mData;
>   // TODO: Native event should have locale information.
>+
>+  RefPtr<TextRangeArray> mTextRangeArray =
>+    mEvent->AsCompositionEvent()->mRanges;

Although, this isn't necessary as I'll say in following comment, you shouldn't use "m" prefix for local variable.

>+  if (!mTextRangeArray)
>+    return;

And you need to use {} even if its block has only one line.

>+  mRanges.Clear();
>+  nsCOMPtr<nsPIDOMWindowInner> window =
>+    do_QueryInterface(aOwner->GetOwnerGlobal());
>+  uint32_t targetClauseOffset = mTextRangeArray->TargetClauseOffset();
>+
>+  for (uint32_t i = 0; i < mTextRangeArray->Length(); i++) {
>+    const TextRange& range = mTextRangeArray->ElementAt(i);
>+    bool isTargetClause =
>+      (targetClauseOffset == range.mStartOffset) ? true : false;
>+    bool isCaret =
>+      (range.mRangeType == /*TextRangeType::eCaret*/ 0x01) ? true : false;
>+    mRanges.AppendElement(new TextClause(
>+      window, range.mStartOffset, range.mEndOffset, isCaret, isTargetClause));
>+  }

You shouldn't initialize mRanges here because in most cases, the attribute is never accessed but this needs to allocate some extra space in the heap. You should initialize it when GetRanges() is called first time. So, mRanges should be a pointer to nsTArray<RefPtr<Clause>> and it should be nullptr until GetRanges() is called.

Note that for this approach, you need to copy WidgetCompositionEvent::mRanges in WidgetCompositionEvent::AssignCompositionEventData(). If you'd not do that, CompositionEvent.ranges returns empty array when GetRanges() is called after event propagation (e.g., event is cached to global variable).

>+void
>+CompositionEvent::GetRanges(nsTArray<RefPtr<TextClause> >& aRange) const

You don't need to worry about ">>" issue. Please remove the space for old compiler.

> class CompositionEvent : public UIEvent
> {

How about do |typedef nsTArray<RefPtr<TextClause>> TextClauseArray;|. Then, you can write it easier.

>+  void GetRanges(nsTArray<RefPtr<TextClause> >& aRetVal) const;

For example, here.

>+  /**
>+   * Array of TextClause.
>+   */
>+  nsTArray<RefPtr<TextClause> > mRanges;

And here. And you don't need the comment because it's enough clear.

>@@ -0,0 +1,71 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:set ts=2 sw=2 sts=2 et cindent: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "mozilla/dom/TextClause.h"
>+#include "mozilla/dom/TextClauseBinding.h"
>+
>+namespace mozilla
>+{

Put the |{| at the end of previous line when you write namespace.

>+namespace dom
>+{

ditto.

>+TextClause::TextClause(nsPIDOMWindowInner* aOwner, int32_t aStartOffset,
>+                       int32_t aEndOffset, bool aIsCaret, bool aIsTargetClause)

Hmm, the offset of TextRange is uint32_t. So, this is lossy... Why don't you use uint32_t here?

>+  : mOwner(aOwner)
>+  , mStartOffset(aStartOffset)
>+  , mEndOffset(aEndOffset)
>+  , mIsCaret(aIsCaret)
>+  , mIsTargetClause(aIsTargetClause)
>+{
>+  MOZ_ASSERT(aOwner);
>+}
>+
>+TextClause::~TextClause()
>+{
>+  // Add |MOZ_COUNT_DTOR(TextClause);| for a non-refcounted object.
>+}
>+
>+JSObject*
>+TextClause::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
>+{
>+  return TextClauseBinding::Wrap(aCx, this, aGivenProto);
>+}

I think that following methods can be inline in the header file.

>+int32_t
>+TextClause::StartOffset() const
>+{
>+  return mStartOffset;
>+}
>+
>+int32_t
>+TextClause::EndOffset() const
>+{
>+  return mEndOffset;
>+}
>+
>+bool
>+TextClause::IsCaret() const
>+{
>+  return mIsCaret;
>+}
>+
>+bool
>+TextClause::IsTargetClause() const
>+{
>+  return mIsTargetClause;
>+}

>diff --git a/dom/events/TextClause.h b/dom/events/TextClause.h
>new file mode 100644
>--- /dev/null
>+++ b/dom/events/TextClause.h
>@@ -0,0 +1,72 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:set ts=2 sw=2 sts=2 et cindent: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef mozilla_dom_TextClause_h
>+#define mozilla_dom_TextClause_h
>+
>+#include "js/TypeDecls.h"
>+#include "mozilla/Attributes.h"
>+#include "mozilla/ErrorResult.h"
>+#include "mozilla/dom/BindingDeclarations.h"
>+#include "nsCycleCollectionParticipant.h"
>+#include "nsWrapperCache.h"
>+
>+namespace mozilla {
>+namespace dom {
>+
>+class TextClause final : public nsISupports,

Please put "," below the ':'.

>+                         public nsWrapperCache
>+{
>+public:
>+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>+  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(TextClause)
>+
>+  nsPIDOMWindowInner* GetParentObject() const
>+  {
>+     return mOwner;
>+  }
>+  TextClause(nsPIDOMWindowInner* aWindow, int32_t aStartOffset, int32_t aEndOffset,
>+             bool aIsCaret, bool aIsTargetClause);
>+
>+  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
>+
>+  int32_t StartOffset() const;
>+
>+  int32_t EndOffset() const;
>+
>+  bool IsCaret() const;
>+
>+  bool IsTargetClause() const;
>+
>+private:
>+  /****************************************************************************
>+   * Variables. For TextRange, please take look at widget/TextRange.h.
>+   ***************************************************************************/
>+  ~TextClause();
>+  nsCOMPtr<nsPIDOMWindowInner> mOwner;
>+  /*
>+   * The start offset of TextRange
>+   */
>+  int32_t mStartOffset;
>+  /*
>+   * The end Offset of TextRange
>+   */
>+  int32_t mEndOffset;
>+  /*
>+   * If the TextRange is a Caret or not
>+   */
>+  bool mIsCaret;
>+  /*
>+   * If the TextRange is target clause or not
>+   */
>+  bool mIsTargetClause;

I think that we don't need the comments because the names are enough clear.

>diff --git a/dom/webidl/CompositionEvent.webidl b/dom/webidl/CompositionEvent.webidl
>--- a/dom/webidl/CompositionEvent.webidl
>+++ b/dom/webidl/CompositionEvent.webidl
>@@ -8,16 +8,23 @@
>  * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C
>  * liability, trademark and document use rules apply.
>  */
> 
> interface CompositionEvent : UIEvent
> {
>   readonly attribute DOMString? data;
>   readonly attribute DOMString  locale;
>+
>+ /*
>+  * ranges is trying to expose TextRangeArray in Gecko so a js-plugin couble be able to know the
>+  * clauses infomation
>+  */

Could add an "*" at the first line? We usually use this style comment:

/**
 *
 */

>+  [ChromeOnly,Cached,Pure]
>+  readonly attribute sequence<TextClause> ranges;
>diff --git a/dom/webidl/TextClause.webidl b/dom/webidl/TextClause.webidl
>new file mode 100644
>--- /dev/null
>+++ b/dom/webidl/TextClause.webidl
>@@ -0,0 +1,15 @@
>+interface TextClause
>+{
>+  //The start offset of TextRange

Could you insert a whitespace after "//"?

>+  readonly attribute long startOffset;
>+
>+  //The end offset of TextRange

Ditto.

>+  readonly attribute long endOffset;
>+
>+  //If the TextRange is Caret or not

Ditto.

>+  readonly attribute boolean isCaret;
>+
>+  //If the TextRange is TargetClause or not

Ditto.

>+  readonly attribute boolean isTargetClause;
>+
>+};
Attachment #8767018 - Flags: review?(masayuki) → review-
Comment hidden (obsolete)
Created attachment 8767833 [details] [diff] [review]
Implement CompositionEvent.ranges

This patch I follow Comment 24 to fix the mentioned issues.

>Note that for this approach, you need to copy WidgetCompositionEvent::mRanges 
>in WidgetCompositionEvent::AssignCompositionEventData(). If you'd not do that, 
>CompositionEvent.ranges returns empty array when GetRanges() is called after 
>event propagation (e.g., event is cached to global variable).


Actually in our js-plugin shim layer, I am using "text" event to get the TextRangeArray address and everything works fine.  Do you think this is ok to use "text" event?

thanks.
Attachment #8767829 - Attachment is obsolete: true
Attachment #8767829 - Flags: review?(masayuki)
Attachment #8767833 - Flags: review?(masayuki)
> Do you think this is ok to use "text" event?

Yes, for now. But it might be removed in the future due to non-standard event.
Comment on attachment 8767833 [details] [diff] [review]
Implement CompositionEvent.ranges

>+void
>+CompositionEvent::GetRanges(nsTArray<RefPtr<TextClause>>& aRange) const

Why not aRanges?
              ~

>+{
>+  RefPtr<TextRangeArray> textRangeArray =
>+    mEvent->AsCompositionEvent()->mRanges;
>+  if (!textRangeArray)
>+    return;

Use {} even if its block has only one line.

>+  aRange.Clear();

Why don't you do this first? If mEvent doesn't have mRanges, you return original aRange.

>+  nsCOMPtr<nsPIDOMWindowInner> window =
>+    do_QueryInterface(mOwner);
>+  uint32_t targetClauseOffset = textRangeArray->TargetClauseOffset();
>+
>+  for (uint32_t i = 0; i < textRangeArray->Length(); i++) {
>+    const TextRange& range = textRangeArray->ElementAt(i);
>+    bool isTargetClause =
>+      (targetClauseOffset == range.mStartOffset) ? true : false;

I don't think this is right. You need to check if the range is clause. A caret range may be at the start of target clause.

>+    aRange.AppendElement(new TextClause(
>+      window, range.mStartOffset, range.mEndOffset, !range.IsClause(), isTargetClause));

Looks like over 80 characters, isn't it?

I think that you should create TextClause and store to a variable first. Then, call AppendElement().

>+  }
>+}

And, this method returns new instance always. So, event.ranges == event.ranges is false. Please cache the result and use it for next call.

>@@ -32,22 +34,27 @@ public:
>   void InitCompositionEvent(const nsAString& aType,
>                             bool aCanBubble,
>                             bool aCancelable,
>                             nsGlobalWindow* aView,
>                             const nsAString& aData,
>                             const nsAString& aLocale);
>   void GetData(nsAString&) const;
>   void GetLocale(nsAString&) const;
>+  void GetRanges(nsTArray<RefPtr<TextClause>>& aRetVal) const;

aRanges is better.

> protected:
>   ~CompositionEvent() {}
> 
>   nsString mData;
>   nsString mLocale;
>+  /**
>+   * Array of TextClause.
>+   */
>+  nsTArray<RefPtr<TextClause>> mRanges;

So, you should use this to cache the result of GetRanges(). And please remove the unnecessary comment.

>diff --git a/dom/events/TextClause.cpp b/dom/events/TextClause.cpp
>+namespace mozilla {
>+namespace dom {
>+
>+  // Only needed for refcounted objects.

No, you shouldn't indent by |namespace foo {|.

>+  NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(TextClause)
>+  NS_IMPL_CYCLE_COLLECTING_ADDREF(TextClause)
>+  NS_IMPL_CYCLE_COLLECTING_RELEASE(TextClause)
>+  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(TextClause)
>+  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
>+  NS_INTERFACE_MAP_ENTRY(nsISupports)
>+  NS_INTERFACE_MAP_END
>+
>+  TextClause::TextClause(nsPIDOMWindowInner* aOwner, uint32_t aStartOffset,
>+                       uint32_t aEndOffset, bool aIsCaret, bool aIsTargetClause)

Odd indent. And please be careful for 80 characters per line rule.

>+    : mOwner(aOwner)
>+    , mStartOffset(aStartOffset)
>+    , mEndOffset(aEndOffset)
>+    , mIsCaret(aIsCaret)
>+    , mIsTargetClause(aIsTargetClause)
>+  {
>+    MOZ_ASSERT(aOwner);
>+  }
>+  JSObject*

Please insert an empty line between methods.

>+  TextClause::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
>+  {
>+    return TextClauseBinding::Wrap(aCx, this, aGivenProto);
>+  }
>+  TextClause::~TextClause()
>+  {
>+    // Add |MOZ_COUNT_DTOR(TextClause);| for a non-refcounted object.

What's this comment?

>+#ifndef mozilla_dom_TextClause_h

Could you put "_" at the last?

>+#define mozilla_dom_TextClause_h

Same here and the last line of this file.

>+  nsPIDOMWindowInner* GetParentObject() const
>+  {
>+     return mOwner;
>+  }

Who is this using?

>+  TextClause(nsPIDOMWindowInner* aWindow, uint32_t aStartOffset, uint32_t aEndOffset,

Over 80 characters.

>+             bool aIsCaret, bool aIsTargetClause);
>+ 
>+  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;

ditto.


>+private:
>+  /*******************************************************************
>+   * Variables. For TextRange, please take look at widget/TextRange.h.
>+   *******************************************************************/

Shouldn't you move this comment after mOwner? And "Variables" isn't necessary. And you should use "//" style comment for explaining member variables.

>+  ~TextClause();
>+  nsCOMPtr<nsPIDOMWindowInner> mOwner;
>+
>+  uint32_t mStartOffset;
>+  uint32_t mEndOffset;
>+  bool mIsCaret;
>+  bool mIsTargetClause;


> interface CompositionEvent : UIEvent
> {
>   readonly attribute DOMString? data;
>   readonly attribute DOMString  locale;
>+
>+ /**
>+  * ranges is trying to expose TextRangeArray in Gecko so a js-plugin couble be able to know the

Over 80 characters.
Attachment #8767833 - Flags: review?(masayuki) → review-
Created attachment 8768625 [details] [diff] [review]
Implement CompositionEvent.ranges

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #28)
> Comment on attachment 8767833 [details] [diff] [review]
> Implement CompositionEvent.ranges
> 
> >+void
> >+CompositionEvent::GetRanges(nsTArray<RefPtr<TextClause>>& aRange) const
> 
> Why not aRanges?
>               ~

Done.

> >+{
> >+  RefPtr<TextRangeArray> textRangeArray =
> >+    mEvent->AsCompositionEvent()->mRanges;
> >+  if (!textRangeArray)
> >+    return;
> 
> Use {} even if its block has only one line.
> 
Done.
> >+  aRange.Clear();
> 
> Why don't you do this first? If mEvent doesn't have mRanges, you return
> original aRange.
> 
I change the code a bit. I think now I don't need to clear aRange because aRange will either return the same array as mRanges or nothing. 

> >+  nsCOMPtr<nsPIDOMWindowInner> window =
> >+    do_QueryInterface(mOwner);
> >+  uint32_t targetClauseOffset = textRangeArray->TargetClauseOffset();
> >+
> >+  for (uint32_t i = 0; i < textRangeArray->Length(); i++) {
> >+    const TextRange& range = textRangeArray->ElementAt(i);
> >+    bool isTargetClause =
> >+      (targetClauseOffset == range.mStartOffset) ? true : false;
> 
> I don't think this is right. You need to check if the range is clause. A
> caret range may be at the start of target clause.
> 
Done.
> >+    aRange.AppendElement(new TextClause(
> >+      window, range.mStartOffset, range.mEndOffset, !range.IsClause(), isTargetClause));
> 
> Looks like over 80 characters, isn't it?

Sorry, I will remember the rule that we don't have a line over 80 characters.

> I think that you should create TextClause and store to a variable first.
> Then, call AppendElement().

I change the code so if mRanges is not empty, we will return it. If it's empty, we will append the element. 

> >+  }
> >+}
> 
> And, this method returns new instance always. So, event.ranges ==
> event.ranges is false. Please cache the result and use it for next call.
> 
Done.

> >@@ -32,22 +34,27 @@ public:
> >   void InitCompositionEvent(const nsAString& aType,
> >                             bool aCanBubble,
> >                             bool aCancelable,
> >                             nsGlobalWindow* aView,
> >                             const nsAString& aData,
> >                             const nsAString& aLocale);
> >   void GetData(nsAString&) const;
> >   void GetLocale(nsAString&) const;
> >+  void GetRanges(nsTArray<RefPtr<TextClause>>& aRetVal) const;
> 
> aRanges is better.
> 
Done.
> > protected:
> >   ~CompositionEvent() {}
> > 
> >   nsString mData;
> >   nsString mLocale;
> >+  /**
> >+   * Array of TextClause.
> >+   */
> >+  nsTArray<RefPtr<TextClause>> mRanges;
> 
> So, you should use this to cache the result of GetRanges(). And please
> remove the unnecessary comment.

Done.

> >diff --git a/dom/events/TextClause.cpp b/dom/events/TextClause.cpp
> >+namespace mozilla {
> >+namespace dom {
> >+
> >+  // Only needed for refcounted objects.
> 
> No, you shouldn't indent by |namespace foo {|.

Done.

> >+  TextClause::TextClause(nsPIDOMWindowInner* aOwner, uint32_t aStartOffset,
> >+                       uint32_t aEndOffset, bool aIsCaret, bool aIsTargetClause)
> 
> Odd indent. And please be careful for 80 characters per line rule.
> 
Done.
> >+    : mOwner(aOwner)
> >+    , mStartOffset(aStartOffset)
> >+    , mEndOffset(aEndOffset)
> >+    , mIsCaret(aIsCaret)
> >+    , mIsTargetClause(aIsTargetClause)
> >+  {
> >+    MOZ_ASSERT(aOwner);
> >+  }
> >+  JSObject*
> 
> Please insert an empty line between methods.

Done.

> >+  TextClause::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
> >+  {
> >+    return TextClauseBinding::Wrap(aCx, this, aGivenProto);
> >+  }
> >+  TextClause::~TextClause()
> >+  {
> >+    // Add |MOZ_COUNT_DTOR(TextClause);| for a non-refcounted object.
> 
> What's this comment?

This is comment is auto generated. Delete it. Done.

> >+#ifndef mozilla_dom_TextClause_h
> 
> Could you put "_" at the last?
> 
> >+#define mozilla_dom_TextClause_h
> 
> Same here and the last line of this file.

Done.

> >+  nsPIDOMWindowInner* GetParentObject() const
> >+  {
> >+     return mOwner;
> >+  }
> 
> Who is this using?

This is auto generated method. It will return a window.

> >+  TextClause(nsPIDOMWindowInner* aWindow, uint32_t aStartOffset, uint32_t aEndOffset,
> 
> Over 80 characters.
> 
> >+             bool aIsCaret, bool aIsTargetClause);
> >+ 
> >+  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
> 
> ditto.

Done. 

> 
> >+private:
> >+  /*******************************************************************
> >+   * Variables. For TextRange, please take look at widget/TextRange.h.
> >+   *******************************************************************/
> Shouldn't you move this comment after mOwner? And "Variables" isn't
> necessary. And you should use "//" style comment for explaining member
> variables.
> 
> >+  ~TextClause();
> >+  nsCOMPtr<nsPIDOMWindowInner> mOwner;
> >+
> >+  uint32_t mStartOffset;
> >+  uint32_t mEndOffset;
> >+  bool mIsCaret;
> >+  bool mIsTargetClause;
 
Done.

> > interface CompositionEvent : UIEvent
> > {
> >   readonly attribute DOMString? data;
> >   readonly attribute DOMString  locale;
> >+
> >+ /**
> >+  * ranges is trying to expose TextRangeArray in Gecko so a js-plugin couble be able to know the
> 
> Over 80 characters.
Done.

Thanks Masayuki, for your time. I will remember those nits and will not make same mistakes again. This patch I follow Comment 28 to fix the previous patch.
Thanks again.
Attachment #8767833 - Attachment is obsolete: true
Attachment #8768625 - Flags: review?(masayuki)
Comment on attachment 8768625 [details] [diff] [review]
Implement CompositionEvent.ranges

>+void
>+CompositionEvent::GetRanges(TextClauseArray& aRanges)
>+{
>+  // If the mRanges is not empty, we return the cached value.
>+  if (!mRanges.IsEmpty()) {
>+    aRanges = mRanges;
>+    return;
>+  }
>+  RefPtr<TextRangeArray> textRangeArray = mEvent->AsCompositionEvent()->mRanges;
>+  if (!textRangeArray) {
>+    return;
>+  }
>+  nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(mOwner);
>+  uint32_t targetClauseOffset = textRangeArray->TargetClauseOffset();
>+
>+  for (uint32_t i = 0; i < textRangeArray->Length(); i++) {

nit: size_t?

>+    const TextRange& range = textRangeArray->ElementAt(i);
>+    if (range.IsClause()) {
>+      bool isTargetClause =
>+        (targetClauseOffset == range.mStartOffset) ? true : false;
>+
>+      mRanges.AppendElement(new TextClause(window, range.mStartOffset,
>+                                           range.mEndOffset, false,
>+                                           isTargetClause));

Sorry, I should've pointed at previous review, though. Why don't you send |const TextRange&| as an argument of the constructor of TextClause? Then, you can unify the start offset, end offset and isClause to one argument. So, this can be |TextClause(window, range, isTargetClause)|.

>+    } else {
>+      mRanges.AppendElement(new TextClause(window, range.mStartOffset,
>+                                           range.mEndOffset, true, false));

Similarly, this can be |TextClause(window, range, false)|.

>diff --git a/dom/events/TextClause.cpp b/dom/events/TextClause.cpp

>+namespace mozilla
>+{

nit: |{| should be at the end of previous line when you write |namespace|.

>+namespace dom
>+{

nit: ditto.

>diff --git a/dom/events/TextClause.h b/dom/events/TextClause.h
>new file mode 100644
>--- /dev/null
>+++ b/dom/events/TextClause.h
>@@ -0,0 +1,58 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:set ts=2 sw=2 sts=2 et cindent: */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef mozilla_dom_TextClause_h_
>+#define mozilla_dom_TextClause_h_

Sorry, I remembered wrong naming rule of include guard. Please remove the last "_" too. So, it should be |mozilla_dom_TextClause_h| in our latest coding rules.

>+namespace mozilla
>+{

nit: put the |{| to the previous line.

>+namespace dom
>+{

nit: same here.

>+
>+class TextClause final : public nsISupports, public nsWrapperCache

Could you write this as:

class TextClause final : public nsISupports
                       , public nsWrapperCache

>+#endif // mozilla_dom_TextClause_h_

nit: fix the last "_" here too.

>+
>+ /**
>+  * ranges is trying to expose TextRangeArray in Gecko so a
>+  *	js-plugin couble be able to know the clauses infomation

odd indent. second line should start at same column as the first line.

>@@ -0,0 +1,15 @@
>+interface TextClause
>+{
>+  // The start offset of TextRange

nit: TextRange? You named this class TextClause... But I wonder, TextClause sounds odd, though because this may be caret range... But up to you because there is already mozilla::TextRange...

>+  readonly attribute long startOffset;
>+
>+  // The end offset of TextRange

Same here.

>+  readonly attribute long endOffset;
>+
>+  // If the TextRange is Caret or not

Same.

>+  readonly attribute boolean isCaret;
>+
>+  // If the TextRange is TargetClause or not

Same.


Sorry, I'd like to check new patch. But looks almost fine to me.

# Sorry for the delay due to working on big patches under editor/libeditor.
Attachment #8768625 - Flags: review?(masayuki) → review-
Created attachment 8770787 [details] [diff] [review]
Implement CompositionEvent.ranges

Hey masayuki,

First of all, thank you so much for your kindness and your patience. This patch I follow Comment 30 to fix the previous patch. I only keep the following part as the same. 

> >@@ -0,0 +1,15 @@
> >+interface TextClause
> >+{
> >+  // The start offset of TextRange
> 
> nit: TextRange? You named this class TextClause... But I wonder, TextClause
> sounds odd, though because this may be caret range... But up to you because
> there is already mozilla::TextRange...

Could we keep the name as TextClause? Because we already have TextRange and furthermore, we have a member to indicate if it's caret or not. Do you think this is fine or not? 

masayuki, thank you so much.
Attachment #8768625 - Attachment is obsolete: true
Attachment #8770787 - Flags: review?(masayuki)
> Could we keep the name as TextClause? Because we already have TextRange and furthermore, we have
> a member to indicate if it's caret or not. Do you think this is fine or not? 

Yeah, we may keep it since I have no better idea... If we rename current TextRange to TextRangeInternal, the name is too long...
Comment on attachment 8770787 [details] [diff] [review]
Implement CompositionEvent.ranges

>+void
>+CompositionEvent::GetRanges(TextClauseArray& aRanges)
>+{
>+  // If the mRanges is not empty, we return the cached value.
>+  if (!mRanges.IsEmpty()) {
>+    aRanges = mRanges;
>+    return;
>+  }
>+  RefPtr<TextRangeArray> textRangeArray = mEvent->AsCompositionEvent()->mRanges;
>+  if (!textRangeArray) {
>+    return;
>+  }
>+  nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(mOwner);
>+  uint32_t targetClauseOffset = textRangeArray->TargetClauseOffset();
>+
>+  for (size_t i = 0; i < textRangeArray->Length(); i++) {
>+    const TextRange& range = textRangeArray->ElementAt(i);
>+    mRanges.AppendElement(new TextClause(window, range, targetClauseOffset));

Hmm, sending target clause offset to TextClause is tricky... I like bool argument better for here.

How about make it bool and define temporary bool variable,

bool isTargetClause = range.IsClause() &&
                      range.mStartOffset == targetClauseOffset;

and send this to the argument?

>+TextClause::TextClause(nsPIDOMWindowInner* aOwner, const TextRange& range,

nit: aRange

>+                       uint32_t targetClauseOffset)

So, this should be |bool aIsTargetClause|.

Sorry, I'd like to check the final version. So, r-'ing even though this is enough good.
Attachment #8770787 - Flags: review?(masayuki) → review-
Hey Masayuki,

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #33)
> Comment on attachment 8770787 [details] [diff] [review]
> Implement CompositionEvent.ranges
> 
> >+void
> >+CompositionEvent::GetRanges(TextClauseArray& aRanges)
> >+{
> >+  // If the mRanges is not empty, we return the cached value.
> >+  if (!mRanges.IsEmpty()) {
> >+    aRanges = mRanges;
> >+    return;
> >+  }
> >+  RefPtr<TextRangeArray> textRangeArray = mEvent->AsCompositionEvent()->mRanges;
> >+  if (!textRangeArray) {
> >+    return;
> >+  }
> >+  nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(mOwner);
> >+  uint32_t targetClauseOffset = textRangeArray->TargetClauseOffset();
> >+
> >+  for (size_t i = 0; i < textRangeArray->Length(); i++) {
> >+    const TextRange& range = textRangeArray->ElementAt(i);
> >+    mRanges.AppendElement(new TextClause(window, range, targetClauseOffset));
> 
> Hmm, sending target clause offset to TextClause is tricky... I like bool
> argument better for here.
> 
> How about make it bool and define temporary bool variable,
> 
> bool isTargetClause = range.IsClause() &&
>                       range.mStartOffset == targetClauseOffset;
> 
> and send this to the argument?
> 

Ok! I will pass bool |isTargetOffest| into |TextClause|. But I will check if |range.IsClause()| in TextClause.cpp so I won't check |range.IsClause()| here in CompositionEvent.cpp. 


There is another thing that I noticed. |TargetClauseOffset()| will return 0 when there is no selected clause. But it could be confused if there is no selected clause or the |range->mStarOffset == 0|. So I will put my code as below. I wonder if the name is ok with |targetOffset| where I actually want the offset when |TextRangeType == eSelectedRawClause or eSelectedClause|.

diff --git a/dom/events/CompositionEvent.cpp b/dom/events/CompositionEvent.cpp

+  const TextRange* targetRange = textRangeArray->GetTargetClause();
+  uint32_t targetOffset = -1;
+  if (targetRange) {
+    targetOffset = targetRange->mStartOffset;
+  }
+  for (size_t i = 0; i < textRangeArray->Length(); i++) {
+    const TextRange& range = textRangeArray->ElementAt(i);
+    mRanges.AppendElement(
+      new TextClause(window, range, range.mStartOffset == targetOffset));
+  }

and 

diff --git a/dom/events/TextClause.cpp b/dom/events/TextClause.cpp

+TextClause::TextClause(nsPIDOMWindowInner* aOwner, const TextRange& aRange,
+                       bool isTargetOffset)

Do you think if it's fine with the name as is*Target*Offset or you think it will be better to name it as is*Selected*Offset? 

thank you so much!
Flags: needinfo?(masayuki)
Sorry for the delay to reply due to a national holiday of Japan.

Hmm, I don't like it... Could we send TextRangeArray and current index? Then, TextClause can initialize itself with whole information.
Flags: needinfo?(masayuki)
Created attachment 8772269 [details] [diff] [review]
This patch I change the last parameter in |CompositionEvent::GetRanges|, which is used to indicate the "target clause", to |const TextRange* targetRange|.

Hey Masayuki,

How about we pass |const TextRange* targetRange| into TextClause so we only need to go through the TextRangeArray once. 

Thank you so much.
Attachment #8770787 - Attachment is obsolete: true
Attachment #8772269 - Flags: review?(masayuki)
Comment on attachment 8772269 [details] [diff] [review]
This patch I change the last parameter in |CompositionEvent::GetRanges|, which is used to indicate the "target clause", to |const TextRange* targetRange|.

>+TextClause::TextClause(nsPIDOMWindowInner* aOwner, const TextRange& aRange,
>+                       const TextRange* targetRange)

I still don't like this arguments, but it's okay since I don't have better idea...

nit: s/targetRange/aTargetRange

>+  : mOwner(aOwner), mIsTargetClause(false)

Break line before |,| and it should be below the |:|.

>+class TextClause final : public nsISupports
>+                       , public nsWrapperCache
>+{
>+public:
>+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>+  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(TextClause)
>+
>+  nsPIDOMWindowInner* GetParentObject() const { return mOwner; }
>+
>+  TextClause(nsPIDOMWindowInner* aWindow, const TextRange& aRange,
>+             const TextRange* targetRange);

s/targetRange/aTargetRange

>+++ b/dom/webidl/TextClause.webidl
>@@ -0,0 +1,15 @@
>+interface TextClause

Wow, I realized that here is no header. Please add header comment same as other webidl files.

>+  // If the TextClause is TargetClause or not
>+  readonly attribute boolean isTargetClause;
>+
>+};

nit: Please remove the last empty line.
Attachment #8772269 - Flags: review?(masayuki) → review+
Created attachment 8773568 [details] [diff] [review]
Implement CompositionEvent.ranges. r=masayuki

The result of try seems fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5044ce195a58&selectedJob=24300063
Attachment #8772269 - Attachment is obsolete: true
Keywords: checkin-needed
Blocks: 1288593
The webidl changes need DOM peer review before this can land.
Keywords: checkin-needed
Comment on attachment 8773568 [details] [diff] [review]
Implement CompositionEvent.ranges. r=masayuki

r+ for the .webidl
Attachment #8773568 - Flags: review+
Created attachment 8774198 [details] [diff] [review]
Implement CompositionEvent.ranges. r=masayuki, r=smaug

Thank you masayuki and smaug for helping and reviewing. This patch it's basically the same patch as Comment 38 but adds smaug as a reviewer of webidl.

whoever helps with checkin, thank you in advance!
Attachment #8773568 - Attachment is obsolete: true
Keywords: checkin-needed

Comment 42

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/493c72689956
Implement CompositionEvent.ranges. r=masayuki, r=smaug
Keywords: checkin-needed

Comment 43

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/493c72689956
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 1294382
You need to log in before you can comment on or make changes to this bug.