Open Bug 239372 Opened 18 years ago Updated 3 years ago

ESM caches profile preferences and constantly gets others

Categories

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

x86
Windows XP
enhancement

Tracking

()

People

(Reporter: timeless, Unassigned)

References

Details

Attachments

(1 obsolete file)

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
Attached patch use observers (obsolete) — Splinter Review
Attachment #145275 - Flags: superreview?(bzbarsky)
Attachment #145275 - Flags: review?(bryner)
I won't be able to get to this for a long time (possibly near three weeks).
Comment on attachment 145275 [details] [diff] [review]
use observers

If you're going to do this, why did you #if 0 the mousewheel prefs?
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.
Status: NEW → ASSIGNED
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.
Attachment #145275 - Flags: review?(bryner) → review+
Comment on attachment 145275 [details] [diff] [review]
use observers

sr=bzbarsky, with bryner's comments addressed.
Attachment #145275 - Flags: superreview?(bzbarsky) → superreview+
Could this have actually hurt Tp on btek?
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
Attachment #145275 - Attachment is obsolete: true
Comment on attachment 145275 [details] [diff] [review]
use observers

mozilla/content/events/src/nsEventStateManager.h	1.106
QA Contact: ian → events
No sense confusing anyone into thinking you're coming back to this.
Assignee: timeless → nobody
Status: ASSIGNED → NEW
For anyone looking for changeset 41b1df0b2262, that was bug 293732, sorry.
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.
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.