Closed Bug 675299 Opened 13 years ago Closed 5 years ago

Preliminary implementation of favicon sync

Categories

(Firefox :: Sync, enhancement)

Product:
Firefox
Component:
Sync
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INACTIVE

People

(Reporter: rnewman, Unassigned)

References

(Depends on 1 open bug,
URL
)

Details

(Whiteboard: [sync:bookmarks])

Attachments

(2 files, 6 obsolete files)

Part 1: extend bookmarks and history items to include favicon GUIDs. v2
(deleted), patch
Details | Diff | Splinter Review
Part 2: implement basic favicons engine (URL-based, bookmarks only). v1
(deleted), patch
Details | Diff | Splinter Review
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.
Attached patch Part 0: bookmarks engine cleanup. v1 (obsolete) (deleted) — — Splinter Review
WIP.
No GUIDs yet, so this is actually storing favicon URIs in each PlacesItem record. Lots to do, of course.
Attached patch WIP for Part 2. (obsolete) (deleted) — — Splinter Review
Some tests, much of the implementation of Store and Engine, lots of SQL :D
Attached patch Part 0a: minor improvements to engines code. (obsolete) (deleted) — — Splinter Review
Attachment #549451 - Attachment is obsolete: true
Attached patch Part 0b: add missing 'let' in service.js. (obsolete) (deleted) — — Splinter Review
Attached patch Part 0c: bookmarks engine cleanup. (obsolete) (deleted) — — Splinter Review
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
Attachment #553377 - Flags: review-
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.
Attachment #553377 - Flags: review-
Depends on: 679279
Attachment #553376 - Attachment is obsolete: true
Attachment #553377 - Attachment is obsolete: true
Comment on attachment 553378 [details] [diff] [review] Part 0c: bookmarks engine cleanup. Moved all these "part 0"s to Bug 679279.
Attachment #553378 - Attachment is obsolete: true
Should strict mode be enabled for all new files?
(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…
(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.
> Filing another bug for strict mode in general… Bug 680593.
Blocks: 709404
We will probably -- I hope! -- end up using a salted hash for favicons, rather than a GUID, so the implementation will change.
Assignee: rnewman → nobody
Status: ASSIGNED → NEW
Whiteboard: [sync:bookmarks]
Depends on: 824188
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
Type: defect → enhancement

Closing as inactive - patch is 7yrs old.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: