629814 - Async history API should ignore place ids

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Mehdi Mulani [:mmm] (I don't check this)]

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.

Comment 1

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: 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.

Comment 2

[Shawn Wilsher :sdwilsh]

I'm inclined to say we should just disallow this at this point, which would be an API change.  mak?

Comment 3

[Marco Bonardo [:mak]]

disallow what exactly? selecting by placeId?

Comment 4

[Shawn Wilsher :sdwilsh]

(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.

Comment 5

[Marco Bonardo [:mak]]

I've always been in favor of NOT exposing database ids... if Sync does not need them it wfm.

Comment 6

[Shawn Wilsher :sdwilsh]

OK then!

Comment 7

[Shawn Wilsher :sdwilsh]

API change, so softblocking betaN

Comment 8

[Philipp von Weitershausen [:philikon]]

Yes pls. As far as Sync is concerned, Place IDs are internal bookkeeping. Sync introduced GUIDs for precisely that reason.

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: 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?

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 509252 [details] [diff] [review]
v1.0

this patch makes me happier.

Comment 11

[Mehdi Mulani [:mmm] (I don't check this)]

(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 )

Comment 12

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/1cb679417a8b

Comment 13

[Eric Shepherd [:sheppy]]

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?

Comment 14

[Shawn Wilsher :sdwilsh]

(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.

Comment 15

[Eric Shepherd [:sheppy]]

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).

Comment 16

[Shawn Wilsher :sdwilsh]

(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.