Remove C++ feed parsing for livemarks on trunk

RESOLVED FIXED

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: Robert Sayre, Assigned: Robert Sayre)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 12 obsolete attachments)

4.00 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
109.89 KB, patch
(not reading, please use seth@sspitzer.org instead)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
The livemark parser is known to be buggy and slow compared the new toolkit component used by the feed preview.

nsILivemarkService and friends seem to be implemented in C++ mostly to reuse the C++ livemark code they inherited. Looks like I can reuse lots of Google code from their moz and js lib to do the alarms and observer stuff. It should result in a big reduction 

The other issue is that Places IDs seem to be 64-bit integers, and that's not safe for JS, which only gets 51 bits of precision. If I just need to pass them around, I can probably use an nsISupportsPrimitive to get it done. I wonder if Places IDs are safe elsewhere in the codebase, though.
(Assignee)

Comment 1

12 years ago
Created attachment 239284 [details] [diff] [review]
stubbed out nsLivemarksService.js, includes Google libs from url-classifier

Comment 2

12 years ago
51 bits should be fine. The IDs just increment as things get added. We want to handle more than a few billion to keep from crashing in edge cases (probably automatic testing applications are the only things that could get us this high), but both 51 and 64 are way beyond the range where we care.
(Assignee)

Comment 3

12 years ago
Created attachment 239347 [details] [diff] [review]
new livemark listener
Attachment #239284 - Attachment is obsolete: true
(Assignee)

Comment 4

12 years ago
Created attachment 239348 [details] [diff] [review]
new livemark listener
Attachment #239347 - Attachment is obsolete: true
(Assignee)

Comment 5

12 years ago
Created attachment 239413 [details] [diff] [review]
fill in nsLivemarksService methods
Attachment #239348 - Attachment is obsolete: true
(Assignee)

Comment 6

12 years ago
Created attachment 239419 [details] [diff] [review]
fix syntax goof
Attachment #239413 - Attachment is obsolete: true
(Assignee)

Comment 7

12 years ago
Created attachment 239518 [details] [diff] [review]
nsIRemoteContainer implemented, timer and shutdown implemented
Attachment #239419 - Attachment is obsolete: true
(Assignee)

Comment 8

12 years ago
This depends on a "make check" driven unit testing in 354401 for places, so we can have unit tests for the livemark stuff.
Depends on: 354401
(Assignee)

Comment 9

12 years ago
Created attachment 240732 [details] [diff] [review]
almost works perfectly. need unit tests for old and new version.
Attachment #239518 - Attachment is obsolete: true
(Assignee)

Comment 10

12 years ago
Created attachment 241255 [details] [diff] [review]
mostly works... refresh interval and shutdown need work

also need tests and leak checks
Attachment #240732 - Attachment is obsolete: true

Updated

12 years ago
Blocks: 355772
(Assignee)

Comment 11

11 years ago
Created attachment 241857 [details] [diff] [review]
add livemarks unit tests

small set to start. already found a possible bug, though.
Attachment #241857 - Flags: review?(dietrich)
Comment on attachment 241857 [details] [diff] [review]
add livemarks unit tests

looks a-ok.

make check
../../../../dist/bin/run-mozilla.sh ../../../../dist/bin/test_all.sh ../../../../dist/bin/test_places
../../../../dist/bin/test_places/test_bookmarks.js: PASS
../../../../dist/bin/test_places/test_livemarks.js: PASS

yum.
Attachment #241857 - Flags: review?(dietrich) → review+
(Assignee)

Updated

11 years ago
Blocks: 356321
(Assignee)

Comment 13

11 years ago
Created attachment 242292 [details] [diff] [review]
add unit tests to the diff
Attachment #241255 - Attachment is obsolete: true
(Assignee)

Comment 14

11 years ago
Created attachment 242298 [details] [diff] [review]
convert nsLivemarkService to JS

big patch, should get review from two people
Attachment #242292 - Attachment is obsolete: true
Attachment #242298 - Flags: superreview?(sspitzer)
Attachment #242298 - Flags: review?(dietrich)
(Assignee)

Comment 15

11 years ago
hmm, +722/-1930. Not bad.
(Assignee)

Comment 16

11 years ago
"<philor> and are you sure we want to switch from our current 5 minute retry on fetching errors, and 60 minute retry on parsing errors, to 10 minutes for everything?"

Hmm, no. But that should be easy enough to change.

> if (!haveAtLeastOneEntry)

This regresses bug 262798, no?
(Assignee)

Comment 18

11 years ago
(In reply to comment #17)
> > if (!haveAtLeastOneEntry)
> 
> This regresses bug 262798, no?

Yes, it does. I forgot about that fix.
Blocks: 341972
Comment on attachment 242298 [details] [diff] [review]
convert nsLivemarkService to JS

>@@ -1599,18 +1599,18 @@ nsNavBookmarks::WriteContainerContents(P
>                                folderId == mBookmarksRoot)) {
>         // don't write out the bookmarks menu or the toolbar folder from the
>         // places root. When writing to bookmarks.html, these are reparented
>         // to the menu, which is the root of the namespace. This provides
>         // better backwards compatability.
>         continue;
>       }
> 
>-      // it could be a regular folder or it could be a livemark
>-      nsLivemarkService* livemarkService = nsLivemarkService::GetLivemarkService();
>+      // it could be a regular folder or it could be a livemarkx
>+      nsCOMPtr<nsILivemarkService> livemarkService(do_GetService(NS_LIVEMARKSERVICE_CONTRACTID, &rv));

nit: extra "x" in "livemarkx"

>+
>+var gIoService = Cc[IO_CONTRACTID].getService(Ci.nsIIOService);
>+var gStringBundle;
>+function GetString(name)
>+{
>+  try {
>+    if (!gStringBundle) {
>+      var bundleService = Cc[SB_CONTRACTID].getService(); 
>+      bundleService = bundleService.QueryInterface(Ci.nsIStringBundleService);
>+      gStringBundle = bundleService.createBundle(PLACES_BUNDLE_URI);
>+    }
>+
>+    if (gStringBundle)
>+      return gStringBundle.GetStringFromName(name);
>+  }
>+  catch (ex) {
>+    LOG("Exception loading string bundle: " + ex.message);
>+  }
>+
>+  return null;
>+}

nit: should use same-line start brace, like the rest of the file

>+
>+  _updateLivemarkChildren:
>+  function LS__updateLivemarkChildren(index, forceUpdate) {
>+    if (this._livemarks[index].locked)
>+      return;
>+    
>+    var livemark = this._livemarks[index];
>+    livemark.locked = true;
>+    try {
>+      // Check the TTL/expiration on this.  If there isn't one,
>+      // then we assume it's never been loaded.  We perform this
>+      // check even when the update is being forced, in case the
>+      // livemark has somehow never been loaded.
>+      var exprTime = this._ans.getAnnotationInt64(livemark.feedURI,
>+                                                  LMANNO_EXPIRATION);
>+      if (!forceUpdate && exprTime > Date.now()) {
>+  // no need to refresh
>+  livemark.locked = false;
>+  return;
>+      }
>+    } 

nit: indentation fix

>+
>+  onContainerMoved:
>+  function LS_onContainerMoved(container, newFolder, newIndex) {
>+    var index = this._getLivemarkIndex(container);
>+    
>+    // Update the annotation that maps the folder URI to the livemark feed URI
>+    var newURI = this._bms.getFolderURI(container);
>+    var oldURI = this._livemarks[index].folderURI;
>+    var feedURIString = this._ans.getAnnotationString(oldURI, LMANNO_FEEDURI);
>+    this._ans.removeAnnotation(oldURI, LMANNO_FEEDURI);
>+    this._ans.setAnnotation(newURI, LMANNO_FEEDURI, feedURIString, 0, 
>+                            this._ans.EXPIRE_NEVER);
>+    
>+    // Update the annotation that maps the folder URI to the livemark site URI
>+    var siteURIString;
>+    try {
>+      siteURIString = this._ans.GetAnnotationString(oldURI, LMANNO_SITEURI);
>+    }
>+    catch (ex) {
>+      // will throw if no annotation
>+    }
>+
>+    if (siteURIString) {
>+      this._ans.removeAnnotation(oldURI, LMANNO_SITEURI);
>+      this._ans.setAnnotation(newURI, LMANNO_SITEURI, siteURIString, 0,
>+                              this._ans.EXPIRE_NEVER);
>+    }
>+  },

it seems like the mutability of "place:" URIs is the only reason this code has to exist, which is kinda high-maintenance. or am i misunderstanding what this method is for?

>+  
>+  /**
>+   * See nsIRequestObserver.idl
>+   */
>+  onStopRequest: function LLL_onStopReqeust(request, context, status) {
>+    if (!Components.isSuccessCode(status)) {
>+      // Something went wrong; try to load again in 10 minutes.
>+      this._setResourceTTL(ERROR_EXPIRATION);
>+      this._isAborted = true;
>+    }

nit: just to reduce unnecessary confusion, probably should remove the "in 10 minutes" from the comment, as it sounds like we might change that value at some point

>+
>+  _setResourceTTL: function LLL__setResourceTTL(seconds) {
>+    var lmService = Cc[LS_CONTRACTID].getService(Ci.nsILivemarkService);
>+    var exptime = Date.now() + seconds;
>+    this._ans.setAnnotationInt64(this._livemark.feedURI,
>+                                 LMANNO_EXPIRATION, exptime, 0,
>+                                 Ci.nsIAnnotationService.EXPIRE_NEVER);
>+  },

lmService is defined, but not used for anything here, AFAICT.

r=me w/ these nits fixed

Also: Old livemarks in the moz_bookmarks_folders table still have the old contract ID as their "type" value, and therefore are no longer treated as livemarks after applying this patch. It's not a huge deal, because the only Places builds are not proper releases, so I don't think we need to worry about upgrading existing livemarks.
However, using the contract ID doesn't seem like a good choice for the long-run because of this. Is there a particular reason that the contract ID was used, or could we use the nsILivemarkService interface name instead?
Attachment #242298 - Flags: review?(dietrich) → review+
(Assignee)

Comment 20

11 years ago
Created attachment 242688 [details] [diff] [review]
patch w/ dietrich and philor's improvements
Attachment #242298 - Attachment is obsolete: true
Attachment #242298 - Flags: superreview?(sspitzer)
> +// Check every 1/2 hour
> +const EXPIRATION = 1800000;

If we're going to go all the way from pre-Places' "pref overridden 1 hour default" to Places C++'s "1 hour default with TODO to read the pref" to "30 minute default with no nod to the abandoned pref" I really think we should do it in a separate bug, not in the middle of 100KB of rewrite.
(Assignee)

Comment 22

11 years ago
(In reply to comment #21)
> > +// Check every 1/2 hour
> > +const EXPIRATION = 1800000;
> 
> If we're going to go all the way from pre-Places' "pref overridden 1 hour
> default" to Places C++'s "1 hour default with TODO to read the pref" to "30
> minute default with no nod to the abandoned pref" I really think we should do
> it in a separate bug, not in the middle of 100KB of rewrite.

I think the best course of action is to file a bug. The point of this patch is to get it in JS so we can rewrite (again) quickly.

(Assignee)

Comment 23

11 years ago
Created attachment 242716 [details] [diff] [review]
patch to check in
Attachment #242688 - Attachment is obsolete: true
(Assignee)

Comment 24

11 years ago
Created attachment 242717 [details] [diff] [review]
patch to check in
Attachment #242716 - Attachment is obsolete: true
(Assignee)

Comment 25

11 years ago
(In reply to comment #14)
> Created an attachment (id=242298) [edit]
> convert nsLivemarkService to JS
> 
> big patch, should get review from two people
> 

Eh, this got a lot of review. Yell at me if you want me to back it out. Happy to follow up with tweaks as well.
(Assignee)

Comment 26

11 years ago
Checking in browser/components/places/content/bookmarkProperties.js;
/cvsroot/mozilla/browser/components/places/content/bookmarkProperties.js,v  <--  bookmarkProperties.js
new revision: 1.25; previous revision: 1.24
done
Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.99; previous revision: 1.98
done
Checking in browser/components/places/content/menu.xml;
/cvsroot/mozilla/browser/components/places/content/menu.xml,v  <--  menu.xml
new revision: 1.58; previous revision: 1.57
done
Checking in browser/components/places/content/toolbar.xml;
/cvsroot/mozilla/browser/components/places/content/toolbar.xml,v  <--  toolbar.xml
new revision: 1.62; previous revision: 1.61
done
Checking in browser/components/places/tests/unit/head_bookmarks.js;
/cvsroot/mozilla/browser/components/places/tests/unit/head_bookmarks.js,v  <--  head_bookmarks.js
new revision: 1.2; previous revision: 1.1
done
Checking in browser/components/places/tests/unit/test_bookmarks.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_bookmarks.js,v  <--  test_bookmarks.js
new revision: 1.2; previous revision: 1.1
done
Checking in browser/components/places/tests/unit/test_livemarks.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_livemarks.js,v  <--  test_livemarks.js
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/components/build/nsToolkitCompsCID.h;
/cvsroot/mozilla/toolkit/components/build/nsToolkitCompsCID.h,v  <--  nsToolkitCompsCID.h
new revision: 1.21; previous revision: 1.20
done
Checking in toolkit/components/places/src/Makefile.in;
/cvsroot/mozilla/toolkit/components/places/src/Makefile.in,v  <--  Makefile.in
new revision: 1.22; previous revision: 1.21
done
Removing toolkit/components/places/src/nsBookmarksFeedHandler.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsBookmarksFeedHandler.cpp,v  <--  nsBookmarksFeedHandler.cpp
new revision: delete; previous revision: 1.14
done
Checking in toolkit/components/places/src/nsBookmarksHTML.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsBookmarksHTML.cpp,v  <--  nsBookmarksHTML.cpp
new revision: 1.26; previous revision: 1.25
done
Removing toolkit/components/places/src/nsLivemarkService.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsLivemarkService.cpp,v  <--  nsLivemarkService.cpp
new revision: delete; previous revision: 1.22
done
Removing toolkit/components/places/src/nsLivemarkService.h;
/cvsroot/mozilla/toolkit/components/places/src/nsLivemarkService.h,v  <--  nsLivemarkService.h
new revision: delete; previous revision: 1.12
done
RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsLivemarkService.js,v
done
Checking in toolkit/components/places/src/nsLivemarkService.js;
/cvsroot/mozilla/toolkit/components/places/src/nsLivemarkService.js,v  <--  nsLivemarkService.js
initial revision: 1.1
done
Checking in toolkit/components/places/src/nsPlacesModule.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsPlacesModule.cpp,v  <--  nsPlacesModule.cpp
new revision: 1.2; previous revision: 1.1
done
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 27

11 years ago
I opened bug 357222 on feed refresh intervals.
Depends on: 357237
(Assignee)

Comment 28

11 years ago
the powers that be want another review here, but it can stay in the tree
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

11 years ago
Attachment #242717 - Flags: review?(sspitzer)
Attachment #242717 - Flags: review?(sspitzer) → review+
(Assignee)

Updated

11 years ago
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.