1428747 - Make EditorBase::GetDOMEventTarget() to return EventTarget* and GetFocusedContent() return nsIContent*

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

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

We're going too much AddRefing and Releasing in hot code paths.

Comment 1

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

Attachment: editor_tweaks.diff

I changed only the callers which looked safe to use raw pointer.

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/e1e886d2d8b2afa756ba865eb39b23e6f816dd03
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1e886d2d8b2afa756ba865eb39b23e6f816dd03
remote: recorded changegroup in replication log in 0.053s

Comment 2

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

Comment on attachment 8940750 [details] [diff] [review]
editor_tweaks.diff

> already_AddRefed<nsIContent>
> EditorBase::GetFocusedContentForIME()
> {
>-  return GetFocusedContent();
>+  nsCOMPtr<nsIContent> content = GetFocusedContent();
>+  return content.forget();

Although looks like that no callers of GetFocusedContentForIME() requires strong pointer.

But it's fine to keep it.

>-already_AddRefed<nsIContent>
>+nsIContent*
> HTMLEditor::GetFocusedContent()
> {
>   nsFocusManager* fm = nsFocusManager::GetFocusManager();
>   NS_ENSURE_TRUE(fm, nullptr);
> 
>   nsCOMPtr<nsIContent> focusedContent = fm->GetFocusedContent();
> 
>   nsCOMPtr<nsIDocument> document = GetDocument();
>   if (NS_WARN_IF(!document)) {
>     return nullptr;
>   }
>   bool inDesignMode = document->HasFlag(NODE_IS_EDITABLE);
>   if (!focusedContent) {
>     // in designMode, nobody gets focus in most cases.
>     if (inDesignMode && OurWindowHasFocus()) {
>       nsCOMPtr<nsIContent> rootContent = document->GetRootElement();
>-      return rootContent.forget();
>+      return rootContent.get();

nsIDocument::GetRootElement() returns raw pointer.
https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/dom/base/nsDocument.cpp#4400-4401

So, this can be just:

return document->GetRootElement();

>-already_AddRefed<EventTarget>
>+EventTarget*
> HTMLEditor::GetDOMEventTarget()
> {
>   // Don't use getDocument here, because we have no way of knowing
>   // whether Init() was ever called.  So we need to get the document
>   // ourselves, if it exists.
>   MOZ_ASSERT(IsInitialized(), "The HTMLEditor has not been initialized yet");
>   nsCOMPtr<mozilla::dom::EventTarget> target = GetDocument();
>-  return target.forget();
>+  return target;

Although, out of scope of this bug. We can make GetDocument() return raw pointer now. It used to retrieving from weak ptr, but now, mDocument is nsCOMPtr, so, current implementation of it doesn't make sense.
https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/editor/libeditor/EditorBase.cpp#596-597,599-600

Comment 3

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

Attachment: editor_tweaks_2.diff

Changed that GetRootElement case.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bcdad9d7f43
Make EditorBase::GetDOMEventTarget() to return EventTarget* and GetFocusedContent() return nsIContent* in order to optimize out some AddRef/Release calls, r=masayuki

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9bcdad9d7f43