Open
Bug 1385989
Opened 7 years ago
Updated 2 years ago
Consider not rejecting if `History.insertMany` doesn't add any new items
Categories
(Toolkit :: Places, enhancement, P3)
Toolkit
Places
Tracking
()
NEW
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)
Comment 1•7 years ago
|
||
(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)
Comment 2•7 years ago
|
||
I'm fine relaxing this. So basically it will never reject.
Updated•7 years ago
|
status-firefox57:
--- → fix-optional
Priority: -- → P3
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•