The default bug view has changed. See this FAQ.

NewTabUtils Links.getLinks can return trailing nulls

RESOLVED FIXED in mozilla22

Status

()

Toolkit
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sfoster, Assigned: sfoster)

Tracking

unspecified
mozilla22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
When a pinned link is unpinned it is set to null in PinnedLinks.links. If it is the last entry this trailing null can get passed into Links.getLinks, and on to the caller as a null in the returned array if there aren't enough links (places) to fill this slot.
(Assignee)

Comment 1

4 years ago
Created attachment 730126 [details] [diff] [review]
pop() entry when unpining the last link, to avoid trailing nulls

This patch heads of the trailing null issue at the source, by truncating the pinned links array (via pop()) when unpinning the last link, rather than setting it to null.
Assignee: nobody → sfoster
Attachment #730126 - Flags: review?(ttaubert)
Comment on attachment 730126 [details] [diff] [review]
pop() entry when unpining the last link, to avoid trailing nulls

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

Suppose we have pinned sites like [null, null, null, null, {something}] and we unpin the last one we'd only pop on entry. I think a better way to handle this would be to search for the first non-null entry starting from the back and then use this.links.splice() to remove trailing null entries.
Attachment #730126 - Flags: review?(ttaubert)
(Assignee)

Comment 3

4 years ago
Created attachment 730145 [details] [diff] [review]
pop() trailing null entries when unpinning
Attachment #730145 - Flags: review?(ttaubert)
(Assignee)

Comment 4

4 years ago
Created attachment 730148 [details] [diff] [review]
pop() trailing null entries when unpinning

Good catch on the [null,null,{something}] case. I've reworked this so it loops from the back of the array, pop-ing null entries. As I can't think of a way to get the index of the last non-null entry without a loop, this single while loop seems to get the job done concisely.
Attachment #730126 - Attachment is obsolete: true
Attachment #730145 - Attachment is obsolete: true
Attachment #730145 - Flags: review?(ttaubert)
Attachment #730148 - Flags: review?(ttaubert)
Comment on attachment 730148 [details] [diff] [review]
pop() trailing null entries when unpinning

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

::: toolkit/modules/NewTabUtils.jsm
@@ +411,5 @@
> +    let links = this.links;
> +    links[index] = null;
> +    // trim trailing nulls
> +    while (links.length && links[links.length-1] == null)
> +      links.pop();

How about something like:

> let i = links.length - 1;
> while (i >= 0 && links[i] == null) i--;
> links.splice(i + 1)

This way we'd only modify the array once instead of calling pop() up to N times.
Attachment #730148 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 6

4 years ago
Created attachment 730152 [details] [diff] [review]
Splice off trailing null entries when unpinning

Ok, I like it. Looks good and works well for me.
Attachment #730148 - Attachment is obsolete: true
Attachment #730152 - Flags: review?(ttaubert)
Comment on attachment 730152 [details] [diff] [review]
Splice off trailing null entries when unpinning

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

Great, thank you for discovering this, btw!
Attachment #730152 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 8

4 years ago
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e67d5f1708
https://hg.mozilla.org/mozilla-central/rev/f1e67d5f1708
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 876313
You need to log in before you can comment on or make changes to this bug.