GetPosition and GetPositionSlowly call to the prefs service _every_time_




19 years ago
10 years ago


(Reporter: shaver, Unassigned)




Firefox Tracking Flags

(Not tracked)



(3 attachments)

...which just seems wasteful.

I considered a couple of solutions to this:

1) cache the pref value on the frame, and add an updater

   Not very good: you'd have tons of pref updaters, and it'd cost us 4 bytes
   per nsTextFrame object

2) cache the pref value in a static value, and install an updater the first time
   through the code (or until it works, if it fails)

   Much better.  I'll attach a patch to that effect.
Created attachment 18258 [details] [diff] [review]
Cache and update pref state in nsTextFrame::gDragOutOfFrameStyle

Comment 2

19 years ago
The patch mostly looks good (thanks for doing this -- should help a lot!), but I
have one concern: in the embedded case, where we don't necessarily have a pref
service, we'll keep calling SetDragOutOfFrameStyleUpdater() every time.  It
might be better to just assume one setting or the other (I'd vote for the
Mac/Unix setting because more platforms use it and I wonder whether Windows
people really *like* the Windows setting ...) in the case where we can't get the
pref service, rather than leaving it undefined so we try again next time.


18 years ago
Assignee: mjudge → anthonyd
Target Milestone: --- → mozilla0.9

Comment 3

18 years ago
reassign to anthonyd, setting to moz0.9, anthony can you modify the patch based 
on Akkana's input and get this one reviewed and checked in?

Comment 4

18 years ago
can do!


Comment 5

18 years ago
okay, i got shaver's patch with akkana's changes in my tree.  i will diff it up 
and attach it shortly for r= and sr='s.

Whiteboard: FIX IN HAND

Comment 6

18 years ago
Created attachment 26564 [details] [diff] [review]
new diff with shavers stuff, and akkana changes

Comment 7

18 years ago
Created attachment 26566 [details] [diff] [review]
BAH! This is the right patch

Comment 8

18 years ago
Cc'ing alecf. Alec, you said that nsIPref::RegisterCallback will eventually
disappear. Would you please take a look at this patch? They are registering a
callback to update a global variable (and not unregistering the callback at
all, since that variable is global, after all). In this kind of case, would we
really want to create an nsIObserver on the heap and store a pointer to it in
a static variable, just so that we can call addObserver/removeObserver? Or will
we end up keeping RegisterCallback around for these types of situations? Also,
if RegisterCallback really is going to be removed, shouldn't there be a comment
to that effect in nsIPref.idl?

Also, the patch doesn't seem to address Akkana's concern (embeddor may not have
prefs), even though you say that you addressed her concern.

Comment 9

18 years ago
normally you might just figure out what global service might actually own the
global variable, and then make that the observer

in this case though, it does look like the right thing is to just make a single
local observer to observe this topic..

Comment 10

18 years ago
setting to mozilla 0.8.1
Target Milestone: mozilla0.9 → mozilla0.8.1

Comment 11

18 years ago
moving to mozilla0.9
Target Milestone: mozilla0.8.1 → mozilla0.9

Comment 12

18 years ago
I should add that I think that in the name of improving performance ASAP, we
should just use RegisterCallback for now.

Comment 13

18 years ago
Moving to mozilla0.9.1.
Whiteboard: FIX IN HAND
Target Milestone: mozilla0.9 → mozilla0.9.1

Comment 14

18 years ago
moving to 0.9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2

Comment 15

18 years ago
im gonna reassign this to shaver since everyone but me and him doesnt seem to 
like the patch.

Assignee: anthonyd → shaver
Asa just told me that we shipped 0.9.2!  Who knew?
Target Milestone: mozilla0.9.2 → mozilla0.9.3


18 years ago
Keywords: perf


18 years ago
Target Milestone: mozilla0.9.3 → mozilla0.9.4

Comment 17

18 years ago
What about simply caching this in the presentation context and using the
nsIPresContext::GetCachedBoolPref() stuff?


18 years ago
Target Milestone: mozilla0.9.4 → mozilla0.9.5

Comment 18

18 years ago
->0.9.5 since we have run out of time

Comment 19

18 years ago
I recently added a sibling nsIPresContext::GetCachedIntPref() if preferred here.
I'm going to give this back to anthonyd, to either fix or WONTFIX at his
leisure.  I don't have time to chase this any more.
Assignee: shaver → anthonyd
Target Milestone: mozilla0.9.5 → ---

Comment 21

17 years ago
changing selection qa to tpreston.
QA Contact: blaker → tpreston

Comment 22

17 years ago
reassigning to mjudge.  mike come talk to me about this one if you want.
Assignee: anthonyd → mjudge


17 years ago
Target Milestone: --- → mozilla1.1alpha
Assignee: mjudge → nobody
QA Contact: tpreston → selection
You need to log in before you can comment on or make changes to this bug.