307741 - Tp regression on creature

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Checkins:
http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1126040297&maxdate=1126046297

Comment 1

[Boris Zbarsky [:bzbarsky]]

Tp on btek also went up in the same time range.  Checkins there: 
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-09-06+15%3A19%3A00&maxdate=2005-09-06+15%3A37%3A00&cvsroot=%2Fcvsroot

My bet is on the focus controller changes.

Comment 2

[Boris Zbarsky [:bzbarsky]]

If the patch for bug 305032 goes on branch, then we'll need to fix this there too...

Comment 3

[Aaron Leventhal]

That was checked in on branch as well, yes. I can help look at a fix next week.

Comment 4

[Asa Dotzler [:asa]]

aaron, can you look into this? Time is getting short. 

Comment 5

[Aaron Leventhal]

If bug 305032 was the problem, then this might not be an issue anymore, because
those changes were reversed when I realized splitwindow was the original cause.
I'll look into it.

Comment 6

[Aaron Leventhal]

(In reply to comment #5)
> If bug 305032 was the problem, then this might not be an issue anymore, because
> those changes were reversed when I realized splitwindow was the original cause.
> I'll look into it.

Scratch that. I see that those changes are in.

Comment 7

[Aaron Leventhal]

Attachment: One less command update on pages which focus in an onload

The patch which regressed Tp didn't actually fix bug 305032, it fixed bug
306076 and bug 306235. This patch doesn't regress either of those.

The reason this works is that there are 2 cases when the current element gets
focused:
1) Focus suppression was on in SetFocusedELement, in which case
mNeedCommandUpdate gets set to true. So, it is already true by the time it
reaches SetSuppressFocus
2) Focus suppression was off in SetFocusedElement, in which case
UpdateCommands() already happened for that element

Comment 8

[Aaron Leventhal]

Mats, Brian, see patch and comment 7.

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 197737 [details] [diff] [review]
One less command update on pages which focus in an onload

Removing this line makes sense, it shouldn't be needed
as you say. However, this line works as a sort of wallpaper
since a series of suppress + unsuppress each triggers a
UpdateCommands() so by removing this line there is a
small risc that we uncover bugs elsewhere, in case the
actions of UpdateCommands() depends on "state" that is
only "ready" in the later calls...

r=mats

Comment 10

[Asa Dotzler [:asa]]

let's get this on the trunk and think about it later for the branch.

Comment 11

[Aaron Leventhal]

(In reply to comment #9)
> (From update of attachment 197737 [details] [diff] [review] [edit])
> Removing this line makes sense, it shouldn't be needed
> as you say. However, this line works as a sort of wallpaper
> since a series of suppress + unsuppress each triggers a
> UpdateCommands() so by removing this line there is a
> small risc that we uncover bugs elsewhere, in case the
> actions of UpdateCommands() depends on "state" that is
> only "ready" in the later calls...

I don't think so because UpdateCommands() won't mark mNeedsUpdateCommands =
PR_TRUE unless the doc was finished loading. If it didn't get to update commands
because it was busy, this code will still UpdateCommands().