416509 - nsAutoRepeatBoxFrame should not cache the "repeat" attribute value in mIsPressMode

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That is a stupid "optimization". It should just check the attribute value every time it's needed, that will remove some code.

Comment 1

[Arpad Borsos [:Swatinem]]

Attachment: v1

This patch removes mIsPressMode and InitRepeatMode and uses AttrValueIs directly where needed.
The patch is against mozilla-central, I will update it and ask for review as soon as mozilla-central is updated or I can figure out how to pull&merge in cvs-trunk-mirror myself.
Can someone please assign this bug to me?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You've been granted 'editbugs' permission so you can assign this to yourself.

This patch is great, but it would be slightly better if there was a private helper method to encapsulate the AttrValueIs check, say "IsActivedOnHover()".

Comment 3

[Arpad Borsos [:Swatinem]]

Attachment: v2

Thanks for the editbugs ;)
This patch uses a helper method, but it's still based on mozilla-central though.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Normally you would ask for review from someone, say me :-)

This is a good fix that we'll land after FF3. We're a bit too close to the end of FF3 to land this sort of cleanup now.

That seemed pretty painless so you might want to get your teeth into some harder bugs :-).

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, when FF3 ships remind me to check this in. Thanks!!!

Comment 6

[Arpad Borsos [:Swatinem]]

Attachment: update to tip

Same patch, updated to apply cleanly on recent mozilla-central, carrying over r+

Comment 7

[Arpad Borsos [:Swatinem]]

mozilla-central has been open for some time now, so I think it's time to get this in.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks for reminding me! Checked in.