Closed Bug 1265836 Opened 8 years ago Closed 8 years ago

Implement browser.history.addUrl

Categories

(WebExtensions :: Untriaged, defect)

Product:
WebExtensions
Component:
Untriaged
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox49 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla49
Iteration:
49.3 - Jun 6
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Whiteboard: [history]triaged)

Attachments

(4 files, 1 obsolete file)

MozReview Request: Bug 1265836 - Part 3: Implement insert and insertMany in History.jsm, r?mak
58 bytes, text/x-review-board-request
mak
: review+
Details
MozReview Request: Bug 1265836 - Part 4: Implement browser.history.addUrl, r?aswan
58 bytes, text/x-review-board-request
aswan
: review+
Details
MozReview Request: Bug 1265836 - Part 1: Replace PlacesUtils.toTime with PlacesUtils.toDate, r?mak r?aswan
58 bytes, text/x-review-board-request
aswan
: review+
mak
: review+
Details
MozReview Request: Bug 1265836 - Part 2: Change normalizeTime to return a date rather than a number, r?aswan
58 bytes, text/x-review-board-request
aswan
: review+
Details
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
No longer depends on: 1265835
Blocks: 1265837
No longer blocks: 1265837
Will there be support to add more details than just the url?
At least the last visit time would be useful for syncing (parts of) history. Just adding a bunch of urls with the current time will break the order of the merged histories.
This seems like a reasonable enhancement to me. Chrome currently only supports specifying a URL, but it is passed in an object, so allowing for a visitDate in that object as well should not break compatibility (although Chrome might not like it).

What do others think?
As I commented on the mail, I think it's a good enhancement, but we should sanitize input to not add visits in the future, cause those can easily break the awesomebar and other stuff.
Thanks Marco. I think I'm going to try to implement `update` in History.jsm to support this. Do you think that check you mention above for future dates should be added to that `update` method, or should that allow future dates and we should just do the check in our code for the API?
Flags: needinfo?(mak77)
The Places API should also prevent future dates, imo.
Flags: needinfo?(mak77)
Marco, I think that rather than throw errors in the `update` method we should return rejected promises. I know that isn't consistent with the other code in History.jsm right now, but I think it's the direction we want to go in. I'm working on making that change right now, but if you like you can still review the existing patch for any other issues you see.
I prefer the APIs to throw when the input is wrong, cause it's much easier to catch, especially if you forget to yield or catch errors. indeed I expect many consumers forget that, and then the bugs become very hard to notice.

Please stick to the other history/bookmarks APIs style.
also not that inside a Task, throwing or rejecting doesn't make a difference.
also, passing wrong input is ALWAYS a coding error, never a runtime error, so it seems to me it makes far more sense to throw.
Comment on attachment 8748336 [details]
MozReview Request: Bug 1265836 - Part 3: Implement insert and insertMany in History.jsm, r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50217/diff/1-2/
Fair enough, thanks Marco. In that case the patch is ready to be reviewed. :)
OK, will do that asap, thanks.
Comment on attachment 8748336 [details]
MozReview Request: Bug 1265836 - Part 3: Implement insert and insertMany in History.jsm, r?mak

https://reviewboard.mozilla.org/r/50217/#review47509

Note: we are designing an API here, so it's worth going through it a couple time and discuss thoughts. I'm giving some suggestions here, part to keep the API simple and part to make it more coherent with the new bookmarking API and future Places APIs. Though, I'm very curious to hear suggestions or counter arguments that maybe we didn't notice yet.

::: toolkit/components/places/History.jsm:188
(Diff revision 2)
>     *      If an element of `visits` has an invalid `date`.
>     * @throws (Error)
>     *      If an element of `visits` is missing `transition` or if
>     *      the value of `transition` is invalid.
>     */
> -  update: function (infos, onResult) {
> +  update: function (pageInfos, onResult) {

let's rename this to insert... this method doesn't really update anything, it can only insert.

Also, I don't think onResult here is really useful. Either a url is added or not, I think we should just resolve in case of success, reject an array of failed urls in case of failure.

Do you think the caller may need any other info or the whole VisitInfo object that failed?

::: toolkit/components/places/History.jsm:213
(Diff revision 2)
> +      if (pageInfo.guid && !isValidGUID(pageInfo.guid)) {
> +        throw new TypeError(`GUID: ${pageInfo.guid} of PageInfo object is invalid`);
> +      }
> +      // Convert pageInfo.url to nsIURI
> +      if (pageInfo.url) {
> +        pageInfo.uri = (pageInfo.url instanceof Ci.nsIURI) ? pageInfo.url : NetUtil.newURI(pageInfo.url);

We should also support URL, not just URI or spec. You can use instanceof URL for that.
Refer to the Bookmarks.jsm code if you need a reference.

Also, I think we should not modify the passed-into object, to avoid bugs in the caller in case it reuses the object. let's build our own internal object

::: toolkit/components/places/History.jsm:226
(Diff revision 2)
> +      }
> +      for (var visit of pageInfo.visits) {
> +        if (!visit.transitionType) {
> +          throw new TypeError("Each visit must have a transitionType");
> +        }
> +        if (!isValidTransitionType(visit.transitionType)) {

Please check the VisitInfo object description, the input properties should be named 'date' and 'transition'

::: toolkit/components/places/History.jsm:237
(Diff revision 2)
> +            throw new TypeError(`visitDate: ${visit.visitDate} cannot be a future date`);
> +          }
> +        } else {
> +          visit.visitDate = Date.now();
> +        }
> +        visit.visitDate = visit.visitDate * 1000;

move the conversion to a toPRTime helper so we can easily serch all the callpoints (you can copy th helper from Bookmarks.jsm, or move it to PlacesUtils)

::: toolkit/components/places/History.jsm:461
(Diff revision 2)
> +    History.TRANSITION_EMBED,
> +    History.TRANSITION_REDIRECT_PERMANENT,
> +    History.TRANSITION_REDIRECT_TEMPORARY,
> +    History.TRANSITION_DOWNLOAD,
> +    History.TRANSITION_FRAMED_LINK
> +  ].indexOf(transitionType) > -1;

includes

::: toolkit/components/places/History.jsm:470
(Diff revision 2)
> + * Is a string a valid GUID?
> + *
> + * @param guid: (String)
> + * @return (Boolean)
> + */
> +function isValidGUID(guid) {

nit: could also put this in PlacesUtils and reuse it also in Bookmarks.jsm

::: toolkit/components/places/History.jsm:863
(Diff revision 2)
> +      handleResult: (placeInfo) => {
> +        onResultData.push(placeInfo);
> +      },
> +      handleCompletion: () => {
> +        notifyOnResult(onResultData, onResult);
> +        resolve(true);

This should also manage errors and reject in case.

handleResult is invoked when a placeInfo is properly inserted, handleError when a placeInfo is not inserted

when handleError is invoked we should collect the failing urls, then in handleCompletion, if some urls failed we should reject an array of the failed urls (or whatever else we decide to reject)

::: toolkit/components/places/tests/history/test_update.js:196
(Diff revision 2)
> +      yield updater("Testing History.update() with a single string guid", x => do_get_guid_for_uri(x), useCallback);
> +    }
> +  } finally {
> +    yield PlacesTestUtils.clearHistory();
> +  }
> +});

Also needs some reject tests, I think it's possible to do that passing some uri that doesn's pass the canAddURI check mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#1091

::: toolkit/components/places/tests/history/test_update.js:198
(Diff revision 2)
> +  } finally {
> +    yield PlacesTestUtils.clearHistory();
> +  }
> +});
> +
> +function run_test() {

nit: this is no more needed in xpcshell tests
Attachment #8748336 - Flags: review?(mak77)
https://reviewboard.mozilla.org/r/50217/#review47509

> let's rename this to insert... this method doesn't really update anything, it can only insert.
> 
> Also, I don't think onResult here is really useful. Either a url is added or not, I think we should just resolve in case of success, reject an array of failed urls in case of failure.
> 
> Do you think the caller may need any other info or the whole VisitInfo object that failed?

Agreed: `insert` makes more sense. :)

I'm not sure if we need `onResult` or not. Perhaps it's useful as the method only returns a boolean, so that would mean that if a caller did want something done with the results they would have to do it manually after the call returns, and they wouldn't have info that only exists after an insert, like guid.

I'm not sure I follow about returning that failure info (whether an array of urls or the whole VisitInfo object). In my experience I have always seen `Promise.reject()` with `Error` objects passed in. Would we simply pass an array instead of an `Error`? I guess the consumer would need to know to expect that. We might want to provide the whole VisitInfo object as you suggest because someone could try to add multiple visits for a single url, and have only some of them fail.

If there are any problems inserting, will/should the entire operation to fail, or can we have a scenario where some visits get added and others do not? How does `asyncHistory.updatePlaces` deal with that? Does it not add anything if there are any errors?
(In reply to Bob Silverberg [:bsilverberg] from comment #15)
> https://reviewboard.mozilla.org/r/50217/#review47509
> 
> > let's rename this to insert... this method doesn't really update anything, it can only insert.
> > 
> > Also, I don't think onResult here is really useful. Either a url is added or not, I think we should just resolve in case of success, reject an array of failed urls in case of failure.
> > 
> > Do you think the caller may need any other info or the whole VisitInfo object that failed?
> 
> Agreed: `insert` makes more sense. :)
> 
> I'm not sure if we need `onResult` or not. Perhaps it's useful as the method
> only returns a boolean, so that would mean that if a caller did want
> something done with the results they would have to do it manually after the
> call returns, and they wouldn't have info that only exists after an insert,
> like guid.

Yes, returning the guid makes sense!

> I'm not sure I follow about returning that failure info (whether an array of
> urls or the whole VisitInfo object). In my experience I have always seen
> `Promise.reject()` with `Error` objects passed in. Would we simply pass an
> array instead of an `Error`? I guess the consumer would need to know to
> expect that. We might want to provide the whole VisitInfo object as you
> suggest because someone could try to add multiple visits for a single url,
> and have only some of them fail.

1. we could have onResult invoked for each successful insert, resolve invoked if all inserts are successful, reject invoked if some/all inserts fail.
At this point if the consumer wants to know WHAT failed, he should filter out the input array using the onResult calls, what's left is the failure.

2. Alternatively onResult could be invoked for both success/failure, and then the consumer should check for guid to know if the page was inserted, or onResult could have a bool "inserted" argument.

3. Or we could have onResult invoked for succesful inserts, and reject an array of the failed pageInfo, so we basically do the filtering work for the consumer.

4. Or we could always resolve and have 2 callbacks, onResult and onError, getting the respective pageInfos. I'd make it reject only if ALL the insertions fail, cause then something is very wrong.

Considered how this kind of API is used, I'd probably go for 4. What do you think?


> If there are any problems inserting, will/should the entire operation to
> fail, or can we have a scenario where some visits get added and others do
> not? How does `asyncHistory.updatePlaces` deal with that? Does it not add
> anything if there are any errors?

I think the most common cases are:
1. something's broken at the db level (or maybe the db is closed already). All the insertions fail.
2. some of the requested insertions are invalid (for example the url doesn't pass the CanAddURI check I linked above), then some insertions succeeds, some fails.
IIRC updatePlaces never bails out.

The chrome API makes everything simpler, by not providing batch insertions. Indeed if we'd make insert not accept an array, the API would be much simpler (resolve to the pageInfo if added, reject otherwise). but it would also be less efficient in case of multiple inserts (this advantage would not be visible from addUrl regardless).

So another possibility could be to make insert a single pageInfo method similar to promiseAddVisit, and then provide an insertMany that works for batches and is implemented as 4. above.
This would also be more coherent with bookmarks insert, that inserts a single bookmark (and in future it will likely grow an applyTree method for batch inserts).
Thoughts?
the single case insert would clearly resolve to pageInfo-with-guid or reject an Error.
last possibility (very smart, maybe too much?), have a single insert that can work in 2 modes.
if you pass in a single pageInfo, it will resolve to pageinfo+guid, if you pass an array of pageInfos, will resolve to nothing.
In both cases it will invoke onResult/onError, so if you pass an array you can use onResult.
in both cases it will reject if all insertions fail.
This is basically a merge of insert and insertMany. The advantage is a single method name to recall, the disadvantage is maybe over-engineering.
Those are a lot of good ideas, Marco. Thanks for the discussion! What I would suggest is going with two methods, one for single insert and one for multiple. What you expect to get back, as well as what you pass in would be different in each case, so I think it would be easier to understand to the consumer if there are two methods. They would work as you suggested above:

insert: accepts a single pageInfo and no callback. Resolves with the resulting pageInfo object (populated with guid, etc) after the insert is complete and rejects if not.
insertMultiple: accepts an array of pageInfo objects (which could potentially only include one, but should be at least one), an onResult callback and an onError callback. Resolves with `true` if any objects were inserted and rejects if no inserts occurred. onResult will be passed the resulting (populated) pageInfo object and onError will be passed the original pageInfo object.

Does that all make sense?

Also, now that we're making this strictly insert rather than update, does it make sense to accept a guid in the passed-in pageInfo object? I'm thinking no.
(In reply to Bob Silverberg [:bsilverberg] from comment #19)
> insert: accepts a single pageInfo and no callback. Resolves with the
> resulting pageInfo object (populated with guid, etc) after the insert is
> complete and rejects if not.
> insertMultiple: accepts an array of pageInfo objects (which could
> potentially only include one, but should be at least one), an onResult
> callback and an onError callback. Resolves with `true` if any objects were
> inserted and rejects if no inserts occurred. onResult will be passed the
> resulting (populated) pageInfo object and onError will be passed the
> original pageInfo object.
> 
> Does that all make sense?

I'm sold!
The only detail I'd still investigate is the boolean return, whether to make it always true, or true if no errors, false if any error.

> Also, now that we're making this strictly insert rather than update, does it
> make sense to accept a guid in the passed-in pageInfo object? I'm thinking
> no.

At the beginning I'd leave that out, we can re-evaluate if Sync ends up really needing it. The name doesn't matter much for that decision, guid would still be a minor detail.
note, both of the callbacks should be optional, clearly.
(In reply to Marco Bonardo [::mak] from comment #20)
> (In reply to Bob Silverberg [:bsilverberg] from comment #19)
> > insert: accepts a single pageInfo and no callback. Resolves with the
> > resulting pageInfo object (populated with guid, etc) after the insert is
> > complete and rejects if not.
> > insertMultiple: accepts an array of pageInfo objects (which could
> > potentially only include one, but should be at least one), an onResult
> > callback and an onError callback. Resolves with `true` if any objects were
> > inserted and rejects if no inserts occurred. onResult will be passed the
> > resulting (populated) pageInfo object and onError will be passed the
> > original pageInfo object.
> > 
> > Does that all make sense?
> 
> I'm sold!
> The only detail I'd still investigate is the boolean return, whether to make
> it always true, or true if no errors, false if any error.
> 

I'm not sure what makes the most sense for that. Is there anything similar in Places with which we should try to be consistent?

> > Also, now that we're making this strictly insert rather than update, does it
> > make sense to accept a guid in the passed-in pageInfo object? I'm thinking
> > no.
> 
> At the beginning I'd leave that out, we can re-evaluate if Sync ends up
> really needing it. The name doesn't matter much for that decision, guid
> would still be a minor detail.

Okay, so you're saying do not accept guid as input in a pageInfo object for now, only url, correct?
(In reply to Bob Silverberg [:bsilverberg] from comment #22)
> (In reply to Marco Bonardo [::mak] from comment #20)
> > The only detail I'd still investigate is the boolean return, whether to make
> > it always true, or true if no errors, false if any error.
> > 
> 
> I'm not sure what makes the most sense for that. Is there anything similar
> in Places with which we should try to be consistent?

History.remove does something similar
209    * @resolve (bool)
210    *      `true` if at least one page was removed, `false` otherwise.

but it sounds like the opposite, it may be confusing
Maybe we should just resolve() to nothing... Why do we need to resolve to a bool?

> Okay, so you're saying do not accept guid as input in a pageInfo object for
> now, only url, correct?

yes, for now let's go with that
(In reply to Marco Bonardo [::mak] from comment #23)
> (In reply to Bob Silverberg [:bsilverberg] from comment #22)
> > (In reply to Marco Bonardo [::mak] from comment #20)
> > > The only detail I'd still investigate is the boolean return, whether to make
> > > it always true, or true if no errors, false if any error.
> > > 
> > 
> > I'm not sure what makes the most sense for that. Is there anything similar
> > in Places with which we should try to be consistent?
> 
> History.remove does something similar
> 209    * @resolve (bool)
> 210    *      `true` if at least one page was removed, `false` otherwise.
> 
> but it sounds like the opposite, it may be confusing
> Maybe we should just resolve() to nothing... Why do we need to resolve to a
> bool?

Good point. :) Nothing it is!

> 
> > Okay, so you're saying do not accept guid as input in a pageInfo object for
> > now, only url, correct?
> 
> yes, for now let's go with that

Great, thanks. :)
https://reviewboard.mozilla.org/r/50217/#review47509

> We should also support URL, not just URI or spec. You can use instanceof URL for that.
> Refer to the Bookmarks.jsm code if you need a reference.
> 
> Also, I think we should not modify the passed-into object, to avoid bugs in the caller in case it reuses the object. let's build our own internal object

I'm looking at the logic for URLs in Bookmarks.jsm. It looks like we should probably do the same complex validation of URLs as is done at https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#1352, and I notice there is also validation for `title` in there too.

Do you think we should move `simpleValidateFunc` and the `VALIDATORS` const to PlacesUtils.jsm so that logic can be shared between Bookmarks.jsm and History.jsm? Or are you suggesting I just copy some of the logic from Bookmarks.jsm into History.jsm?
for now I'd just copy the parts you care about, I don't think we are at the point we can write a well tested and generic enough input parser.
Comment on attachment 8748336 [details]
MozReview Request: Bug 1265836 - Part 3: Implement insert and insertMany in History.jsm, r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50217/diff/2-3/
Attachment #8748336 - Attachment description: MozReview Request: Bug 1265836 - Part 1: Implement update in History.jsm, r?mak → MozReview Request: Bug 1265836 - Part 1: Implement insert and insertMultiple in History.jsm, r?mak
Attachment #8748336 - Flags: review?(mak77)
https://reviewboard.mozilla.org/r/50217/#review47509

> This should also manage errors and reject in case.
> 
> handleResult is invoked when a placeInfo is properly inserted, handleError when a placeInfo is not inserted
> 
> when handleError is invoked we should collect the failing urls, then in handleCompletion, if some urls failed we should reject an array of the failed urls (or whatever else we decide to reject)

As we discussed, I am now rejecting if *no* visits have been added when calling `insertMultiple`.

> Also needs some reject tests, I think it's possible to do that passing some uri that doesn's pass the canAddURI check mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#1091

I'm not sure how to test `onError` or rejection as I cannot figure out how to construct a `PageInfo` object that will pass the validation that I've built into `insert` and then generate an error when passed into `asyncHistory.updatePlaces`. I added validation for `canAddURI` into `insert`, so that won't work, unless I remove that validation, but I think it's good to have in `insert`. Can you think of a way of constructing test data that would generate an error?
https://reviewboard.mozilla.org/r/50217/#review48211

::: toolkit/components/places/tests/history/test_insert.js:254
(Diff revision 3)
> +      Assert.equal(expected, yield PlacesTestUtils.visitsInDB(url), `visitsInDB for ${url} is ${expected}`);
> +    }
> +  });
> +
> +  try {
> +    // FIXME: Find a way to test onError

As mentioned above, I'm not sure how to generate test data to test `onError` and am hoping you might have a suggestion for this.
I had a thought about the WebExtensions API for this. Chrome's `addUrl` method accepts a callback, but returns nothing to that callback. It seems to me that it would be more useful if we were to return the created `HistoryItem`. This would diverge from what Chrome does, but would be, imo, an improvement. I don't think it would break anything for people who want to use the API the same as Chrome's (i.e., do not expect an argument passed to the callback), so I'm not sure if there is a downside.

From an implementation perspective it would be easy to do with the new History.insert method that we're adding.

What do people think about making this change to our version of the API? Kris, Andy, Andrew?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(aswan)
Flags: needinfo?(amckay)
FTR, we're already diverging from Chrome by allowing people to pass in a title, date and transition in addition to the `url` that Chrome accepts.
Blocks: 1271675
Iteration: --- → 49.2 - May 23
(In reply to Bob Silverberg [:bsilverberg] from comment #30)
> I had a thought about the WebExtensions API for this. Chrome's `addUrl`
> method accepts a callback, but returns nothing to that callback. It seems to
> me that it would be more useful if we were to return the created
> `HistoryItem`. This would diverge from what Chrome does, but would be, imo,
> an improvement. I don't think it would break anything for people who want to
> use the API the same as Chrome's (i.e., do not expect an argument passed to
> the callback), so I'm not sure if there is a downside.
> 
> From an implementation perspective it would be easy to do with the new
> History.insert method that we're adding.
> 
> What do people think about making this change to our version of the API?
> Kris, Andy, Andrew?

From a quick glance at the history docs, I don't really see what value the HistoryItem would have to the caller...
Flags: needinfo?(aswan)
(In reply to Andrew Swan [:aswan] from comment #32)
> (In reply to Bob Silverberg [:bsilverberg] from comment #30)
> > I had a thought about the WebExtensions API for this. Chrome's `addUrl`
> > method accepts a callback, but returns nothing to that callback. It seems to
> > me that it would be more useful if we were to return the created
> > `HistoryItem`. 
> > 
> > What do people think about making this change to our version of the API?
> > Kris, Andy, Andrew?
> 
> From a quick glance at the history docs, I don't really see what value the
> HistoryItem would have to the caller...

Fair enough. It will give them the id that was generated, but it's questionable as to whether that would have value to them. I wanted to add it so that I could use it in my tests, to verify that what I get back from the method is what I expect, but there are other ways to do that. What do others think?
I don't really see an advantage either, but it is in line with most of the other APIs. Generally, the call that creates something also returns a representation of the thing that was actually created.
(In reply to Bob Silverberg [:bsilverberg] from comment #33)
> Fair enough. It will give them the id that was generated, but it's
> questionable as to whether that would have value to them. I wanted to add it
> so that I could use it in my tests, to verify that what I get back from the
> method is what I expect, but there are other ways to do that. What do others
> think?

If we're going to change the API to return something, I'd much rather it be the visit object than just the ID.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(amckay)
Comment on attachment 8748336 [details]
MozReview Request: Bug 1265836 - Part 3: Implement insert and insertMany in History.jsm, r?mak

https://reviewboard.mozilla.org/r/50217/#review48717

::: toolkit/components/places/History.jsm:135
(Diff revision 3)
>    fetch: function (guidOrURI) {
>      throw new Error("Method not implemented");
>    },
>  
>    /**
> -   * Adds a set of visits for one or more page.
> +   * Adds a single visit for a page.

Doesn't this allow to add a set of visits for one page, rather than just one visit?

::: toolkit/components/places/History.jsm:237
(Diff revision 3)
>     *      If an element of `visits` has an invalid `date`.
>     * @throws (Error)
>     *      If an element of `visits` is missing `transition` or if
>     *      the value of `transition` is invalid.
>     */
> -  update: function (infos, onResult) {
> +  insertMultiple: function (pageInfos, onResult, onError) {

I suggested insertMany instead of insertMultiple, first cause it's shorter and short APIs are easier to remember, second cause we have a previous API (onManyFrecenciesChanged();) using Many.

Though, I'm not English mother tongue, so it's likely I'm wrong. Could Many be misinterpreted like it should only be used for a lot of entries and not for 2 or 3? is that the reason you went for Multiple?

::: toolkit/components/places/History.jsm:472
(Diff revision 3)
> + * @param pageInfo: (PageInfo)
> + * @return (PageInfo)
> + */
> +function validatePageInfo(pageInfo) {
> +  let info = {
> +    title: pageInfo.title,

One could pass null or undefined, or something that is not a string.
We must verify this is either null, undefined or a string. if it's a non-empty string, we pass it through.
if it's an empty string, null or undefined, we continue but don't set the property in the output object, so all of the 3 have the same behavior.
Otherwise we throw.

just to be a little bit more coherent with bookmarks https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#36

::: toolkit/components/places/History.jsm:485
(Diff revision 3)
> +  info.uri = validateUrl(pageInfo.url);
> +  if (!info.uri) {
> +    throw new TypeError(`url property of PageInfo object: ${pageInfo.url} is not a valid url`);
> +  }
> +
> +  if (!PlacesUtils.history.canAddURI(info.uri)) {

I'd let this pass-through. For various reasons:
1. this will allow us to test failure cases more easily
2. the consumer may have a list of uris taken from somewhere, the values don't matter, the fact one of those can't be added shouldn't prevent adding the others
3. the canAddURI list could evolve in time, and then it would not be a coding error, thus I think we should not throw.

::: toolkit/components/places/History.jsm:494
(Diff revision 3)
> +  if (!pageInfo.visits || !Array.isArray(pageInfo.visits) || !pageInfo.visits.length) {
> +    throw new TypeError("PageInfo object must have an array of visits");
> +  }
> +  for (let inVisit of pageInfo.visits) {
> +    let visit = {
> +      visitDate: Date.now(),

Date.now() is a timestamp, but visitDate must be a Date object.

::: toolkit/components/places/History.jsm:496
(Diff revision 3)
> +  }
> +  for (let inVisit of pageInfo.visits) {
> +    let visit = {
> +      visitDate: Date.now(),
> +      transitionType: inVisit.transition,
> +      referrerURI: validateUrl(inVisit.referrer),

what if the consumer doesn't pass referrer?
I honestly think validateUrl should throw for anything that is not an url, otherwise it may hide bugs.

Also, we got `referrer` in input, we should not return `referrerURI` to the caller...

I think we must decouple input vaidation and transformation into 2 separate methods.
First we validata user input and we build an object that is coherent with History.jsm API (url is an URL, visitDate is a Date, referrer is an URL)...
This is the object we return to the caller, an object he can be reused to pass it to another History.jsm API. Object reuse is important here, if we transform it in a way it loses info, it's not ok.

Later in code we may need an util to convert this object into an updatePlaces valid object, this is an implementation detail that doesn't matter to the external consumer and in future we could change it, by changing updatePlaces or directly implementing it in js.

::: toolkit/components/places/History.jsm:499
(Diff revision 3)
> +      visitDate: Date.now(),
> +      transitionType: inVisit.transition,
> +      referrerURI: validateUrl(inVisit.referrer),
> +    };
> +
> +    if (!inVisit.transition) {

I think, like addUrl, we should make transition optional and default to TRANSITION_LINK

::: toolkit/components/places/History.jsm:553
(Diff revision 3)
> +  } else if (typeof(url) === "string") {
> +    return NetUtil.newURI(url);
> +  } else if (url instanceof URL) {
> +    return PlacesUtils.toURI(url);
> +  } else {
> +    return undefined;

As I said, I think we should be stricter.

::: toolkit/components/places/History.jsm:944
(Diff revision 3)
> +    PlacesUtils.asyncHistory.updatePlaces(pageInfo, {
> +      handleError: error => {
> +        reject(error);
> +      },
> +      handleResult: pageInfo => {
> +        resultPageInfo = pageInfo;

unfortunately, this object is not API compatible with PageInfo, so we'll need some abstraction (that hopefully will disappear in the future)

::: toolkit/components/places/tests/history/test_insert.js:9
(Diff revision 3)
> +// Tests for `History.insert`, as implemented in History.jsm
> +
> +"use strict";
> +
> +add_task(function* cleanup() {
> +  yield PlacesTestUtils.clearHistory();

should not be needed at the test start, cause every xpcshell test runs with its own profile
Attachment #8748336 - Flags: review?(mak77)
Sorry, lots of comments. Designing APIs is not easy :/

On the other side, this looks great, thank you for doing this, we could move a lot (if not all) internal consumers to this. And thank you for moving stuff to PlacesUtils, that will be generally useful to all the other code.
Comment on attachment 8748336 [details]
MozReview Request: Bug 1265836 - Part 3: Implement insert and insertMany in History.jsm, r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50217/diff/3-4/
Attachment #8748336 - Attachment description: MozReview Request: Bug 1265836 - Part 1: Implement insert and insertMultiple in History.jsm, r?mak → MozReview Request: Bug 1265836 - Part 1: Implement insert and insertMany in History.jsm, r?mak
Attachment #8748336 - Flags: review?(mak77)
https://reviewboard.mozilla.org/r/50217/#review47509

> I'm not sure how to test `onError` or rejection as I cannot figure out how to construct a `PageInfo` object that will pass the validation that I've built into `insert` and then generate an error when passed into `asyncHistory.updatePlaces`. I added validation for `canAddURI` into `insert`, so that won't work, unless I remove that validation, but I think it's good to have in `insert`. Can you think of a way of constructing test data that would generate an error?

Now that I removed the check for `canAddURI` I was able to add a reject test. I think there's only one scenario where this method rejects (`insertMany` when no items are inserted) so I only added one test. Let me know if there are others you think need to be added.
https://reviewboard.mozilla.org/r/50217/#review48717

> Doesn't this allow to add a set of visits for one page, rather than just one visit?

Good catch. Fixed.

> I suggested insertMany instead of insertMultiple, first cause it's shorter and short APIs are easier to remember, second cause we have a previous API (onManyFrecenciesChanged();) using Many.
> 
> Though, I'm not English mother tongue, so it's likely I'm wrong. Could Many be misinterpreted like it should only be used for a lot of entries and not for 2 or 3? is that the reason you went for Multiple?

No, you're not wrong, insertMany is fine. I'm not sure why I changed it to `insertMultiple`. I've fixed this.
(In reply to Marco Bonardo [::mak] from comment #37)
> Sorry, lots of comments. Designing APIs is not easy :/
> 
> On the other side, this looks great, thank you for doing this, we could move
> a lot (if not all) internal consumers to this. And thank you for moving
> stuff to PlacesUtils, that will be generally useful to all the other code.

No need to apologize, Marco. I appreciate all of your comments. I am learning a lot and it is helping me to improve the code. I really appreciate the time you are taking to give me such detailed comments. I think this will end up being a nice API once we're done. :)
Comment on attachment 8748336 [details]
MozReview Request: Bug 1265836 - Part 3: Implement insert and insertMany in History.jsm, r?mak

https://reviewboard.mozilla.org/r/50217/#review49107

almost there!

::: toolkit/components/places/History.jsm:140
(Diff revision 4)
> -   * Adds a set of visits for one or more page.
> +   * Adds a number of visits for a single page.
>     *
>     * Any change may be observed through nsINavHistoryObserver
>     *
>     * @note This function recomputes the frecency of the page automatically,
>     * regardless of the value of property `frecency` passed as argument.

these comments look outdated, there is no frecency argument... just remove both of the @notes, none of them look useful.

::: toolkit/components/places/History.jsm:151
(Diff revision 4)
>     *        - a property `visits`, as specified by the definition of
>     *          `PageInfo`, which MUST contain at least one visit.
>     *      If a property `title` is provided, the title of the page
>     *      is updated.
>     *      If the `visitDate` of a visit is not provided, it defaults
>     *      to now.

Add a comment regarding default value for transition, also to the Many API.

::: toolkit/components/places/History.jsm:175
(Diff revision 4)
> +   *      If `pageInfo` does not have a `visits` property or if the
> +   *      value of `visits` is ill-typed or is an empty array.
> +   * @throws (Error)
> +   *      If an element of `visits` has an invalid `date`.
> +   * @throws (Error)
> +   *      If an element of `visits` is missing `transition` or if

I think the former is no more true if we make TRANSITION_LINK the default value

::: toolkit/components/places/History.jsm:187
(Diff revision 4)
> +
> +    let info = validatePageInfo(pageInfo);
> +    let updateInfo = convertForUpdatePlaces(info);
> +
> +    return PlacesUtils.withConnectionWrapper("History.jsm: insert",
> +      db => insert(db, info, updateInfo));

just pass info, and do the conversion inside the implementation method.

So it gets a pageInfo and returns a pageInfo.
Will make simpler to change in future.

::: toolkit/components/places/History.jsm:195
(Diff revision 4)
> +  /**
> +   * Adds a number of visits for a number of pages.
> +   *
> +   * Any change may be observed through nsINavHistoryObserver
> +   *
> +   * @note This function recomputes the frecency of the page automatically,

ditto

::: toolkit/components/places/History.jsm:212
(Diff revision 4)
> +   *      to now.
> +   * @param onResult: (function(PageInfo))
> +   *      A callback invoked for each page inserted.
> +   * @param onError: (function(PageInfo))
> +   *      A callback invoked for each page which generated an error
> +   *      when am insert was attempted.

typo: am

::: toolkit/components/places/History.jsm:235
(Diff revision 4)
>     *      If a `PageInfo` does not have a `visits` property or if the
>     *      value of `visits` is ill-typed or is an empty array.
>     * @throws (Error)
>     *      If an element of `visits` has an invalid `date`.
>     * @throws (Error)
>     *      If an element of `visits` is missing `transition` or if

ditto

::: toolkit/components/places/History.jsm:257
(Diff revision 4)
> +      throw new TypeError(`onError: ${onError} is not a valid function`);
> +    }
> +
> +    for (var pageInfo of pageInfos) {
> +      let info = validatePageInfo(pageInfo);
> +      let updateInfo = convertForUpdatePlaces(info);

I'd probably pass the pageInfo style object to insertMany, and get back a pageInfo style object

The conversion for updatePlaces is an intern detail of insertMany implementation, can happen inside it

::: toolkit/components/places/History.jsm:950
(Diff revision 4)
> +      handleError: error => {
> +        reject(error);
> +      },
> +      handleResult: () => {},
> +      handleCompletion: () => {
> +        resolve(pageInfo);

you must "merge" the return objects, adding guid to the pageInfo style object...

pageInfo -> updateInfo -> updatePlaces -> updateInfo -> pageInfo

There should be no need for a conversion, just do info.guid = pageInfo.guid and so on... you can have a mergeUpdateInfoToPageInfo helper for that.

::: toolkit/components/places/History.jsm:967
(Diff revision 4)
> +    PlacesUtils.asyncHistory.updatePlaces(pageInfos, {
> +      handleError: (resultCode, pageInfo) => {
> +        onErrorData.push(pageInfo);
> +      },
> +      handleResult: pageInfo => {
> +        onResultData.push(pageInfo);

ditto

::: toolkit/components/places/tests/history/test_insert.js:107
(Diff revision 4)
> +  );
> +});
> +
> +add_task(function* test_history_insert() {
> +  const TEST_URL = "http://mozilla.com/";
> +  yield PlacesTestUtils.addVisits(TEST_URL);

you are adding a new API to add visits, I guess it should be used everywhere here, rather than using existing helpers. So this also increases coverage.

And I'd also like to change PromiseAddVisits internals to use PlacesUtils.history.insert... That will largely increase test coverage!

::: toolkit/components/places/tests/history/test_insert.js:119
(Diff revision 4)
> +    let title = "Visit " + Math.random();
> +
> +    let pageInfo = {
> +      title,
> +      visits: [
> +        {transition: TRANSITION_LINK, referrer: referrer, date: date,}

need a test that doesnt pass a transition and check the resulting object has transition_link

::: toolkit/components/places/tests/history/test_insert.js:138
(Diff revision 4)
> +    }
> +    if (date) {
> +      Assert.equal(Number(date),
> +                   Number(result.visits[0].date),
> +                   "date of visit is correct");
> +    }

check the result has a valid guid?

::: toolkit/components/places/tests/history/test_insert.js:221
(Diff revision 4)
> +      let onErrorUrls = [];
> +      result = yield PlacesUtils.history.insertMany(pageInfos, pageInfo => {
> +        let url = pageInfo.uri.spec;
> +        Assert.ok(GOOD_URLS.includes(url), "onResult callback called for correct uri");
> +        onResultUrls.push(url);
> +        Assert.equal(`Visit to ${url}`, pageInfo.title, "onResult callback provides the correct title");

check guids, as well
Attachment #8748336 - Flags: review?(mak77)
Comment on attachment 8748336 [details]
MozReview Request: Bug 1265836 - Part 3: Implement insert and insertMany in History.jsm, r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50217/diff/4-5/
Attachment #8748336 - Flags: review?(mak77)
https://reviewboard.mozilla.org/r/50217/#review49107

> you are adding a new API to add visits, I guess it should be used everywhere here, rather than using existing helpers. So this also increases coverage.
> 
> And I'd also like to change PromiseAddVisits internals to use PlacesUtils.history.insert... That will largely increase test coverage!

Actually, that line is cruft from when the function used to accept a `guid` as well. We have no need to add any history outside of the `inserter` function, so I'm just going to remove it. I will change `PromiseAddVisits` to use the new `insert` method though. Should I do that as part of this patch, or should we file a follow-up bug?
(In reply to Bob Silverberg [:bsilverberg] from comment #44)
> Actually, that line is cruft from when the function used to accept a `guid`
> as well. We have no need to add any history outside of the `inserter`
> function, so I'm just going to remove it. I will change `PromiseAddVisits`
> to use the new `insert` method though. Should I do that as part of this
> patch, or should we file a follow-up bug?

At this point I think better a follow-up.
You don't have to fix everything, but I'd like one bug to deprecate PlacesUtils.promiseUpdatePlace in favor of PlacesUtils.history.insert, and one bug to change PlacesTestUtils.addVisits to use PlacesUtils.history.insertMany internally.

Long term, I'd like to restrict direct calls to updatePlaces to only those code points that really-really need it and have everything else go through PlacesUtils.history.insert(Many)
Comment on attachment 8748336 [details]
MozReview Request: Bug 1265836 - Part 3: Implement insert and insertMany in History.jsm, r?mak

https://reviewboard.mozilla.org/r/50217/#review49383

LGTM! thank you for following all the comments.

I think this is good enough to proceed, we can handle leftovers in follow-ups when issues will be found. It's possible converting existing methods to this new API will undisclose something, but it's not critical to be perfect at this stage.

::: toolkit/components/places/PlacesUtils.jsm:269
(Diff revision 5)
> +   * Is a string a valid GUID?
> +   *
> +   * @param guid: (String)
> +   * @return (Boolean)
> +   */
> +  isValidGUID: function PU_isValidGUID(guid) {

nit: as I said in the other bug, you could use ES6 shorthands here, so just isValidGuid(guid)

in new js implemented Places APIs we are trying to use CamelCase for guid, so please use Guid in code and GUID in comments. This is more consistent with how we always wrote Id and ItemId.
I know this is not consistent with legacy cpp APIs, but I hope one day we'll reach that point.

::: toolkit/components/places/PlacesUtils.jsm:282
(Diff revision 5)
> +   *        the URL object to convert.
> +   * @return nsIURI for the given URL.
> +   */
> +  toURI: function PU_toURI(url) {
> +    return NetUtil.newURI(url.href);
> +  },

what if url is a string? At this point, let's make this more generic and support converting both a string or an URL.
Attachment #8748336 - Flags: review?(mak77) → review+
Comment on attachment 8748336 [details]
MozReview Request: Bug 1265836 - Part 3: Implement insert and insertMany in History.jsm, r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50217/diff/5-6/
Blocks: 1272684
Blocks: 1272686
https://reviewboard.mozilla.org/r/50217/#review49107

> Actually, that line is cruft from when the function used to accept a `guid` as well. We have no need to add any history outside of the `inserter` function, so I'm just going to remove it. I will change `PromiseAddVisits` to use the new `insert` method though. Should I do that as part of this patch, or should we file a follow-up bug?

I opened bug 1272684 and bug 1272686 as follow-ups.
(In reply to Kris Maglione [:kmag] from comment #35)
> (In reply to Bob Silverberg [:bsilverberg] from comment #33)
> > Fair enough. It will give them the id that was generated, but it's
> > questionable as to whether that would have value to them. I wanted to add it
> > so that I could use it in my tests, to verify that what I get back from the
> > method is what I expect, but there are other ways to do that. What do others
> > think?
> 
> If we're going to change the API to return something, I'd much rather it be
> the visit object than just the ID.

Getting back to the discussion about what, if anything, the `addUrl` method should return, it does make sense that we'd return a `VisitItem` rather than a `HistoryItem`, based on the types available for the Chrome API. However, the current code for PlacesUtils.History.insert does not return most of what we'd need for a `VisitItem` [1]. All we'd really get is the unique `id` of the page and the `visitTime`. It would be missing the `visitId`, `referringVisitId` and `transition`. We'd have to do a separate lookup to get those, which doesn't seem worthwhile considering that the usefulness of having this info by the caller is questionable. So I think it makes sense to go back to what Chrome does and just return nothing. Please let me know if you disagree, otherwise I will proceed with that.

[1] https://developer.chrome.com/extensions/history#type-VisitItem
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(aswan)
(In reply to Bob Silverberg [:bsilverberg] from comment #49)
> So I think it makes sense to
> go back to what Chrome does and just return nothing. Please let me know if
> you disagree, otherwise I will proceed with that.

sounds good to me.
Flags: needinfo?(aswan)
Note that this includes changes to ExtensionUtils.jsm which will likely land via bug 1265834 before this, so I will rebase it at that time.
I also plan to add more tests which will retrieve the history via browser.history.search to confirm it has been added with the correct data, again once bug 1265834 lands.

Review commit: https://reviewboard.mozilla.org/r/52946/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52946/
Comment on attachment 8748336 [details]
MozReview Request: Bug 1265836 - Part 3: Implement insert and insertMany in History.jsm, r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50217/diff/6-7/
Attachment #8753027 - Flags: review?(aswan)
Comment on attachment 8753027 [details]
MozReview Request: Bug 1265836 - Part 4: Implement browser.history.addUrl, r?aswan

https://reviewboard.mozilla.org/r/52946/#review49804

::: browser/components/extensions/ext-history.js:7
(Diff revision 1)
>  /* vim: set sts=2 sw=2 et tw=80: */
>  "use strict";
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
>  XPCOMUtils.defineLazyGetter(this, "History", () => {

Since History is referenced at the top level, there's really no benefit to having this be lazy...

::: browser/components/extensions/ext-history.js:25
(Diff revision 1)
> +function getTransitionType(transition) {
> +  let transitionType = History.TRANSITION_LINK;
> +  if (transition) {
> +    let transitionType = TRANSITION_TO_TRANSITION_TYPES_MAP.get(transition);
> +    if (!transitionType) {
> +      throw new Error(`|${transition}| is not a supported transition for history`);
> +    }
> +  }
> +  return transitionType;
> +}

This would be shorter and I think easier to follow if you just provided a default value of "link" for transition.

::: browser/components/extensions/ext-history.js:47
(Diff revision 1)
> +          transition = getTransitionType(details.transition);
> +        } catch (error) {
> +          return Promise.reject({message: error.message});
> +        }
> +        if (details.visitTime) {
> +          date = new Date(normalizeTime(details.visitTime));

This feels clunky, normalizeTime() creates a Date object then calls valueOf() and the object is discarded but this code immediately creates a new Date object.
I think the sensible thing would be for normalizeTime() to return a Date object and callers can call valueOf() if they prefer a number.

::: browser/components/extensions/schemas/history.json:199
(Diff revision 1)
>                "url": {
>                  "type": "string",
>                  "description": "The URL to add."
> +              },

The docs don't really specify anything, but seems like we should have a format property here to constrain this to something that looks like a valid URL?
Ah, reading further I see you test for this so I guess this is checked down inside History instead of here.  That seems okay to me, but perhaps document the constraint here?

::: browser/components/extensions/schemas/history.json:206
(Diff revision 1)
>                  "description": "The URL to add."
> +              },
> +              "title": {
> +                "type": "string",
> +                "optional": true,
> +                "description": "The URL to add."

copypasta?

::: browser/components/extensions/test/browser/browser_ext_history.js:117
(Diff revision 1)
> +      details.title = `Title for ${type}`;
> +      let options = {details, type};
> +      return options;
> +    }
> +
> +    function sendPostAddMessage(options) {

I'm finding this whole test case a little confusing.  For starters, why bother sending a message after each call to addUrl()?  You're not doing any sort of synchronization between the background script and the main test code so you could just send a single message after you've made all the calls and then check everything at once.
I think that would let you simplify the options/details code here too.

::: browser/components/extensions/test/browser/browser_ext_history.js:204
(Diff revision 1)
> +  function* checkAddUrl(type) {
> +    let testUrl = yield extension.awaitMessage(type);
> +    ok(yield PlacesTestUtils.isPageInDB(testUrl), `${testUrl} found in history database`);
> +  }

Can you check title and visitTime too?
Attachment #8753027 - Flags: review?(aswan)
Comment on attachment 8753027 [details]
MozReview Request: Bug 1265836 - Part 4: Implement browser.history.addUrl, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52946/diff/1-2/
Attachment #8753027 - Flags: review?(aswan)
Comment on attachment 8753027 [details]
MozReview Request: Bug 1265836 - Part 4: Implement browser.history.addUrl, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52946/diff/2-3/
Attachment #8753027 - Attachment description: MozReview Request: Bug 1265836 - Part 2: Implement browser.history.addUrl, f?aswan → MozReview Request: Bug 1265836 - Part 2: Implement browser.history.addUrl, r?aswan
https://reviewboard.mozilla.org/r/52946/#review49804

> This would be shorter and I think easier to follow if you just provided a default value of "link" for transition.

Good idea. I was unable to simply provide a default value for the `transition` argument in the function definition because the framework sets it to `null` which isn't the same as `undefined` so the default doesn't override `null`. Instead I added a one liner to set the default at the top of the function body.

> This feels clunky, normalizeTime() creates a Date object then calls valueOf() and the object is discarded but this code immediately creates a new Date object.
> I think the sensible thing would be for normalizeTime() to return a Date object and callers can call valueOf() if they prefer a number.

That makes sense. I've changed it. We'll need to change it in downloads as well, and also the code that's about to land for `history.search`, so I'll leave this issue open and add a new commit to this bug once those changes land.

> Can you check title and visitTime too?

Unfortunately I cannot at the moment because I don't get anything back from the method and we don't have a simple way of getting back information about an individual history item at this time. I plan to add more tests once `history.search` lands, as I mentioned in the commit message, so I will leave this issue open to track the fact that that needs to be done.
https://reviewboard.mozilla.org/r/52946/#review49804

> Good idea. I was unable to simply provide a default value for the `transition` argument in the function definition because the framework sets it to `null` which isn't the same as `undefined` so the default doesn't override `null`. Instead I added a one liner to set the default at the top of the function body.

Okay, or you could just punt on it here and do it as part of 1272676

> Unfortunately I cannot at the moment because I don't get anything back from the method and we don't have a simple way of getting back information about an individual history item at this time. I plan to add more tests once `history.search` lands, as I mentioned in the commit message, so I will leave this issue open to track the fact that that needs to be done.

I don't understand, you're back in chrome context here so you can use PlacesTestUtils or use Places directly, I don't see what history.search() has to do with it.
https://reviewboard.mozilla.org/r/52946/#review49804

> Okay, or you could just punt on it here and do it as part of 1272676

You've lost me. I don't know what this issue of default values for functions has to do with bug 1272676. There is another bug open which is meant to allow us to specify default values for things in the schema, which is bug 1262250. Is that the one you meant to reference?

> I don't understand, you're back in chrome context here so you can use PlacesTestUtils or use Places directly, I don't see what history.search() has to do with it.

There's nothing in PlacesTestUtils that will fetch a history record for a URL, nor is there in History.jsm. There is a stub function in History.jsm called `fetch` which is destined to do this, but it hasn't been implemented yet. So it looked like there wasn't a simple way (i.e., via an API) to get that info. I've looked through all of PlacesUtils.jsm and found `promisePlaceInfo` [1] which looks promising (pun intended), so maybe that will do the trick. I'll try that. 

Although it is certainly possible, I think we want to avoid writing code that requires low level access to the Places db in test code, especially when we will have a friendly way of doing it once our search API lands.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#1608
https://reviewboard.mozilla.org/r/52946/#review49804

> You've lost me. I don't know what this issue of default values for functions has to do with bug 1272676. There is another bug open which is meant to allow us to specify default values for things in the schema, which is bug 1262250. Is that the one you meant to reference?

whoops, I hit the wrong reply button, that comment was meant to be about the changes to normalizeTime()

> There's nothing in PlacesTestUtils that will fetch a history record for a URL, nor is there in History.jsm. There is a stub function in History.jsm called `fetch` which is destined to do this, but it hasn't been implemented yet. So it looked like there wasn't a simple way (i.e., via an API) to get that info. I've looked through all of PlacesUtils.jsm and found `promisePlaceInfo` [1] which looks promising (pun intended), so maybe that will do the trick. I'll try that. 
> 
> Although it is certainly possible, I think we want to avoid writing code that requires low level access to the Places db in test code, especially when we will have a friendly way of doing it once our search API lands.
> 
> [1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#1608

Is `History.fetch()` the thing that `browser.history.search()` is going to use?  If you're going to be doing that anyway, why not do that first and then complete this and land it second?
https://reviewboard.mozilla.org/r/52946/#review49804

> That makes sense. I've changed it. We'll need to change it in downloads as well, and also the code that's about to land for `history.search`, so I'll leave this issue open and add a new commit to this bug once those changes land.

Replying to the comment above, which was added to the wrong line:

Ah, that makes more sense. :)  I'd like to see this land before bug 1272676, on which I haven't even started yet, so better to make sure this one is clean, I think. Bug 1265834, which includes the initial change to the downloads code, has landed on inbound, so I expect it to land fully soon, at which point I will add the commit I was talking about above.

> Is `History.fetch()` the thing that `browser.history.search()` is going to use?  If you're going to be doing that anyway, why not do that first and then complete this and land it second?

No, `History.fetch` isn't currently needed, so I don't have immediate plans to implement it. It _may_ be needed for the `onVisited` event, but that's still being discussed. This is why my plan is to just use `browser.history.search` to do the extra checks in the tests for `addUrl`, which is also why I timed it to land first. It has finally landed (at least to inbound for now), so hopefully today it will fully land and I can rebase this patch and use `history.search` to beef up the tests, at which point this can land too.
This change is unrelated to the bug, but other changes being implemented for the bug have broken the existing test.
Rather than hack the test and open a separate bug to fix it later and improve deleteRange, I thought it
worthwhile to include this minor patch in the set of patches for the bug.

Review commit: https://reviewboard.mozilla.org/r/53646/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53646/
Comment on attachment 8748336 [details]
MozReview Request: Bug 1265836 - Part 3: Implement insert and insertMany in History.jsm, r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50217/diff/7-8/
Comment on attachment 8753027 [details]
MozReview Request: Bug 1265836 - Part 4: Implement browser.history.addUrl, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52946/diff/3-4/
https://reviewboard.mozilla.org/r/52946/#review49804

> Replying to the comment above, which was added to the wrong line:
> 
> Ah, that makes more sense. :)  I'd like to see this land before bug 1272676, on which I haven't even started yet, so better to make sure this one is clean, I think. Bug 1265834, which includes the initial change to the downloads code, has landed on inbound, so I expect it to land fully soon, at which point I will add the commit I was talking about above.

I've made the changes discussed above. I had to fix a few other things related to changes that landed in bug 1265834, and I've done those in separate small commits.

> No, `History.fetch` isn't currently needed, so I don't have immediate plans to implement it. It _may_ be needed for the `onVisited` event, but that's still being discussed. This is why my plan is to just use `browser.history.search` to do the extra checks in the tests for `addUrl`, which is also why I timed it to land first. It has finally landed (at least to inbound for now), so hopefully today it will fully land and I can rebase this patch and use `history.search` to beef up the tests, at which point this can land too.

I've added checks for title and visitTime which make use of `history.search`.
Attachment #8753027 - Flags: review?(aswan)
Comment on attachment 8753027 [details]
MozReview Request: Bug 1265836 - Part 4: Implement browser.history.addUrl, r?aswan

https://reviewboard.mozilla.org/r/52946/#review50748

The comment on the test is just a suggestion, but I think the normalizeTime() needs to get fixed before landing this.

::: browser/components/extensions/test/browser/browser_ext_history.js:208
(Diff revision 4)
> +    browser.history.addUrl(details).then(() => {
> +      return browser.history.search({text: details.url});
> +    }).then(results => {
> +      browser.test.assertEq(1, results.length, "1 result found when searching for added URL");
> +      urlsAdded.push({details, result: results[0]});
> +
> +      details = addDetails({visitTime: new Date()}, "with_date");
> +      return browser.history.addUrl(details);
> +    }).then(() => {
> +      return browser.history.search({text: details.url});
> +    }).then(results => {
> +      browser.test.assertEq(1, results.length, "1 result found when searching for added URL");
> +      urlsAdded.push({details, result: results[0]});
> +
> +      details = addDetails({visitTime: Date.now()}, "with_ms_number");
> +      return browser.history.addUrl(details);
> +    }).then(() => {
> +      return browser.history.search({text: details.url});
> +    }).then(results => {
> +      browser.test.assertEq(1, results.length, "1 result found when searching for added URL");
> +      urlsAdded.push({details, result: results[0]});
> +
> +      details = addDetails({visitTime: Date.now().toString()}, "with_ms_string");
> +      return browser.history.addUrl(details);
> +    }).then(() => {
> +      return browser.history.search({text: details.url});
> +    }).then(results => {
> +      browser.test.assertEq(1, results.length, "1 result found when searching for added URL");
> +      urlsAdded.push({details, result: results[0]});
> +
> +      details = addDetails({visitTime: new Date().toISOString()}, "with_iso_string");
> +      return browser.history.addUrl(details);
> +    }).then(() => {
> +      return browser.history.search({text: details.url});
> +    }).then(results => {
> +      browser.test.assertEq(1, results.length, "1 result found when searching for added URL");
> +      urlsAdded.push({details, result: results[0]});
> +
> +      details = addDetails({transition: "typed"}, "valid_transition");
> +      return browser.history.addUrl(details);
> +    }).then(() => {
> +      return browser.history.search({text: details.url});

I think this whole block would be easier to read if you wrap the repeated bits (construct details, call addUrl(), call search() ) into a helper and then write a sequence of calls to that helper.

::: toolkit/components/extensions/ExtensionUtils.jsm:1204
(Diff revision 4)
> - *      The number of milliseconds since the epoch for the date
> + *      A Date object
>   */
>  function normalizeTime(date) {
>    // Of all the formats we accept the "number of milliseconds since the epoch as a string"
>    // is an outlier, everything else can just be passed directly to the Date constructor.
> -  const result = new Date((typeof date == "string" && /^\d+$/.test(date))
> +  return new Date((typeof date == "string" && /^\d+$/.test(date))

This should be in a separate commit and folded together with the download changes.  If somebody tries to do a bisect and tests this commit, download tests will be broken.
Comment on attachment 8754000 [details]
MozReview Request: Bug 1265836 - Part 1: Replace PlacesUtils.toTime with PlacesUtils.toDate, r?mak r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53644/diff/1-2/
Attachment #8754000 - Attachment description: MozReview Request: Bug 1265836 - Part 3: Switch history.search to use toDate instead of the removed toTime, r?aswan → MozReview Request: Bug 1265836 - Part 1: Replace PlacesUtils.toTime with PlacesUtils.toDate, r?mak r?aswan
Attachment #8754001 - Attachment description: MozReview Request: Bug 1265836 - Part 4: Switch history.deleteRange to use normalizeTime, r?aswan → MozReview Request: Bug 1265836 - Part 2: Change normalizeTime to return a date rather than a number, r?aswan
Attachment #8748336 - Attachment description: MozReview Request: Bug 1265836 - Part 1: Implement insert and insertMany in History.jsm, r?mak → MozReview Request: Bug 1265836 - Part 3: Implement insert and insertMany in History.jsm, r?mak
Attachment #8753027 - Attachment description: MozReview Request: Bug 1265836 - Part 2: Implement browser.history.addUrl, r?aswan → MozReview Request: Bug 1265836 - Part 4: Implement browser.history.addUrl, r?aswan
Attachment #8754000 - Flags: review?(mak77)
Attachment #8753027 - Flags: review?(aswan)
Comment on attachment 8754001 [details]
MozReview Request: Bug 1265836 - Part 2: Change normalizeTime to return a date rather than a number, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53646/diff/1-2/
Comment on attachment 8748336 [details]
MozReview Request: Bug 1265836 - Part 3: Implement insert and insertMany in History.jsm, r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50217/diff/8-9/
Comment on attachment 8753027 [details]
MozReview Request: Bug 1265836 - Part 4: Implement browser.history.addUrl, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52946/diff/4-5/
Attachment #8754002 - Attachment is obsolete: true
Attachment #8754002 - Flags: review?(aswan)
https://reviewboard.mozilla.org/r/52946/#review50748

> I think this whole block would be easier to read if you wrap the repeated bits (construct details, call addUrl(), call search() ) into a helper and then write a sequence of calls to that helper.

I wanted to do that, but wasn't sure how to make it work with the promises. I guess maybe the helper itself would have to create a promise to wrap the whole thing in? I need a way to guarantee that all of the individual pieces are complete before I try to check the urls in the main test code. I'll take another look at this to see if I can figure it out.
Comment on attachment 8754000 [details]
MozReview Request: Bug 1265836 - Part 1: Replace PlacesUtils.toTime with PlacesUtils.toDate, r?mak r?aswan

https://reviewboard.mozilla.org/r/53644/#review50936

::: browser/components/extensions/ext-history.js:27
(Diff revision 2)
>  function convertNavHistoryResultNode(node) {
>    return {
>      id: node.pageGuid,
>      url: node.uri,
>      title: node.title,
> -    lastVisitTime: PlacesUtils.toTime(node.time),
> +    lastVisitTime: PlacesUtils.toDate(node.time).valueOf(),

I'd rather use .getTime() everywhere... Dunno, it looks more readable to me.
Attachment #8754000 - Flags: review?(mak77) → review+
Comment on attachment 8754000 [details]
MozReview Request: Bug 1265836 - Part 1: Replace PlacesUtils.toTime with PlacesUtils.toDate, r?mak r?aswan

https://reviewboard.mozilla.org/r/53644/#review51038

The bits under browser/ seem fine to me once you and mak agree on the naming.
Attachment #8754000 - Flags: review?(aswan) → review+
Comment on attachment 8754001 [details]
MozReview Request: Bug 1265836 - Part 2: Change normalizeTime to return a date rather than a number, r?aswan

https://reviewboard.mozilla.org/r/53646/#review51042

::: toolkit/components/extensions/ExtensionUtils.jsm:1191
(Diff revision 2)
>      return set.has(listener);
>    }
>  }
>  
>  /**
> - * Returns a number which represents the number of milliseconds since the epoch
> + * Returns a Date object.

The original comment wasn't great but this would be clearer if it said something like "convert any of several different representations of a date+time to a Date object."

::: toolkit/components/extensions/ext-downloads.js:253
(Diff revision 2)
>      } else {
>        return normalizeTime(arg);
>      }
>    }
>  
> -  const startedBefore = normalizeDownloadTime(query.startedBefore, true);
> +  const startedBefore = normalizeDownloadTime(query.startedBefore, true).valueOf();

can you just move the valueOf() into normalizeDownloadTime()
Attachment #8754001 - Flags: review?(aswan) → review+
https://reviewboard.mozilla.org/r/52946/#review50748

> I wanted to do that, but wasn't sure how to make it work with the promises. I guess maybe the helper itself would have to create a promise to wrap the whole thing in? I need a way to guarantee that all of the individual pieces are complete before I try to check the urls in the main test code. I'll take another look at this to see if I can figure it out.

something like:
```
function* doOneUrl(...) {
  ...
  yield browser.history.addUrl(...);
  ...
  let results = yield browser.history.search(...);
}

for (let url of urls) {
  yield doOneUrl(...);
}
```
https://reviewboard.mozilla.org/r/52946/#review50748

> something like:
> ```
> function* doOneUrl(...) {
>   ...
>   yield browser.history.addUrl(...);
>   ...
>   let results = yield browser.history.search(...);
> }
> 
> for (let url of urls) {
>   yield doOneUrl(...);
> }
> ```

I've tried doing this but I cannot seem to get it to work, I think becuase the code needs to run in the background script, not in the main test code (because of calls to `browser.history`). I cannot get `yield` to work. I'm going to try something different using message passing between the main test and the background script.
Comment on attachment 8754000 [details]
MozReview Request: Bug 1265836 - Part 1: Replace PlacesUtils.toTime with PlacesUtils.toDate, r?mak r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53644/diff/2-3/
Comment on attachment 8754001 [details]
MozReview Request: Bug 1265836 - Part 2: Change normalizeTime to return a date rather than a number, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53646/diff/2-3/
Comment on attachment 8748336 [details]
MozReview Request: Bug 1265836 - Part 3: Implement insert and insertMany in History.jsm, r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50217/diff/9-10/
Comment on attachment 8753027 [details]
MozReview Request: Bug 1265836 - Part 4: Implement browser.history.addUrl, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52946/diff/5-6/
https://reviewboard.mozilla.org/r/52946/#review50748

> I've tried doing this but I cannot seem to get it to work, I think becuase the code needs to run in the background script, not in the main test code (because of calls to `browser.history`). I cannot get `yield` to work. I'm going to try something different using message passing between the main test and the background script.

I rewrote it to pass messages between the main test and the backgound script. I recall that this may have been what I initially did, and you took issue with it, but perhaps this version is better than that one. It does remove all of the duplication, and seems to work well, so hopefully this one is okay.
Comment on attachment 8753027 [details]
MozReview Request: Bug 1265836 - Part 4: Implement browser.history.addUrl, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52946/diff/6-7/
https://reviewboard.mozilla.org/r/52946/#review50748

> I rewrote it to pass messages between the main test and the backgound script. I recall that this may have been what I initially did, and you took issue with it, but perhaps this version is better than that one. It does remove all of the duplication, and seems to work well, so hopefully this one is okay.

I think the way you have it now is a lot more readable and no need to change it but for future reference, I think you could avoid the message passing using Task.spawn (https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm#spawn()) from the extension.
Attachment #8753027 - Flags: review?(aswan) → review+
Comment on attachment 8753027 [details]
MozReview Request: Bug 1265836 - Part 4: Implement browser.history.addUrl, r?aswan

https://reviewboard.mozilla.org/r/52946/#review51522
Iteration: 49.2 - May 23 → 49.3 - Jun 6
Keywords: checkin-needed
Flags: needinfo?(bob.silverberg)
Giorgio, toolkit/components/extensions/test/mochitest/test_ext_webrequest.html is crashing the browser on Linux x64 debug, OS X 10.10 debug and Windows 7 debug. It doesn't seem to crash any of the opt builds though. You mentioned you might be able to take a look at this.

toolkit/components/extensions/test/mochitest/test_ext_schema.html also seems to be crashing the browser on OS X 10.10 debug and Windows 7 debug.

I cannot reproduce these crashes locally, and I have no idea what in my patch could be causing it as it seems totally unrelated to those tests.
Flags: needinfo?(g.maone)
I submitted another try [1] specifying just the paths for WebExtensions code, in the hopes that it will both reproduce and complete faster. If it does reproduce then I'll try submitting trys for each patch individually (i.e., Part 1, then Part 1 & 2, etc.) to see if we can identify which patch is causing this. Other than that I'm not really sure what to do, so if anyone has any advice I would really appreciate it.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3683ff12603
I was able to reproduce the crash locally on a debug build and have narrowed down the culprit to the Part 4 patch attached to this bug. With Parts 1, 2 and 3 applied I am unable to reproduce the crash, but with Part 4 applied I can reliably reproduce it. The three try runs listed above seem to agree with this - without Part 4 there is no crash.

I noticed that the crash was happening after the last test in the suite, so I experimented by removing some of the tests towards the end of the suite and the crash still happened, after whatever was the last test, so it seems like it has to do with code that’s executed after the tests have finished running.

This crash is happening at the end of both the chome tests and the plain mochitests in toolkit/components/extensions/test/mochitest.

By removing other tests I found that running any of the following chrome tests will cause the crash:
test_chrome_ext_downloads_download.html
test_chrome_ext_downloads_misc.html
test_chrome_ext_downloads_search.html

As well as the following plain mochitest:
Test_ext_downloads.html

Only those four tests cause the crash, so it seems to be related to downloads. It seems odd that Part 4 doesn’t even touch toolkit at all, it only changes 3 files which are:
browser/components/extensions/ext-history.js
browser/components/extensions/schemas/history.json
browser/components/extensions/test/browser/browser_ext_history.js

I also noticed that there is an intermittent filed which looks similar to the error that we’re seeing, which is Bug 1272894. There may be something similar happening.

I’m going to bisect my patch to see if I can locate the exact change that results in this crash.
Depends on: 1277405
Comment on attachment 8754000 [details]
MozReview Request: Bug 1265836 - Part 1: Replace PlacesUtils.toTime with PlacesUtils.toDate, r?mak r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53644/diff/3-4/
Comment on attachment 8754001 [details]
MozReview Request: Bug 1265836 - Part 2: Change normalizeTime to return a date rather than a number, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53646/diff/3-4/
Comment on attachment 8748336 [details]
MozReview Request: Bug 1265836 - Part 3: Implement insert and insertMany in History.jsm, r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50217/diff/10-11/
Comment on attachment 8753027 [details]
MozReview Request: Bug 1265836 - Part 4: Implement browser.history.addUrl, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52946/diff/7-8/
I think this is ready to land again.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(g.maone)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50edd919870a
Part 1: Replace PlacesUtils.toTime with PlacesUtils.toDate. r=mak, r=aswan
https://hg.mozilla.org/integration/mozilla-inbound/rev/e83eea3e0b6a
Part 2: Change normalizeTime to return a date rather than a number. r=aswan
https://hg.mozilla.org/integration/mozilla-inbound/rev/d328f6474fcb
Part 3: Implement insert and insertMany in History.jsm. r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/463ee98e93e9
Part 4: Implement browser.history.addUrl. r=aswan
Keywords: checkin-needed
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: