Closed Bug 963105 Opened 6 years ago Closed 6 years ago

Improve performance of #label-control formatAccessKey

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: enndeakin, Assigned: enndeakin)

References

Details

Attachments

(1 file)

For instance, it should at least return earlier if no accesskey or text is present.
Attached patch improveaccesskeySplinter Review
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attachment #8364450 - Flags: review?(neil)
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?
(In reply to neil@parkwaycc.co.uk 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.
Blocks: 944947
(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 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.
Attachment #8364450 - Flags: review?(neil) → review+
https://hg.mozilla.org/mozilla-central/rev/9b281bb1bbea
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 964025
No longer depends on: 964025
Depends on: 967432
The regression that this fixes[1] exists on mozilla-aurora. Are we able to uplift this as well?

[1]: bug 967691
Flags: needinfo?(enndeakin)
(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.
Depends on: 970049
Flags: needinfo?(enndeakin)
Depends on: 964025
You need to log in before you can comment on or make changes to this bug.