Batch reflows when loading user prefs

VERIFIED FIXED in M16

Status

P2
normal
VERIFIED FIXED
19 years ago
3 years ago

People

(Reporter: pierre, Assigned: eric)

Tracking

({perf})

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

19 years ago
This bug is extracted from the email thread we had yesterday on <c-
t@netscape.com> ("Please Review: Reflow Counts Statistics").

sfraser wrote:
----
As the profile manager sets up user preferences, the pref callbacks
in nsPresContext get called for each of the following preferences (for me)
(I added a printf in nsPresContext::PreferenceChanged() to catch these):

Reflow because pref browser.background_color changed
Reflow because pref browser.use_document_colors changed
Reflow because pref browser.wfe.use_windows_colors changed
Reflow because pref font.size.variable.ar changed
Reflow because pref font.size.fixed.ar changed
Reflow because pref font.size.variable.el changed
Reflow because pref font.size.fixed.el changed
Reflow because pref font.size.variable.he changed
Reflow because pref font.size.fixed.he changed
Reflow because pref font.size.variable.ja changed
Reflow because pref font.size.fixed.ja changed
Reflow because pref font.size.variable.ko changed
Reflow because pref font.size.fixed.ko changed
Reflow because pref font.size.variable.th changed
Reflow because pref font.size.fixed.th changed
Reflow because pref font.size.variable.tr changed
Reflow because pref font.size.fixed.tr changed
Reflow because pref font.size.variable.x-baltic changed
Reflow because pref font.size.fixed.x-baltic changed
Reflow because pref font.size.variable.x-central-euro changed
Reflow because pref font.size.fixed.x-central-euro changed
Reflow because pref font.size.variable.x-cyrillic changed
Reflow because pref font.size.fixed.x-cyrillic changed
Reflow because pref font.size.variable.x-unicode changed
Reflow because pref font.size.fixed.x-unicode changed
Reflow because pref font.size.variable.x-western changed
Reflow because pref font.size.fixed.x-western changed
Reflow because pref font.size.variable.zh-CN changed
Reflow because pref font.size.fixed.zh-CN changed
Reflow because pref font.size.variable.zh-TW changed
Reflow because pref font.size.fixed.zh-TW changed
Reflow because pref browser.use_document_fonts changed
Reflow because pref font.name.serif.ar changed
Reflow because pref font.name.sans-serif.ar changed
Reflow because pref font.name.monospace.ar changed
Reflow because pref font.name.cursive.ar changed
Reflow because pref font.name.fantasy.ar changed
Reflow because pref font.name.serif.el changed
Reflow because pref font.name.sans-serif.el changed
Reflow because pref font.name.monospace.el changed
Reflow because pref font.name.cursive.el changed
Reflow because pref font.name.fantasy.el changed
Reflow because pref font.name.serif.he changed
Reflow because pref font.name.sans-serif.he changed
Reflow because pref font.name.monospace.he changed
Reflow because pref font.name.cursive.he changed
Reflow because pref font.name.fantasy.he changed
Reflow because pref font.name.serif.ja changed
Reflow because pref font.name.sans-serif.ja changed
Reflow because pref font.name.monospace.ja changed
Reflow because pref font.name.cursive.ja changed
Reflow because pref font.name.fantasy.ja changed
Reflow because pref font.name.serif.ko changed
Reflow because pref font.name.sans-serif.ko changed
Reflow because pref font.name.monospace.ko changed
Reflow because pref font.name.cursive.ko changed
Reflow because pref font.name.fantasy.ko changed
Reflow because pref font.name.serif.th changed
Reflow because pref font.name.sans-serif.th changed
Reflow because pref font.name.monospace.th changed
Reflow because pref font.name.cursive.th changed
Reflow because pref font.name.fantasy.th changed
Reflow because pref font.name.serif.tr changed
Reflow because pref font.name.sans-serif.tr changed
Reflow because pref font.name.monospace.tr changed
Reflow because pref font.name.cursive.tr changed
Reflow because pref font.name.fantasy.tr changed
Reflow because pref font.name.serif.x-baltic changed
Reflow because pref font.name.sans-serif.x-baltic changed
Reflow because pref font.name.monospace.x-baltic changed
Reflow because pref font.name.cursive.x-baltic changed
Reflow because pref font.name.fantasy.x-baltic changed
Reflow because pref font.name.serif.x-central-euro changed
Reflow because pref font.name.sans-serif.x-central-euro changed
Reflow because pref font.name.monospace.x-central-euro changed
Reflow because pref font.name.cursive.x-central-euro changed
Reflow because pref font.name.fantasy.x-central-euro changed
Reflow because pref font.name.serif.x-cyrillic changed
Reflow because pref font.name.sans-serif.x-cyrillic changed
Reflow because pref font.name.monospace.x-cyrillic changed
Reflow because pref font.name.cursive.x-cyrillic changed
Reflow because pref font.name.fantasy.x-cyrillic changed
Reflow because pref font.name.serif.x-unicode changed
Reflow because pref font.name.sans-serif.x-unicode changed
Reflow because pref font.name.monospace.x-unicode changed
Reflow because pref font.name.cursive.x-unicode changed
Reflow because pref font.name.fantasy.x-unicode changed
Reflow because pref font.name.serif.x-western changed
Reflow because pref font.name.sans-serif.x-western changed
Reflow because pref font.name.monospace.x-western changed
Reflow because pref font.name.cursive.x-western changed
Reflow because pref font.name.fantasy.x-western changed
Reflow because pref font.name.serif.zh-CN changed
Reflow because pref font.name.sans-serif.zh-CN changed
Reflow because pref font.name.monospace.zh-CN changed
Reflow because pref font.name.cursive.zh-CN changed
Reflow because pref font.name.fantasy.zh-CN changed
Reflow because pref font.name.serif.zh-TW changed
Reflow because pref font.name.sans-serif.zh-TW changed
Reflow because pref font.name.monospace.zh-TW changed
Reflow because pref font.name.cursive.zh-TW changed
Reflow because pref font.name.fantasy.zh-TW changed
Reflow because pref browser.screen_resolution changed
Reflow because pref font.default changed
Reflow because pref font.size.fixed.x-western changed
Reflow because pref font.size.variable.x-western changed

This is a well-known problem, I believe.
----

And buster replied:
----
Isn't this exactly the kind of thing nsIPresShell::BeginReflowBatching() is 
designed for?  When you know you're going to do a bunch of work, and it's 
unimportant to display partial results, we should be able to collect the reflow
requests and handle them all at once when the caller is done making changes. 
---
(Reporter)

Updated

19 years ago
Keywords: perf

Comment 1

19 years ago
Hmmm.  I would have thought this stuff is happening when there isn't even a
window open.  Why are there reflows going on at all?  We're certainly not
showing the prefs panel during startup!

What is the recommended fix for this bug?  Is it actually the profile manager
that needs to change?
(Reporter)

Comment 2

19 years ago
Marked P2/M16.

This problem has nothing to do with the Preferences dialog. I assigned this bug 
to "Profile Manager BackEnd" because it seemed to be the most logical component. 
Maybe it should go to "Preferences Backend", I don't know.

The thing is: we start the application with default prefs, the Profile manager is 
displayed, the user selects a profile, then we load the rest of the application 
with the preferences that correspond to the selected profile. When we load either 
prefs (the default ones or the selected ones), we should tell the PresShell to 
batch the reflows. That's the bug. My guess was that the module in charge of 
loading the prefs was the Profile Manager. If I'm wrong, please reassign.
Priority: P3 → P2
Target Milestone: --- → M16

Comment 3

19 years ago
Neeti, this is really a prefs thing.
Assignee: selmer → neeti

Comment 4

19 years ago
The preference callback nsPresContext::PreferenceChanged() is being called 
"only" for the prefs which are registered for callbacks by
the nsPresContext and not all the prefs  in all.js( there are lots more prefs in 
all.js for which which the callback is not called). 

The prefs 
which have been registered by nsPresContext are

mPrefs->RegisterCallback("browser.screen_resolution", PrefChangedCallback, 
(void*)this);
D:\seamonkey\mozilla\layout\base\src\nsPresContext.cpp(308):    
mPrefs->RegisterCallback("browser.use_document_fonts", PrefChangedCallback, 
(void*)this);
mPrefs->RegisterCallback("browser.use_document_colors", PrefChangedCallback, 
(void*)this);
mPrefs->RegisterCallback("browser.background_color", PrefChangedCallback, 
(void*)this);
mPrefs->RegisterCallback("browser.foreground_color", PrefChangedCallback, 
(void*)this);
mPrefs->RegisterCallback("browser.base_font_scaler", PrefChangedCallback, 
(void*)this);
mPrefs->RegisterCallback("browser.wfe.use_windows_colors", PrefChangedCallback, 
(void*)this);
mPrefs->RegisterCallback("font.", PrefChangedCallback, (void*)this);
mPrefs->RegisterCallback("browser.display.direction", PrefChangedCallback, 
(void*)this);
pref->RegisterCallback("intl.charset.detector", MyPrefChangedCallback, nsnull);

I agree, we are getting a lot of callbacks(52) for 
nsPresContext::PreferenceChanged(), but libpref sends those callbacks, only if 
you have registered for them.

Libpref has no ability to tell PresShell to batch the reflows. I believe, 
evaughan has a fix for this problem. 

Neeti
Assignee: neeti → pierre
(Assignee)

Comment 5

19 years ago
Yes I do have a fix for this. I'll close out when I check in. Reassigning to me.
Assignee: pierre → evaughan

Comment 6

19 years ago
*** Bug 36015 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 7

19 years ago
Eric: Nisheeth proposed a solution where the pref callbacks are ignored until 
BeginLoad() is called. I don't how that fits with your strategy.
(Assignee)

Comment 8

19 years ago
Not sure. My solution is more of a global solution that works for all 
StyleChanges everywhere not just the initial prefs. It currently works quite 
well.
Status: NEW → ASSIGNED
(Assignee)

Comment 9

19 years ago
fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED

Comment 10

19 years ago
sairuh,

Should this bug be yours?
QA Contact: gbush → sairuh
no longer see this problem. verif.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.