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

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Dão Gottwald [:dao]]

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.

Comment 1

[Dão Gottwald [:dao]]

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").

Comment 2

[jason r]

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?

Comment 3

[Dão Gottwald [:dao]]

(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!

Comment 4

[Dão Gottwald [:dao]]

Jason, do you still intend to work on this?

Comment 5

[Nitish]

Hello Dao, I am new to Open Source Development. Is it possible for me to work on this?

Comment 6

[Dão Gottwald [:dao]]

(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.

Comment 7

[Milind L (:milindl)]

Attachment: Bug 1342025 - reduce calls to ctrlTab.updatePreview

Added if condition to do so
r=:dao

Review commit: https://reviewboard.mozilla.org/r/117290/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117290/

Comment 8

[Milind L (:milindl)]

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

Comment 9

[Dão Gottwald [:dao]]

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

Comment 10

[Milind L (:milindl)]

Comment on attachment 8843689 [details]
Bug 1342025 - reduce calls to ctrlTab.updatePreview

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117290/diff/1-2/

Comment 11

[Milind L (:milindl)]

Comment on attachment 8843689 [details]
Bug 1342025 - reduce calls to ctrlTab.updatePreview

https://reviewboard.mozilla.org/r/117288/#review118954

Comment 12

[Milind L (:milindl)]

I have made the changes, I think it should be OK now
Thanks

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8843689 [details]
Bug 1342025 - reduce calls to ctrlTab.updatePreview

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

Looks good!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17b068bfce7c
reduce calls to ctrlTab.updatePreview. r=dao

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17b068bfce7c

Comment 16

[Dão Gottwald [:dao]]

Filed a followup: bug 1345773