Improve array-input-property handling

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

unspecified
mozilla35
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Iteration: --- → 34.3
Points: --- → 5
Marco, can you please add this to the iteration? It's basically split off of bug 984903 and blocks it.
Flags: needinfo?(mmucci)
Status: NEW → ASSIGNED
Flags: qe-verify-
Summary: Improve arrary-input-property handling → Improve array-input-property handling
Posted patch patch (obsolete) — Splinter Review
Fix some bugs. Make SetItemAnnotation (Now Annotate) a testcase for this API (covered in the tests).
Attachment #8479858 - Attachment is obsolete: true
Attachment #8479858 - Flags: review?(mak77)
Attachment #8479970 - Flags: review?(mak77)
Added to IT 34.3
Flags: needinfo?(mmucci) → firefox-backlog+
Posted patch patchSplinter Review
Found more bug as I finished the test for bug 1059256.
Attachment #8479970 - Attachment is obsolete: true
Attachment #8479970 - Flags: review?(mak77)
Attachment #8480369 - Flags: review?(mak77)
Iteration: 34.3 → 35.1
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?
Attachment #8480369 - Flags: review?(mak77) → review+
(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.
sorry had to backout this change for xpcshell test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=48186886&tree=Fx-Team
Iteration: 35.1 → 35.2
https://hg.mozilla.org/mozilla-central/rev/9bc8e0f6bce3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.