963105 - Improve performance of #label-control formatAccessKey

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

For instance, it should at least return earlier if no accesskey or text is present.

Comment 1

[Neil Deakin]

Attachment: improveaccesskey

Comment 2

[[e-mail]]

Comment on attachment 8364450 [details] [diff] [review]
improveaccesskey

>+            var accessKey = this.accessKey;
>+            // No need to remove existing formatting the first time.
>+            if (firstTime && !accessKey)
>+              return;
>+
>+            if (this.mInsertSeparator === undefined) {
[if and try were nested the wrong way around]
>+            }
>+
>             if (!this.mUnderlineAccesskey)
>               return;
Can we use lazy field evaluation to achieve this, i.e. move the initialisation from the <constructor> to the definition of mUnderlineAccesskey rather than the formatAccessKey method?

Comment 3

[Neil Deakin]

(In reply to [e-mail] from comment #2)
> Can we use lazy field evaluation to achieve this, i.e. move the
> initialisation from the <constructor> to the definition of
> mUnderlineAccesskey rather than the formatAccessKey method?

Fields get compiled for every usage so I'm not sure that would be faster.

Comment 4

[[e-mail]]

(In reply to Neil Deakin from comment #3)
> (In reply to comment #2)
> > Can we use lazy field evaluation to achieve this, i.e. move the
> > initialisation from the <constructor> to the definition of
> > mUnderlineAccesskey rather than the formatAccessKey method?
> 
> Fields get compiled for every usage so I'm not sure that would be faster.

Fair enough, you need to optimise for performance here. [Now if we could only share variables between bindings so that we only needed to evaluate them once per document or some such...]

Comment 5

[[e-mail]]

Comment on attachment 8364450 [details] [diff] [review]
improveaccesskey

>+            try {
>+              if (this.mInsertSeparator === undefined) {
It makes more sense to me to condition the entire try block i.e.
if (this.mInsertSeparator === undefined) {
  try {
    // rest of code moved from constructor
  }
  catch (e) {
    this.mInsertSeparator = true;
  }
}
r=me with that fixed.

Comment 6

[Neil Deakin]

https://hg.mozilla.org/integration/fx-team/rev/9b281bb1bbea

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b281bb1bbea

Comment 8

[Mike Conley (:mconley) (:⚙️)]

The regression that this fixes[1] exists on mozilla-aurora. Are we able to uplift this as well?

[1]: bug 967691

Comment 9

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #8)
> The regression that this fixes[1] exists on mozilla-aurora. Are we able to
> uplift this as well?
> 
> [1]: bug 967691

The target milestone here is 29. This fix should be in 29 already.