Closed Bug 648364 Opened 9 years ago Closed 9 years ago

Replace custom helpers with Services.jsm, NetUtil.jsm, FileUtils.jsm, PlacesUtils.jsm equivalents

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: philikon, Assigned: philikon)

References

Details

(Whiteboard: [prune][qa-])

Attachments

(7 files, 3 obsolete files)

No description provided.
Summary: Replace custom helpers with Services.jsm, NetUtil.jsm, FileUtils.jsm equivalents → Replace custom helpers with Services.jsm, NetUtil.jsm, FileUtils.jsm, PlacesUtils.jsm equivalents
CCing Jen. This is the remainder of the compat code removal work.
Attached patch Part 1 (v1): Services.jsm (obsolete) — Splinter Review
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #533494 - Flags: review?(rnewman)
Attached patch Part 2 (v1): PlacesUtils.jsm (obsolete) — Splinter Review
Attachment #533496 - Flags: review?(rnewman)
Attached patch Part 3 (v1): XPCOMUtils.jsm (obsolete) — Splinter Review
Attachment #533497 - Flags: review?(rnewman)
Comment on attachment 533494 [details] [diff] [review]
Part 1 (v1): Services.jsm

Beautifully formulaic.

Two identical nits:

> +    let logins = Services.logins.findLogins({}, login.hostname, login.formSubmitURL,
>                                        login.httpRealm);

and

> +    return Services.logins.QueryInterface(Ci.nsIInterfaceRequestor)
>                      .getInterface(Ci.mozIStorageConnection);

Indentation on the second line no longer matches the first line.

I will trust that it builds and tests pass: I am trying to avoid being labeled a masochist ;)
Attachment #533494 - Flags: review?(rnewman) → review+
Address review comments
Attachment #533494 - Attachment is obsolete: true
Rebase on top of Part 1 (v1.1)
Attachment #533497 - Attachment is obsolete: true
Attachment #533497 - Flags: review?(rnewman)
Attachment #533551 - Flags: review?(rnewman)
Attachment #533562 - Flags: review?(rnewman)
Attachment #533563 - Flags: review?(rnewman)
Attachment #533550 - Flags: review+
Comment on attachment 533496 [details] [diff] [review]
Part 2 (v1): PlacesUtils.jsm

+    let mRoot = PlacesUtils.bookmarks.createFolder(root, "mobile", -1);
+    PlacesUtils.annotations.setItemAnnotation(mRoot, MOBILEROOT_ANNO, 1, 0,
+                                PlacesUtils.annotations.EXPIRE_NEVER);
+    PlacesUtils.annotations.setItemAnnotation(mRoot, EXCLUDEBACKUP_ANNO, 1, 0,
+                                PlacesUtils.annotations.EXPIRE_NEVER);

and

+          PlacesUtils.annotations.setItemAnnotation(itemId, DESCRIPTION_ANNO, val, 0,
+                                      PlacesUtils.annotations.EXPIRE_NEVER);

and

+          PlacesUtils.annotations.setItemAnnotation(itemId, SIDEBAR_ANNO, true, 0,
+                                      PlacesUtils.annotations.EXPIRE_NEVER);

and

+        PlacesUtils.annotations.setItemAnnotation(itemId, SMART_BOOKMARKS_ANNO, val, 0,
+                                    PlacesUtils.annotations.EXPIRE_NEVER);

... indentation. (Our style guide suggests that multi-line params should each get their own line, but I don't think it applies in this case! :D)


           if (!parent)
-            parent = Svc.Bookmark.getFolderIdForItem(itemid);
-          Svc.Bookmark.moveItem(itemid, parent, idx - delta);
+            parent = PlacesUtils.bookmarks.getFolderIdForItem(itemid);
+          PlacesUtils.bookmarks.moveItem(itemid, parent, idx - delta);

While you're touching this part, can we get {} around the if consequent?

Otherwise, bravo!

Sidenote: I can't wait for standardization of anonymous function syntax.
Attachment #533496 - Flags: review?(rnewman) → review+
Comment on attachment 533551 [details] [diff] [review]
Part 3 (v1.1): XPCOMUtils.jsm

http://etler.com/clapping-b.gif
Attachment #533551 - Flags: review?(rnewman) → review+
Comment on attachment 533562 [details] [diff] [review]
Part 4 (v1): FileUtils.jsm

http://etler.com/applause.gif
Attachment #533562 - Flags: review?(rnewman) → review+
Attachment #533563 - Flags: review?(rnewman) → review+
Addressed rnewman's review comments
Attachment #533496 - Attachment is obsolete: true
Attachment #533748 - Flags: review+
Depends on: 658371
One last part: removing some (brain-)dead test helpers. Also make SyncTestingInfrastructure sane(r). Need to rethinking this for the long term, but in the short term this already so much better.
Attachment #534179 - Flags: review?(rnewman)
Comment on attachment 534179 [details] [diff] [review]
Part 6 (v1): dead test code

Looks good to me! Interested to see all greens when you push this :D
Attachment #534179 - Flags: review?(rnewman) → review+
Re

  https://hg.mozilla.org/services/services-central/rev/2c2e81d7fc04

I believe there's a missing case here:

      case "ProfD":
        return ds.get("CurProcD", Ci.nsIFile);

Patch arriving momentarily.
Sorry for failing to spot this in review.
Attachment #534516 - Flags: review?(philipp)
Attachment #534516 - Flags: review?(philipp) → review+
Depends on: 659777
Blocks: 647605
No longer blocks: 647605
Blocks: 559156
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.