792297 - Focus manager needs to flush in CheckIfFocusable, causing textbox.xml's select() method to flush twice!

Status: RESOLVED INCOMPLETE

(Core :: XUL, defect)

Description

[(no longer active)]

Can we somehow avoid this, perhaps by caching this information on the content node, and holding a weak ptr to the pres context to check whether we're in print preview mode?

Comment 1

[Neil Deakin]

Cache which information? The flush is done to get the updated frame, which is already stored on the content node.

It might be feasible to only have CheckIfFocusable flush from the first call in nsFocusManager::SetFocusInner though.

Comment 2

[(no longer active)]

(In reply to comment #1)
> Cache which information? The flush is done to get the updated frame, which is
> already stored on the content node.

I was talking about the focusability information.

> It might be feasible to only have CheckIfFocusable flush from the first call in
> nsFocusManager::SetFocusInner though.

That would surely be better than the current situation!

Comment 3

[(no longer active)]

Bug 792390 should help some too!

Comment 4

[Neil Deakin]

(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> (In reply to comment #1)
> > Cache which information? The flush is done to get the updated frame, which is
> > already stored on the content node.
> 
> I was talking about the focusability information.
> 

I'm still not clear what you're referring to here. What are thinking should be cached?

Comment 5

[(no longer active)]

(In reply to comment #4)
> (In reply to Ehsan Akhgari [:ehsan] from comment #2)
> > (In reply to comment #1)
> > > Cache which information? The flush is done to get the updated frame, which is
> > > already stored on the content node.
> > 
> > I was talking about the focusability information.
> > 
> 
> I'm still not clear what you're referring to here. What are thinking should be
> cached?

For example, if the focusability information can be computed at reflow time, we can probably get away with caching that as a flag on the content node.  I don't really know how we compute that information which is why I can't pinpoint how it could be cached exactly.  Or am I just barking at the wrong tree?

Comment 6

[Neil Deakin]

It seems like it would be very slow to precompute focusability for every element when only a tiny fraction of them ever get focused. Also, the focusability can change without a reflow by changing an element's tabindex for example.

Comment 7

[(no longer active)]

(In reply to comment #1)
> It might be feasible to only have CheckIfFocusable flush from the first call in
> nsFocusManager::SetFocusInner though.

So, let's just do this part in this bug then.

Comment 8

[Neil Deakin]

Attachment: patch that only flushes once on focus

Comment 9

[(no longer active)]

So is this ready for review?

Comment 10

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

(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> For example, if the focusability information can be computed at reflow time,
> we can probably get away with caching that as a flag on the content node.  I
> don't really know how we compute that information which is why I can't
> pinpoint how it could be cached exactly.  Or am I just barking at the wrong
> tree?
That information might not be valid anymore when we flush next time

Comment 11

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

Comment on attachment 665620 [details] [diff] [review]
patch that only flushes once on focus

What does this patch help? If we have just flushed, the next flush would be
anyway no-op.
And this makes code more complicated.

Comment 12

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

Comment on attachment 665620 [details] [diff] [review]
patch that only flushes once on focus

So unless I see a profile which this might help, I think we
should not do this.

Comment 13

[Neil Deakin]

I'm guessing that the need to update the focus ring and draw the selection during the focus/select events causes the need to flush again afterwards.

Comment 14

[Neil Deakin]

Need more info from Ehsan about why he thinks this was a performance issue.

Comment 15

[(no longer active)]

Here's a profile of this: http://people.mozilla.com/~bgirard/cleopatra/?report=bf5d5e321d9f6db3ad1f0ba435196e1e216c34f1

Comment 16

[Dão Gottwald [:dao]]

Is this still relevant?

Comment 17

[(no longer active)]

The profile link doesn't work any more, I don't remember what the specific issue was.