Closed Bug 906281 Opened 6 years ago Closed 6 years ago

each nsEventStateManager should not register as a pref observer

Categories

(Core :: User events and focus handling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: dbaron, Assigned: masayuki)

References

Details

(Keywords: perf)

Attachments

(4 files)

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.
FWIW I have a rough plan for fixing the prefs service in bug 901795.  But yes to fixing this too...
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Comment on attachment 792759 [details] [diff] [review]
part.1 Make nsEventStateManager::mClickHoldContextMenu static variable

I don't know why this is a member variable...
Attachment #792759 - Flags: review?(bugs)
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.
Attachment #792760 - Flags: review?(bugs)
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.
Attachment #792761 - Flags: review?(bugs)
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.
Attachment #792762 - Flags: review?(bugs)
Attachment #792760 - Flags: review?(bugs) → review+
Comment on attachment 792759 [details] [diff] [review]
part.1 Make nsEventStateManager::mClickHoldContextMenu static variable

Ok, this is useful with the part 3 patch...
Attachment #792759 - Flags: review?(bugs) → review+
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.
Attachment #792761 - Flags: review?(bugs) → review+
Attachment #792762 - Flags: review?(bugs) → review+
(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.
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.