675299 - Preliminary implementation of favicon sync

Status: RESOLVED INACTIVE

(Firefox :: Sync, enhancement)

Description

[Richard Newman [:rnewman]]

This is the work-item bug for implementing the Sync side of favicon sync. The Places part is Bug 674238, but isn't strictly necessary for this.

This whole engine will eventually be rewritten for async repositories, and will see various iterations upon the evolving favicons layer.

Starting a fresh bug to avoid all the built-up crap in Bug 428378.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Part 0: bookmarks engine cleanup. v1

WIP.

Comment 2

[Richard Newman [:rnewman]]

Attachment: Part 1: extend bookmarks and history items to include favicon GUIDs. v1

No GUIDs yet, so this is actually storing favicon URIs in each PlacesItem record. Lots to do, of course.

Comment 3

[Richard Newman [:rnewman]]

Attachment: WIP for Part 2.

Some tests, much of the implementation of Store and Engine, lots of SQL :D

Comment 4

[Richard Newman [:rnewman]]

Attachment: Part 0a: minor improvements to engines code.

Comment 5

[Richard Newman [:rnewman]]

Attachment: Part 0b: add missing 'let' in service.js.

Comment 6

[Richard Newman [:rnewman]]

Attachment: Part 0c: bookmarks engine cleanup.

Comment 7

[Richard Newman [:rnewman]]

Attachment: Part 1: extend bookmarks and history items to include favicon GUIDs. v2

Comment 8

[Richard Newman [:rnewman]]

Attachment: Part 2: implement basic favicons engine (URL-based, bookmarks only). v1

Comment 9

[Philipp von Weitershausen [:philikon]]

Comment on attachment 553377 [details] [diff] [review]
Part 0b: add missing 'let' in service.js.

`for each` is an oddball. It doesn't take let or var. See https://developer.mozilla.org/en/JavaScript/Reference/Statements/for_each...in

Comment 10

[Philipp von Weitershausen [:philikon]]

Comment on attachment 553377 [details] [diff] [review]
Part 0b: add missing 'let' in service.js.

Never mind, I didn't read all of the spec. Interesting that var/let is now required in strict mode!

Please file separate bug for these, though. This shouldn't clutter up this bug.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 553378 [details] [diff] [review]
Part 0c: bookmarks engine cleanup.

Moved all these "part 0"s to Bug 679279.

Comment 12

[Gregory Szorc [:gps]]

Should strict mode be enabled for all new files?

Comment 13

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #12)
> Should strict mode be enabled for all new files?

Yes, and I will do that for favicons. Filing another bug for strict mode in general…

Comment 14

[Philipp von Weitershausen [:philikon]]

(In reply to Gregory Szorc [:gps] from comment #12)
> Should strict mode be enabled for all new files?

Just talked this through with rnewman. Yes, all new code should use strict mode. Good catch, Greg.

Comment 15

[Richard Newman [:rnewman]]

> Filing another bug for strict mode in general…

Bug 680593.

Comment 16

[Richard Newman [:rnewman]]

We will probably -- I hope! -- end up using a salted hash for favicons, rather than a GUID, so the implementation will change.

Comment 17

[janet [:janet]]

Closing as inactive - patch is 7yrs old.