Closed Bug 1451412 Opened 7 years ago Closed 6 years ago

Use a documentFragment to build the site data list

Categories

(Firefox :: Settings UI, enhancement, P3)

60 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: johannh, Assigned: trisha)

References

(Blocks 1 open bug)

Details

(Whiteboard: [storage-v2])

Attachments

(1 file, 2 obsolete files)

Instead of attaching every richlistboxitem to the richlistbox directly we should probably just make a documentFragment and attach it there. https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/browser/components/preferences/siteDataSettings.js#199
Trisha, would you like to work on this? You solved bug 1388502 which was very similar already :)
Flags: needinfo?(guptatrisha97)
Assignee: nobody → guptatrisha97
Flags: needinfo?(guptatrisha97)
(In reply to Johann Hofmann [:johannh] from comment #1) > Trisha, would you like to work on this? You solved bug 1388502 which was > very similar already :) Sure, thanks! :)
Attached patch bug1451412.patch (obsolete) (deleted) — Splinter Review
I have made a work-in-progress patch for feedback. Does this look sound enough? Thanks.
Attachment #8967247 - Flags: feedback?(jhofmann)
Status: NEW → ASSIGNED
Comment on attachment 8967247 [details] [diff] [review] bug1451412.patch Review of attachment 8967247 [details] [diff] [review]: ----------------------------------------------------------------- Hi Trisha, did you try the patch? There are a few small issues but I'd like to make sure you know how to check your code is working. The site data manager is in about:preferences > Privacy & Security > Cookies & Site Data > Manage Data You can populate the list by just browsing to a couple of sites that add cookies, e.g. https://mozilla.org Let me know if you run into any troubles!
Attachment #8967247 - Flags: feedback?(jhofmann)
(In reply to Johann Hofmann [:johannh] from comment #4) > Comment on attachment 8967247 [details] [diff] [review] > bug1451412.patch > > Review of attachment 8967247 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hi Trisha, did you try the patch? There are a few small issues but I'd like > to make sure you know how to check your code is working. > > The site data manager is in about:preferences > Privacy & Security > Cookies > & Site Data > Manage Data > > You can populate the list by just browsing to a couple of sites that add > cookies, e.g. https://mozilla.org > > Let me know if you run into any troubles! I have amended my code but need to confirm the testing method once. You mean after I browse through a site that adds cookies, it should be added in the "Manage Data" section of the site data manager?
(In reply to Trisha from comment #5) > (In reply to Johann Hofmann [:johannh] from comment #4) > > Comment on attachment 8967247 [details] [diff] [review] > > bug1451412.patch > > > > Review of attachment 8967247 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Hi Trisha, did you try the patch? There are a few small issues but I'd like > > to make sure you know how to check your code is working. > > > > The site data manager is in about:preferences > Privacy & Security > Cookies > > & Site Data > Manage Data > > > > You can populate the list by just browsing to a couple of sites that add > > cookies, e.g. https://mozilla.org > > > > Let me know if you run into any troubles! > > I have amended my code but need to confirm the testing method once. You mean > after I browse through a site that adds cookies, it should be added in the > "Manage Data" section of the site data manager? Yup :)
Attached patch bug1451512.patch (obsolete) (deleted) — Splinter Review
This patch is not fully complete because I cannot see the populated list on testing. It doesn't give any error on build though(except that "this.appendChild isn't a function" which is strange to me). Can you please help me out here? Thanks!
Attachment #8967247 - Attachment is obsolete: true
Attachment #8980080 - Flags: review?(jhofmann)
Comment on attachment 8980080 [details] [diff] [review] bug1451512.patch Review of attachment 8980080 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/siteDataSettings.js @@ +201,2 @@ > } > + this.appendChild(fragment); This should be this._list.appendChild :)
Attachment #8980080 - Flags: review?(jhofmann)
Attached patch bug1451512.patch (deleted) — Splinter Review
Attachment #8980080 - Attachment is obsolete: true
Attachment #8980168 - Flags: review?(jhofmann)
Comment on attachment 8980168 [details] [diff] [review] bug1451512.patch Review of attachment 8980168 [details] [diff] [review]: ----------------------------------------------------------------- That looks perfect, thank you!
Attachment #8980168 - Flags: review?(jhofmann) → review+
Keywords: checkin-needed
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b782fd480072 Use a DocumentFragment to build the site data list r=johannh
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: