Closed Bug 1228976 Opened 4 years ago Closed 4 years ago

Remove array comprehensions from Places

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: mak, Assigned: bmanojkumar24, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 4 obsolete files)

Array comprehensions are non-standard, we should stop using them and replace existing usage with .map, .from, .filter or other array methods, depending on the code.

This search finds some false positives but it's a good starting point:
http://mxr.mozilla.org/mozilla-central/search?string=\[%23for+%23&regexp=1&find=places
Will you be happy with a lot of little patches in this bug, or do you want new bugs per fix?
I'm fine with either multiple patches here, or a single big patch. I'm not a fan of tens of dependent bugs for cleanups.
Hi, I would like to work on this bug. Can you please assign it to me.Thanks.
Sure, thank you for helping
Assignee: nobody → bmanojkumar24
Attached patch 1228976.patch (obsolete) — Splinter Review
Hi, can you please review the patch. It contains a lot of changes. Thanks.
Attachment #8694235 - Flags: review?(mak77)
Comment on attachment 8694235 [details] [diff] [review]
1228976.patch

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

::: browser/components/places/content/controller.js
@@ +1292,5 @@
>  
>      let itemsToSelect = [];
>      if (PlacesUIUtils.useAsyncTransactions) {
>        if (ip.isTag) {
> +        let uris = items.filter(item => if ("uri" in item)).map(item => NetUtil.newURI(item.uri));

I don't think this works, having an if without a body is likely a syntax.
Should be (item => "uri" in item)

::: browser/components/places/content/treeView.js
@@ +153,5 @@
>  
>      // A node is removed form the view either if it has no parent or if its
>      // root-ancestor is not the root node (in which case that's the node
>      // for which nodeRemoved was called).
> +    let ancestors = PlacesUtils.nodeAncestors(aNode).map(x => x);

There are a couple problems here, first map(x => x) doesn't do anything...
But mostly, PlacesUtils.nodeAncestors is a generator, the code here was trying to build an array out of all the entries returned by the generator.
I think you can use Array.from(PlacesUtils.nodeAncestors(aNode)) here

::: toolkit/components/places/Bookmarks.jsm
@@ +739,5 @@
>        }
>  
>        yield db.executeCached(
>          `UPDATE moz_bookmarks
> +         SET ${tuples.keys().map(v => tuples.get(v).fragment || `${v} = :${v}`).join(", ")}

this doesn't work, keys() is an iterator
you can use Array.from(t.keys()).map(...

@@ +1409,5 @@
>    yield removeOrphanAnnotations(db);
>  
>    // TODO (Bug 1087576): this may leave orphan tags behind.
>  
> +  let urls = itemsRemoved.filter(item => if(item.url)).map(item => item.url);

the if() cannot work.
Should be .filter(item => "url" in item).map(

::: toolkit/components/places/History.jsm
@@ +527,5 @@
>   * @return (Promise)
>   */
>  var cleanupPages = Task.async(function*(db, pages) {
> +  yield invalidateFrecencies(db, pages.filter(p => if (p.hasForeign || p.hasVisits)).map(p => p.id));
> +  yield removePagesById(db, pages.filter(p => if (!p.hasForeign && !p.hasVisits)).map(p => p.id));

ditto, these ifs don't work

::: toolkit/components/places/PlacesTransactions.jsm
@@ +1266,5 @@
>        if (PlacesUtils.getBookmarksForURI(oldURI, {}).length == 0)
>          PlacesUtils.tagging.untagURI(oldURI, oldURITags);
>  
>        let currentNewURITags = PlacesUtils.tagging.getTagsForURI(aURI);
> +      newURIAdditionalTags = oldURITags.filter(t => if (!currentNewURITags.includes(t)));

broken if

@@ +1432,5 @@
>          throw new Error("Failed to get info for the specified item (guid: " +
>                          guid + "). Ex: " + ex);
>        }
>      }
> +    let toRestore = aGuids.map(guid => yield promiseBookmarksTree(guid));

I don't think this works cause map is synchronous and won't want for yield.
I fear we'll need to use a simple for...of loop here

@@ +1503,5 @@
>          onRedo.push(createTxn.redo.bind(createTxn));
>        }
>        else {
>          let currentTags = PlacesUtils.tagging.getTagsForURI(currentURI);
> +        let newTags = aTags.filter(t => if (!currentTags.includes(t)));

broken if

@@ +1544,5 @@
>        let currentURI = uri;
>        let tagsToRemove;
>        let tagsSet = PlacesUtils.tagging.getTagsForURI(currentURI);
>        if (aTags.length > 0)
> +        tagsToRemove = aTags.filter(t => if (tagsSet.includes(t)));

ditto

::: toolkit/components/places/tests/bookmarks/test_bookmarks_reorder.js
@@ +60,5 @@
>        url: "http://example3.com/",
>        parentGuid: PlacesUtils.bookmarks.unfiledGuid
>      }
>    ];
> +  let sorted = bookmarks.map(bm => yield PlacesUtils.bookmarks.insert(bm));

should use for...of

::: toolkit/components/places/tests/unit/test_async_transactions.js
@@ +1160,5 @@
>      }
>      function ensureTagsUnset() {
>        for (let url of urls) {
>          let expectedTags = tagsRemoved.length == 0 ?
> +           [] : preRemovalTags.get(url).filter(tag => if (!tagsRemoved.includes(tag)));

broken if

@@ +1593,5 @@
>                             , parentGuid: rootGuid }).transact();
>      guids.push(bmGuid);
>    });
>  
> +  let originalInfos = guids.map(guid => yield PlacesUtils.promiseBookmarksTree(guid));

use for...of

@@ +1633,5 @@
>        }
>      }
>    });
>  
> +  let originalInfos = guids.map(guid => yield PlacesUtils.promiseBookmarksTree(guid));

ditto

::: toolkit/components/places/tests/unit/test_null_interfaces.js
@@ +47,5 @@
>        return true;
>      }
>  
>      do_print(`Generating an array of functions to test service: ${s}`);
> +    for (let n of Object.keys(s).filter(i => if (okName(i))).sort()) {

broken if

@@ +86,5 @@
>            tryAgain = false;
>          }
>          catch(ex) {
>            do_print("Caught some unexpected exception.. dumping");
> +          do_print(ex.map(i => [i, ex[i]]).join("\n"));

ex is an exception, not an array

::: toolkit/components/places/tests/unit/test_promiseBookmarksTree.js
@@ +56,5 @@
>  
>    let expectedAnnos = PlacesUtils.getAnnotationsForItem(aItem.id);
>    if (expectedAnnos.length > 0) {
>      let annosToString = annos => {
> +      return annos.map(a => (a.name + ":" + a.value)).sort().join(",");

the rounded parenthesis look bad in this context, better remove them
Attachment #8694235 - Flags: review?(mak77)
Attached patch 1228976.patch (obsolete) — Splinter Review
Hey, sorry about the if conditions, I should have realized it.Please review patch now.Thanks.
Attachment #8694235 - Attachment is obsolete: true
Attachment #8694671 - Flags: review?(mak77)
I found another comprehension here that the previous search didn't found, cause it has wrong spacing, could you please fix this as well?

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesTransactions.jsm#1043
Comment on attachment 8694671 [details] [diff] [review]
1228976.patch

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

::: toolkit/components/places/tests/bookmarks/test_bookmarks_reorder.js
@@ +62,5 @@
>      }
>    ];
>    let sorted = [for (bm of bookmarks) yield PlacesUtils.bookmarks.insert(bm)]
> +  let sorted = [];
> +  for(let bm of bookmarks) {

a couple things while you are updating the patch

please always add a whitespace between for and ( (happens multiple times in the patch)

in this specific case you forgot to remove the original comprehension
Attachment #8694671 - Flags: review?(mak77)
Attached patch 1228976.patch (obsolete) — Splinter Review
I have made changes. Please review now,  Thanks.
Attachment #8694671 - Attachment is obsolete: true
Attachment #8694684 - Flags: review?(mak77)
Comment on attachment 8694684 [details] [diff] [review]
1228976.patch

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

Almost ready, still something to fix.

::: toolkit/components/places/PlacesTransactions.jsm
@@ +1040,5 @@
>      }
>      if (annos.length > 0) {
>        if (!aRestoring && aExcludingAnnotations.length > 0) {
> +        annos = annos.filter(a => !aExcludingAnnotations.includes(a.name))
> +

Missing semicolon and the leftover newline here can be removed

@@ +1094,5 @@
>          if (aAnnos.length)
>            PlacesUtils.setAnnotationsForItem(itemId, aAnnos);
>          if (aTags.length > 0) {
>            let currentTags = PlacesUtils.tagging.getTagsForURI(aURI);
> +          aTags = aTags.filter(t => if (!currentTags.includes(t)));

this is still broken

::: toolkit/components/places/tests/unit/test_null_interfaces.js
@@ +47,5 @@
>        return true;
>      }
>  
>      do_print(`Generating an array of functions to test service: ${s}`);
> +    for (let n of Object.keys(s).filter(i => okName(i)) .sort()) {

there's an additional space before .sort() that breaks this.

@@ +86,5 @@
>            tryAgain = false;
>          }
>          catch(ex) {
>            do_print("Caught some unexpected exception.. dumping");
> +          do_print(Object.keys(ex).map(i => [i, ex[i]]).join("\n"));

OK, I honestly don't understand what this was trying to do, it just looks broken after testing it.

let's just remove this generic catch (ex) clause, the test harness will catch the exception and do the right thing.
Attachment #8694684 - Flags: review?(mak77) → feedback+
Attached patch 1228976.patch (obsolete) — Splinter Review
Hey, I made appropriate changes. :)
Attachment #8694684 - Attachment is obsolete: true
Attachment #8695228 - Flags: review?(mak77)
Comment on attachment 8695228 [details] [diff] [review]
1228976.patch

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

yep, it looks good!

I'm pushing it to the Try server now.
Attachment #8695228 - Flags: review?(mak77) → review+
Looks like there's a problem

SyntaxError: missing ] after element list at resource://gre/modules/PlacesTransactions.jsm:684
Keywords: checkin-needed
Comment on attachment 8695228 [details] [diff] [review]
1228976.patch

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

yep, it looks good!

I'm pushing it to the Try server now.

::: toolkit/components/places/PlacesTransactions.jsm
@@ +680,5 @@
>        let input = DefineTransaction.verifyInput(aInput, aRequiredProps,
>                                                  aOptionalProps);
>        let executeArgs = [this,
> +                         ...aRequiredProps.map(prop => input[prop]),
> +                         ...aOptionalProps.map(prop => input[prop]);

ah yes, you removed the most external "]", here we are building an array out of arrays
Comment on attachment 8695228 [details] [diff] [review]
1228976.patch

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

::: toolkit/components/places/tests/unit/test_async_transactions.js
@@ +1302,2 @@
>    let sep = {};
> +  let postSep = ["c","b","a"].map(l => { title: l, url });

These don't work due to the braces, thet are handled as arrow function braces rather than object ones, it needs an explicit return
or new Object
Attached patch 1228976.patchSplinter Review
Hey, I have made the changes. parenthesis around the object works i guess :) |["c","b","a"].map(l => ({ title: l, url }))|
Attachment #8695228 - Attachment is obsolete: true
Attachment #8695778 - Flags: review?(mak77)
yes, there are plenty of options :)
Blocks: 1230471
Comment on attachment 8695778 [details] [diff] [review]
1228976.patch

You fixed only one of the 2 instances.
But no worries, since the patch was good enough and already reviewed I just fixed the remaining bit and pushed.
Attachment #8695778 - Flags: review?(mak77)
Thank you so much marko :). If there are any bugs related to porting (to es6) .  please let me know.
https://hg.mozilla.org/mozilla-central/rev/ba9eedfd155a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.