The default bug view has changed. See this FAQ.

browser-ctrlTab.js calls ctrlTab.updatePreview too frequently for TabAttrModified events

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
a month ago
17 days ago

People

(Reporter: dao, Assigned: milindl, Mentored)

Tracking

({good-first-bug, perf})

Trunk
Firefox 55
good-first-bug, perf
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [good first bug][lang=js])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a month ago
See https://hg.mozilla.org/mozilla-central/annotate/0eef1d5a39366059677c6d7944cfe8a97265a011/browser/base/content/browser-ctrlTab.js#l475

We only care about certain attributes there, namely "label", "busy", "image" and "selected". These days TabAttrModified gets dispatched for more tab attributes that this code doesn't really care about, thus it should detect these cases. This can be done by looking at event.detail.changed, which is an array of the names of the attributes that changed.
(Reporter)

Comment 1

a month ago
Note that the feature implemented by this code is disabled by default and can be enabled in about:preferences#general ("Ctrl+Tab cycles through tabs in recently used order").
Keywords: perf
(Reporter)

Updated

a month ago
Mentor: dao+bmo@mozilla.com

Comment 2

a month ago
Hey Dao is anybody currently working on this bug? I'm in a class at my university and this seems like a bug we'd like to dig around in and try to fix. If I'm understanding this correctly the function updatePreview is being called on too many attribute tags and you want this reduced to only label, busy, image, and selected tags?
(Reporter)

Comment 3

a month ago
(In reply to jason r from comment #2)
> Hey Dao is anybody currently working on this bug?

No, not yet.

> I'm in a class at my
> university and this seems like a bug we'd like to dig around in and try to
> fix. If I'm understanding this correctly the function updatePreview is being
> called on too many attribute tags and you want this reduced to only label,
> busy, image, and selected tags?

Exactly!
(Reporter)

Comment 4

22 days ago
Jason, do you still intend to work on this?
Flags: needinfo?(renaudjj)

Comment 5

21 days ago
Hello Dao, I am new to Open Source Development. Is it possible for me to work on this?
(Reporter)

Comment 6

21 days ago
(In reply to Nitish from comment #5)
> Hello Dao, I am new to Open Source Development. Is it possible for me to
> work on this?

Since we haven't heard from Jason in more than a week, please go ahead.
Flags: needinfo?(renaudjj)
Comment hidden (mozreview-request)
(Assignee)

Updated

20 days ago
Attachment #8843689 - Flags: review?(dao+bmo)
(Assignee)

Comment 8

20 days ago
Hi, I'm also new to contributing here.
I have tried to fix this bug to the best of my capabilities.
I started on this yesterday, without noticing Nitish's comment, but I've sent him a mail so
we can collaborate on this, like the "How to submit patches" guide says.

Thanks
(Reporter)

Comment 9

20 days ago
mozreview-review
Comment on attachment 8843689 [details]
Bug 1342025 - reduce calls to ctrlTab.updatePreview

https://reviewboard.mozilla.org/r/117290/#review118952

Looks good apart from some minor issues...

::: browser/base/content/browser-ctrlTab.js:478
(Diff revision 1)
>          this._initRecentlyUsedTabs();
>          break;
>        case "TabAttrModified":
> -        // tab attribute modified (e.g. label, busy, image, selected)
> +        // tab attribute modified (i.e. label, busy, image, selected)
> +	// update preview only if tab attribute modified in the list
> +	if (["label", "busy", "image", "selected"].reduce(

Please use 'some' instead of 'reduce'

::: browser/base/content/browser-ctrlTab.js:479
(Diff revision 1)
>          break;
>        case "TabAttrModified":
> -        // tab attribute modified (e.g. label, busy, image, selected)
> +        // tab attribute modified (i.e. label, busy, image, selected)
> +	// update preview only if tab attribute modified in the list
> +	if (["label", "busy", "image", "selected"].reduce(
> +	    (res, elem) => res || (event.detail.changed.indexOf(elem) !== -1),

Please use 'includes' instead of 'indexOf'

::: browser/base/content/browser-ctrlTab.js:481
(Diff revision 1)
> -        // tab attribute modified (e.g. label, busy, image, selected)
> +        // tab attribute modified (i.e. label, busy, image, selected)
> +	// update preview only if tab attribute modified in the list
> +	if (["label", "busy", "image", "selected"].reduce(
> +	    (res, elem) => res || (event.detail.changed.indexOf(elem) !== -1),
> +	    false)) {
> -        for (let i = this.previews.length - 1; i >= 0; i--) {
> +            for (let i = this.previews.length - 1; i >= 0; i--) {

Intentation is off by two spaces
Attachment #8843689 - Flags: review?(dao+bmo) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 11

20 days ago
mozreview-review
Comment on attachment 8843689 [details]
Bug 1342025 - reduce calls to ctrlTab.updatePreview

https://reviewboard.mozilla.org/r/117288/#review118954
(Assignee)

Comment 12

20 days ago
I have made the changes, I think it should be OK now
Thanks
(Reporter)

Comment 13

20 days ago
mozreview-review
Comment on attachment 8843689 [details]
Bug 1342025 - reduce calls to ctrlTab.updatePreview

https://reviewboard.mozilla.org/r/117290/#review119122

Looks good!
Attachment #8843689 - Flags: review?(dao+bmo) → review+

Comment 14

20 days ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17b068bfce7c
reduce calls to ctrlTab.updatePreview. r=dao

Comment 15

18 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/17b068bfce7c
Status: NEW → RESOLVED
Last Resolved: 18 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Reporter)

Updated

17 days ago
Assignee: nobody → i.milind.luthra
(Reporter)

Comment 16

17 days ago
Filed a followup: bug 1345773
Blocks: 1345773
You need to log in before you can comment on or make changes to this bug.