1388502 - List All Tabs dropdown should use a document fragment to populate list

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P3)

Description

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

Document fragment: https://developer.mozilla.org/en-US/docs/Web/API/Document/createDocumentFragment

Here's where we'd want to do that: http://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/browser/base/content/tabbrowser.xml#7902-7905

Comment 1

[Justin Dolske [:Dolske]]

I assume the key bit here is "Since the document fragment is in memory and not part of the main DOM tree, appending children to it does not cause page reflow".

Comment 2

[Florian Quèze [:florian]]

(In reply to Justin Dolske [:Dolske] from comment #1)
> I assume the key bit here is "Since the document fragment is in memory and
> not part of the main DOM tree, appending children to it does not cause page
> reflow".

I think it's more to avoid DOM mutation events being fired for each item.

Comment 3

[Trisha Gupta [:trisha]]

let tabs = gBrowser.visibleTabs;
var fragment = document.createDocumentFragment();
for (var i = 0; i < tabs.length; i++) {
    if (!tabs[i].pinned)
      var li = document.createElement('li');
      li.textContent = tabs[i];
      fragment.appendChild(li);
});
element.appendChild(fragment);          

Broadly, this is what I feel should be the code, but I do not think it is completely sound. Can you tell me where I'm going wrong?

Comment 4

[Johann Hofmann [:johannh]]

(In reply to Trisha from comment #3)

That's going in the right direction! A couple of things to look out for:

> let tabs = gBrowser.visibleTabs;
> var fragment = document.createDocumentFragment();

Please use let instead of var for newer code

> for (var i = 0; i < tabs.length; i++) {
>     if (!tabs[i].pinned)

In this case you need to add curly braces around the entire block that
is guarded by the if condition, because without braces only the next
line will be dependent on "if (!tabs[i].pinned)"

>       var li = document.createElement('li');
>       li.textContent = tabs[i];

Note that you still need to use this._createTabMenuItem[0] to create the menu item. The problem
is that _createTabMenuItem appends the item directly[1]. You could change it to return the item instead,
and then append that returned item to your documentFragment.

[0] https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/browser/base/content/tabbrowser.xml#2104
[1] https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/browser/base/content/tabbrowser.xml#2118

>       fragment.appendChild(li);
> });
> element.appendChild(fragment);

This should probably be this.appendChild :)

By the way, a great way to get feedback instead of a full review is to attach a WIP patch and use the feedback? flag to ask someone to take a look and provide some hints (pasting code into the comments doesn't really scale in the long run :).

Thanks!

Comment 5

[Trisha Gupta [:trisha]]

Attachment: bug1388502.patch

Thanks for your advice, will use draft patches for feedback from now on. How does this look now?

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8959632 [details] [diff] [review]
bug1388502.patch

Review of attachment 8959632 [details] [diff] [review]:
-----------------------------------------------------------------

This looks much cleaner, two things you still need to do:

In the loop, please don't use this.appendChild directly. Instead use fragment.appendChild and append the fragment to "this" afterwards. You already did this in the previous version you posted.

You also need to change the _createTabMenuItem method to return the element it created instead of appending it directly.

Thanks!

Comment 7

[Trisha Gupta [:trisha]]

Attachment: bug1388502.patch

Made the changes as per what you suggested. Hope this is fine.

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8959787 [details] [diff] [review]
bug1388502.patch

Review of attachment 8959787 [details] [diff] [review]:
-----------------------------------------------------------------

That looks perfect, thank you. Let's give it a small try run and set checkin-needed to get the patch landed afterwards.

Comment 9

[Johann Hofmann [:johannh]]

Ah note on the commit message: The format is good generally, but there seem to be newlines in between:

> Bug 1388502
>  -
> used a document fragment to populate list
> . r=johannh

You should probably try to format it as a single line.

> Bug 1388502 - used a document fragment to populate list. r=johannh

And please use present tense/imperative and capitalize the first letter:

> Bug 1388502 - Use a document fragment to populate "all tabs" list. r=johannh

Thanks :)

Comment 10

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a44b47d4fdca721f076085b3a90bd5ad08858f4

Comment 11

[Johann Hofmann [:johannh]]

Can you please fix up the commit message and set the "checkin-needed" flag in the keywords field afterwards?

Comment 12

[Trisha Gupta [:trisha]]

Attachment: bug1388502.patch

Done! Thanks.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7acce8430bc
Use a document fragment to populate "all tabs" list. r=johannh

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f7acce8430bc