Closed Bug 792297 Opened 7 years ago Closed 2 years ago

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

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: ehsan, Unassigned)

References

Details

(Keywords: perf, Whiteboard: [Snappy][has profile])

Attachments

(1 file)

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?
Blocks: 783064
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.
(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!
Bug 792390 should help some too!
(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?
(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?
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.
Blocks: 792922
(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.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
So is this ready for review?
OS: Mac OS X → All
Hardware: x86 → All
Keywords: perf
Attachment #665620 - Flags: review?(bugs)
(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 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 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.
Attachment #665620 - Flags: review?(bugs) → review-
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.
Need more info from Ehsan about why he thinks this was a performance issue.
Flags: needinfo?(ehsan)
Assignee: enndeakin → nobody
Whiteboard: [Snappy] → [Snappy][has profile]
Is this still relevant?
Flags: needinfo?(ehsan)
The profile link doesn't work any more, I don't remember what the specific issue was.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Flags: needinfo?(ehsan)
Resolution: --- → INCOMPLETE
See Also: → 1356034
You need to log in before you can comment on or make changes to this bug.