1385989 - Consider not rejecting if `History.insertMany` doesn't add any new items

Status: NEW

(Toolkit :: Places, enhancement, P3)

Description

[Lina Butler [:lina]]

`insertMany` currently rejects with "No items were added to history": https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/components/places/History.jsm#1367 However, `updatePlaces` skips URLs that history can't store, like `javascript:` bookmarklets: https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/components/places/History.cpp#2838-2842

This means that, if an array of visits contains at least one valid URL, the promise will resolve and the invalid URLs will be silently ignored. But, if it contains no valid URLs, it'll reject.

I wonder if we could have `insertMany` resolve the promise unconditionally, even if no items were added. This would help Sync switch to using `History.insertMany`, without needing to manually call `PlacesUtils.history.canAddURI` for every record.

Bob, I think you originally implemented `insertMany`. Would you be OK with changing the behavior?

Comment 1

[Bob Silverberg [:bsilverberg]]

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #0)
> 
> Bob, I think you originally implemented `insertMany`. Would you be OK with
> changing the behavior?

Yes, that would be fine for my use of `insertMany`. I notice that the docs for insertMany say that it will throw an error if any URLs are encountered which should not be added to history [1], which sounds like it would be an issue for your use case, but I don't actually see that implemented in the code anywhere. It would be interesting to know if that's a bug in the docs or in the method.

[1] http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/components/places/History.jsm#240-242

Comment 2

[Marco Bonardo [:mak]]

I'm fine relaxing this. So basically it will never reject.