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

RESOLVED FIXED in Firefox 61

Status

()

P3
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: mconley, Assigned: trisha, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [fxperf:p5])

Attachments

(1 attachment, 2 obsolete attachments)

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".
Priority: -- → P3
(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.
Whiteboard: [fxperf] → [fxperf:p5]
Mentor: jhofmann, prathikshaprasadsuman
(Assignee)

Updated

8 months ago
Assignee: nobody → guptatrisha97
(Assignee)

Comment 3

8 months ago
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?
(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!
(Assignee)

Comment 5

8 months ago
Created attachment 8959632 [details] [diff] [review]
bug1388502.patch

Thanks for your advice, will use draft patches for feedback from now on. How does this look now?
Attachment #8959632 - Flags: feedback?(jhofmann)
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!
Attachment #8959632 - Flags: feedback?(jhofmann) → feedback+
(Assignee)

Comment 7

8 months ago
Created attachment 8959787 [details] [diff] [review]
bug1388502.patch

Made the changes as per what you suggested. Hope this is fine.
Attachment #8959632 - Attachment is obsolete: true
Attachment #8959787 - Flags: review?(jhofmann)
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.
Attachment #8959787 - Flags: review?(jhofmann) → review+
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 :)
Can you please fix up the commit message and set the "checkin-needed" flag in the keywords field afterwards?
Flags: needinfo?(guptatrisha97)
(Assignee)

Comment 12

8 months ago
Created attachment 8960462 [details] [diff] [review]
bug1388502.patch

Done! Thanks.
Attachment #8959787 - Attachment is obsolete: true
Flags: needinfo?(guptatrisha97)
Attachment #8960462 - Flags: review+
(Assignee)

Updated

8 months ago
Keywords: checkin-needed

Comment 13

8 months ago
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7acce8430bc
Use a document fragment to populate "all tabs" list. r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f7acce8430bc
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.