906281 - each nsEventStateManager should not register as a pref observer

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Every nsEventStateManager object (one per document?) registers itself as a preference observer, and then unregisters on destruction.  This unregistration uses a search through a linked list (something that we should perhaps also fix).  This can add up to a noticeable amount of time when a lot of tabs are open; see the end of bug 906269 comment 0.

However, the observer function that is called for *each* event state manager does only:
 * manipulation of global variables
 * manipulation of a variable that could easily be changed to a global (mClickHoldContextMenu)

This should be converted into a single preference observer or set thereof rather than having one observer for every nsEventStateManager.

Comment 1

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

Yes. That pref handling is ancient, originally from
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/events/src/nsEventStateManager.cpp&rev=1.107 (early 2000)

Comment 2

[Justin Lebar (not reading bugmail)]

FWIW I have a rough plan for fixing the prefs service in bug 901795.  But yes to fixing this too...

Comment 3

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

Attachment: part.1 Make nsEventStateManager::mClickHoldContextMenu static variable

Comment 4

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

Attachment: part.2 Get rid of "nglayout.events.dispatchLeftClickOnly" due to not used

Comment 5

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

Attachment: part.3 Create nsEventStateManager::Prefs for capsuling the preferences management

Comment 6

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

Attachment: part.4 Use Preferences::AddIntVarCache() for accesskey prefs in nsEventStateManager::Prefs

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=5b65cd7ec8bb

Comment 7

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

Comment on attachment 792759 [details] [diff] [review]
part.1 Make nsEventStateManager::mClickHoldContextMenu static variable

I don't know why this is a member variable...

Comment 8

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

Comment on attachment 792760 [details] [diff] [review]
part.2 Get rid of "nglayout.events.dispatchLeftClickOnly" due to not used

Both this global variable and pref are not used now.

Comment 9

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

Comment on attachment 792761 [details] [diff] [review]
part.3 Create nsEventStateManager::Prefs for capsuling the preferences management

Let's capsule the pref values into a nested class, nsEventStateManager::Prefs. This helps next patch which makes access key code simpler.

Comment 10

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

Comment on attachment 792762 [details] [diff] [review]
part.4 Use Preferences::AddIntVarCache() for accesskey prefs in nsEventStateManager::Prefs

By this patch, only "dom.popup_allowed_events" uses the observer. I'm not sure why nsDOMEvent observes it directly.

Additionally, I rename the variables and methods which store or return modifier mask value.

Comment 11

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

Comment on attachment 792759 [details] [diff] [review]
part.1 Make nsEventStateManager::mClickHoldContextMenu static variable

Ok, this is useful with the part 3 patch...

Comment 12

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

Comment on attachment 792761 [details] [diff] [review]
part.3 Create nsEventStateManager::Prefs for capsuling the preferences management

Could have an array of prefs for which callback is registered, and use that
array also in Shutdown. But doesn't matter much since there are just few
prefs.

Comment 13

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

(In reply to Olli Pettay [:smaug] from comment #12)
> Comment on attachment 792761 [details] [diff] [review]
> part.3 Create nsEventStateManager::Prefs for capsuling the preferences
> management
> 
> Could have an array of prefs for which callback is registered, and use that
> array also in Shutdown. But doesn't matter much since there are just few
> prefs.

I think that when we need to do it, we should add new methods to Preferences.h which takes pref names array.

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/493cd772def7
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb09b18e4f14
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4ae4a2c3d2f
https://hg.mozilla.org/integration/mozilla-inbound/rev/38f6e57370ac

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/38f6e57370ac
https://hg.mozilla.org/mozilla-central/rev/e4ae4a2c3d2f
https://hg.mozilla.org/mozilla-central/rev/eb09b18e4f14
https://hg.mozilla.org/mozilla-central/rev/493cd772def7

Comment 16

[preetiraikwal9]

When I open these websites my browser stopped working, why this happen ?
sites : http://ncert-solutions.in/ncert-solutions-class-12-maths/
http://tellifagirllikesyou.com/how-to-know-if-a-girl-likes-you/