Async history API should ignore place ids

RESOLVED FIXED in mozilla2.0b12

Status

()

Toolkit
Places
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mmm, Assigned: sdwilsh)

Tracking

({dev-doc-complete})

Trunk
mozilla2.0b12
x86
Mac OS X
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [softblocker])

Attachments

(2 attachments)

If you set a placeId on a mozIPlaceInfo and then pass it into updatePlaces, you run into an assertion failure at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/History.cpp#1336 rather than an error being returned.
Created attachment 508030 [details] [diff] [review]
Tests

These tests should be merged in to test_async_history_api.js after a fix has been landed.

The tests are not complete either, as the expected error code for a duplicate placeId is not known.
(Assignee)

Comment 2

6 years ago
I'm inclined to say we should just disallow this at this point, which would be an API change.  mak?
disallow what exactly? selecting by placeId?
(Assignee)

Comment 4

6 years ago
(In reply to comment #3)
> disallow what exactly? selecting by placeId?
specifying the place id to insert as.  Basically, we'd only support guid or uri.
I've always been in favor of NOT exposing database ids... if Sync does not need them it wfm.
(Assignee)

Comment 6

6 years ago
OK then!
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Summary: Async history visit API does not allow setting a placeId → Async history API should ignore place ids
(Assignee)

Comment 7

6 years ago
API change, so softblocking betaN
blocking2.0: --- → betaN+
Whiteboard: [softblocker]
Yes pls. As far as Sync is concerned, Place IDs are internal bookkeeping. Sync introduced GUIDs for precisely that reason.
(Assignee)

Comment 9

6 years ago
Created attachment 509252 [details] [diff] [review]
v1.0

This makes us ignore the place id all together.  Mehdi, can you update your test to check that we ignore the place id?
Attachment #509252 - Flags: superreview?(robert.bugzilla)
Attachment #509252 - Flags: review?(mak77)
(Assignee)

Updated

6 years ago
Whiteboard: [softblocker] → [softblocker][needs review mak][needs sr rstrong]
Attachment #509252 - Flags: superreview?(robert.bugzilla) → superreview+
Whiteboard: [softblocker][needs review mak][needs sr rstrong] → [softblocker][needs review mak]
Comment on attachment 509252 [details] [diff] [review]
v1.0

this patch makes me happier.
Attachment #509252 - Flags: review?(mak77) → review+
(Assignee)

Updated

6 years ago
Whiteboard: [softblocker][needs review mak] → [softblocker][can land]
(In reply to comment #9)
> Created attachment 509252 [details] [diff] [review]
> v1.0
> 
> This makes us ignore the place id all together.  Mehdi, can you update your
> test to check that we ignore the place id?

Will do in next version! (for reference, tests are in bug 628865 )
Blocks: 628805
No longer blocks: 628805
(Assignee)

Comment 12

6 years ago
http://hg.mozilla.org/mozilla-central/rev/1cb679417a8b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Keywords: dev-doc-needed
Resolution: --- → FIXED
Whiteboard: [softblocker][can land] → [softblocker]
Target Milestone: --- → mozilla2.0b12
Version: unspecified → Trunk
Blocks: 628805
This looks like an internal API that's not exposed to the public anywhere. Not sure there's much to document here. I did update the list of exceptions on mozIAsyncHistory, but otherwise, is there anything that needs to be done here?
(Assignee)

Comment 14

6 years ago
(In reply to comment #13)
> This looks like an internal API that's not exposed to the public anywhere. Not
> sure there's much to document here. I did update the list of exceptions on
> mozIAsyncHistory, but otherwise, is there anything that needs to be done here?
Well, mozIAsyncHistory isn't an internal API, and that's what was changed here.  If that's what you updated, that's all that needed to be done.
The problem is that I don't see any actual API change here. As far as I can tell, it simply isn't looking at the placeId anymore; it doesn't seem to affect documentation in any appreciable way (at least not how the documentation currently reads).
(Assignee)

Comment 16

6 years ago
(In reply to comment #15)
> The problem is that I don't see any actual API change here. As far as I can
> tell, it simply isn't looking at the placeId anymore; it doesn't seem to affect
> documentation in any appreciable way (at least not how the documentation
> currently reads).
Ah, well, it used to, and I thought it was documented that way.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.