552237 - Move to FocusManager

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Christopher Henderson]

nsIFocusController is out, and nsIFocusManager is in. There are a few places where we need to move to the new paradigm.

Comment 1

[Christopher Henderson]

Attachment: Fix v1.1

Use nsIFocusManager. Pretty much just an API change, with some Gecko macros thrown in for good measure.

Comment 2

[Nick Kreeger]

Comment on attachment 432485 [details] [diff] [review]
Fix v1.1

Looks straight forward.

Comment 3

[Christopher Henderson]

Attachment: Fix v1.2

Turns out it's easy to hang the browser with Fix v1.1. SRT:

1. Open http://caminobrowser.org/help/
2. Cmd-click on the "Camino forum" link to open it in a new foreground tab
3. When loaded, press spacebar

The problem comes from the fact that GetFocusedWindow() always returns NS_OK, even when the focused window is null. NS_ENSURE_SUCCESS always succeeds, and we end up dereferencing a null pointer. This new patch uses NS_ENSURE_TRUE instead. It also removes the useless check after calling GetDesignMode.

Comment 4

[Stuart Morgan]

Yeah, that's the sort of thing that has encouraged me to always ignore rv and check pointers instead :P

Comment 5

[Stuart Morgan]

Comment on attachment 432967 [details] [diff] [review]
Fix v1.2

>+#include "nsServiceManagerUtils.h"

Add this in alphabetical order, like the rest of the includes already are (or is there a broken core header dependency that you are working around?)

>+  nsCOMPtr<nsIDOMElement> focusedElement;
>+  rv = fm->GetFocusedElement(getter_AddRefs(focusedElement));
>+  NS_ENSURE_SUCCESS(rv, nsnull);

Guess what else always returns NS_OK! Although in this case it shouldn't matter it's (currently at least) only used for a do_QI

New rule, now that I've been reminded why I didn't like this style: if you want to have NS_ENSURE_SUCCESS checks on everything for extra debug info, that's fine, but any pointer you are going to dereference must be explicitly checked, because that can never lie. It would be great if you could make a patch for your other recent changes that used this pattern, as I'm now concerned we may have introduced latent or edge-cases crashers.

>+    if (designMode.EqualsLiteral("on"))
>+      isFocused = YES;

It's weird to mix the early-return style you've switched to with an only-return-at-the-end top level structure. I'm fine with the early return, but make it consistent throughout the method: change all other instances of |isFocused = foo;| to |return foo;|, remove the variable, and make the last statement |return NO;|

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #5)
> because that can never lie. It would be great if you could make a patch for
> your other recent changes that used this pattern, as I'm now concerned we may
> have introduced latent or edge-cases crashers.

Bug 553864.

Comment 7

[Christopher Henderson]

Attachment: Fix v1.3

Fix with added macros and sr comments addressed.

Comment 8

[Stuart Morgan]

Comment on attachment 434091 [details] [diff] [review]
Fix v1.3

sr=smorgan. Thanks for reworking the checks, and sorry I didn't remember some important details when you first asked about the macro use!

Comment 9

[Stuart Morgan]

Actually, one more thing that just occurred to me: have you tested that everything you are ENSURE-ing is an error condition?

For example:
+      nsCOMPtr<nsIEditingSession> editSession = do_GetInterface(docshell, &rv);
+      NS_ENSURE_SUCCESS(rv, NULL);
If there's no edit in progress, can this be NULL without that being an error?

Or
+  fm->GetFocusedElement(getter_AddRefs(focusedElement));
+  NS_ENSURE_TRUE(focusedElement, nsnull);
when nothing is focused?

We don't want error spew every time we check for a condition that just doesn't happen to be true.

Comment 10

[Christopher Henderson]

Attachment: Followup to deal with focusedElement

The editSession object is fine, but focusedElement can frequently be null. This followup patch just returns nsnull in that case, without logging.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 434091 [details] [diff] [review]
Fix v1.3

Pushed this to the test repo: http://hg.mozilla.org/users/alqahira_ardisson.org/camino-1.9.2-test/rev/606940ab4576

We now build out of the box :-)

Comment 12

[Stuart Morgan]

Comment on attachment 434752 [details] [diff] [review]
Followup to deal with focusedElement

sr=smorgan

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Follow-up landed in the test repo: http://hg.mozilla.org/users/alqahira_ardisson.org/camino-1.9.2-test/rev/7801cdbe96a0

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

And I forgot to mark I had landed these :(

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/5b0a589f113f and, for the follow-up, http://hg.mozilla.org/camino/rev/777a3f1aa248