All users were logged out of Bugzilla on October 13th, 2018

Remove calls to addVisit from the "unit" folder

RESOLVED FIXED in mozilla20

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Depends on: 1 bug)

Trunk
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
This removes more nested asynchronous addVisit calls.
(Assignee)

Comment 1

6 years ago
Created attachment 647125 [details] [diff] [review]
The patch

Nothing special here, except that promiseAddVisits now rejects on failure with a
descriptive exception, that includes the result code.

This makes it possible for test tasks to check expected exceptions resulting
from the asynchronous call, with the usual try/catch construct.
Attachment #647125 - Flags: feedback?(dteller)
(Assignee)

Comment 2

6 years ago
Note that some tests from this folder are still missing, a new patch will follow.
(Assignee)

Comment 3

6 years ago
Created attachment 647618 [details] [diff] [review]
Remove all calls

This removes all the synchronous calls, and works.

We can decide whether we want to do more cleanup on a case-by-case basis.
Attachment #647125 - Attachment is obsolete: true
Attachment #647125 - Flags: feedback?(dteller)
Attachment #647618 - Flags: feedback?(dteller)
Code is a little too long for me to be able to grok it at once, but so far, I like what I see.
Comment on attachment 647618 [details] [diff] [review]
Remove all calls

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

Given the number of files in the patch, I had a quick look at a few of the files. Could you reply to these questions before I continue?

(clearing the flags to make space on my dashboard, do not hesitate to re-ask me for feedback once you have replied)

::: toolkit/components/places/tests/head_common.js
@@ +586,5 @@
> +  let deferred = Promise.defer();
> +  waitForFrecency(aURI, aValidator, deferred.resolve);
> +  return deferred.promise;
> +}
> +

I don't see |Promise| defined/imported anywhere.

@@ +940,5 @@
> +      do_throw(ex, stack);
> +    }
> +  );
> +}
> +

addVisits does not return the promise, is this normal?

::: toolkit/components/places/tests/unit/test_000_frecency.js
@@ +57,5 @@
>  var results = [];
>  var matchCount = 0;
>  var now = Date.now();
>  var prefPrefix = "places.frecency.";
> +

Given that most people are unfamiliary with Task.js, you should probably add at least some type documentation on this function (and explain somewhere that Task.spawn handles the waiting).

@@ +198,5 @@
>  }
>  
>  function run_test() {
>    do_test_pending();
> +  Task.spawn(function () {

As for |Promise|, I don't see where you import |Task|.

@@ +205,5 @@
> +    }
> +
> +    // sort results by frecency
> +    results.sort(function(a,b) a[1] - b[1]);
> +    results.reverse();

I realize that this was in the initial implementation, but why not reverse in the above function, e.g. by writing |b[1] - a[1]|?

@@ +209,5 @@
> +    results.reverse();
> +    // Make sure there's enough results returned
> +    prefs.setIntPref("browser.urlbar.maxRichResults", results.length);
> +
> +    //results.every(function(el) { dump("result: " + el[1] + ": " + el[0].spec + " (" + el[2] + ")\n"); return true; })

I _guess_ you can dump this commented-out line.

@@ +211,5 @@
> +    prefs.setIntPref("browser.urlbar.maxRichResults", results.length);
> +
> +    //results.every(function(el) { dump("result: " + el[1] + ": " + el[0].spec + " (" + el[2] + ")\n"); return true; })
> +
> +    yield promiseAsyncUpdates();

Where's |promiseAsyncUpdates| defined?

::: toolkit/components/places/tests/unit/test_248970.js
@@ +44,5 @@
>                         "http://www.google.de/redirect-temporary/"];
>  /**
>   * Function fills history, one for each transition type.
>   */
> +function task_fill_history_visitedURI() {

As for other tasks, mention that this is meant to be used with Task.spawn.

@@ +160,5 @@
> +{
> +  run_next_test();
> +}
> +
> +add_test_task(function test_execute()

Where's |add_test_task| defined?

@@ +268,5 @@
> +    }
> +  });
> +
> +  Services.prefs.clearUserPref("browser.privatebrowsing.keep_current_session");
> +});

I have not checked all of this in detail, but this looks pretty mechanical, so I'll just take your word on it.

::: toolkit/components/places/tests/unit/test_317472.js
@@ -54,5 @@
>    do_check_eq(histsvc.getCharsetForURI(TEST_URI), charset);
>    // get charset from bookmarked page
>    do_check_eq(histsvc.getCharsetForURI(TEST_BOOKMARKED_URI), charset);
>  
> -  promiseClearHistory().then(continue_test);

Wait, there were already promises in the previous version? Or is this based on another patch? I'm lacking some context here.
Attachment #647618 - Flags: feedback?(dteller)
(Assignee)

Comment 6

6 years ago
(In reply to David Rajchenbach Teller from comment #5)
> ::: toolkit/components/places/tests/unit/test_317472.js
> @@ -54,5 @@
> >    do_check_eq(histsvc.getCharsetForURI(TEST_URI), charset);
> >    // get charset from bookmarked page
> >    do_check_eq(histsvc.getCharsetForURI(TEST_BOOKMARKED_URI), charset);
> >  
> > -  promiseClearHistory().then(continue_test);
> 
> Wait, there were already promises in the previous version? Or is this based
> on another patch? I'm lacking some context here.

This is part of a series of patches that are basically ordered by bug number,
the first being bug 775495 and the second bug 775580, all blocking bug 775489.

Most helper functions are defined in "head_common.js".

> addVisits does not return the promise, is this normal?

Since addVisits continues to behave like it did before, it just invokes the
callback, while promiseAddVisits is the version that returns the promise.

> ::: toolkit/components/places/tests/unit/test_000_frecency.js
> Given that most people are unfamiliary with Task.js, you should probably add
> at least some type documentation on this function (and explain somewhere
> that Task.spawn handles the waiting).

Yes, I think we should add comments in strategic positions, and also redirect
to the Task.jsm documentation where appropriate.

> @@ +205,5 @@
> > +    }
> > +
> > +    // sort results by frecency
> > +    results.sort(function(a,b) a[1] - b[1]);
> > +    results.reverse();
> 
> I realize that this was in the initial implementation, but why not reverse
> in the above function, e.g. by writing |b[1] - a[1]|?

Makes sense :-) I've not touched the code because I wanted this conversion to
be as mechanical as possible, but I can definitely fix instances like this.
(In reply to Paolo Amadini [:paolo] from comment #6)
> (In reply to David Rajchenbach Teller from comment #5)
> > ::: toolkit/components/places/tests/unit/test_317472.js
> > @@ -54,5 @@
> > >    do_check_eq(histsvc.getCharsetForURI(TEST_URI), charset);
> > >    // get charset from bookmarked page
> > >    do_check_eq(histsvc.getCharsetForURI(TEST_BOOKMARKED_URI), charset);
> > >  
> > > -  promiseClearHistory().then(continue_test);
> > 
> > Wait, there were already promises in the previous version? Or is this based
> > on another patch? I'm lacking some context here.
> 
> This is part of a series of patches that are basically ordered by bug number,
> the first being bug 775495 and the second bug 775580, all blocking bug
> 775489.
> 
> Most helper functions are defined in "head_common.js".

In that case, could you add some dependencies? This will make the reviewer's life easier. I suspect the dependencies will have to go back to the Task.jsm and Promise bugs.
(Assignee)

Updated

6 years ago
Depends on: 778694
(Assignee)

Comment 8

6 years ago
(In reply to David Rajchenbach Teller [:Yoric] from comment #7)
> In that case, could you add some dependencies? This will make the reviewer's
> life easier. I suspect the dependencies will have to go back to the Task.jsm
> and Promise bugs.

I've added a dependency chain that starts from Promise and Task and leads up to
this patch.
(Assignee)

Comment 9

6 years ago
Created attachment 683528 [details] [diff] [review]
Updated patch
Attachment #647618 - Attachment is obsolete: true
Attachment #683528 - Flags: review?(mak77)
Comment on attachment 683528 [details] [diff] [review]
Updated patch

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

As for the other bug, try run with multiple X results please :)

::: toolkit/components/places/tests/unit/test_000_frecency.js
@@ +129,5 @@
>        }
>        else
>          matchTitle = calculatedURI.spec.substr(calculatedURI.spec.lastIndexOf("/")+1);
> +      if (visitType) {
> +        yield promiseAddVisits({

I don't understand the if (visitType)... the previous code was not doing that, it seems to hide a bug in the test?

@@ +209,5 @@
> +    results.reverse();
> +    // Make sure there's enough results returned
> +    prefs.setIntPref("browser.urlbar.maxRichResults", results.length);
> +
> +    //results.every(function(el) { dump("result: " + el[1] + ": " + el[0].spec + " (" + el[2] + ")\n"); return true; })

add a // DEBUG above this, otherwise looks like a bug. I'm sure I wrote this lot of time ago

@@ +212,5 @@
> +
> +    //results.every(function(el) { dump("result: " + el[1] + ": " + el[0].spec + " (" + el[2] + ")\n"); return true; })
> +
> +    yield promiseAsyncUpdates();
> +  }).then(continue_test, do_report_unexpected_exception);

so why don't you move all of this into an add_task, as well as continue_test may be just added with add_task?

::: toolkit/components/places/tests/unit/test_454977.js
@@ +9,5 @@
>  
> +// Returns the Place ID corresponding to an added visit.
> +function task_add_visit(aURI, aVisitType) {
> +
> +  // Add the visit asynchronously, and save its visit ID.

nit: remove useless newline?

::: toolkit/components/places/tests/unit/test_asyncExecuteLegacyQueries.js
@@ +13,5 @@
> +    let options = PlacesUtils.history.getNewQueryOptions();
> +    options.sortingMode = Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING;
> +    let query = PlacesUtils.history.getNewQuery();
> +  
> +    PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)

trailing spaces above

::: toolkit/components/places/tests/unit/test_history.js
@@ +184,5 @@
>    do_check_true(root.childCount > 0);
>    root.containerOpen = false;
>  
> +  // We used to test bug 400544 (a referrer that is not in the DB gets added),
> +  // but the asynchronous API does not add referrers if not present.

I doubt anybody will ever read this comment... just remove it. Sad we lose a test but this doesn't change anything.

::: toolkit/components/places/tests/unit/test_history_observer.js
@@ +62,5 @@
>      do_check_true(aURI.equals(testuri));
>      do_check_true(aVisitID > 0);
>      do_check_eq(aTime, testtime);
> +    // TODO: updatePlaces cannot add a visit with session ID equal to 0.
> +    // do_check_eq(aSessionID, 0);

I'm not sure why this test was checking 0, I'd expect 1 probably... what do we get? I'd be fine just checking it's a number > 0

::: toolkit/components/places/tests/unit/test_history_removeAllPages.js
@@ +23,5 @@
> +      PlacesUtils.history.removeObserver(this, false);
> +      deferred.resolve();
> +    },
> +  
> +    QueryInterface: XPCOMUtils.generateQI([

there are various trailing spaces in this object

@@ +34,2 @@
>  
> +let promiseInit;

add comment on what this is used for

@@ +63,5 @@
> +  ]);
> +
> +  // Note: We used to add an invalid transtion, but this cannot be done anymore
> +  // with the asynchronous history API.
> +  // { uri: uri("http://invalid.mozilla.org/"), transition: 0 }

win win, get rid of this :)

@@ +96,3 @@
>  
> +  // Clear history and wait for the onClearHistory notification.
> +  let promiseWaitClearHistory = promiseOnClearHistoryObserved(); 

trailing space

::: toolkit/components/places/tests/unit/test_nsINavHistoryViewer.js
@@ +154,5 @@
> +          }
> +        }, null);
> +        do_check_false(resultObserver.inBatchMode);
> +  
> +        // nsINavHistoryResultObserver.containerClosed

various trailing spaces were added in this test

::: toolkit/components/places/tests/unit/test_placesTxn.js
@@ +655,5 @@
> +  
> +    txn.redoTransaction();
> +    do_check_true(annosvc.pageHasAnnotation(testURI, TEST_ANNO));
> +  
> +    run_next_test();

trailing spaces in this test as well

::: toolkit/components/places/tests/unit/test_removeVisitsByTimeframe.js
@@ +72,5 @@
> +                             TEST_URI,
> +                             bmsvc.DEFAULT_INDEX,
> +                             "bookmark title");
> +  
> +        promiseAsyncUpdates().then(this.continue_run.bind(this));

trailing spaces

::: toolkit/components/places/tests/unit/test_result_sort.js
@@ +118,5 @@
> +      checkOrder(id2, id3, id1);
> +      result.sortingMode = NHQO.SORT_BY_FRECENCY_ASCENDING;
> +      checkOrder(id1, id3, id2);
> +  
> +      root.containerOpen = false;

trailing spaces
Attachment #683528 - Flags: review?(mak77) → review+
(Assignee)

Comment 11

6 years ago
(In reply to Marco Bonardo [:mak] from comment #10)
> ::: toolkit/components/places/tests/unit/test_000_frecency.js
> @@ +129,5 @@
> >        }
> >        else
> >          matchTitle = calculatedURI.spec.substr(calculatedURI.spec.lastIndexOf("/")+1);
> > +      if (visitType) {
> > +        yield promiseAddVisits({
> 
> I don't understand the if (visitType)... the previous code was not doing
> that, it seems to hide a bug in the test?

AddVisits used to work with a transition type "0", updatePlaces doesn't. I'm not
sure what this test was meant to do, do you know something more?
I think transition 0 was a case supported originally in Places (before FF3), but fixed just after the first release, we likely don't care to test it.
(Assignee)

Comment 13

6 years ago
Created attachment 687071 [details] [diff] [review]
Revised patch
Attachment #683528 - Attachment is obsolete: true
Attachment #687071 - Flags: feedback?(mak77)

Updated

6 years ago
Attachment #687071 - Flags: feedback?(mak77) → review+
(Assignee)

Comment 14

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/94d11f7d9114
Target Milestone: --- → mozilla20

Comment 15

6 years ago
https://hg.mozilla.org/mozilla-central/rev/94d11f7d9114
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 821781

Updated

6 years ago
Depends on: 824074
You need to log in before you can comment on or make changes to this bug.