1059257 - Improve array-input-property handling

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Asaf Romano (gone)]

This allows passing "tag: foo" rather than tags: ["foo"].

Since this code is still a moving target (and it'd be nice to take it out of PlacesTransaction.jsm) I'm saving the documentation for another bug.

Comment 1

[Asaf Romano (gone)]

Attachment: patch

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Marco, can you please add this to the iteration? It's basically split off of bug 984903 and blocks it.

Comment 3

[Asaf Romano (gone)]

Attachment: patch

Fix some bugs. Make SetItemAnnotation (Now Annotate) a testcase for this API (covered in the tests).

Comment 4

[Marco Mucci [:MarcoM]]

Added to IT 34.3

Comment 5

[Asaf Romano (gone)]

Attachment: patch

Found more bug as I finished the test for bug 1059256.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8480369 [details] [diff] [review]
patch

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

r=me with the following addressed

::: toolkit/components/places/PlacesTransactions.jsm
@@ +593,2 @@
>    for (let name of aNames) {
> +    let propName = name;

I'm missing why these assignments...

@@ +600,5 @@
> +          throw new Error(`Invalid value for input property ${propName}`);
> +        return aTransformFunction ? aTransformFunction(aValue) : aValue;
> +      },
> +
> +      validateInput(aInput, aRequired) {

this looks broken syntax

@@ +632,5 @@
> +    },
> +
> +    // We allow setting either the array property itself (e.g. uris, or a single
> +    // element of it (uri, in that example), that would then be transformed into
> +    // a single-element array.

unclear comment, broken parenthesis...

@@ +676,3 @@
>  
> +  let propName = aRequiredProps.length == 1 ?
> +                 aRequiredProps[0] : aOptionalProps[0];

some more comments would help here, this code looks strange. If aRequiredProps.length is 1, we use it, if it's 0 then we use aOptionalProps[0], and that might be fine.
But I don't understand why use aOptionalProps[0] if aRequiredProps.length is 2 or greater... It might be the case we can't reach that case but that's unclear from the code.

@@ +1111,5 @@
> +PT.Annotate = DefineTransaction(["GUID", "annotations"]);
> +PT.Annotate.prototype = {
> +  execute: function* (aGUID, aNewAnnos) {
> +    let itemId = yield PlacesUtils.promiseItemId(aGUID);
> +    let currentAnnos = PlacesUtils.getAnnotationsForItem(itemId);

getAnnotationsForItem makes me sad :( (bug 1047819) but there's no alternative

@@ +1117,5 @@
> +    for (let newAnno of aNewAnnos) {
> +      let currentAnno = currentAnnos.find( a => a.name == newAnno.name );
> +      if (!!currentAnno)
> +        undoAnnos.push(currentAnno);
> +      else // An unset value removes the annoation.

typo: annoation

and I'd prefer these to be braced and the comment to be on its own line...

::: toolkit/components/places/tests/unit/test_async_transactions.js
@@ +1242,5 @@
> +  yield PT.undo();
> +  verifyAnnoValues(null, 2);
> +
> +  yield PT.undo();
> +  verifyAnnoValues(1, 2);

might reduce a little bit newline noise in this new test?

Comment 7

[Asaf Romano (gone)]

(In reply to Marco Bonardo [:mak] (needinfo? me) from comment #6)

> ::: toolkit/components/places/PlacesTransactions.jsm
> @@ +593,2 @@
> >    for (let name of aNames) {
> > +    let propName = name;
> 
> I'm missing why these assignments...

Because of Third World bug 449811. Try this Chrome-compatible script in Scratchpad and in Chrome's console (you may need to turn on "experimental javascript features" in about:flags):

(function() {
  "use strict";
  let f = null;
  for (let i of [2,3,4]) {
    if (i == 2)
      f = function() { return i; };
  }
  return f();
}());

Spoiler: 2 in Chrome; 4 in Firefox.

Even though the issue surprised me (not for the first time though!), I didn't comment about this at first because it'd strange to comment about language semantics. However, now that I know this issue might be fixed, and probably sooner than later (Chrome gets it right, and it seems this old bug has "contemporary" dependencies), I guess I should.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

:/

Comment 9

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/fx-team/rev/a58a8a698e3d

I forgot to add the comment I promised; will do in the other bug.

Comment 10

[Carsten Book [:Tomcat]]

sorry had to backout this change for xpcshell test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=48186886&tree=Fx-Team

Comment 11

[Asaf Romano (gone)]

Relanded: https://hg.mozilla.org/integration/fx-team/rev/9bc8e0f6bce3

The test-issue was actually caused by the patch for bug 1058566.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9bc8e0f6bce3