Last Comment Bug 855270 - NewTabUtils Links.getLinks can return trailing nulls
: NewTabUtils Links.getLinks can return trailing nulls
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla22
Assigned To: Sam Foster [:sfoster]
:
Mentors:
Depends on: 876313
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-27 06:43 PDT by Sam Foster [:sfoster]
Modified: 2013-05-27 00:15 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
pop() entry when unpining the last link, to avoid trailing nulls (639 bytes, patch)
2013-03-27 06:50 PDT, Sam Foster [:sfoster]
no flags Details | Diff | Splinter Review
pop() trailing null entries when unpinning (705 bytes, patch)
2013-03-27 07:15 PDT, Sam Foster [:sfoster]
no flags Details | Diff | Splinter Review
pop() trailing null entries when unpinning (705 bytes, patch)
2013-03-27 07:20 PDT, Sam Foster [:sfoster]
ttaubert: feedback+
Details | Diff | Splinter Review
Splice off trailing null entries when unpinning (717 bytes, patch)
2013-03-27 07:44 PDT, Sam Foster [:sfoster]
ttaubert: review+
Details | Diff | Splinter Review

Description Sam Foster [:sfoster] 2013-03-27 06:43:00 PDT
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.
Comment 1 Sam Foster [:sfoster] 2013-03-27 06:50:45 PDT
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.
Comment 2 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-03-27 06:59:04 PDT
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.
Comment 3 Sam Foster [:sfoster] 2013-03-27 07:15:10 PDT
Created attachment 730145 [details] [diff] [review]
pop() trailing null entries when unpinning
Comment 4 Sam Foster [:sfoster] 2013-03-27 07:20:11 PDT
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.
Comment 5 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-03-27 07:28:20 PDT
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.
Comment 6 Sam Foster [:sfoster] 2013-03-27 07:44:20 PDT
Created attachment 730152 [details] [diff] [review]
Splice off trailing null entries when unpinning

Ok, I like it. Looks good and works well for me.
Comment 7 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2013-03-27 07:49:44 PDT
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!
Comment 8 Sam Foster [:sfoster] 2013-03-27 08:06:49 PDT
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e67d5f1708
Comment 9 Ryan VanderMeulen [:RyanVM] 2013-03-27 14:14:26 PDT
https://hg.mozilla.org/mozilla-central/rev/f1e67d5f1708

Note You need to log in before you can comment on or make changes to this bug.