Last Comment Bug 629814 - Async history API should ignore place ids
: Async history API should ignore place ids
Status: RESOLVED FIXED
[softblocker]
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla2.0b12
Assigned To: Shawn Wilsher :sdwilsh
:
Mentors:
Depends on:
Blocks: 628805
  Show dependency treegraph
 
Reported: 2011-01-28 15:46 PST by Mehdi Mulani [:mmm] (I don't check this)
Modified: 2011-03-03 12:50 PST (History)
5 users (show)
sdwilsh: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+


Attachments
Tests (1.88 KB, patch)
2011-01-28 15:53 PST, Mehdi Mulani [:mmm] (I don't check this)
no flags Details | Diff | Review
v1.0 (5.94 KB, patch)
2011-02-02 14:59 PST, Shawn Wilsher :sdwilsh
mak77: review+
robert.strong.bugs: superreview+
Details | Diff | Review

Description Mehdi Mulani [:mmm] (I don't check this) 2011-01-28 15:46:12 PST
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) 2011-01-28 15:53:16 PST
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.
Comment 2 Shawn Wilsher :sdwilsh 2011-02-02 09:57:00 PST
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] 2011-02-02 10:07:09 PST
disallow what exactly? selecting by placeId?
Comment 4 Shawn Wilsher :sdwilsh 2011-02-02 10:40:58 PST
(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] 2011-02-02 14:24:55 PST
I've always been in favor of NOT exposing database ids... if Sync does not need them it wfm.
Comment 6 Shawn Wilsher :sdwilsh 2011-02-02 14:57:29 PST
OK then!
Comment 7 Shawn Wilsher :sdwilsh 2011-02-02 14:58:44 PST
API change, so softblocking betaN
Comment 8 Philipp von Weitershausen [:philikon] 2011-02-02 14:59:24 PST
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 2011-02-02 14:59:41 PST
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?
Comment 10 Marco Bonardo [::mak] 2011-02-02 16:45:56 PST
Comment on attachment 509252 [details] [diff] [review]
v1.0

this patch makes me happier.
Comment 11 Mehdi Mulani [:mmm] (I don't check this) 2011-02-02 17:29:15 PST
(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 2011-02-03 14:39:50 PST
http://hg.mozilla.org/mozilla-central/rev/1cb679417a8b
Comment 13 Eric Shepherd [:sheppy] 2011-02-17 06:31:30 PST
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 2011-02-17 07:18:04 PST
(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] 2011-02-17 07:24:23 PST
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 2011-02-17 09:24:00 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.