745410 - Remove remaining microsummary and dynamic container support from bookmarks

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P2)

Description

[Gregory Szorc [:gps]]

I believe we're getting rid of microsummaries on bookmarks for the next engine version.

Comment 1

[Richard Newman [:rnewman]]

It's probably worth reviewing

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsINavBookmarksService

and looking for more obsolete features!

Comment 2

[shreyas]

Hi,

I would like to work on this bug. Can you please suggest how should I go about it?

Thanks in advance
shreyas

Comment 3

[Richard Newman [:rnewman]]

Take a look at

https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions#Getting_started

to get a working Firefox build.

Once you're done with that, take a look at services/sync/modules/engines/bookmarks.js, and familiarize yourself with the overall structure.

Compare that file with the link in Comment 1. You should see that some features have been removed, and no longer need to be supported in bookmarks.js.

Let me know if you need more help!

Comment 4

[shreyas]

So Basically what I need to do is to look for the methods and features marked obsolete in https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsINavBookmarksService
and remove/replace them in bookmarks.js right?

Comment 5

[shreyas]

Is the bug still on?

Comment 6

[Richard Newman [:rnewman]]

Yes!

Comment 7

[shreyas]

I compared the link and the file. I couldn't find any uses of deprecated methods from that interface present in bookmarks.js.

Comment 8

[Richard Newman [:rnewman]]

Not just methods; also 'things' -- like microsummaries.

Comment 9

[shreyas]

Attachment: bug_745410.diff

Is it correct?

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8565498 [details] [diff] [review]
bug_745410.diff

This looks fine to me. I don't think it can land until we remove Sync 1.1 support. Here's why:

* Most microsummary support was removed in Bug 524091. What's left here allows microsummary records to fall back to being ordinary bookmarks when Sync applies them.

* Sync 1.5 clients are guaranteed not to have uploaded any microsummary records to Sync 1.5 servers.

* Technically, Sync 1.1 servers can still contain microsummary records. We shouldn't drop those on the floor when a client downloads them.

Rather than adding logic here to change things for 1.5 only, let's just leave this here.

N.B., we also need to make sure the docs are up to date:

http://docs.services.mozilla.com/sync/objectformats.html


So let's leave this patch on the bug, and we can apply it a few months from now. Sound good?

Comment 11

[shreyas]

Ok, no problem.

Comment 12

[Anup Kumar [:reznord]]

(In reply to Richard Newman [:rnewman] from comment #10)
> Comment on attachment 8565498 [details] [diff] [review]
> bug_745410.diff
> 
> This looks fine to me. I don't think it can land until we remove Sync 1.1
> support. Here's why:
> 
> * Most microsummary support was removed in Bug 524091. What's left here
> allows microsummary records to fall back to being ordinary bookmarks when
> Sync applies them.
> 
> * Sync 1.5 clients are guaranteed not to have uploaded any microsummary
> records to Sync 1.5 servers.
> 
> * Technically, Sync 1.1 servers can still contain microsummary records. We
> shouldn't drop those on the floor when a client downloads them.
> 
> Rather than adding logic here to change things for 1.5 only, let's just
> leave this here.
> 
> N.B., we also need to make sure the docs are up to date:
> 
> http://docs.services.mozilla.com/sync/objectformats.html
> 
> 
> So let's leave this patch on the bug, and we can apply it a few months from
> now. Sound good?

Is this bug still there? Its been a long time.

Comment 13

[Richard Newman [:rnewman]]

1.1 support hasn't been removed yet. But soon.

Comment 14

[Edouard Oger [:eoger]]

Should we get back to this now?

Comment 15

[Mark Hammond [:markh] [:mhammond]]

(In reply to Edouard Oger [:eoger] from comment #14)
> Should we get back to this now?

Yeah, I think we should - we've effectively killed 1.1 support in 54.

Comment 16

[Edouard Oger [:eoger]]

I don't think anyone is gonna take this bug since it's 2 years old

Comment 17

[Edouard Oger [:eoger]]

Attachment: Bug 745410 - Remove microsummary support from Sync.

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

Comment 18

[Lina Butler [:lina]]

Comment on attachment 8846091 [details]
Bug 745410 - Remove microsummary support from Sync.

https://reviewboard.mozilla.org/r/119172/#review121156

Nice!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c8eac7852f6
Remove microsummary support from Sync. r=kitcambridge

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for build failures like https://treeherder.mozilla.org/logviewer.html#?job_id=83120084&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/76d437394bf331626b9be6b2dcad8a6fec61979a

Comment 21

[Edouard Oger [:eoger]]

Comment on attachment 8846091 [details]
Bug 745410 - Remove microsummary support from Sync.

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

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b53ca1627c0
Remove microsummary support from Sync. r=kitcambridge

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b53ca1627c0