539517 - remove synchronous Bookmarks::SetItemIndex API

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P2)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

Could we deprecate in 4.0 please, bug 634401 demonstrates this is still a big problem.

Comment 2

[Shawn Wilsher :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

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 512683 [details] [diff] [review]
patch v1.0

I suppose also deprecating needs SR?

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(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

[Robert Strong (they/them - no direct email)]

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.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(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

[Robert Strong (they/them - no direct email)]

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.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

we need to use PLACES_WARN_DEPRECATE here, and set @deprecated into the javadoc.

Comment 10

[Anup Kumar [:reznord]]

Hi, 

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

Regards,
Anup

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

we are now assigning bugs on first patch attachment, but glad to have you on board for this.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

not actionable until bug 1095425 is fixed and we can use PlacesTransactions

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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"?

Comment 14

[Lina Butler [:lina]]

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

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Bug 539517 - remove synchronous Bookmarks::SetItemIndex API.

Review commit: https://reviewboard.mozilla.org/r/218760/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218760/

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8949444 [details]
Bug 539517 - remove synchronous Bookmarks::SetItemIndex API.

https://reviewboard.mozilla.org/r/218760/#review224740

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8949444 [details]
Bug 539517 - remove synchronous Bookmarks::SetItemIndex API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218760/diff/1-2/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9889530a351f
remove synchronous Bookmarks::SetItemIndex API. r=standard8

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9889530a351f