Create form data backend library for handling form related actions

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Daniel Gherasim, Assigned: Daniel Gherasim)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox28 fixed, firefox29 fixed, firefox30 fixed, firefox31 fixed, firefox-esr24 fixed)

Details

(Whiteboard: [lib])

Attachments

(1 attachment, 15 obsolete attachments)

14.31 KB, patch
Andrei Eftimie
: review+
Andrei Eftimie
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
As we talked on bug 879418 , we need a new Metro library to handle the Form Data for our tests.
The library will most likely live in metrofirefox/lib/ui/ and will handle the autocomplete popup or different types of form elements.
(Assignee)

Comment 1

4 years ago
Forget to mention that other component that might be handled in this library is the FormHistory.jsm service.
Why should this be a UI library? As discussed on bug 879418 we need a backend library to handle actions like clear all form data. Also why should this be Metro only? FormHistory.jsm is available for both Firefox and Metro.
(Assignee)

Updated

4 years ago
Summary: Create form data library for metro → Create form data backend library for handling form related actions
Whiteboard: [metro]
(Assignee)

Updated

4 years ago
Assignee: nobody → daniel.gherasim
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Created attachment 8376314 [details] [diff] [review]
new_formData_library_v1.patch

This is what I've done so far.

Henrik, can you give me a small feedback on this if I'm on the right way ?

For review I'll ask someone else first.
Attachment #8376314 - Flags: feedback?(hskupin)
Comment on attachment 8376314 [details] [diff] [review]
new_formData_library_v1.patch

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

You did a lot of work here, what I haven't expected. We usually don't have classes for modules which are calling back-end code. Having to create a class instance each time is a bit overhead. Why cannot we have simple methods in this module similar to the others, which allow us to directly call them? Then private methods should not be exported.

::: firefox/lib/form-data.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I would call this file simply forms.js.

@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * @fileoverview
> + * The FormDataAPI adds support for accessing and interacting with form data

This name is not how this module is called.

@@ +13,5 @@
> + *              firstUsedStart, firstUsedEnd, lastUsedStart, lastUsedEnd,
> + *              agedWeight, bucketSize, expiryDate, maxTimeGroundings,
> + *              timeGroupingSize, prefixWeight, boundaryWeight
> + *
> + * @version 1.0.0

We never used that. Just drop it off.

@@ +25,5 @@
> +/**
> + * Constructor
> + */
> +function FormHistoryManager() {
> +  this._controller = null;

I don't see why controller is ever necessary. No back-end module has to require or use it.

@@ +263,5 @@
> +        if(aResult) {
> +          // Do something if a result is avaible
> +        }
> +      }
> +    }

Same structure as for the autocomplete one. You might want to create a more generalized one, which takes a callback for the handleResult case?

@@ +310,5 @@
> +
> +      handleResult: function(aResultArray) {
> +        resultsArray.push(aResultArray);
> +      }
> +    }

Same here. Try to have a single definition of such a synchronizing method.

::: firefox/lib/tests/testFormData.js
@@ +44,5 @@
> +  expect.equal(autoCompleteResults.length, COUNT_DIFFERENT,
> +               "Results number is correct");
> +
> +  // Search for entries that have a specific value and get the frecency for them
> +  var searchItems = formData._search(["timesUsed"],

I don't expect that other code could call private methods. There is a problem in the abstraction of the class.
Attachment #8376314 - Flags: feedback?(hskupin) → feedback-
(Assignee)

Comment 5

4 years ago
Created attachment 8378954 [details] [diff] [review]
new_formData_library_v2.patch

This is the library and the test for every exported method avaible.
Attachment #8376314 - Attachment is obsolete: true
Attachment #8378954 - Flags: review?(andrei.eftimie)
Attachment #8378954 - Flags: review?(andreea.matei)

Comment 6

4 years ago
Comment on attachment 8378954 [details] [diff] [review]
new_formData_library_v2.patch

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

I know Henrik suggested this should not be organised like the other UI classes.
But I feel we still need some structure here. ATM we have a bunch of functions all calling each other without any direct relationship.
And this makes it hard to understand and follow this whole modules logic.

I would argue that we still need to group this things.
Even if we don't have a `class` which we instantiate, we could group them so the API makes more sense.

I encourage to have a discussion about it.

My proposal would be to have some sort of `form` object that would encapsulate most if not all functions we have here.
Also the names are pretty verbose. I'll need to take a deeper look and try coming up with a sounding API calls.

I'm leaving the review flag up for now until I gather my thoughts on this.

::: firefox/lib/forms.js
@@ +29,5 @@
> +
> +const NOOP = function noop() {};
> +
> +var callbacks = {
> +  resultHandle : NOOP,

nit: please preserve the spacing, the other properties/methods don't have a space before the colon. This one should probably be the same.

@@ +54,5 @@
> + * @params {Object} aEntry
> + *         The new entry to be added
> + */
> +function addNewEntryToField(aField, aEntry)
> +{

nit: as per our style guide please leave the curly braces on the line before

@@ +107,5 @@
> + */
> +function clearEntriesFromField(aField, aFilters)
> +{
> +  var buffer = {fieldname: aField};
> +  for(var index in aFilters) {

nit: space after for

@@ +128,5 @@
> + *        Form field name (id if missing)
> + *
> + * @returns {Integer} Number of results avaible
> + */
> +function countAllEntriesFromField(aField)

Do we need this function? 
This only calls countEntriesFromField without the second argument.

Comment 7

4 years ago
Since this is a Backend API it should live under /lib, not under /firefox.

Now lets get to the API part, I'm proposing to fold most functions we have
into more general purpose ones that follow both the implementation of
FormHistory.jsm and use some general patters we have in other library code.

Basically something along the following lines:

add(aField, aValue)
count([aField, [aSpec]])
clear([aField, [aSpec]])
get(aField, [aSpec])
search(aSearchTerms, aSpec)
update(aField, aSpec)

Where in cases like `clear` and `count` with no arguments we'll return
everything. If we have the `aField` argument we'll return all entries for the
spefied field and with the aSpec object we can pass aditional filter information
to the queries.

Remove and clear can be folded into `clear`. Without arguments it would clear
all history. With the `aField` argument it would only affect the specified field
and by passing further inforamtion into `aSpec` we can handle the rest of the 
scenarious.

With the `update` method we should be able to update a specific entry by the 
criteria specified with the `aSpec` object. (This should also cover the
previous `bump` method).

We only end up with 6 functions, I don't feel we need any other organizational 
changes (as I was proposing in the previous comment).

If anyone has something to add, please do.

Updated

4 years ago
Attachment #8378954 - Flags: review?(andrei.eftimie)
Attachment #8378954 - Flags: review?(andreea.matei)
Attachment #8378954 - Flags: review-
(Assignee)

Comment 8

4 years ago
Created attachment 8385151 [details] [diff] [review]
new_formData_library_v3.patch

Given that what Andrei asked was small refactoring of the library, I updated it now with his specifications.

As we had discussions about making a library COMPLETE from the first time or just put the things we need in our current tests, I'll make a minimized patch of this library and put it along with the test for bug 879418 so we can land it. Also, when this is ready, we can land it too.
Attachment #8378954 - Attachment is obsolete: true
Attachment #8385151 - Flags: feedback?
(Assignee)

Updated

4 years ago
Attachment #8385151 - Flags: review?(andrei.eftimie)
Attachment #8385151 - Flags: review?(andreea.matei)
Attachment #8385151 - Flags: feedback?
(Assignee)

Updated

4 years ago
Blocks: 879418

Comment 9

4 years ago
Comment on attachment 8385151 [details] [diff] [review]
new_formData_library_v3.patch

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

::: lib/forms.js
@@ +30,5 @@
> +var callbacks = {
> +  resultHandle: NOOP,
> +  completed: false,
> +  handleCompletion: function(aReason) {
> +    if(aReason === 0)

nit: missing space before the bracket, and please wrap even single-line `if` statements with curly braces

@@ +52,5 @@
> + * @params {String} aValue
> + *         The value of the new entry to be added
> + */
> +function add(aField, aValue)
> +{

nit: as per our guide please have all block starting curly braces at the end of the previous line

@@ +54,5 @@
> + */
> +function add(aField, aValue)
> +{
> +  if(!(aField !== void 0 && aField.length > 0) ||
> +     !(aValue !== void 0 && aValue.length > 0)) {

We want to make sure that these arguments are strings.
The length property will return true for non-empty string, non-empty array, a function with at least 1 argument. These are all different than void.
So this check is not reliable.

I think `typeof` would be our best bet in this case.
We can probably keep the length check

@@ +55,5 @@
> +function add(aField, aValue)
> +{
> +  if(!(aField !== void 0 && aField.length > 0) ||
> +     !(aValue !== void 0 && aValue.length > 0)) {
> +    throw new Error("Failed loading parameters");

Instead of throw we should use the assertion module.

@@ +76,5 @@
> +}
> +
> +/**
> + * Count entries in database
> + *

We should add short description on how this function works.

@@ +97,5 @@
> +{
> +  var count = 0;
> +  var queryData = {};
> +
> +  if(aQueryData !== void 0) {

This whole block is over-engineered.

If we call `count` without any arguments, we expect all results otherwise filter the results based on the properties of `aQueryData`
So we should just do:
> queryData = aQueryData || {};

Then forward the request.

@@ +147,5 @@
> +  var queryData = {
> +    op: "remove"
> +  };
> +
> +  if(aQueryData !== void 0) {

Same as above.
I think the following will do the trick.
> var queryData = aQueryData || {};
> queryData.op = "remove";

@@ +194,5 @@
> +  var searchString = "";
> +  var queryData = {};
> +  var resultsArray = [];
> +
> +  if(aQueryData !== void 0) {

The same as above.

@@ +280,5 @@
> + */
> +function update(aField, aSpec)
> +{
> +  if(!(aField !== void 0 && aField.length > 0) ||
> +     !(aSpec !== void 0 && aSpec.value !== void 0 && aSpec.value.length > 0)) {

Please also update these as in the `add` function.

@@ +289,5 @@
> +
> +  var getGUID = search(["guid"], {fieldname: aField, value: aSpec.value});
> +  changes.guid = getGUID[0].guid;
> +
> +  if(aSpec.bump !== void 0 && aSpec.bump === true) {

The second check should be enough.

@@ +296,5 @@
> +  }
> +
> +  for(var index in aSpec) {
> +    changes[index] = aSpec[index];
> +  }

I would make it similar to what I've described in the other functions.
Start changes as aSpec, then change the relevant parts (guid, bump).

::: lib/tests/testFormData.js
@@ +101,5 @@
> +  forms.clear([INPUT_FIELD, {
> +    value: INPUT_ENTRY
> +  }]);
> +
> +  countResults = forms.count([INPUT_FIELD]);

We should send an object, not an array. {fieldname: INPUT_FIELD}
Attachment #8385151 - Flags: review?(andrei.eftimie)
Attachment #8385151 - Flags: review?(andreea.matei)
Attachment #8385151 - Flags: review-
(Assignee)

Comment 10

4 years ago
Created attachment 8387574 [details] [diff] [review]
new_formData_library_v3.1.patch

Heh, good tip from you with removing al that code and use directly objects insted.
Thanks a lot.

Updated the patch now.
Attachment #8385151 - Attachment is obsolete: true
Attachment #8387574 - Flags: review?(andrei.eftimie)
Attachment #8387574 - Flags: review?(andreea.matei)

Comment 11

4 years ago
Comment on attachment 8387574 [details] [diff] [review]
new_formData_library_v3.1.patch

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

Thanks Daniel, looks much better now.

Still have a few issues to solve.

::: lib/forms.js
@@ +80,5 @@
> + * Count entries in database that meets some filters
> + * Returns total number of entries if the parameter is missing
> + *
> + * @param {Object} [aSpec]
> + *        Filters to be meet in counting

This description needs a small rewrite.

@@ +151,5 @@
> + * Returns results for a specific field following some filters
> + *
> + * @param {Array} aSpec
> + *        Data for the get query
> + * @param {String} [aSpec

nit: missing name, closing square bracket

@@ +173,5 @@
> +  var searchString = aSpec.searchString || "";
> +  delete aSpec.searchString;
> +
> +  function handleResult(aResultArray) {
> +    resultsArray.push(aResultArray);

Why are we using an array?
I'm assuming this function will not be invoked multiple times.
If we get back an array in `aResultArray`, this will actually return [[value1, value2]]

@@ +193,5 @@
> + *
> + * @param {Array} aSearchTerms
> + *                Array of terms (fields) to select for the entries
> + * @param {Object} aSpec
> + *                Filters to apply for the searching

nit: indentation

@@ +209,5 @@
> + * @returns {Object[]} Autocomplete results that match the filters
> + */
> +function search(aSearchTerms, aSpec) {
> +  var resultsArray = [];
> +  var searchTerms = aSearchTerms || [];

The `searchTerms` should also allow a single string.

@@ +251,5 @@
> +function update(aSpec) {
> +  var changes = aSpec || {};
> +  changes.op = "update";
> +
> +  if(!(typeof changes.guid === 'string' && changes.guid.length > 0)) {

I think you cam simply this as:
> if (!changes.guid && typeof changes.guid !== "string")

@@ +255,5 @@
> +  if(!(typeof changes.guid === 'string' && changes.guid.length > 0)) {
> +    assert.fail("Guid not specified correctly")
> +  }
> +
> +  if(changes.bump === true) {

nit: space between `if` and paranthesis
We might also have this near the `changes.op = "update"` line.

Also thinking that we might make use of the ternary operator for this case.
> changes.op = changes.bump ? "bump" : "update";

::: lib/tests/testFormData.js
@@ +11,5 @@
> +const INPUT_TEXTS = ["John", "Ema", "Yoana", "John"];
> +const INPUT_FIELD = "ship_fname";
> +const COUNT_DIFFERENT = 3;
> +const INPUT_ENTRY = "TestEntry";
> +const INPUT_ENTRY2 = "TestEntryIncaCeva";

Leave the test entry value in English.

@@ +13,5 @@
> +const COUNT_DIFFERENT = 3;
> +const INPUT_ENTRY = "TestEntry";
> +const INPUT_ENTRY2 = "TestEntryIncaCeva";
> +
> +function setupModule(module) {

nit: aModule

@@ +17,5 @@
> +function setupModule(module) {
> +  module.controller = mozmill.getBrowserController();
> +}
> +
> +function teardownModule(module) {

nit: aModule

Also, we should clear the db of the entries we are inserting in this test.

@@ +33,5 @@
> +  expect.equal(countResults, 1, "New entry added.");
> +
> +  // Count all results for a field
> +  countResults = forms.count({fieldname: INPUT_FIELD});
> +  expect.equal(countResults, 1, "New entry added");

You are doing this twice.

@@ +54,5 @@
> +    submitButton.click();
> +    controller.waitForPageLoad();
> +
> +    controller.open(TEST_DATA);
> +    controller.waitForPageLoad();

I think these should be at the beginning of the loop.

@@ +61,5 @@
> +  var autoCompleteResults = forms.get({fieldname: INPUT_FIELD, searchString: INPUT_ENTRY});
> +  expect.equal(autoCompleteResults.length, 1, "Results number is correct");
> +
> +  autoCompleteResults = forms.get({fieldname: INPUT_FIELD});
> +  expect.equal(autoCompleteResults.length, 4, "Results number is correct");

Shouldn't this be 5?
We have 4 entries from INPUT_TEXTS and 1 from INPUT_ENTRY

@@ +66,5 @@
> +
> +  countResults = forms.count({fieldname: INPUT_FIELD});
> +  expect.equal(countResults, 4, "New entries added");
> +
> +  var searchResults = forms.search(["timesUsed"], {value: INPUT_ENTRY});

The searchTerm should also allow a single string.
Attachment #8387574 - Flags: review?(andrei.eftimie)
Attachment #8387574 - Flags: review?(andreea.matei)
Attachment #8387574 - Flags: review-
(Assignee)

Comment 12

4 years ago
(In reply to Andrei Eftimie from comment #11)
> @@ +173,5 @@
> > +  var searchString = aSpec.searchString || "";
> > +  delete aSpec.searchString;
> > +
> > +  function handleResult(aResultArray) {
> > +    resultsArray.push(aResultArray);
> 
> Why are we using an array?
> I'm assuming this function will not be invoked multiple times.
> If we get back an array in `aResultArray`, this will actually return
> [[value1, value2]]
> 

It's actually called multiple times vor every entry found.
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/FormHistory.jsm#1053

> 
> @@ +61,5 @@
> > +  var autoCompleteResults = forms.get({fieldname: INPUT_FIELD, searchString: INPUT_ENTRY});
> > +  expect.equal(autoCompleteResults.length, 1, "Results number is correct");
> > +
> > +  autoCompleteResults = forms.get({fieldname: INPUT_FIELD});
> > +  expect.equal(autoCompleteResults.length, 4, "Results number is correct");
> 
> Shouldn't this be 5?
> We have 4 entries from INPUT_TEXTS and 1 from INPUT_ENTRY
> 

One from INPUT_TEXTS is repeated twice, so it will be counted as one but have the timesUsed = 2.
(Assignee)

Comment 13

4 years ago
Created attachment 8388529 [details] [diff] [review]
new_formData_library_v3.2.patch
Attachment #8388529 - Flags: review?(andrei.eftimie)
Attachment #8388529 - Flags: review?(andreea.matei)
(Assignee)

Updated

4 years ago
Attachment #8387574 - Attachment is obsolete: true

Comment 14

4 years ago
Comment on attachment 8388529 [details] [diff] [review]
new_formData_library_v3.2.patch

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

::: lib/forms.js
@@ +36,5 @@
> +    }
> +  },
> +
> +  handleFailure: function(aError) {
> +    throw("Failed with: " + aError);

This should probably be assert.fail

@@ +57,5 @@
> +function add(aField, aValue) {
> +  if (!aField && typeof aField !== "string" ||
> +      !aValue && typeof aValue !== "string") {
> +    assert.fail("Failed loading parameters");
> +  }

We should use assert.ok here.

@@ +69,5 @@
> +  callbacks.resultHandle = NOOP;
> +  callbacks.completed = false;
> +  FormHistory.update(changes, callbacks);
> +
> +  // Wait for the clear to end

This comment is probably not needed.

@@ +151,5 @@
> + * Returns results for a specific field following some filters
> + *
> + * @param {Array} aSpec
> + *        Data for the get query
> + * @param {String} [aSpec]

aSpec should only be described once. And its type is object.
And I don't think it can be optional.

@@ +160,5 @@
> + * @param {Date} [aSpec.expiryDate]
> + * @param {Object} [aSpec.maxTimeGroundings]
> + * @param {Object} [aSpec.timeGroupingSize]
> + * @param {Object} [aSpec.prefixWeight]
> + * @param {Object} [aSpec.boundaryWeight]

Are these all objects?

@@ +161,5 @@
> + * @param {Object} [aSpec.maxTimeGroundings]
> + * @param {Object} [aSpec.timeGroupingSize]
> + * @param {Object} [aSpec.prefixWeight]
> + * @param {Object} [aSpec.boundaryWeight]
> +

nit: extra line

@@ +167,5 @@
> + * @returns {Object[]} Autocomplete results that match the filters
> + */
> +function get(aSpec) {
> +  var searchString = "";
> +  var queryData = aSpec || {};

We have both `queryData` and `changes` used throughout the file.
We should consolidate them. Maybe use `spec` as most libraries are.

@@ +169,5 @@
> +function get(aSpec) {
> +  var searchString = "";
> +  var queryData = aSpec || {};
> +  var resultsArray = [];
> +  var searchString = aSpec.searchString || "";

Shouldn't use `aSpec` once a local version is created. (Because this will fail if the function doesn't receive an argument)

@@ +170,5 @@
> +  var searchString = "";
> +  var queryData = aSpec || {};
> +  var resultsArray = [];
> +  var searchString = aSpec.searchString || "";
> +  delete aSpec.searchString;

This deletes from the wrong object.

@@ +190,5 @@
> +
> +/**
> + * Search for entries and return specific terms for them
> + *
> + * @param {String|Array} [aSearchTerms]

I think this shouldn't be optional.

@@ +192,5 @@
> + * Search for entries and return specific terms for them
> + *
> + * @param {String|Array} [aSearchTerms]
> + *        Term or array of terms (fields) to select for the entries
> + * @param {Object} aSpec

Is this optional?
All its children are marked optional...

@@ +209,5 @@
> + * @returns {Object[]} Autocomplete results that match the filters
> + */
> +function search(aSearchTerms, aSpec) {
> +  var resultsArray = [];
> +  var searchTerms = (typeof aSearchTerms === "string") ? [aSearchTerms] : (aSearchTerms || []);

I'm not sure on defaulting to an empty array. Will that work?
We can have both: [""], []

@@ +252,5 @@
> +  var changes = aSpec || {};
> +
> +  if (!changes.guid && typeof changes.guid !== "string") {
> +    assert.fail("Guid not specified correctly");
> +  }

assert.ok
Attachment #8388529 - Flags: review?(andrei.eftimie)
Attachment #8388529 - Flags: review?(andreea.matei)
Attachment #8388529 - Flags: review-
(Assignee)

Comment 15

4 years ago
Created attachment 8391140 [details] [diff] [review]
new_formData_library_v3.3.patch

(In reply to Andrei Eftimie from comment #14)
> @@ +151,5 @@
> > + * Returns results for a specific field following some filters
> > + *
> > + * @param {Array} aSpec
> > + *        Data for the get query
> > + * @param {String} [aSpec]
> 
> aSpec should only be described once. And its type is object.
> And I don't think it can be optional.
> 

Right, here aSpec.fieldname is not optional.

> @@ +160,5 @@
> > + * @param {Date} [aSpec.expiryDate]
> > + * @param {Object} [aSpec.maxTimeGroundings]
> > + * @param {Object} [aSpec.timeGroupingSize]
> > + * @param {Object} [aSpec.prefixWeight]
> > + * @param {Object} [aSpec.boundaryWeight]
> 
> Are these all objects?
> 

Actually floats, updated them.

> @@ +167,5 @@
> > + * @returns {Object[]} Autocomplete results that match the filters
> > + */
> > +function get(aSpec) {
> > +  var searchString = "";
> > +  var queryData = aSpec || {};
> 
> We have both `queryData` and `changes` used throughout the file.
> We should consolidate them. Maybe use `spec` as most libraries are.
> 

Done!

> @@ +190,5 @@
> > +
> > +/**
> > + * Search for entries and return specific terms for them
> > + *
> > + * @param {String|Array} [aSearchTerms]
> 
> I think this shouldn't be optional.
> 

It's actually optional, if no searchTerm is given, then all of them will be selected.
I used an single aSpec object afterall, so we don't have an optional parameter before another one.

> @@ +192,5 @@
> > + * Search for entries and return specific terms for them
> > + *
> > + * @param {String|Array} [aSearchTerms]
> > + *        Term or array of terms (fields) to select for the entries
> > + * @param {Object} aSpec
> 
> Is this optional?
> All its children are marked optional...
> 

It's optional, we retrieve all results if no filter is present!
Done!

> @@ +209,5 @@
> > + * @returns {Object[]} Autocomplete results that match the filters
> > + */
> > +function search(aSearchTerms, aSpec) {
> > +  var resultsArray = [];
> > +  var searchTerms = (typeof aSearchTerms === "string") ? [aSearchTerms] : (aSearchTerms || []);
> 
> I'm not sure on defaulting to an empty array. Will that work?
> We can have both: [""], []
> 

Right, an empty string is enough here if searchTerms are not present.
Attachment #8388529 - Attachment is obsolete: true
Attachment #8391140 - Flags: review?(andrei.eftimie)
Attachment #8391140 - Flags: review?(andreea.matei)

Comment 16

4 years ago
Comment on attachment 8391140 [details] [diff] [review]
new_formData_library_v3.3.patch

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

Looks good.

Please fix the nits mentioned below and request a review from Henrik.

::: lib/forms.js
@@ +54,5 @@
> + * @params {String} aValue
> + *         The value of the new entry to be added
> + */
> +function add(aField, aValue) {
> +  assert.ok(aField && typeof aField === "string" && aValue && typeof aValue === "string",

This check should be broken up on 2 lines.
We are using 80 characters as a soft wrap, but we should try to limit ourselves to 80 whenever we can, and this is a place where we can do it easily.

@@ +67,5 @@
> +  callbacks.resultHandle = NOOP;
> +  callbacks.completed = false;
> +  FormHistory.update(spec, callbacks);
> +
> +  assert.waitFor(() => callbacks.completed, "New entry has been added for field");

80c break as above.

@@ +101,5 @@
> +  callbacks.resultHandle = handleResult;
> +  callbacks.completed = false;
> +  FormHistory.count(spec, callbacks);
> +
> +  assert.waitFor(() => callbacks.completed, "Number of history form results retrieved");

80c break as above.

@@ +173,5 @@
> +  callbacks.completed = false;
> +  callbacks.resultHandle = handleResult;
> +  FormHistory.getAutoCompleteResults(searchString, spec, callbacks);
> +
> +  assert.waitFor(() => callbacks.completed, "Form History results retrieved");

80c break as above.

@@ +204,5 @@
> +  var resultsArray = [];
> +  var spec = aSpec || {};
> +  var selectTerms = "";
> +
> +  if(spec.selectTerms && typeof spec.selectTerms === "string")

Please always use curly braces on if/else statements.

@@ +207,5 @@
> +
> +  if(spec.selectTerms && typeof spec.selectTerms === "string")
> +    selectTerms = [spec.selectTerms];
> +  else
> +    if(Array.isArray(spec.selectTerms))

nit: missing space after `if`

@@ +220,5 @@
> +  callbacks.resultHandle = handleResult;
> +  callbacks.completed = false;
> +  FormHistory.search(selectTerms, spec, callbacks);
> +
> +  assert.waitFor(() => callbacks.completed, "Form History search results retrieved");

80c break as above.

@@ +248,5 @@
> + */
> +function update(aSpec) {
> +  var spec = aSpec || {};
> +
> +  assert.ok(spec.guid && typeof spec.guid === "string", "Guid not specified correctly");

80c break as above.
Attachment #8391140 - Flags: review?(andrei.eftimie)
Attachment #8391140 - Flags: review?(andreea.matei)
Attachment #8391140 - Flags: review-
(Assignee)

Comment 17

4 years ago
Created attachment 8392793 [details] [diff] [review]
new_formData_library_v3.4.patch

Thanks, nits are fixed now.
Asked Henrik for review.
Attachment #8391140 - Attachment is obsolete: true
Attachment #8392793 - Flags: review?(hskupin)
Comment on attachment 8392793 [details] [diff] [review]
new_formData_library_v3.4.patch

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

::: lib/forms.js
@@ +15,5 @@
> + *
> + * Valid search filters for entries:
> + *              firstUsedStart, firstUsedEnd, lastUsedStart, lastUsedEnd,
> + *              agedWeight, bucketSize, expiryDate, maxTimeGroundings,
> + *              timeGroupingSize, prefixWeight, boundaryWeight

What are those entries for at this place. We should add descriptions to parameters for the appropriate methods.

@@ +18,5 @@
> + *              agedWeight, bucketSize, expiryDate, maxTimeGroundings,
> + *              timeGroupingSize, prefixWeight, boundaryWeight
> + *
> + */
> +

You missed the declaration of strict.

@@ +29,5 @@
> +
> +var callbacks = {
> +  resultHandle: NOOP,
> +  completed: false,
> +  handleCompletion: function(aReason) {

nit: missing space after function. Please check also other lines of code in this file.

@@ +36,5 @@
> +    }
> +  },
> +
> +  handleFailure: function(aError) {
> +    assert.fail("Failed with: " + aError);

Given that this is back-end, it should be an expect, or? I would like to see our tests to continue until a real problem is caught.

@@ +54,5 @@
> + * @params {String} aValue
> + *         The value of the new entry to be added
> + */
> +function add(aField, aValue) {
> +  assert.ok(aField && typeof aField === "string" &&

There is no need to check for `aField`. The typeof is enough.

@@ +56,5 @@
> + */
> +function add(aField, aValue) {
> +  assert.ok(aField && typeof aField === "string" &&
> +            aValue && typeof aValue === "string",
> +            "Failed loading parameters");

I would do both in separate asserts, and give a clear failure message.

@@ +65,5 @@
> +    value: aValue
> +  };
> +
> +  callbacks.resultHandle = NOOP;
> +  callbacks.completed = false;

What you really want here is that Callback is a class which initializes the values correctly. With having a single object here, which also gets shared between calls make us not thread-safe.

@@ +69,5 @@
> +  callbacks.completed = false;
> +  FormHistory.update(spec, callbacks);
> +
> +  assert.waitFor(() => callbacks.completed,
> +                 "New entry has been added for field");

We might add which entry for which field.

@@ +74,5 @@
> +}
> +
> +/**
> + * Count entries in database that meets some filters
> + * Returns total number of entries if the parameter is missing

Please move this line under @returns

@@ +158,5 @@
> + * @param {Float} [aSpec.boundaryWeight]
> + *
> + * @returns {Object[]} Autocomplete results that match the filters
> + */
> +function get(aSpec) {

get() what? This name needs to be clearer

@@ +182,5 @@
> +  return resultsArray;
> +}
> +
> +/**
> + * Search for entries and return specific terms for them

This is a search for form data right? I would not specialize it for auto-complete entries.

@@ +186,5 @@
> + * Search for entries and return specific terms for them
> + *
> + * @param {Object} [aSpec]
> + *        Filters to apply for the searching
> + * @param {Array|String} [aSpec.selectTerms=""]

I would only allow an array so we can pass it through to the API.

@@ +256,5 @@
> +function update(aSpec) {
> +  var spec = aSpec || {};
> +
> +  assert.ok(spec.guid && typeof spec.guid === "string",
> +            "Guid not specified correctly");

`assert.ok(spec.guid, ...)` should be enough here i think. I don't think we have to explicitly check for the type, or should we?

@@ +259,5 @@
> +  assert.ok(spec.guid && typeof spec.guid === "string",
> +            "Guid not specified correctly");
> +
> +  spec.op = spec.bump ? "bump" : "update";
> +  delete spec.bump;

Why do we have to delete it?

@@ +265,5 @@
> +  callbacks.resultHandle = NOOP;
> +  callbacks.completed = false;
> +  FormHistory.update(spec, callbacks);
> +
> +  // Wait for the clear to end

Clear? It's an update.

::: lib/tests/testFormData.js
@@ +25,5 @@
> +function testFormData() {
> +  controller.open(TEST_DATA);
> +  controller.waitForPageLoad();
> +
> +  forms.clear();

This has to be part of setupModule().

@@ +38,5 @@
> +  // Count all resunts in DB
> +  countResults = forms.count();
> +  expect.equal(countResults, 1, "New entry added");
> +
> +  // Count all results that meets a criteria os sth

os sth?

@@ +41,5 @@
> +
> +  // Count all results that meets a criteria os sth
> +  countResults = forms.count({fieldname: INPUT_FIELD, timesUsed: 1});
> +
> +  var inputField = new elementslib.ID(controller.tabs.activeTab, INPUT_FIELD);

Do this first in this test and use inputField.getNode().id for the form lib calls. We should not overuse constants in our tests. It's useful for test data and results but not for elements.

@@ +42,5 @@
> +  // Count all results that meets a criteria os sth
> +  countResults = forms.count({fieldname: INPUT_FIELD, timesUsed: 1});
> +
> +  var inputField = new elementslib.ID(controller.tabs.activeTab, INPUT_FIELD);
> +  assert.ok(inputField.exists(), "Name field has been found");

Why do we need this?

@@ +44,5 @@
> +
> +  var inputField = new elementslib.ID(controller.tabs.activeTab, INPUT_FIELD);
> +  assert.ok(inputField.exists(), "Name field has been found");
> +
> +  var submitButton = new elementslib.ID(controller.tabs.activeTab, "SubmitButton");

Same as for the input field.

@@ +49,5 @@
> +
> +  // Add some more fields
> +  INPUT_TEXTS.forEach(function (aInput) {
> +    controller.open(TEST_DATA);
> +    controller.waitForPageLoad();

I don't see why we need this. The page is already loaded.

@@ +57,5 @@
> +    controller.waitForPageLoad();
> +  });
> +
> +  var autoCompleteResults = forms.get({fieldname: INPUT_FIELD, searchString: INPUT_ENTRY});
> +  expect.equal(autoCompleteResults.length, 1, "Results number is correct");

Be more detailed in your comment like 'One form entry for test string has been found'.

Also you could combine those two lines, and move the message to the next line. Same for nearly everything below.
Attachment #8392793 - Flags: review?(hskupin) → review-

Comment 19

4 years ago
(In reply to Henrik Skupin (:whimboo) from comment #18)
> @@ +54,5 @@
> > + * @params {String} aValue
> > + *         The value of the new entry to be added
> > + */
> > +function add(aField, aValue) {
> > +  assert.ok(aField && typeof aField === "string" &&
>
> There is no need to check for `aField`. The typeof is enough.

This is to rule out empty string.

> @@ +186,5 @@
> > + * Search for entries and return specific terms for them
> > + *
> > + * @param {Object} [aSpec]
> > + *        Filters to apply for the searching
> > + * @param {Array|String} [aSpec.selectTerms=""]
> 
> I would only allow an array so we can pass it through to the API.

I specifically asked for this. In most cases we will only use 1 term.
Always wrapping it in an Array doesn't make sense.

If its a String we pass it further as ["String"] so the underlying API always receives an Array.

But as a consumer of the API why not make it easier for us when writing tests?
(In reply to Andrei Eftimie from comment #19)
> > > +function add(aField, aValue) {
> > > +  assert.ok(aField && typeof aField === "string" &&
> >
> > There is no need to check for `aField`. The typeof is enough.
> 
> This is to rule out empty string.

Why should an empty string not be possible? I wouldn't restrict that value, but let the API decide how to handle it. As of now it could hide an API implementation detail.

> > > + * @param {Object} [aSpec]
> > > + *        Filters to apply for the searching
> > > + * @param {Array|String} [aSpec.selectTerms=""]
> > 
> > I would only allow an array so we can pass it through to the API.
> 
> I specifically asked for this. In most cases we will only use 1 term.
> Always wrapping it in an Array doesn't make sense.

Why not? What's complicated in using `search(obj, ["search_string"])` instead? This would give a sane and clear API which depends on fixed parameter types.
(Assignee)

Comment 21

4 years ago
Created attachment 8399879 [details] [diff] [review]
new_formData_library_v4.patch

I have updated all the changes requested.

(In reply to Henrik Skupin (:whimboo) from comment #18)
> @@ +259,5 @@
> > +  assert.ok(spec.guid && typeof spec.guid === "string",
> > +            "Guid not specified correctly");
> > +
> > +  spec.op = spec.bump ? "bump" : "update";
> > +  delete spec.bump;
>
> Why do we have to delete it?
>

Because we pass through the spec all the update filters and "bump" it's not one of the filters, we use it just locally to test if it's actually an bump update.
Attachment #8392793 - Attachment is obsolete: true
Attachment #8399879 - Flags: review?(andrei.eftimie)
Attachment #8399879 - Flags: review?(andreea.matei)

Comment 22

4 years ago
(In reply to daniel.gherasim from comment #21)
> Created attachment 8399879 [details] [diff] [review]
> new_formData_library_v4.patch
> 
> I have updated all the changes requested.
> 
> (In reply to Henrik Skupin (:whimboo) from comment #18)
> > @@ +259,5 @@
> > > +  assert.ok(spec.guid && typeof spec.guid === "string",
> > > +            "Guid not specified correctly");
> > > +
> > > +  spec.op = spec.bump ? "bump" : "update";
> > > +  delete spec.bump;
> >
> > Why do we have to delete it?
> >
> 
> Because we pass through the spec all the update filters and "bump" it's not
> one of the filters, we use it just locally to test if it's actually an bump
> update.

I think the question was. Does this hurt if we pass it along? It should just be ignored, right?

Comment 23

4 years ago
Comment on attachment 8399879 [details] [diff] [review]
new_formData_library_v4.patch

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

In general this looks good to me.

Please fix the mentioned items and request a review from Henrik.

::: lib/forms.js
@@ +46,5 @@
> +    }
> +  },
> +
> +  /**
> +   * Handle a failure if it accurrs

nit: occurs

@@ +251,5 @@
> + */
> +function search(aSpec) {
> +  var resultsArray = [];
> +  var spec = aSpec || {};
> +  var selectTerms = spec.selectTerms || "";

This should default to []

::: lib/tests/testFormData.js
@@ +42,5 @@
> +  countResults = forms.count();
> +  expect.equal(countResults, 1, "Number of all avaible entries is correct.");
> +
> +  // Count all results that meets a criteria
> +  countResults = forms.count({fieldname: inputField.getNode().id, timesUsed: 1});

I think in comment 18 Henrik suggested to not declare these as variables, instead used them directly in the expect calls. (Of course, where appropriate)

@@ +64,5 @@
> +               "Number of form entries found for field '" + inputField.getNode().id +
> +               "' is correct");
> +
> +  var searchResults = forms.search({selectTerms: ["timesUsed", "guid"],
> +                                   value: INPUT_ENTRY});

nit: indentation missing 1 space. Also in other places in this file.
Attachment #8399879 - Flags: review?(andrei.eftimie)
Attachment #8399879 - Flags: review?(andreea.matei)
Attachment #8399879 - Flags: review-
(Assignee)

Comment 24

4 years ago
(In reply to Andrei Eftimie from comment #23)
> @@ +251,5 @@
> > + */
> > +function search(aSpec) {
> > +  var resultsArray = [];
> > +  var spec = aSpec || {};
> > +  var selectTerms = spec.selectTerms || "";
> 
> This should default to []
> 

In API we have aSelectTerms = (aSelectTerms) ?  aSelectTerms : validFields;
so if the parameter is missing, we need to specify a value that will return false,
when ([]) returns true, so I let it default as undefined.
(Assignee)

Comment 25

4 years ago
Created attachment 8400017 [details] [diff] [review]
new_formData_library_v4.1.patch

I made the changes, thanks !
Attachment #8399879 - Attachment is obsolete: true
Attachment #8400017 - Flags: review?(hskupin)
(Assignee)

Comment 26

4 years ago
Created attachment 8400018 [details] [diff] [review]
new_formData_library_v4.2.patch

Small nit missed.
Attachment #8400017 - Attachment is obsolete: true
Attachment #8400017 - Flags: review?(hskupin)
Attachment #8400018 - Flags: review?(hskupin)
Comment on attachment 8400018 [details] [diff] [review]
new_formData_library_v4.2.patch

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

::: lib/forms.js
@@ +18,5 @@
> + * @param {Function} aHandleResult
> + *        Function to trigger on the result we retrieve
> + */
> +function Callback(aHandleResult) {
> +  this._resultHandle = aHandleResult;

I would make this parameter optional and define it as empty function. So we can get rid of the NOOP constant above.

@@ +19,5 @@
> + *        Function to trigger on the result we retrieve
> + */
> +function Callback(aHandleResult) {
> +  this._resultHandle = aHandleResult;
> +  this._completed = false;

It's a basic boolean. I don't see a reason why to make it private and wrap it behind getters.

@@ +33,5 @@
> +    return this._completed;
> +  },
> +
> +  /**
> +   * Change the state of the callback object if it completed completed successfully

nit: completed is duplicated.

@@ +35,5 @@
> +
> +  /**
> +   * Change the state of the callback object if it completed completed successfully
> +   *
> +   * @param {Integer} aReason=0|1

Please correct the syntax.

@@ +37,5 @@
> +   * Change the state of the callback object if it completed completed successfully
> +   *
> +   * @param {Integer} aReason=0|1
> +   *        The state of the callback once it finishes
> +   *        aReason is either 0 if successful or 1 if an error occurred.

handleCompletion will never be called in case of an error. Therefor handleFailure exists.

@@ +118,5 @@
> + *        Entries last accessed after or at this time
> + * @param {Date} [aSpec.lastUsedEnd]
> + *        Entries last accessed before or at this time
> + *
> + * @returns {Integer} Number of results avaible

In JavaScript we do not have Integer, but Number

@@ +123,5 @@
> + * Returns total number of entries if the parameter is missing
> + */
> +function count(aSpec) {
> +  var count = 0;
> +  var spec = aSpec || {};

First check the parameters and then define local variables.

@@ +129,5 @@
> +  function handleResult(aResult) {
> +    count = aResult;
> +  }
> +
> +  var callback = new Callback(handleResult);

Please use fat arrow function.

@@ +199,5 @@
> +function getResults(aSpec) {
> +  var spec = aSpec || {};
> +
> +  assert.ok(spec.fieldname && typeof spec.fieldname === "string",
> +            "Field Name not specified correctly");

I don't see why we need spec.fieldname check here. The typeof should be enough.

@@ +209,5 @@
> +  function handleResult(aResultArray) {
> +    resultsArray.push(aResultArray);
> +  }
> +
> +  var callback = new Callback(handleResult);

fat arrow method should also work fine here.
Attachment #8400018 - Flags: review?(hskupin) → review-
(Assignee)

Comment 28

4 years ago
Created attachment 8401101 [details] [diff] [review]
new_formData_library_v4.3.patch

Changes made.

(In reply to Henrik Skupin (:whimboo) from comment #27)
> @@ +37,5 @@
> > +   * Change the state of the callback object if it completed completed successfully
> > +   *
> > +   * @param {Integer} aReason=0|1
> > +   *        The state of the callback once it finishes
> > +   *        aReason is either 0 if successful or 1 if an error occurred.
> 
> handleCompletion will never be called in case of an error. Therefor
> handleFailure exists.
> 

In case of an error, both handleFailure & handleCompletion will be called. The last one with aReason parameter set to 1.
Attachment #8400018 - Attachment is obsolete: true
Attachment #8401101 - Flags: review?(andrei.eftimie)
Attachment #8401101 - Flags: review?(andreea.matei)

Comment 29

4 years ago
Comment on attachment 8401101 [details] [diff] [review]
new_formData_library_v4.3.patch

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

Looks good to me.

::: lib/forms.js
@@ +16,5 @@
> + * @param {Function} [aHandleResult]
> + *        Function to trigger on the result we retrieve
> + */
> +function Callback(aHandleResult) {
> +  this._resultHandle = aHandleResult || function noop() {};

Just a small change: since you changed this, I think you can leave it as a anonymous function, I don't see a reason to name it.
Attachment #8401101 - Flags: review?(andrei.eftimie)
Attachment #8401101 - Flags: review?(andreea.matei)
Attachment #8401101 - Flags: review+
(Assignee)

Comment 30

4 years ago
Created attachment 8403134 [details] [diff] [review]
new_formData_library_v4.4.patch

Thanks! Last 2 reviews were nits and small changes.
It is a new library so asking for a final review.
Attachment #8401101 - Attachment is obsolete: true
Attachment #8403134 - Flags: review?(hskupin)
Comment on attachment 8403134 [details] [diff] [review]
new_formData_library_v4.4.patch

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

::: lib/forms.js
@@ +108,5 @@
> + * @param {Date} [aSpec.lastUsedEnd]
> + *        Entries last accessed before or at this time
> + *
> + * @returns {Number} Number of results avaible
> + * Returns total number of entries if the parameter is missing

A description exists once and is always at the top even if it is about the return value.

@@ +186,5 @@
> +  var spec = aSpec || {};
> +  var searchString = spec.searchString || "";
> +  delete spec.searchString;
> +
> +  assert.ok(typeof spec.fieldname === "string", "Field Name not specified correctly");

As said in my last review, please do those checks first.

@@ +190,5 @@
> +  assert.ok(typeof spec.fieldname === "string", "Field Name not specified correctly");
> +
> +  var resultsArray = [];
> +
> +  var callback = new Callback((aResultArray) => { resultsArray.push(aResultArray); });

Do we really get an array for the result here or is this callback executed for each result?

@@ +232,5 @@
> + * Results will contain properties specified by aSpec.selectTerms
> + */
> +function search(aSpec) {
> +  var spec = aSpec || {};
> +  var selectTerms = spec.selectTerms;

I would add a blank line above to separate checks for parameters, and other declarations and modifications.

@@ +279,5 @@
> +  var spec = aSpec || {};
> +  spec.op = spec.bump ? "bump" : "update";
> +  delete spec.bump;
> +
> +  assert.ok(spec.guid, "Guid not specified correctly");

Same as above.
Attachment #8403134 - Flags: review?(hskupin) → review-
(Assignee)

Comment 32

4 years ago
(In reply to Henrik Skupin (:whimboo) from comment #31)
> Review of attachment 8403134 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +190,5 @@
> > +  assert.ok(typeof spec.fieldname === "string", "Field Name not specified correctly");
> > +
> > +  var resultsArray = [];
> > +
> > +  var callback = new Callback((aResultArray) => { resultsArray.push(aResultArray); });
> 
> Do we really get an array for the result here or is this callback executed
> for each result?
> 

It's actually called multiple times vor every entry found which is an object with proprietes.
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/FormHistory.jsm#1053

So the final result will be : [{text: "", frecency: "", ...},{},{}]. 
But I'll change the 'aResultArray' parameter's name to aResult so we don't create confusion here.
(Assignee)

Comment 33

4 years ago
Created attachment 8403165 [details] [diff] [review]
new_formData_library_v4.5.patch

Thanks!
Updated.
Attachment #8403134 - Attachment is obsolete: true
Attachment #8403165 - Flags: review?(andrei.eftimie)
Attachment #8403165 - Flags: review?(andreea.matei)
Comment on attachment 8403165 [details] [diff] [review]
new_formData_library_v4.5.patch

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

Seems like the changes requested have been fixed now.
Attachment #8403165 - Flags: review?(hskupin)
Attachment #8403165 - Flags: review?(andrei.eftimie)
Attachment #8403165 - Flags: review?(andreea.matei)
Attachment #8403165 - Flags: review+
Comment on attachment 8403165 [details] [diff] [review]
new_formData_library_v4.5.patch

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

::: lib/forms.js
@@ +182,5 @@
> + *
> + * @returns {Object[]} Autocomplete results that match the filters
> + */
> +function getResults(aSpec) {
> +  assert.ok(typeof aSpec.fieldname === "string", "Field Name not specified correctly");

Any reason why you have removed 'var spec = aSpec || {};'?

@@ +236,5 @@
> +
> +  delete spec.selectTerms;
> +  var resultsArray = [];
> +
> +  var callback = new Callback((aResult) => { resultsArray.push(aResult); });

No need for brackets. Just do 'aResult => resultsArray.push(aResult)'.

@@ +275,5 @@
> + * @param {Date} [aSpec.lastUsedEnd]
> + *        Entries last accessed before or at this time
> + */
> +function update(aSpec) {
> +  assert.ok(aSpec.guid, "Guid not specified correctly");

Same as above for the 'spec' definition.
Attachment #8403165 - Flags: review?(hskupin) → review-
(Assignee)

Comment 36

4 years ago
Created attachment 8406740 [details] [diff] [review]
new_formData_library_v4.6.patch

(In reply to Henrik Skupin (:whimboo) from comment #35)
> Comment on attachment 8403165 [details] [diff] [review]
> new_formData_library_v4.5.patch
> 
> Review of attachment 8403165 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/forms.js
> @@ +182,5 @@
> > + *
> > + * @returns {Object[]} Autocomplete results that match the filters
> > + */
> > +function getResults(aSpec) {
> > +  assert.ok(typeof aSpec.fieldname === "string", "Field Name not specified correctly");
> 
> Any reason why you have removed 'var spec = aSpec || {};'?
> 

You said we should assert first and if aSpec is not defined, this line doesn't make sense after the assertions.

I moved the asserts right after 'var spec = aSpec || {}' line. Hope it's ok now.
Attachment #8406740 - Flags: review?(andrei.eftimie)
Attachment #8406740 - Flags: review?(andreea.matei)
Comment on attachment 8406740 [details] [diff] [review]
new_formData_library_v4.6.patch

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

Just some changes I missed last time, otherwise please request review from Henrik.

::: lib/forms.js
@@ +66,5 @@
> + *         The value of the new entry to be added
> + */
> +function add(aField, aValue) {
> +  assert.ok(typeof aField === "string", "Field is defined");
> +  assert.ok(typeof aValue === "string", "Value is defined");

Please use assert.equal here.

@@ +183,5 @@
> + * @returns {Object[]} Autocomplete results that match the filters
> + */
> +function getResults(aSpec) {
> +  var spec = aSpec || {};
> +  assert.ok(typeof spec.fieldname === "string", "Field Name not specified correctly");

Same here.
Attachment #8406740 - Flags: review?(andrei.eftimie)
Attachment #8406740 - Flags: review?(andreea.matei)
Attachment #8406740 - Flags: review+
(Assignee)

Comment 38

4 years ago
Created attachment 8408173 [details] [diff] [review]
new_formData_library_v4.7.patch

Thanks!
Attachment #8403165 - Attachment is obsolete: true
Attachment #8406740 - Attachment is obsolete: true
Attachment #8408173 - Flags: review?(hskupin)
Comment on attachment 8408173 [details] [diff] [review]
new_formData_library_v4.7.patch

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

Please fix the nit and we are good.

::: lib/forms.js
@@ +114,5 @@
> +function count(aSpec) {
> +  var spec = aSpec || {};
> +  var count = 0;
> +
> +  var callback = new Callback(aResult => count = aResult);

Why have the brackets been removed around the fat array function block? Please put them back given that we have two separate operations here (=> and =).
Attachment #8408173 - Flags: review?(hskupin) → review+
(Assignee)

Comment 40

4 years ago
Created attachment 8410973 [details] [diff] [review]
new_formData_library_v4.8.patch

Fixed it. Once this can landed we can take care of bug 879418 too.
Attachment #8408173 - Attachment is obsolete: true
Attachment #8410973 - Flags: review?(andrei.eftimie)
Attachment #8410973 - Flags: review?(andreea.matei)
Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/e00a029d01e2 (default)

We'd need backporting for this as it will be used in other tests (please also file a follow up bug to update the tests).
status-firefox31: --- → fixed
Whiteboard: [lib]
(Assignee)

Updated

4 years ago
Blocks: 1000163

Comment 42

4 years ago
Since this only adds a new file/lib (which is not used yet in any other tests) it can be safely landed across branches.

Transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/ef113b61f3de (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/c3e58b104f07 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/e8d77af0400f (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/86ba258cdc84 (mozilla-esr24)
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox28: --- → fixed
status-firefox29: --- → fixed
status-firefox30: --- → fixed
status-firefox-esr24: --- → fixed
OS: Windows 8.1 → All
Resolution: --- → FIXED

Comment 43

4 years ago
Comment on attachment 8410973 [details] [diff] [review]
new_formData_library_v4.8.patch

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

Seems we forgot to update this flag.
Attachment #8410973 - Flags: review?(andrei.eftimie)
Attachment #8410973 - Flags: review?(andreea.matei)
Attachment #8410973 - Flags: review+
Attachment #8410973 - Flags: checkin+
You need to log in before you can comment on or make changes to this bug.