239372 - ESM caches profile preferences and constantly gets others

Status: NEW

(Core :: DOM: Events, enhancement, P5)

Description

[timeless]

here's the ilst (culled from the patch)
accessibility.accesskeycausesactivation
accessibility.browsewithcaret
accessibility.tabfocus
nglayout.events.dispatchLeftClickOnly
ui.key.generalAccessKey
mousewheel.withshiftkey.action
mousewheel.withshiftkey.numlines
mousewheel.withshiftkey.sysnumlines
mousewheel.withcontrolkey.action
mousewheel.withcontrolkey.numlines
mousewheel.withcontrolkey.sysnumlines
mousewheel.withaltkey.action
mousewheel.withaltkey.numlines
mousewheel.withaltkey.sysnumlines
mousewheel.withnokey.action
mousewheel.withnokey.numlines
mousewheel.withnokey.sysnumlines

Comment 1

[timeless]

Attachment: use observers

Comment 2

[Boris Zbarsky [:bzbarsky]]

I won't be able to get to this for a long time (possibly near three weeks).

Comment 3

[Brian Ryner (not reading)]

Comment on attachment 145275 [details] [diff] [review]
use observers

If you're going to do this, why did you #if 0 the mousewheel prefs?

Comment 4

[timeless]

the mousewheel things currently constantly hit the pref service. i'm adding the
structure now, i'd rather convert that code later. if you really want me to
convert that code now, i can.

Comment 5

[Brian Ryner (not reading)]

Comment on attachment 145275 [details] [diff] [review]
use observers


Ok, I'll just review what you have so far.

>--- nsEventStateManager.cpp
>+++ nsEventStateManager.cpp
>+PRBool nsEventStateManager::sLeftClickOnly = PR_TRUE;
>+PRBool nsEventStateManager::sKeyCausesActivation = PR_TRUE;
>+PRUint32 nsEventStateManager::sInstanceCount = 0;
>+PRInt32 nsEventStateManager::sGeneralAccesskeyModifier = -1; // magic value of -1 means uninitialized
>+PRInt32 nsEventStateManager::sTabFocusModel = nsTabFocusModel::eTabFocus_any;

I'd personally prefer static file-scope variables rather than static class
variables.  I think they perform better on some architectures.

> NS_IMETHODIMP
> nsEventStateManager::Init()
> {
>   nsresult rv;
>   nsCOMPtr<nsIObserverService> observerService = 
>            do_GetService("@mozilla.org/observer-service;1", &rv);
>   if (NS_SUCCEEDED(rv))
>   {
>     observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, PR_TRUE);
>   }
> 
>   rv = getPrefBranch();
> 
>   if (NS_SUCCEEDED(rv)) {
>-    mPrefBranch->GetBoolPref("nglayout.events.dispatchLeftClickOnly",
>-                             &mLeftClickOnly);
>+    if (sInstanceCount==1) {

if (sInstanceCount == 1) {    to be consistent with the rest of the file.

>+  if (mPrefBranch) {
>+    mPrefBranch->RemoveObserver("accessibility.accesskeycausesactivation", this);
>+    mPrefBranch->RemoveObserver("accessibility.browsewithcaret", this);
>+    mPrefBranch->RemoveObserver("accessibility.tabfocus", this);
>+    mPrefBranch->RemoveObserver("nglayout.events.dispatchLeftClickOnly", this);
>+    mPrefBranch->RemoveObserver("ui.key.generalAccessKey", this);
>+    mPrefBranch->RemoveObserver("mousewheel.withshiftkey.action", this);

You #if 0'd adding the observers for mousewheel.*, please do the same for
RemoveObserver.

r=bryner with those fixed.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 145275 [details] [diff] [review]
use observers

sr=bzbarsky, with bryner's comments addressed.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Could this have actually hurt Tp on btek?

Comment 8

[timeless]

Comment on attachment 145275 [details] [diff] [review]
use observers

mozilla/content/events/src/nsEventStateManager.cpp	1.488
mozilla/content/events/src/nsEventStateManager.cpp	1.489

yes this would hurt launch. it should improve other things. it's possible

Comment 9

[timeless]

Comment on attachment 145275 [details] [diff] [review]
use observers

mozilla/content/events/src/nsEventStateManager.h	1.106

Comment 10

[Phil Ringnalda (:philor)]

No sense confusing anyone into thinking you're coming back to this.

Comment 11

[[e-mail]]

For anyone looking for changeset 41b1df0b2262, that was bug 293732, sorry.

Comment 12

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.