Closed Bug 1388502 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Tabbed Browser, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: mconley, Assigned: trisha, Mentored)

References

Details

(Whiteboard: [fxperf:p5])

Attachments

(1 file, 2 obsolete files)

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]
Whiteboard: [fxperf] → [fxperf:p5]
Mentor: jhofmann, prathikshaprasadsuman
Assignee: nobody → guptatrisha97
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!
Attached patch bug1388502.patch (obsolete) — Splinter Review
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+
Attached patch bug1388502.patch (obsolete) — Splinter Review
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)
Attached patch bug1388502.patchSplinter Review
Done! Thanks.
Attachment #8959787 - Attachment is obsolete: true
Flags: needinfo?(guptatrisha97)
Attachment #8960462 - Flags: review+
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: