Open Bug 1385989 Opened 3 years ago Updated 3 years ago

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

Categories

(Toolkit :: Places, enhancement, P3)

enhancement

Tracking

()

Tracking Status
firefox57 --- fix-optional

People

(Reporter: lina, Unassigned)

Details

`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?
Flags: needinfo?(bob.silverberg)
(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
Flags: needinfo?(bob.silverberg)
I'm fine relaxing this. So basically it will never reject.
You need to log in before you can comment on or make changes to this bug.