Closed
Bug 539517
Opened 15 years ago
Closed 7 years ago
remove synchronous Bookmarks::SetItemIndex API
Categories
(Toolkit :: Places, defect, P2)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: mak, Assigned: mak)
References
(Blocks 1 open bug, )
Details
(Whiteboard: [fxsearch])
Attachments
(1 file, 1 obsolete file)
This method is dangerous since it allows anyone to set indices in bookmarks table at own pleasure, all the other APIs are taking care of setting correct positions and avoid holes, this one is not.
And it doesn't look really useful, you can just insert bookmarks in a certain order, or moveItem.
We actually use it only in transactions manager, but i think we can easily avoid it.
should be deprecated in 3.7 and removed in 4.0.
Assignee | ||
Comment 1•14 years ago
|
||
Could we deprecate in 4.0 please, bug 634401 demonstrates this is still a big problem.
Comment 2•14 years ago
|
||
Comment on attachment 512683 [details] [diff] [review]
patch v1.0
Can you please review my patch to turn off the dang warnings this will add too please? It's in your review queue ;)
r=sdwilsh
Attachment #512683 -
Flags: review?(sdwilsh) → review+
Assignee | ||
Comment 3•14 years ago
|
||
Comment on attachment 512683 [details] [diff] [review]
patch v1.0
I suppose also deprecating needs SR?
Attachment #512683 -
Flags: superreview?(robert.bugzilla)
Assignee | ||
Comment 4•14 years ago
|
||
(In reply to comment #2)
> Can you please review my patch to turn off the dang warnings this will add too
> please? It's in your review queue ;)
yes, I am a bit behind on reviews, will spend most of the day on them.
Comment 5•14 years ago
|
||
Comment on attachment 512683 [details] [diff] [review]
patch v1.0
Just checked with bsmedberg on irc about deprecating this late in the game.
bsmedberg> rs: please don't, it's not worth it
<rs> bsmedberg: can I get details so I know why in the future?
<bsmedberg> rs: last time I checked, marking a method as [deprecated] will issue a compiler error for C++ which implements the interface
<bsmedberg> That may have changed, but I just don't want to deal with figuring it out.
<bsmedberg> Oh, and quickstubs dies if you try to use depreacted with it, but that would show up quickly as a compile error.
Attachment #512683 -
Flags: superreview?(robert.bugzilla) → superreview-
Assignee | ||
Comment 6•14 years ago
|
||
(In reply to comment #5)
> Comment on attachment 512683 [details] [diff] [review]
> patch v1.0
>
> Just checked with bsmedberg on irc about deprecating this late in the game.
> bsmedberg> rs: please don't, it's not worth it
> <rs> bsmedberg: can I get details so I know why in the future?
> <bsmedberg> rs: last time I checked, marking a method as [deprecated] will
> issue a compiler error for C++ which implements the interface
> <bsmedberg> That may have changed, but I just don't want to deal with figuring
> it out.
> <bsmedberg> Oh, and quickstubs dies if you try to use depreacted with it, but
> that would show up quickly as a compile error.
So, this discussion is mostly the same in bug 584982
The "compiler error" is just a compiler warning on msvc, we can get rid of them (see last comments in bug 584982) with #pragmas
Regarding other issues, we already have [deprecated] methods, and nobody ever complained, nor a bug is filed on any broken functionality. But I don't know what quickstubs are supposed to be in our case?
Comment 7•14 years ago
|
||
From talking with bsmedberg this has broken consumers in the past
<Ms2ger> Warning, iirc
<Ms2ger> And not for gcc
<khuey> I think it's just a compiler warning
<khuey> but it does kill quickstubs
<khuey> for now
<bsmedberg> it is a warning, but I don't like it
<Ms2ger> Why?
<bsmedberg> Because you can't suppress it, if you're implementing the interface as backwards-compatibility glue.
* cjones doesn't think we should be allowed to mark methods deprecated before changing all in-tree callers
<bsmedberg> It's not the callers, that part is fine.
<bsmedberg> It's the implementers.
<Ms2ger> Really?
<Ms2ger> I didn't think these caused warnings too
<bsmedberg> Yes, at least last I checked.
Assignee | ||
Comment 8•14 years ago
|
||
ok, thank you for your investigation!
Will discuss the thing with Shawn, that was working on removing the warnings.
we could decide to just use the javadoc @deprecated for no. At this point I guess if we should also remove existing [deprecated] entries considered nobody found issues with them so far.
Assignee | ||
Updated•13 years ago
|
Assignee: mak77 → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 9•10 years ago
|
||
we need to use PLACES_WARN_DEPRECATE here, and set @deprecated into the javadoc.
Mentor: mak77
Assignee | ||
Updated•10 years ago
|
Attachment #512683 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Whiteboard: [good first bug][lang=cpp]
Comment 10•10 years ago
|
||
Hi,
I am interested in working on this bug. So, please I request you to assign that bug to me.
Regards,
Anup
Assignee | ||
Comment 11•10 years ago
|
||
we are now assigning bugs on first patch attachment, but glad to have you on board for this.
Assignee | ||
Comment 12•9 years ago
|
||
not actionable until bug 1095425 is fixed and we can use PlacesTransactions
Assignee | ||
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Updated•7 years ago
|
Priority: P3 → P2
Summary: deprecate bookmarks service SetItemIndex API → remove synchronous Bookmarks::SetItemIndex API
Whiteboard: [fxsearch]
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 13•7 years ago
|
||
Kit, should we retain the "Set synced orphan indices" test in test_sync_utils.js, or is that case already covered by "Move synced orphan using async API"?
Flags: needinfo?(kit)
Comment 14•7 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #13)
> Kit, should we retain the "Set synced orphan indices" test in
> test_sync_utils.js, or is that case already covered by "Move synced orphan
> using async API"?
It's already covered; feel free to delete the test.
Flags: needinfo?(kit)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8949444 [details]
Bug 539517 - remove synchronous Bookmarks::SetItemIndex API.
https://reviewboard.mozilla.org/r/218760/#review224740
Attachment #8949444 -
Flags: review?(standard8) → review+
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/9889530a351f
remove synchronous Bookmarks::SetItemIndex API. r=standard8
Comment 19•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•