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

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 3

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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/.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Ah, I missed currentDoc->HasFlag(NODE_IS_EDITABLE).

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Oh, wait, currentDoc can be null, I think. Add a null check.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

It is event listener. Some other event listener may have removed the target from document, I think.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, but the handler checks if the focused content is still the editor's content.

https://hg.mozilla.org/integration/mozilla-inbound/rev/795511f1212d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/795511f1212d