Closed Bug 978023 Opened 6 years ago Closed 6 years ago

Rename nsIMEStateManager and nsTextStateManager and move into mozilla::

Categories

(Core :: DOM: Events, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(4 files, 3 obsolete files)

I think that nsIMEStateManager should be mozilla::IMEStateManager.

Additionally, nsTextStateManager should be separated to other files since it doesn't so depend on nsIMEStateManager.

But I have a concern. TextStateManager sounds strange to me. It observes focused editor for now. And notifies widget of the text content change, selection change, layout change and getting/losing focus.

In the future this might be used for entire document for screen reader support of TSF (Windows).

So, I think that mozilla::ContentObserver is better new name.

How do you think?
For safety, this moves nsTextStateManager's implementation without any logic changes.
Attachment #8385928 - Attachment is obsolete: true
Attachment #8386594 - Flags: review?(bugs)
This patch cleans up the ContentObserver (nsTextStateManger) implementation.

* Hides mWidget, mEditableNode, mSelection and mRootContent as private.
* Uses early return style.
* Uses NS_ENSURE_SUCCESS(_VOID) for checking the result of nsContentEventHandler methods.

I'll create patches for nsIMEStateManager after these patches are reviewed.
Attachment #8386595 - Flags: review?(bugs)
Comment on attachment 8386594 [details] [diff] [review]
part.1 nsTextStateManager should be mozilla::ContentObserver and separated from nsIMEStateManager.cpp

Can't you create the new file using hg copy.
I think the patch would be easier to review since it would show just
one hg copy + removing extra lines.

(Also, I think hg blame would work better in that case.)


ContentObserver is too generic name.
We have already nsIMutationObserver and nsIDocumentObserver interfaces, 
and MutationObserver exposed to JS.
Could it be IMEContentObserver?
Attachment #8386594 - Flags: review?(bugs) → review-
Attachment #8386595 - Flags: review?(bugs) → review+
Comment on attachment 8387389 [details] [diff] [review]
part.1 nsTextStateManager should be mozilla::IMEContentObserver and separated from nsIMEStateManager.cpp

>+IMEContentObserver::Reflow(DOMHighResTimeStamp aStart,
>+                        DOMHighResTimeStamp aEnd)
indentation
Attachment #8387389 - Flags: review?(bugs) → review+
Comment on attachment 8387390 [details] [diff] [review]
part.2 Clean up IMEContentObserver


>+nsresult
>+IMEContentObserver::GetSelectionAndRoot(nsISelection** aSelection,
>+                                        nsIContent** aRootContent) const
>+{
>+  if (!mEditableNode || !mSelection) {
>+    return NS_ERROR_NOT_AVAILABLE;
>+  }
>+
>+  NS_ASSERTION(mSelection && mRootContent, "uninitialized content observer");
>+  NS_ADDREF(*aSelection = mSelection);
>+  NS_ADDREF(*aRootContent = mRootContent);
>+  return NS_OK;
>+}
I don't quite understand why this is needed, but 
perhaps some other patch uses it.

>-  if (count > 0 && mWidget) {
>-    nsContentUtils::AddScriptRunner(
>-      new SelectionChangeEvent(this, causedByComposition));
>+  if (!(count > 0 && mWidget)) {
>+    return NS_OK;
>   }
>+
>+  nsContentUtils::AddScriptRunner(
>+    new SelectionChangeEvent(this, causedByComposition));
>   return NS_OK;

I think this change makes the code harder to understand.
I know early returns are usual preferred, but I wouldn't do it in
this kind of case, just because !(count > 0 && mWidget) is harder to understand than
count > 0 && mWidget.
>@@ -424,47 +447,53 @@ IMEContentObserver::ContentRemoved(nsIDo
>                                    nsIContent* aPreviousSibling)
> {
>   bool causedByComposition = IsEditorHandlingEventForComposition();
>   if (causedByComposition &&
>       !mUpdatePreference.WantChangesCausedByComposition()) {
>     return;
>   }
> 
>-  uint32_t offset = 0, childOffset = 1;
>-  if (NS_FAILED(nsContentEventHandler::GetFlatTextOffsetOfRange(
>-                    mRootContent, NODE_FROM(aContainer, aDocument),
>-                    aIndexInContainer, &offset)))
>-    return;
>+  uint32_t offset = 0;
>+  nsresult rv =
>+    nsContentEventHandler::GetFlatTextOffsetOfRange(mRootContent,
>+                                                    NODE_FROM(aContainer,
>+                                                              aDocument),
>+                                                    aIndexInContainer, &offset);
>+  NS_ENSURE_SUCCESS_VOID(rv);
> 
>   // get offset at the end of the deleted node
>-  if (aChild->IsNodeOfType(nsINode::eTEXT))
>-    childOffset = aChild->TextLength();
>-  else if (0 < aChild->GetChildCount())
>-    childOffset = aChild->GetChildCount();
>+  MOZ_ASSERT(!aChild->IsNodeOfType(nsINode::eTEXT) ||
>+             aChild->GetChildCount() <= INT32_MAX);
I don't understand this assert. Why INT32_MAX check?
And how could text node have any child nodes?
Just remove the assert? Or fix it.
I guess you mean
MOZ_ASSERT(aChild->IsNodeOfType(nsINode::eTEXT) || aChild->GetChildCount() <= INT32_MAX)
or
MOZ_ASSERT(!aChild->IsNodeOfType(nsINode::eTEXT) || aChild->TextLength() <= INT32_MAX)

> #endif // mozilla_IMEContentObserver_h_
>\ No newline at end of file
Add a newline?
Attachment #8387390 - Flags: review?(bugs) → review+
Comment on attachment 8387391 [details] [diff] [review]
part.3 Rename nsIMEStateManager to mozilla::IMEStateManager

>+/**
>+ * IMEStateManager manages InputContext (e.g., active editor type, IME enabled
>+ * state and IME open state) of nsIWidget instances, manages IMEContentObserver
>+ * and provides useful API of IME.
of or for?
Attachment #8387391 - Flags: review?(bugs) → review+
Attachment #8387392 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #10)
> >+nsresult
> >+IMEContentObserver::GetSelectionAndRoot(nsISelection** aSelection,
> >+                                        nsIContent** aRootContent) const
> >+{
> >+  if (!mEditableNode || !mSelection) {
> >+    return NS_ERROR_NOT_AVAILABLE;
> >+  }
> >+
> >+  NS_ASSERTION(mSelection && mRootContent, "uninitialized content observer");
> >+  NS_ADDREF(*aSelection = mSelection);
> >+  NS_ADDREF(*aRootContent = mRootContent);
> >+  return NS_OK;
> >+}
> I don't quite understand why this is needed, but 
> perhaps some other patch uses it.

Because IMEContentObserver::mSelection, mRootContent and mEditableNode are now private members. So, we need something getter method for IMEStateManager. I think that we don't need to create them for each since there is no other users for them.

> 
> >-  if (count > 0 && mWidget) {
> >-    nsContentUtils::AddScriptRunner(
> >-      new SelectionChangeEvent(this, causedByComposition));
> >+  if (!(count > 0 && mWidget)) {
> >+    return NS_OK;
> >   }
> >+
> >+  nsContentUtils::AddScriptRunner(
> >+    new SelectionChangeEvent(this, causedByComposition));
> >   return NS_OK;
> 
> I think this change makes the code harder to understand.
> I know early returns are usual preferred, but I wouldn't do it in
> this kind of case, just because !(count > 0 && mWidget) is harder to
> understand than
> count > 0 && mWidget.

Okay, I'll cancel this change before landing.

> >@@ -424,47 +447,53 @@ IMEContentObserver::ContentRemoved(nsIDo
> >                                    nsIContent* aPreviousSibling)
> > {
> >   bool causedByComposition = IsEditorHandlingEventForComposition();
> >   if (causedByComposition &&
> >       !mUpdatePreference.WantChangesCausedByComposition()) {
> >     return;
> >   }
> > 
> >-  uint32_t offset = 0, childOffset = 1;
> >-  if (NS_FAILED(nsContentEventHandler::GetFlatTextOffsetOfRange(
> >-                    mRootContent, NODE_FROM(aContainer, aDocument),
> >-                    aIndexInContainer, &offset)))
> >-    return;
> >+  uint32_t offset = 0;
> >+  nsresult rv =
> >+    nsContentEventHandler::GetFlatTextOffsetOfRange(mRootContent,
> >+                                                    NODE_FROM(aContainer,
> >+                                                              aDocument),
> >+                                                    aIndexInContainer, &offset);
> >+  NS_ENSURE_SUCCESS_VOID(rv);
> > 
> >   // get offset at the end of the deleted node
> >-  if (aChild->IsNodeOfType(nsINode::eTEXT))
> >-    childOffset = aChild->TextLength();
> >-  else if (0 < aChild->GetChildCount())
> >-    childOffset = aChild->GetChildCount();
> >+  MOZ_ASSERT(!aChild->IsNodeOfType(nsINode::eTEXT) ||
> >+             aChild->GetChildCount() <= INT32_MAX);
> I don't understand this assert. Why INT32_MAX check?
> And how could text node have any child nodes?
> Just remove the assert? Or fix it.
> I guess you mean
> MOZ_ASSERT(aChild->IsNodeOfType(nsINode::eTEXT) || aChild->GetChildCount()
> <= INT32_MAX)
> or
> MOZ_ASSERT(!aChild->IsNodeOfType(nsINode::eTEXT) || aChild->TextLength() <=
> INT32_MAX)

Oh, I meant:

MOZ_ASSERT((aChild->IsNodeOfType(nsINode::eTEXT) && aChild->TextLength() <= INT32_MAX) ||
             (!aChild->IsNodeOfType(nsINode::eTEXT) && aChild->GetChildCount() <= INT32_MAX));

Both TextLength() and GetChildCount() are uint32_t. However, the nodeLength is int32_t since it's the type of the 3rd argument of GetFlatTextOffsetOfRange(). Therefore, we should check it on debug build. It may cause a bug with huge contents.
So, I'll do just MOZ_ASSERT(nodeLength >= 0); must be the simplest.
You need to log in before you can comment on or make changes to this bug.