Closed Bug 805306 Opened 7 years ago Closed 7 years ago

Get rid of nsIMEStateManager::OnTextStateBlur() and nsIMEStateManager::OnTextStateFocus()

Categories

(Core :: Internationalization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(1 file)

nsIMEStateManager::OnTextStateBlur() and nsIMEStateManager::OnTextStateFocus() are not good method for callers since there is similar method nsIMEStateManager::OnChangeFocus().

All of them should be merged.
Comment on attachment 674936 [details] [diff] [review]
Patch

>-nsresult
>-nsIMEStateManager::OnTextStateBlur(nsPresContext* aPresContext,
>-                                   nsIContent* aContent)
>+void
>+nsIMEStateManager::DestroyTextStateManager()
> {
>-  if (!sTextStateObserver || sTextStateObserver->mDestroying ||
>-      sTextStateObserver->mEditableNode ==
>-          GetRootEditableNode(aPresContext, aContent))
>-    return NS_OK;
>+  if (!sTextStateObserver || sTextStateObserver->mDestroying) {
>+    return;
>+  }
> 
>   sTextStateObserver->mDestroying = true;
>   sTextStateObserver->mWidget->OnIMEFocusChange(false);
>   sTextStateObserver->Destroy();
>   NS_RELEASE(sTextStateObserver);
>-  return NS_OK;
>+  return;
> }

Oops, I'll remove the last return before landing.
Comment on attachment 674936 [details] [diff] [review]
Patch


>+  bool     IsManagingFor(nsPresContext* aPresContext, nsIContent* aContent);
Or just IsManaging


>+
>+  nsCOMPtr<nsIContent> focusedContent = mEditor->GetFocusedContent();
>+  NS_ENSURE_TRUE(focusedContent, NS_OK);
>+  nsIDocument* currentDoc = focusedContent->GetCurrentDoc();
Do you use currentDoc for anything?

>+  nsCOMPtr<nsIPresShell> ps = GetPresShell();
>+  NS_ENSURE_TRUE(ps, NS_OK);
>+  nsIMEStateManager::OnFocusInEditor(ps->GetPresContext(),
>+    currentDoc->HasFlag(NODE_IS_EDITABLE) ? nullptr : focusedContent);

Could we not do this in Editor code. We should be able to handle
nsIMEStateManager::* calls in one or two places (I'm thinking ESM and nsFocusManager), not 
splinter them to even more place.
Attachment #674936 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #3)
> >+  nsCOMPtr<nsIContent> focusedContent = mEditor->GetFocusedContent();
> >+  NS_ENSURE_TRUE(focusedContent, NS_OK);
> >+  nsIDocument* currentDoc = focusedContent->GetCurrentDoc();
> Do you use currentDoc for anything?

For checking if the editor is for designMode.

> >+  nsCOMPtr<nsIPresShell> ps = GetPresShell();
> >+  NS_ENSURE_TRUE(ps, NS_OK);
> >+  nsIMEStateManager::OnFocusInEditor(ps->GetPresContext(),
> >+    currentDoc->HasFlag(NODE_IS_EDITABLE) ? nullptr : focusedContent);
> 
> Could we not do this in Editor code. We should be able to handle
> nsIMEStateManager::* calls in one or two places (I'm thinking ESM and
> nsFocusManager), not 
> splinter them to even more place.

Editor already notifies some events in editor to nsIMEStateManager:
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditorEventListener.cpp#501
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#317
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#496

I think that calling nsIMEStateManager method from editor is the most simple way to notify the editor state.
# Anyway, it might be better to move nsTextStateManager under editor/base/.
Ah, I missed currentDoc->HasFlag(NODE_IS_EDITABLE).
Attachment #674936 - Flags: review- → review+
Oh, wait, currentDoc can be null, I think. Add a null check.
(In reply to Olli Pettay [:smaug] from comment #6)
> Oh, wait, currentDoc can be null, I think. Add a null check.

Oh, really? Even the content now gets focus?
It is event listener. Some other event listener may have removed the target from document, I think.
Hmm, but the handler checks if the focused content is still the editor's content.

https://hg.mozilla.org/integration/mozilla-inbound/rev/795511f1212d
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/795511f1212d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.