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)
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)
(deleted),
patch
|
johannh
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•7 years ago
|
||
Trisha, would you like to work on this? You solved bug 1388502 which was very similar already :)
Flags: needinfo?(guptatrisha97)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → guptatrisha97
Flags: needinfo?(guptatrisha97)
Assignee | ||
Comment 2•7 years ago
|
||
(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! :)
Assignee | ||
Comment 3•7 years ago
|
||
I have made a work-in-progress patch for feedback. Does this look sound enough? Thanks.
Attachment #8967247 -
Flags: feedback?(jhofmann)
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•7 years ago
|
||
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)
Assignee | ||
Comment 5•7 years ago
|
||
(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?
Reporter | ||
Comment 6•7 years ago
|
||
(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 :)
Assignee | ||
Comment 7•6 years ago
|
||
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)
Reporter | ||
Comment 8•6 years ago
|
||
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)
Assignee | ||
Comment 9•6 years ago
|
||
Attachment #8980080 -
Attachment is obsolete: true
Attachment #8980168 -
Flags: review?(jhofmann)
Reporter | ||
Comment 10•6 years ago
|
||
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+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 11•6 years ago
|
||
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
Comment 12•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in
before you can comment on or make changes to this bug.
Description
•