Closed Bug 353434 Opened 14 years ago Closed 13 years ago

Remove C++ feed parsing for livemarks on trunk

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: sayrer, Assigned: sayrer)

References

Details

Attachments

(2 files, 12 obsolete files)

4.00 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
109.89 KB, patch
moco
: review+
Details | Diff | Splinter Review
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.
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.
Attached patch new livemark listener (obsolete) — Splinter Review
Attachment #239284 - Attachment is obsolete: true
Attached patch new livemark listener (obsolete) — Splinter Review
Attachment #239347 - Attachment is obsolete: true
Attachment #239348 - Attachment is obsolete: true
Attached patch fix syntax goof (obsolete) — Splinter Review
Attachment #239413 - Attachment is obsolete: true
Attachment #239419 - Attachment is obsolete: true
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
Attachment #239518 - Attachment is obsolete: true
also need tests and leak checks
Attachment #240732 - Attachment is obsolete: true
Blocks: 355772
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+
Blocks: 356321
Attached patch add unit tests to the diff (obsolete) — Splinter Review
Attachment #241255 - Attachment is obsolete: true
Attached patch convert nsLivemarkService to JS (obsolete) — Splinter Review
big patch, should get review from two people
Attachment #242292 - Attachment is obsolete: true
Attachment #242298 - Flags: superreview?(sspitzer)
Attachment #242298 - Flags: review?(dietrich)
hmm, +722/-1930. Not bad.
"<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?
(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+
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.
(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.

Attached patch patch to check in (obsolete) — Splinter Review
Attachment #242688 - Attachment is obsolete: true
Attachment #242716 - Attachment is obsolete: true
(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.
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
Closed: 13 years ago
Resolution: --- → FIXED
I opened bug 357222 on feed refresh intervals.
Depends on: 357237
the powers that be want another review here, but it can stay in the tree
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #242717 - Flags: review?(sspitzer)
Attachment #242717 - Flags: review?(sspitzer) → review+
Status: REOPENED → RESOLVED
Closed: 13 years ago13 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.