58409 - GetPosition and GetPositionSlowly call to the prefs service _every_time_

Status: NEW

(Core :: DOM: Selection, defect, P4)

Description

[Mike Shaver (:shaver emeritus)]

...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.

Comment 1

[Mike Shaver (:shaver emeritus)]

Attachment: Cache and update pref state in nsTextFrame::gDragOutOfFrameStyle

Comment 2

[Akkana Peck]

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.

Comment 3

[rubydoo123]

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

[anthonyd]

can do!

anthonyd

Comment 5

[anthonyd]

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.

anthonyd

Comment 6

[anthonyd]

Attachment: new diff with shavers stuff, and akkana changes

Comment 7

[anthonyd]

Attachment: BAH! This is the right patch

Comment 8

[Erik van der Poel]

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

[Alec Flett]

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

[anthonyd]

setting to mozilla 0.8.1
anthonyd

Comment 11

[rubydoo123]

moving to mozilla0.9

Comment 12

[Alec Flett]

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

Comment 13

[kinmoz]

Moving to mozilla0.9.1.

Comment 14

[anthonyd]

moving to 0.9.2

Comment 15

[anthonyd]

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

anthonyd

Comment 16

[Mike Shaver (:shaver emeritus)]

Asa just told me that we shipped 0.9.2!  Who knew?

Comment 17

[rbs]

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

Comment 18

[chris hofmann]

->0.9.5 since we have run out of time

Comment 19

[rbs]

I recently added a sibling nsIPresContext::GetCachedIntPref() if preferred here.

Comment 20

[Mike Shaver (:shaver emeritus)]

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.

Comment 21

[Blake Ross]

changing selection qa to tpreston.

Comment 22

[anthonyd]

reassigning to mjudge.  mike come talk to me about this one if you want.

Comment 23

[Jens Stutte [:jstutte]]

Bulk-downgrade of unassigned, >=5 years untouched DOM/Storage bugs' priority.

If you have reason to believe this is wrong (especially for the severity), please write a comment and ni :jstutte.

Comment 24

[Henri Sivonen (:hsivonen) (away from Bugzilla until 2026-01-05)]

The attached patch is no longer the right fix, but sadly, this report is still relevant.

We should at the very least be using static prefs here.