remove synchronous Bookmarks::SetItemIndex API

RESOLVED FIXED in Firefox 60

Status

()

Toolkit
Places
P2
normal
RESOLVED FIXED
8 years ago
3 months ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks: 1 bug)

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [fxsearch], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 512683 [details] [diff] [review]
patch v1.0

Could we deprecate in 4.0 please, bug 634401 demonstrates this is still a big problem.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attachment #512683 - Flags: review?(sdwilsh)
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+
Comment on attachment 512683 [details] [diff] [review]
patch v1.0

I suppose also deprecating needs SR?
Attachment #512683 - Flags: superreview?(robert.bugzilla)
(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 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-
(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?
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.
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.
Blocks: 484096
Blocks: 438718
Assignee: mak77 → nobody
Status: ASSIGNED → NEW
we need to use PLACES_WARN_DEPRECATE here, and set @deprecated into the javadoc.
Mentor: mak77
Blocks: 897395
Attachment #512683 - Attachment is obsolete: true
Whiteboard: [good first bug][lang=cpp]
Hi, 

I am interested in working on this bug. So, please I request you to assign that bug to me. 

Regards,
Anup
we are now assigning bugs on first patch attachment, but glad to have you on board for this.
not actionable until bug 1095425 is fixed and we can use PlacesTransactions
Mentor: mak77
Depends on: 1095425
Whiteboard: [good first bug][lang=cpp]
Priority: -- → P3
Priority: P3 → P2
Summary: deprecate bookmarks service SetItemIndex API → remove synchronous Bookmarks::SetItemIndex API
Whiteboard: [fxsearch]
Blocks: 1083465
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)
(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: nobody → mak77
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 16

3 months 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

3 months ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/9889530a351f
remove synchronous Bookmarks::SetItemIndex API. r=standard8

Comment 19

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9889530a351f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months 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.