Not only that, it can contend for a lock! Bad!

I don't think we need this before we merge the places branch, but I think we should really really fix this before final.

I think part of this is bug 613477, the star checks for livemarks! Livemarks themselves instead should start largely delayed something like 15 seconds after browser startup, and they delay updates even more.

I don't see any other traffic right now, thus I'm marking as fixed on the branch. If you notice anything I missed, feel free to comment regarding it.

I was too optimistic, I just ran txul on my Mac and I'm seeing livemarks service startup in the PRLog. We should be initing it after 15 seconds, so considering the timeout in talos tests, it's most likely still a problem. Not someting that runs when we open a window, but it's still part of one of the txul cycles.

So, I have a large rewrite that makes livemarks children no more bookmarks, and livemarks service API async (it runs a single async query on the service startup and nothing more). This has various advantages: - faster bookmarks queries (no need to filter out) - faster locationbar queries (no need to filter out) - no mainthread io - if a livemark is not used, doesn't consume resources (load-on-demand) - no special type, a livemark is just an _empty_ folder with 2 annotations. - largely simplified API and code, no crazy delayed startup, delayed loading, chunking or whatever. - simplifies migration the the new schema, we have just bookmarks, folders and separators, nice for a simplified hierarchy table. - code is in the frontend views, so no weight for implementers not using the feature, like mobile. The remaining issues are: - favicons and visited status liveupdate. The "fake" implementation doesn't distinguish visited articles from unvisited so far, nor it auto updates when an entry is visited. I'll see if there's an easy way out, the icon would be enough. - Sync, its code and tests have to be fixed. the good news is that the new approach should be easier, just requires to save a folder and a couple annotations, no special ignores or excludes. Ideally if Sync already manages annotations on folders, we are done, apart using the new API where possible. - tests, there are about 30 to be fixed, I may decide to remove some though, where there is duplication. - addons: I could probably let in the old deprecated API for a while, as a compatibility shim.

checkpointing work.

This is the new interface i've implemented, it worked pretty well as a replacement. The new service loads and reloads livemarks on demand, reloading on a timer can eventually be implemented externally, I wanted to reduce the feature to the bare minimum we need, since it's the direction we need for faster rewrites. The interface is based on callbacks: livemarks.addLivemark(arguments, function (aStatus, aLivemark) { if (Components.isSuccessCode(aStatus)) { aLivemark.getEntries(function (aStatus, aEntries) { if (Components.isSuccessCode(aStatus)) { // do something with aEntries array } }); } }); And alternatively getLivemark if one exists, and aLivemark.reload() to force a fetch. While I finish reordering the rewrite patch I'd appreciate to get some feedback on this (also because at the last minute I decided to split this new interface and completely deprecate the old one, as we did for history and favicons, so I have to rewrite a small part). Remaining issues: - fix Sync - add an isVisited interface to mozIAsyncHistory to mark fixed children (And we want that interface soon or later)

ehr, I noticed there is some indentation error, will fix those locally, looks like I did something crazy.

Comment on attachment 574655 [details] [diff] [review] Proposed interface Review of attachment 574655 [details] [diff] [review]: ----------------------------------------------------------------- I would also like to know if Sync may have issues with this interface. Note that livemarks will continue to be bookmarks folders, but they will be simpler: empty folders with 2 annotations (feedURI and siteURI).

Sorry, forgot to fix some wrong comments.

saving work

Comment on attachment 574671 [details] [diff] [review] Proposed interface v1.1 Review of attachment 574671 [details] [diff] [review]: ----------------------------------------------------------------- Will livemarks still appear in bookmark queries? If so, some of my comments might be moot… Gonna clear f? to indicate that I think this is incomplete wrt implementing something like Sync without dropping down to SQL. Thanks! ::: toolkit/components/places/mozIAsyncLivemarks.idl @@ +70,5 @@ > + * > + * @throws NS_ERROR_INVALID_ARG if aParentId is invalid, or aIndex is invalid, > + * or aFeedURI is not a valid nsIURI. > + * > + * @note To remove this livemark use nsINavBookmarksService::RemoveItem. That doesn't seem particularly elegant… @@ +76,5 @@ > + void addLivemark(in long long aParentId, > + in AString aName, > + in nsIURI aSiteURI, > + in nsIURI aFeedURI, > + in long aIndex, Optional GUID, please. (Generate one otherwise.) @@ +92,5 @@ > + * @throws NS_ERROR_INVALID_ARG if the id is invalid or a null callback is > + * passed into. > + */ > + void getLivemark(in long long aFolderId, > + in mozILivemarkCallback aCallback); We also need a way to find out which livemarks have been modified since some timestamp, to fetch them all, and determine whether a livemark with a given ID exists. (Perhaps other things too…) Given that this is a nice shiny new API, I'd rather avoid having to drop down to SQL, as we do in all our other engines! @@ +118,5 @@ > +}; > + > +[scriptable, uuid(9f6fdfae-db9a-4bd8-bde1-148758cf1b18)] > +interface mozILivemark : nsISupports > +{ Needs a GUID field and a modified time. "Modified" implies that something has changed about the livemark itself -- URL, name, etc. -- not its entries.

(In reply to Richard Newman [:rnewman] from comment #14) > Will livemarks still appear in bookmark queries? If so, some of my comments > might be moot… Yes, what changes from current situation is that they will be empty, rather than having children, so on Sync side this should be simpler to manage. To be clear: - before the patch: * a livemark is a bookmark folder with children representing articles and up to 5 annotations - after the patch: * a livemark is an empty bookmark folder with up to 2 annotations > ::: toolkit/components/places/mozIAsyncLivemarks.idl > @@ +70,5 @@ > > + * > > + * @throws NS_ERROR_INVALID_ARG if aParentId is invalid, or aIndex is invalid, > > + * or aFeedURI is not a valid nsIURI. > > + * > > + * @note To remove this livemark use nsINavBookmarksService::RemoveItem. > > That doesn't seem particularly elegant… heh, I may add a method to remove them, but it would just be an alias to removeItem. > @@ +76,5 @@ > > + void addLivemark(in long long aParentId, > > + in AString aName, > > + in nsIURI aSiteURI, > > + in nsIURI aFeedURI, > > + in long aIndex, > > Optional GUID, please. (Generate one otherwise.) I can add that, how do you handle these today? Consider this does the same as before, it's just supposed async. I'm all for improving the API for Sync. > @@ +92,5 @@ > > + * @throws NS_ERROR_INVALID_ARG if the id is invalid or a null callback is > > + * passed into. > > + */ > > + void getLivemark(in long long aFolderId, > > + in mozILivemarkCallback aCallback); > > We also need a way to find out which livemarks have been modified since some > timestamp, to fetch them all, and determine whether a livemark with a given > ID exists. (Perhaps other things too…) You already do this for bookmarks folders, ins't it? And this is just a bookmark folder as before. > @@ +118,5 @@ > > +}; > > + > > +[scriptable, uuid(9f6fdfae-db9a-4bd8-bde1-148758cf1b18)] > > +interface mozILivemark : nsISupports > > +{ > > Needs a GUID field and a modified time. "Modified" implies that something > has changed about the livemark itself -- URL, name, etc. -- not its entries. The uris are not supposed to change, my idea is that if you want to change the feedURI you should delete the livemark and make a new one. The only things that may change are the name and the position, that are tracked by bookmarks engine, I suppose?

Comment on attachment 574671 [details] [diff] [review] Proposed interface v1.1 I'm going to implement some change discussed with Richard and patch (or at least try) Sync, at least we'll have a clearer vision. So clearing requests for now, I have the dynamic containers patch that needs Robert's love first :)

Summarizing my conclusions from our chat for posterity: * Livemarks no longer have children, so we can remove the ignore checks in Sync. * No need for _setGUID for livemarks: use the constructor argument that mak will add :D * feedURI and siteURI will be immutable. Delete and re-add with same GUID. Did I miss anything?

feedURI will be immutable, siteURI won't be changeable through the api but may actually be updated by parsing the rss and finding a new siteURI into it.

note that still, you may ignore the siteURI change and next feed fetching will set it again...

(In reply to Marco Bonardo [:mak] from comment #18) > feedURI will be immutable, siteURI won't be changeable through the api but > may actually be updated by parsing the rss and finding a new siteURI into it. Yeah, my concern is about what happens when we pull down a record (not necessarily created by Firefox!) that includes a feedURI change. (E.g., if someone writes a NetNewsWire sync client…) We'll need a way to set that, because it's a meaningful operation. So long as we can chain delete-and-insert, and rely on those operations not failing, then I'm happy. (In reply to Marco Bonardo [:mak] from comment #19) > note that still, you may ignore the siteURI change and next feed fetching > will set it again... True. I'm happy declaring siteURI as a property that Firefox will ignore.

- uses jsvals so the interface is simpler to use from js (you can avoid specifying the optional properties). - get or remove a livemark either by id or guid, or even both, but guid will win in such a case. - A simple cpp function builds a jsval that looks like a mozILivemarkInfo object, so legacy cpp code can call into it - mozILivemarkInfo includes the guid and the parent id So, the code to append a livemark with an empty siteURI to the toolbar is something like: PlacesUtils.livemarks.addLivemark({ title: "some title" , parentId: PlacesUtils.toolbarFolderId , feedURI: some_nsIURI }, callback); and to get it: PlacesUtils.livemarks.getLivemark({ guid: known_guid }, callback); Apart the reload methods it's likely future bookmarking APIs will also include methods to handle livemarks, since it will then be easier for Sync to manage all the same. Anything I may still try to improve for Sync?

Comment on attachment 575949 [details] [diff] [review] Proposed interface v2.0 Review of attachment 575949 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, with the continuing caveat that — if this API is intended to stand vaguely alone — it needs to expose a modification (or, for immutable records, creation) timestamp for the records that it exposes. Put differently: the kind of information that we currently get in onChanged notifications, particularly the lastModified time, needs to be available as a search criterion and as part of each record. I imagine that querying will be taken care of by the bookmarks APIs, I imagine, but it seems to me that we'll get callbacks from those searches with mozILivemarkEntry results, and those need to expose both guids and lastModified, just as bookmarks do/should. The alternative to this is that we have to run a SQL query for each result we get, or bypass this API entirely, in order to get modification times. I want to avoid that. ::: toolkit/components/places/mozIAsyncLivemarks.idl @@ +76,5 @@ > + * > + * @throws NS_ERROR_INVALID_ARG if the id is invalid. > + */ > + [implicit_jscontext] > + void removeLivemark(in jsval aLivemarkInfo); Doesn't this need a callback? (Otherwise, is it synchronous (evil), or do you assume that the database removal will succeed (aieee)?)

I honestly see any method that is not ReloadLivemarks() to be overridden and likely duped by future bookmarks API. The mozILivemarkInfo interface will likely just inherit from a mozIBookmarkInfo interface, as well as creation and removal can be handled by the future bookmarks API, but we don't have it yet, so I have to provide an alternative in the meanwhile. I can't work on the whole new bookmarks API now. Adding a lastModified field to mozILivemarkInfo is feasible and won't be expensive. Regarding the callback in the removal, I suppose getting a null info object in the callback is fine for you? I don't think I ever cared about checking removals status!

(In reply to Marco Bonardo [:mak] from comment #23) > I can't work on > the whole new bookmarks API now. Nope, understood; just wanted to make sure that whatever does land in the mean time strikes a good balance of completeness! > Adding a lastModified field to mozILivemarkInfo is feasible and won't be > expensive. Works for me :) > Regarding the callback in the removal, I suppose getting a null info object > in the callback is fine for you? I don't think I ever cared about checking > removals status! Yup, just error/no error is enough for me. We care if we can't delete a record…

This should address all Richard's comments, I have updated patches for backend/frontend/tests... still looking at the Sync part.

Note regarding the old API: all methods are deprecated but working, apart start(), stopUpdateLivemarks(), setSiteURI() and setFeedURI() that are no-op. The first 2 because we won't update anymore livemarks if not on-demand, the last 2 becase setSiteURI was bogus (practically on the next reload we were just overwriting it, so no point in setting it) and setFeedURI makes no sense, the feedURI identifies a livemark, changing the feedURI is like creating a new livemark, thus is simpler to manage removal+addition of a new one.

Richard, I need someone from your team looking into this patch, I ran xpcshell but I have no idea how to check tps tests and functionality. I mostly removed instances of deprecated APIs, but I admit I had to cheat, since addLivemark is fake-async, but I don't see other ways, unless we completely rewrite the Sync bookmarks engine. Once bookmarks have an async API, and the sync bookmarks engine uses it, we can go back and make livemarks creation properly asynchronous (deleting the old synchronous API). Note I also killed the setSiteURI and setFeedURI tests and sync, I don't think Sync should take care of those, but you may disagree (that's why I think it's better if someone on your side finishes the changes), Places won't support anymore setting those from the API. I attached all the patches so you can apply them to work on the Sync part.

(In reply to Marco Bonardo [:mak] from comment #30) > Richard, I need someone from your team looking into this patch… I will take a look toward the end of this week. (Thanksgiving and travel…)

Sure, I'm going to proceed with my queue in the meanwhile, holy mq.

OK, I'm at a level where I consider the functionality at an acceptable level. I'll attach patches and make a trybuild.

BEWARE: don't use your profile with these builds, they contain a schema bump. https://tbpl.mozilla.org/?tree=Try&rev=f410d4065eba http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mak77@bonardo.net-f410d4065eba

Comment on attachment 577945 [details] [diff] [review] frontend v2.2 I noticed some warnings due to frontend changes, so I'm temporarily putting this out of the review batch to look into them.

This solves the warnings I noticed. Some of the views code will improve with time, but I don't think we should block this on perfection.

And updated builds may appear here (see comment 37 before trying) http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mak77@bonardo.net-83f17ac71221

Comment on attachment 577943 [details] [diff] [review] interface v2.2 Review of attachment 577943 [details] [diff] [review]: ----------------------------------------------------------------- why are mozILivemark and mozILivemarkInfo separate interfaces? no red flags, otherwise.

(In reply to Dietrich Ayala (:dietrich) from comment #41) > Comment on attachment 577943 [details] [diff] [review] [diff] [details] [review] > interface v2.2 > > Review of attachment 577943 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > why are mozILivemark and mozILivemarkInfo separate interfaces? no red flags, > otherwise. well, one describes the livemark the other one has methods to interact with it and any stuff that is not "input". First I separated them because I think mozILivemarkInfo in future will just be merged or derive from a mozIBookmarkInfo (we already have mozIVisitInfo and mozIPlaceInfo, so it's being added to the family). mozILivemarkInfo is the const object you pass to methods, while mozILivemark is a live-wrapper that the callbacks will hand back to you, to actually interact with the livemark. Would be crazy to ask the caller to pass into methods as input.

Comment on attachment 577944 [details] [diff] [review] backend v2.2 Review of attachment 577944 [details] [diff] [review]: ----------------------------------------------------------------- some initial comments. should get feedback review from a JS person on the livemarkInfoToJSVal bit in Helpers.* ::: toolkit/components/places/Database.cpp @@ +1339,5 @@ > +Database::MigrateV14Up() > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + // Livemarks children are no more bookmarks. nit: s/more/longer/ and make clear in the comment what the query is actually doing. ::: toolkit/components/places/PlacesUtils.jsm @@ +619,5 @@ > + uri = PlacesUtils.annotations.getItemAnnotation(bNode.itemId, this.LMANNO_SITEURI); > + } catch (ex) { > + uri = PlacesUtils.annotations.getItemAnnotation(bNode.itemId, this.LMANNO_FEEDURI); > + } > + return uri + NEWLINE + bNode.title; move to a function/method since this whole bit is duplicated below. DRY. @@ +2809,5 @@ > } > > + // if the item is a livemark container we will not save its children. > + if (PlacesUtils.annotations.itemHasAnnotation(aItemId, > + PlacesUtils.LMANNO_FEEDURI)) since livemarks are still a thing, why remove a helper like itemIsLivemark? ::: toolkit/components/places/nsLivemarkService.js @@ +96,4 @@ > > +/** > + * Serializes a callback to the Places Storage async thread, to ensure any > + * peding changes have been applied. typo nit: pending @@ +98,5 @@ > + * Serializes a callback to the Places Storage async thread, to ensure any > + * peding changes have been applied. > + * > + * WARNING: Any external LivemarkService API should go through this, to properly > + * access the internal cache. what does this comment mean? please elaborate the comment such that our children's children can understand it :) @@ +103,5 @@ > + * > + * @param aCallback > + * The callback function to be invoked. > + */ > +function enqueueToAsyncThread(aCallback) wow, this is super hack. is there a bug filed to fix this in the platform? @@ +126,5 @@ > +//////////////////////////////////////////////////////////////////////////////// > +//// LivemarkService > + > +function LivemarkService() > +{ how/when is this initialized? first-use? @@ +277,5 @@ > + }); > + this._guids[guid] = id; > + } > + stmt.finalize(); > + }, seems about all the same code as in the ctor for filling cache async (the statement type aside). can we share that in a method, passing in the statement type? DRYFTW @@ +389,5 @@ > + { > + if (!aFeedURI || !(aFeedURI instanceof Ci.nsIURI)) { > + throw Cr.NS_ERROR_INVALID_ARG; > + } > + this._reportDeprecatedMethod(); report deprecated usage before param checking, here and elsewhere @@ +428,5 @@ > + > + // The addition is done synchronously due to the fact importExport service > + // and JSON backups require that. The notification is async though. > + // Once bookmarks are async, this may be properly fixed. > + let livemark; never used outside of the try block, unnecessary? @@ +448,5 @@ > + } > + > + // Updating the cache even if it has not yet been populated doesn't matter, > + // it will just be overwritten. But must be done now, otherwise the > + // service would return broken data during notifications. i don't understand - how would the service return broken data during notifications?

(In reply to Dietrich Ayala (:dietrich) from comment #43) > should get feedback review from a JS person on the livemarkInfoToJSVal bit > in Helpers.* Ok, this may happen later too, to avoid having intermixed reviews. > @@ +2809,5 @@ > > } > > > > + // if the item is a livemark container we will not save its children. > > + if (PlacesUtils.annotations.itemHasAnnotation(aItemId, > > + PlacesUtils.LMANNO_FEEDURI)) > > since livemarks are still a thing, why remove a helper like itemIsLivemark? Because it's synchronous and we can't allow that. Btw I think I did not remove it, I added a deprecated warning for now, but I don't want to use it in internal code. > @@ +98,5 @@ > > + * Serializes a callback to the Places Storage async thread, to ensure any > > + * peding changes have been applied. > > + * > > + * WARNING: Any external LivemarkService API should go through this, to properly > > + * access the internal cache. > > what does this comment mean? please elaborate the comment such that our > children's children can understand it :) I hope it will be fixed properly by then! Btw, any API should enqueue its work to the async thread since the cache is populated asynchronously, and we can't use it before it's populated. Likely when we will be able to kill the synchronous API this may be simplified. > @@ +103,5 @@ > > + * > > + * @param aCallback > > + * The callback function to be invoked. > > + */ > > +function enqueueToAsyncThread(aCallback) > > wow, this is super hack. is there a bug filed to fix this in the platform? This was working, but it was explicitly removed, so the filed fix was actually to disallow pushing js runnables to other threads. I posted the bug number that did that in the comment. > @@ +126,5 @@ > > +//////////////////////////////////////////////////////////////////////////////// > > +//// LivemarkService > > + > > +function LivemarkService() > > +{ > > how/when is this initialized? first-use? yes, like any other service. There is no more the need to enqueue work and do crazy hacks since it will initialize its cache asynchronously. > @@ +277,5 @@ > > + }); > > + this._guids[guid] = id; > > + } > > + stmt.finalize(); > > + }, > > seems about all the same code as in the ctor for filling cache async (the > statement type aside). can we share that in a method, passing in the > statement type? DRYFTW nope the Storage APIs are different, trying to share would make an unreadable code imo. Btw the sync part can be removed once we remove the old nsILivemark inferface from the code base (a couple releases after the deprecation). > @@ +448,5 @@ > > + } > > + > > + // Updating the cache even if it has not yet been populated doesn't matter, > > + // it will just be overwritten. But must be done now, otherwise the > > + // service would return broken data during notifications. > > i don't understand - how would the service return broken data during > notifications? Well, you ask to create a livemark, it is created sychronously (for now) since bookmarks service is synchronous and other components like importExport or json backup require it to be synchronous. When it's created it fires bookmarks and annotations notifications. If you listen to a notification and use one of the deprecated synchronous APIs to ask if it's a livemark, you get the wrong answer.

Comment on attachment 577944 [details] [diff] [review] backend v2.2 Review of attachment 577944 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/nsLivemarkService.js @@ +296,5 @@ > + this._reportDeprecatedMethod(); > + this._ensureSynchronousCache(); > + > + if (!aParentId || aParentId < 1 || > + aIndex < -1 || why not default to default index? ::: toolkit/components/places/nsPlacesImportExportService.cpp @@ +990,5 @@ > > if (frame.mPreviousFeed) { > // The is a live bookmark. We create it here since in HandleLinkBegin we > // don't know the title. > + jsval livemark = livemarkInfoToJSVal( nit: indent @@ +1856,5 @@ > + feedSpec); > + > + NS_ENSURE_SUCCESS(rv, rv); > + nsCOMPtr<nsIURI> feedURI = do_CreateInstance(NS_SIMPLEURI_CONTRACTID); > + rv = feedURI->SetSpec(NS_ConvertUTF16toUTF8(feedSpec)); do you need to convert to nsIURI here anymore now that we're not using livemarks.getFeedURI? we're converting right back to string below in WriteEscapedUrl(). @@ +1874,5 @@ > + rv = mAnnotationService->GetItemAnnotationString(folderId, > + NS_LITERAL_CSTRING(LMANNO_SITEURI), > + siteSpec); > + nsCOMPtr<nsIURI> siteURI = do_CreateInstance(NS_SIMPLEURI_CONTRACTID); > + rv = siteURI->SetSpec(NS_ConvertUTF16toUTF8(siteSpec)); ditto

Comment on attachment 578132 [details] [diff] [review] frontend v2.3 Review of attachment 578132 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/places/content/browserPlacesViews.js @@ +174,2 @@ > // If a static menuitem is selected, or if the root node is selected, > // the insertion point is inside the folder, at the end. maybe update the comment below? ::: browser/components/places/content/controller.js @@ +211,4 @@ > case "placesCmd_reload": > // Livemark containers > var selectedNode = this._view.selectedNode; > + return selectedNode && !!selectedNode._feedURI; nit: why prefix with _ if you're accessing it publicly? ::: browser/components/places/src/PlacesUIUtils.jsm @@ +1050,5 @@ > + aCallback(aNode, aIsVisited); > + }); > + }, > + > + newFakePlacesNode: function PUIU_newFakePlacesNode(aOverrides) { plz add jsdoc for this

(In reply to Dietrich Ayala (:dietrich) from comment #46) > ::: browser/components/places/content/controller.js > @@ +211,4 @@ > > case "placesCmd_reload": > > // Livemark containers > > var selectedNode = this._view.selectedNode; > > + return selectedNode && !!selectedNode._feedURI; > > nit: why prefix with _ if you're accessing it publicly? it's an expando, our code style on them is to prepend with _ as if it was private. I'm not sure what's the global convention but it's internally consistent.

(In reply to Dietrich Ayala (:dietrich) from comment #45) > ::: toolkit/components/places/nsLivemarkService.js > @@ +296,5 @@ > > + this._reportDeprecatedMethod(); > > + this._ensureSynchronousCache(); > > + > > + if (!aParentId || aParentId < 1 || > > + aIndex < -1 || > > why not default to default index? The existing api throws, I'm just respecting the idl. Plus I feel like it's better to not hide caller's errors. > @@ +1856,5 @@ > > + feedSpec); > > + > > + NS_ENSURE_SUCCESS(rv, rv); > > + nsCOMPtr<nsIURI> feedURI = do_CreateInstance(NS_SIMPLEURI_CONTRACTID); > > + rv = feedURI->SetSpec(NS_ConvertUTF16toUTF8(feedSpec)); > > do you need to convert to nsIURI here anymore now that we're not using > livemarks.getFeedURI? we're converting right back to string below in > WriteEscapedUrl(). No, and I actually found a bug looking at this...

Richard, feedback ping?

Comment on attachment 577943 [details] [diff] [review] interface v2.2 Review of attachment 577943 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! Sorry for the delay. ::: toolkit/components/places/mozIAsyncLivemarks.idl @@ +140,5 @@ > + */ > + readonly attribute long index; > + > + /** > + * Time when this livemark was last modified. Probably helpful to mention that modifying *contents* doesn't count.

Comment on attachment 576781 [details] [diff] [review] Sync v2.0 Review of attachment 576781 [details] [diff] [review]: ----------------------------------------------------------------- A few nits, and one potentially serious pitfall. Clearing flag until that's addressed. ::: services/sync/modules/engines/bookmarks.js @@ +647,2 @@ > return "livemark"; > return "folder"; If you're touching this, please add some { }. (And see later comment about helper.) @@ +742,5 @@ > + , index: PlacesUtils.bookmarks.DEFAULT_INDEX > + , feedURI: Utils.makeURI(record.feedUri) > + , siteURI: siteURI > + , guid: record.id > + }, See later style comment. @@ +751,5 @@ > + ", " + aLivemark.siteURI.spec + ", " + > + aLivemark.feedURI.spec + ", GUID " + > + aLivemark.guid); > + } > + }).bind(this) Is there any possibility of addLivemark failing? If so, this approach needs to be rethought, because tracking of failed inserts -- or syncs -- will stop working. (Sync will catch exceptions higher up and abort appropriately. That won't happen if you call a callback instead of throwing.) We would need to either spin the event loop, guarantee that we won't throw here, or wait for our async rewrite to finish before landing this change. ::: services/sync/tests/unit/test_bookmark_engine.js @@ +355,5 @@ > let oldID = store.idForGUID(oldR.id); > _("Old ID: " + oldID); > do_check_eq(bms.getItemType(oldID), bms.TYPE_FOLDER); > + do_check_false(PlacesUtils.annotations > + .itemHasAnnotation(oldID, PlacesUtils.LMANNO_FEEDURI)); This pattern is popping up a lot. Perhaps PlacesUtils needs to grow a helper function or two? ::: services/sync/tps/extensions/tps/modules/bookmarks.jsm @@ +805,5 @@ > + { parentId: this.props.folder_id > + , title: this.props.livemark > + , sireURI: siteURI > + , feedURI: Services.io.newURI(this.props.feedUri, null, null) > + , index: PlacesUtils.bookmarks.DEFAULT_INDEX Nittery: This is generally not our object style in services, which is: https://mxr.mozilla.org/services-central/source/services-central/services/sync/modules/jpakeclient.js#514 I'd put this in a local variable for clarity.

Comment on attachment 578132 [details] [diff] [review] frontend v2.3 While I work on the Sync part, I'd like to get some feedback from Mano, since I'm not super happy with some of the hacks here

Comment on attachment 578132 [details] [diff] [review] frontend v2.3 Feedbacked over IRC. To summarize: 1. Views should only take care of: (a) checking if a certrain folder-id is a livemark (b) if so, let the livemark service know of their container (registerForUpdate(aContainer)) 2. The livemark service will then call invalidateContainer * refreshLivemark should therefore take no callback * 3. The livemark service should provide an async getNodes methods which will produce the fake nodes. 4. If possible, most, if not all, of livemark-specific interfaces here should be completely avoided. Liveamark children are not stored, so we shouldn't use our future db-ifaces to represent them. Rather, result-nodes api should be used.

This should address previous comments regarding Sync.

Comment on attachment 595733 [details] [diff] [review] frontend v3.0 Mano, this implements something more similar to what you suggested. Please let me know if may work for you.

not setting feedback again, Mano please consider the feedback as a global flag on the approach. So far I think I've fixed all the bugs I found using it, the only remaining defect is bug 725964, though I don't think we should block on it. Surely some feed like planet mozilla is a bit more annoying to open, but removing livemarks work from hitting at random times is more important atm.

I'll review this on Tuesday morning, if that's ok with you.

Comment on attachment 595735 [details] [diff] [review] Sync v3.0 Review of attachment 595735 [details] [diff] [review]: ----------------------------------------------------------------- r+ with simplifications :D ::: services/sync/modules/engines/bookmarks.js @@ +632,5 @@ > let type = bms.getItemType(itemId); > > switch (type) { > case bms.TYPE_FOLDER: > + if (PlacesUtils.annotations.itemHasAnnotation(itemId, FEEDURI_ANNO)) { Is there any reason not to implement itemIsLivemark in PlacesUtils? Also, let's just use PlacesUtils.LMANNO_FEEDURI here and kill FEEDURI_ANNO. @@ +748,5 @@ > + aLivemark.guid); > + spinningCb(); > + } > + else { > + spinningCb.throw(aStatus); This isn't necessary. spinningCb takes a data argument, which is returned from wait(). So you can just: PlacesUtils.livemarks.addLivemark(livemark, spinningCb); let aStatus = spinningCb.wait(); … in which we assume that the created livemark has the fields that we asked for. If we care about the result, then you can do something like: PlacesUtils.livemarks.addLivemark(livemark, function (aStatus, aLivemark) { spinningCb([aStatus, aLivemark]) }); let [aStatus, aLivemark] = spinningCb.wait(); @@ +770,5 @@ > } > > + if (newId) { > + this._log.trace("Setting GUID of new item " + newId + " to " + record.id); > + this._setGUID(newId, record.id); Add a comment here that addLivemark already sets its GUID, so we don't need to do so here. @@ +1034,3 @@ > record = new Livemark(collection, id); > + record.feedUri = > + PlacesUtils.annotations.getItemAnnotation(placeId, FEEDURI_ANNO); This anno is a string, right?

(In reply to Richard Newman [:rnewman] from comment #64) > Comment on attachment 595735 [details] [diff] [review] > Sync v3.0 > > Review of attachment 595735 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ with simplifications :D > > ::: services/sync/modules/engines/bookmarks.js > @@ +632,5 @@ > > let type = bms.getItemType(itemId); > > > > switch (type) { > > case bms.TYPE_FOLDER: > > + if (PlacesUtils.annotations.itemHasAnnotation(itemId, FEEDURI_ANNO)) { > > Is there any reason not to implement itemIsLivemark in PlacesUtils? Yes, there is already and is what I want to get rid of. Soon I'll begin the work on async bookmarks and the Sync engine won't need to do this synchronous stuff, I want to deprecate the livemark APIs before though. > @@ +1034,3 @@ > > record = new Livemark(collection, id); > > + record.feedUri = > > + PlacesUtils.annotations.getItemAnnotation(placeId, FEEDURI_ANNO); > > This anno is a string, right? yes, the annotations api returns a string here

Back-end looks good overall, so this is a review more of seocond-pass review than feedback. > // Firefox 12 uses schema version 17. > >+ if (currentSchemaVersion < 18) { >+ rv = MigrateV18Up(); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+ >+ // Firefox 13 uses schema version 18. >+ Hrm, as nothing actually changes in the schema (we're just removing rows, right?), it seems better to do this the first time the livemark service is initialized... which, hrm, may be never, That would would cause frecency bugs. Sigh. >diff --git a/toolkit/components/places/Helpers.cpp b/toolkit/components/places/Helpers.cpp >--- a/toolkit/components/places/Helpers.cpp >+++ b/toolkit/components/places/Helpers.cpp >+jsval >+livemarkInfoToJSVal(PRInt64 aId, >+ const nsACString& aGUID, >+ const nsAString& aTitle, >+ PRInt64 aParentId, >+ PRInt32 aIndex, >+ nsCOMPtr<nsIURI>& aFeedURI, >+ nsCOMPtr<nsIURI>& aSiteURI) >+{ :( This is just for import/export, right? Both of which are soon moving to JS... Maybe just add a comment noting this is temporay. However, if you think C++ users are likely to be used in general, maybe you should make AddLivemark accept either a js objct or a property bag. > /** > * Determines whether or not a node is a readonly folder. > * @param aNode >@@ -583,16 +588,32 @@ var PlacesUtils = { > let concreteId = PlacesUtils.getConcreteItemId(cNode); > return [PlacesUtils.getFolderContents(concreteId, false, true).root, true]; > } > > // If we didn't create our own query, do not alter the node's open state. > return [cNode, false]; > } > >+ function gatherLivemarkUrl(aNode) { >+ try { >+ return PlacesUtils.annotations.getItemAnnotation(aNode.itemId, >+ this.LMANNO_SITEURI); >+ } catch (ex) { >+ return PlacesUtils.annotations.getItemAnnotation(aNode.itemId, >+ this.LMANNO_FEEDURI); >+ } >+ } >+ Any reason not to always use the feed uri? The current behavior is quite unexpected IMO (Drag your planet mozilla feed on your tabbar). >+ this.livemarks.addLivemark( >+ { title: aData.title >+ , feedURI: feedURI >+ , parentId: aContainer >+ , index: aIndex >+ , lastModified: aData.lastModified >+ , siteURI: siteURI >+ }, >+ (function(aStatus, aLivemark) { >+ if (Components.isSuccessCode(aStatus)) { Note my later comment about this parameter. >- function PU_addLazyBookmarkObserver(aObserver) { >+ function PU_addLazyBookmarkObserver(aObserver, aWeakOwner) { > if (this._isServiceInstantiated("@mozilla.org/browser/nav-bookmarks-service;1")) { >- this.bookmarks.addObserver(aObserver, false); >+ this.bookmarks.addObserver(aObserver, !!aWeakOwner); nit: === true, or just pass aWeakOwner as is and let XPConnect fix it for you. >+ /** >+ * Generates a RESULT_TYPE_URI nsINavHistoryResultNode object. >+ * >+ * @param aParentNode >+ * The container parent node. >+ * @param aURI >+ * nsIURI for the node. >+ * @param aTitle >+ * Title for the node. >+ */ >+ newStaticURINode: Not sure if static is the best wording here, given that the frontend code considers them non-static (compared to "Open 'Planet Mozilla'") :) >+ let now = Date.now() * 1000; >+ return { Object.freeze >+XPCOMUtils.defineLazyGetter(PlacesUtils, "livemarks", function() { >+ return Cc["@mozilla.org/browser/livemark-service;2"]. >+ getService(Ci.nsILivemarkService). >+ QueryInterface(Ci.mozIAsyncLivemarks); >+}); > Can mozIAsyncLivemarks inherit nsILivemarkService for now? >diff --git a/toolkit/components/places/nsLivemarkService.js b/toolkit/components/places/nsLivemarkService.js >--- a/toolkit/components/places/nsLivemarkService.js >+++ b/toolkit/components/places/nsLivemarkService.js >+ addLivemark: function LS_addLivemark(aLivemarkInfo, >+ aLivemarkCallback) >+ { >+ if (!aLivemarkInfo || >+ (aLivemarkInfo.parentId && aLivemarkInfo.parentId < 1) || >+ !aLivemarkInfo.index || aLivemarkInfo.index < Ci.nsINavBookmarksService.DEFAULT_INDEX || >+ !aLivemarkInfo.feedURI || !(aLivemarkInfo.feedURI instanceof Ci.nsIURI) || >+ (!!aLivemarkInfo.siteURI && !(aLivemarkInfo.siteURI instanceof Ci.nsIURI)) || The instanceof check is enough for feedURI. For the other properties, I prefer "in" checks. >+ // The addition is done synchronously due to the fact importExport service >+ // and JSON backups require that. The notification is async though. >+ // Once bookmarks are async, this may be properly fixed. >+ try { >+ // Disallow adding a livemark inside another livemark. >+ if (aLivemarkInfo.parentId in this._livemarks) { >+ throw new Components.Exception("", Cr.NS_ERROR_INVALID_ARG); >+ } >+ >+ let livemark = new Livemark(aLivemarkInfo); >+ if (this._itemAdded && this._itemAdded.id == livemark.id) { >+ livemark.index = this._itemAdded.index; >+ if (!aLivemarkInfo.guid) { >+ livemark.guid = this._itemAdded.guid; >+ } >+ if (!aLivemarkInfo.lastModified) { >+ livemark.lastModified = this._itemAdded.lastModified; >+ } nit: some braces to remove here. >+ } >+ >+ // Updating the cache even if it has not yet been populated doesn't >+ // matter since it will just be overwritten. But must be done now, >+ // otherwise checking for the livemark using any deprecated synchronous >+ // API from an onItemAnnotation notification would give a wrong result.>+ nit: But /it/ must >+ if (aLivemarkCallback) { >+ this._onCacheReady(function LS_addLivemark_ETAT() { >+ try { >+ aLivemarkCallback.getLivemark(Cr.NS_OK, livemark); >+ } catch(ex) {} >+ }); >+ } >+ } catch (ex) { >+ if (aLivemarkCallback) { >+ this._onCacheReady(function LS_addLivemark_fail_ETAT() { >+ try { >+ aLivemarkCallback.getLivemark(ex.result, null); Not sure if there's much point in passing in passing the exception type. I would simplify it with a boolean. Your call. >+ } catch(ex) {} ex2 please. Maybe report the error? >+ }); >+ } >+ } Can you use try/catch/finally here and remove the duplicated code? >+ }, >+ >+ removeLivemark: function LS_removeLivemark(aLivemarkInfo, aLivemarkCallback) >+ { Some similar nits for this method ("in" checks, braces, error reporting). >+ _reloadNextLivemark: function LS__reloadNextLivemark() >+ { >+ // Find first livemark to be reloaded. >+ for (let id in this._livemarks) { ... >+ this._newReloadTimer(); Timers can be reinitialized. >+ * @param aLivemarkInfo >+ * Object containing information on the livemark. If the livemark is is is is. > > Livemark.prototype = { > loadGroup: null, >- locked: null, >+ id: -1, >+ title: "", >+ parentId: -1, >+ index: -1, >+ feedURI: null, >+ siteURI: null, >+ expireTime: 0, >+ nit: There's no need to set these in the prototype.

Please ignore the garbage in my first sentence :)

So, as I said, this looks great in general, and much less hacky than the previous iteration. Here are some random review comments for this part. I don't think there's much more to fix here, but once this is finalized, I would like to do another pass. Thanks for doing this work, really. >diff --git a/browser/components/distribution.js b/browser/components/distribution.js >--- a/browser/components/distribution.js >+++ b/browser/components/distribution.js > get _livemarkSvc() { > let svc = Cc["@mozilla.org/browser/livemark-service;2"]. >- getService(Ci.nsILivemarkService); >+ getService[Ci.mozIAsyncLivemarks]. >+ QueryInterface(Ci.nsILivemarkService); Any reason not to use PlacesUtils here? >diff --git a/browser/components/places/content/browserPlacesViews.js b/browser/components/places/content/browserPlacesViews.js >--- a/browser/components/places/content/browserPlacesViews.js >+++ b/browser/components/places/content/browserPlacesViews.js >+ // Set an expando on the node, controller will use it to build >+ // its metadata. Which reminds me, you should make sure that the new faked-nodes implementation also allows expands once they're xpconnect-wrapped. >+ aPopup._siteURIMenuseparator = document.createElement("menuseparator"); >+ aPopup.insertBefore(aPopup._siteURIMenuseparator, >+ aPopup.childNodes.item(aPopup._startMarker + 1)); nit: indent. >diff --git a/browser/components/places/content/editBookmarkOverlay.js b/browser/components/places/content/editBookmarkOverlay.js >--- a/browser/components/places/content/editBookmarkOverlay.js >+++ b/browser/components/places/content/editBookmarkOverlay.js >@@ -15,16 +15,17 @@ >+ * Marco Bonardo <mak77@bonardo.net> Don't! >diff --git a/browser/components/places/content/treeView.js b/browser/components/places/content/treeView.js >--- a/browser/components/places/content/treeView.js >+++ b/browser/components/places/content/treeView.js >+ // Persist constainers open status. containers. > let resource = this._getResourceForNode(node); > if (resource) { > const openLiteral = PlacesUIUtils.RDF.GetResource("http://home.netscape.com/NC-rdf#open"); > const trueLiteral = PlacesUIUtils.RDF.GetLiteral("true"); > >- if (node.containerOpen) >+ // Never persist open livemarks. >+ if (node.containerOpen || node._feedURI) Could you check _feedURI existence earlier? >diff --git a/browser/components/places/src/PlacesUIUtils.jsm b/browser/components/places/src/PlacesUIUtils.jsm >--- a/browser/components/places/src/PlacesUIUtils.jsm >+++ b/browser/components/places/src/PlacesUIUtils.jsm >@@ -1058,17 +1058,17 @@ var PlacesUIUtils = { > // If the left pane has already been built, use the name->id map > // cached in PlacesUIUtils. > for (let [name, id] in Iterator(this.leftPaneQueries)) { > if (aItemId == id) > queryName = name; > } > } > return queryName; >- } >+ }, > }; > hrm?

Comment on attachment 595731 [details] [diff] [review] interface v3.0 the interface got some changes, so I'd like a second-pass before moving to SR. Later or tomorrow I'll fix Mano's comments and we should be at a good point.

(In reply to Mano from comment #66) > >+jsval > >+livemarkInfoToJSVal(PRInt64 aId, > This is just for import/export, right? Both of which are soon moving to JS... > Maybe just add a comment noting this is temporay. Yes, this is just a temporary converter for importExportService, added a comment. > >+ function gatherLivemarkUrl(aNode) { > >+ try { > >+ return PlacesUtils.annotations.getItemAnnotation(aNode.itemId, > >+ this.LMANNO_SITEURI); > > Any reason not to always use the feed uri? The current behavior is quite > unexpected IMO Yes, the reason is that it's not part of this bug, I just kept the existing behavior. Btw I prefer as it is, question of personal taste probably. In any case the discussion does not belong here. > >+ newStaticURINode: > > Not sure if static is the best wording here, given that the frontend code > considers them non-static (compared to "Open 'Planet Mozilla'") :) Well, sometimes they come back! I changed static to dynamic, to recall the good old dynamic containers :p > >+ let now = Date.now() * 1000; > >+ return { > > Object.freeze I can't freeze it, cause livemarks service doesn't know what's the parentNode when passing out nodes and the view has to set it. Similar issue for accessCount that is updated asynchronously. I may seal it, if you wish, though I have some doubts regarding properties added by the views that just get these through nodes. > >+XPCOMUtils.defineLazyGetter(PlacesUtils, "livemarks", function() { > >+ return Cc["@mozilla.org/browser/livemark-service;2"]. > >+ getService(Ci.nsILivemarkService). > >+ QueryInterface(Ci.mozIAsyncLivemarks); > >+}); > > > > Can mozIAsyncLivemarks inherit nsILivemarkService for now? It could, though what's the point? Anyone can use the PlacesUtils.livemarks getter and nsILivemarkService should die asap. > >diff --git a/toolkit/components/places/nsLivemarkService.js b/toolkit/components/places/nsLivemarkService.js > nit: some braces to remove here. well, yes, we're not always consistent, the code style says to always brace, we sometimes do, sometimes not, I think I always braced in these patches so likely I won't play the "remove all braces" game for now :) > Not sure if there's much point in passing in passing the exception type. I > would simplify > it with a boolean. Your call. Well, I initially thought about a boolean, though in future we may want to distinguish some specific errors and the boolean would demonstrate ineffective, so I went the xpcom way, to stay a bit more generic. > >+ } catch(ex) {} > > ex2 please. Maybe report the error? I'm not sure, this is an exception thrown by the callback, not an internal error. I'd rather leave to the caller taking care of itself.

fixes typos I wrongly introduced with the latest changes.

Comment on attachment 595731 [details] [diff] [review] interface v3.0 Review of attachment 595731 [details] [diff] [review]: ----------------------------------------------------------------- looks ok. one question: you're marking the sync apis as deprecated and the new async apis as experimental, which leaves us with no recommended api. seems like we should mark the one as deprecated once we feel comfortable to remove the experimental designation from the new one. or just feel comfortable now, and not mark them experimental. ::: toolkit/components/places/mozIAsyncLivemarks.idl @@ +188,5 @@ > + * The nsINavHistoryResultObserver that should be notified of changes > + * to the livemark contents. > + */ > + void registerForUpdates(in nsINavHistoryContainerResultNode aContainerNode, > + in nsINavHistoryResultObserver aResultObserver); hmmm. i kinda want to use generic event listener interface here, but that's for future i guess.

(In reply to Dietrich Ayala (:dietrich) from comment #75) > looks ok. one question: you're marking the sync apis as deprecated and the > new async apis as experimental, which leaves us with no recommended api. > seems like we should mark the one as deprecated once we feel comfortable to > remove the experimental designation from the new one. or just feel > comfortable now, and not mark them experimental. I marked as experimental cause as soon as we start working on new bookmarking APIs we may find we need changes. Though the old API is really bad and should indeed be removed asap (would kill lots of internal complications). I guess I'll not mark the new one as experimental and evaluate changes in future as usual. > hmmm. i kinda want to use generic event listener interface here, but that's > for future i guess. Well there is a really good reason this is using a ResultObserver, and is that we have to notify an associated result of any internal change. I'm not sure how hard would be to move to more generic listeners off-hand.

Comment on attachment 597524 [details] [diff] [review] frontend v3.2 >diff --git a/browser/components/places/content/editBookmarkOverlay.js b/browser/components/places/content/editBookmarkOverlay.js >--- a/browser/components/places/content/editBookmarkOverlay.js >+++ b/browser/components/places/content/editBookmarkOverlay.js >+ this._staticTitle = aInfo && aInfo.staticTitle ? aInfo.staticTitle: ""; nit: space before : please. > _getItemStaticTitle: function EIO__getItemStaticTitle() { >- if (this._itemId == -1) >- return PlacesUtils.history.getPageTitle(this._uri); >- >- return PlacesUtils.bookmarks.getItemTitle(this._itemId); >+ let title = ""; >+ if (this._itemId == -1) { >+ title = PlacesUtils.history.getPageTitle(this._uri); >+ } >+ else { >+ title = PlacesUtils.bookmarks.getItemTitle(this._itemId); >+ } >+ return title || this._staticTitle; This is kind of messy. Wouldn't it better to always use the staticTitle, if available? Consider the following scenario: 1. My feed has entries with "static" urls, for example foo.tld/feedEntry.php.rules.not?entry=[1-20] 2. I open the feed, on monday, visit all entries. 3. I open the feed on Tuesday. The entries have the same urls, but with new titles (in the feed) So, if I'm not reading this wrong, until I decide to visit the entries anyway, they will show up with old titles - indicating that the feed hasn't changed much... Now, I know livemark-items-as-bookmarks were/are bad, but would it really be as bad to set places items (with no visits) for livemark items, so you could avoid some special casing here and there. If you do keep the new static titles, you should probably name it _staticTitleOverride, not _staticTitle. >+ // Hide the field if there can't be any title. >+ if (namePicker.value == "" && this._readOnly) { >+ this._hiddenRows.push("name"); >+ this._showHideRows(); >+ } What's this for? I'm not saying it's bad (or good), but it seems unrelated. >diff --git a/browser/components/places/content/treeView.js b/browser/components/places/content/treeView.js >--- a/browser/components/places/content/treeView.js >+++ b/browser/components/places/content/treeView.js > >+ _populateLivemarkContainer: function PTV__populateLivemarkContainer(aNode) { >+ PlacesUtils.livemarks.getLivemark({ id: aNode.itemId }, >+ (function (aStatus, aLivemark) { >+ let placesNode = aNode; >+ if (!Components.isSuccessCode(aStatus) || !placesNode.containerOpen) >+ return; >+ Please comment here that containerOpen is checked because getLivemark is async. > nodeHistoryDetailsChanged: > function PTV_nodeHistoryDetailsChanged(aNode, aUpdatedVisitDate, > aUpdatedVisitCount) { >+ if (!aNode.parent) >+ return missing |;| Anyway, why is this check needed? >+ if (aNode.parent._feedURI) { >+ // Find the node in the parent. >+ let parentRow = this._flatList ? 0 : this._getRowForNode(aNode.parent); >+ for (let i = parentRow; i < this._rows.length; i++) { >+ let child = this.nodeForTreeIndex(i); >+ if (child.uri == aNode.uri) { >+ if (aUpdatedVisitCount) >+ child._cellProperties.push(this._getAtomFor("visited")); >+ else >+ delete child._cellProperties; Not sure what's going on here. aUpdatedVisitCount could indicate visits removal, right? To simplify this, I would delete _cellProperties either way. >diff --git a/browser/themes/*stripe/places/places.css b/browser/themes/*stripe/places/places.css Note I haven't tested the css changes across platforms. r=mano otherwise.

Comment on attachment 597948 [details] [diff] [review] patch v.3.3 > I can't freeze it, cause livemarks service doesn't know what's the parentNode when passing out nodes and the view has to set it. Similar issue for accessCount that is updated asynchronously. 1. Actually, you might want the view to provide the parent node. Setting the parent node from the view may actually break when/if xpconnect gets in the middle, telling you that acessCount is read-only... [Hey, do you make sure to create different node-sets for the same-livemark accessed by different views?] 2. You wouldn't have a similar issue with accessCount if the async-listener was set at the same scope in which you create the node. Maybe PlacesUtils isn't a good place for the implementation, after all. Addressing the first point is actually important, I think. I can live without the other part (+Object.freeze), but it would be nice to fix it (and shouldn't be hard). More on xpconnect fun after the break! <br/> >+ registerForUpdates: function LM_registerForUpdates(aContainerNode, >+ aResultObserver) >+ { >+ // Fetching the node from the key breaks xpconnect due to the ClassInfo, >+ // so use an array for now. >+ if (!this._resultObservers[aContainerNode]) { >+ this._resultObservers[aContainerNode] = [aContainerNode, aResultObserver]; >+ } You've three better options: 1. Implement toString for your nodes. 2. Implement wrappedJSObject for your nodes 3. ***** WeakMap! ***** Just go with 3, please ;) >+ }, >+ >+ unregisterForUpdates: function LM_unregisterForUpdates(aContainerNode) >+ { >+ if (this._resultObservers[aContainerNode]) { >+ delete this._resultObservers[aContainerNode]; Haven't you just said this is broken? :) Well, given that XPConnect _might_ _not always_ be in the middle (js->js<-js->js), I can see how this may not be broken, but you should fix this anwyay. >+ updateURIVisitedStatus: >+ function LM_updateURIVisitedStatus(aURI, aVisitedStatus) >+ { >+ if (Object.keys(this._resultObservers).length > 0) { >+ let nodes = this.nodes; >+ for (let i = 0; i < nodes.length; i++) { >+ if (nodes[i].uri == aURI.spec) { >+ Services.tm.mainThread.dispatch((function () { >+ this._updateNodeVisitedStatus(nodes[i], aVisitedStatus); >+ }).bind(this), Ci.nsIThread.DISPATCH_NORMAL); I would just try/catch-report instead. BTW, do you know why does this xpconnect magic doesn't work for our runBatched stuff? > /** > * Terminates the livemark entry, cancelling any ongoing load. > * Must be invoked before destroying the entry. > */ > terminate: function LM_terminate() > { >- this.folderId = null; >+ // Ensure to not leak nodes or views. >+ for (let node in this._resultObservers) { >+ delete this._resultObservers[node]; >+ } GC won't catch this!? All the more reason to go with 3! >+ this._nodes = []; At this point, the nodes should be unreferenced by the views. Given that they're created in the global scope of this object, I cannot see how this could leak.

(In reply to Mano from comment #77) > > _getItemStaticTitle: function EIO__getItemStaticTitle() { > So, if I'm not reading this wrong, until I decide to visit the entries > anyway, they will show up with > old titles - indicating that the feed hasn't changed much... > > Now, I know livemark-items-as-bookmarks were/are bad, but would it really be > as bad to set places items (with no visits) for > livemark items, so you could avoid some special casing here and there. We can't keep places items with no visits, and we should not. If you remember in the past we ended up with exagerated sized databases cause annotations were keeping places alive for no valid reasons. Btw, I see your point about an entry that may change title and we'd get the wrong one from history here, so I'll probably consider this an override and try to use it before history when provided. > >+ // Hide the field if there can't be any title. > >+ if (namePicker.value == "" && this._readOnly) { > >+ this._hiddenRows.push("name"); > >+ this._showHideRows(); > >+ } > > What's this for? I'm not saying it's bad (or good), but it seems unrelated. I think I introduced this for some feed entry that had no title and was showing a pointless empty name field. I honestly don't remember if I did that before or after I introduces the _staticTitle override. I guess I may remove it and if and when we hit that case we can fix it apart. > > nodeHistoryDetailsChanged: > > function PTV_nodeHistoryDetailsChanged(aNode, aUpdatedVisitDate, > > aUpdatedVisitCount) { > >+ if (!aNode.parent) > >+ return > > missing |;| Anyway, why is this check needed? we may try to notify visits before the view has actually added the nodes to the view, so those nodes still have a null parent. It's worth a comment.

(In reply to Mano from comment #78) > > I can't freeze it, cause livemarks service doesn't know what's the parentNode when passing out nodes and the view has to set it. Similar issue for accessCount that is updated asynchronously. > > 1. Actually, you might want the view to provide the parent node. Setting > the parent node from the view may actually break when/if xpconnect gets in > the middle, telling you that acessCount is read-only... > > [Hey, do you make sure to create different node-sets for the > same-livemark accessed by different views?] No, and this is one of the issues I was telling you where we should have a getNodesForContainer(aContainer) instead of .nodes. would allow to create different node sets and associate the parent before handing out the node. As you said. > 2. You wouldn't have a similar issue with accessCount if the async-listener > was set at the same scope in which you create the node. Maybe PlacesUtils > isn't a good place for the implementation, after all. > > Addressing the first point is actually important, I think. I can live > without the other part (+Object.freeze), but it would be nice to fix it (and > shouldn't be hard). So you suggest not using PlacesUtils to generate the node, rather do that with an util in nsLivemarkService? Not sure how I may freeze the object for accessCount async setter. > >+ registerForUpdates: function LM_registerForUpdates(aContainerNode, > >+ aResultObserver) > >+ { > >+ // Fetching the node from the key breaks xpconnect due to the ClassInfo, > >+ // so use an array for now. > >+ if (!this._resultObservers[aContainerNode]) { > >+ this._resultObservers[aContainerNode] = [aContainerNode, aResultObserver]; > >+ } > > You've three better options: > 1. Implement toString for your nodes. > 2. Implement wrappedJSObject for your nodes > 3. ***** WeakMap! ***** > > Just go with 3, please ;) Sure, so as I asked you by mail, I need a separate internal representation for entries (at this point I'll just go back to the old [{uri, title}]) and use it to feed the nodes that will magically die when the view doesn't use them anymore. > >+ updateURIVisitedStatus: > >+ function LM_updateURIVisitedStatus(aURI, aVisitedStatus) > >+ { > >+ if (Object.keys(this._resultObservers).length > 0) { > >+ let nodes = this.nodes; > >+ for (let i = 0; i < nodes.length; i++) { > >+ if (nodes[i].uri == aURI.spec) { > >+ Services.tm.mainThread.dispatch((function () { > >+ this._updateNodeVisitedStatus(nodes[i], aVisitedStatus); > >+ }).bind(this), Ci.nsIThread.DISPATCH_NORMAL); > > I would just try/catch-report instead. Instead of what? I'm dispatching asynchronously to avoid blocking the ui for the whole loop. > BTW, do you know why does this xpconnect magic doesn't work for our > runBatched stuff? Not sure what you mean. > > terminate: function LM_terminate() > > { > >- this.folderId = null; > >+ // Ensure to not leak nodes or views. > >+ for (let node in this._resultObservers) { > >+ delete this._resultObservers[node]; > >+ } > > GC won't catch this!? It does, though we should try to avoid relying to deeply on GC, cause it's more expensive than doing manual cleanup, and it's easy in this case.

final iteration for SR

address comments and IRC discussion

fixes a couple obvious wrong javadocs

try builds, as usual don't use with your default profile till this lands. https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mak77@bonardo.net-d3bd08007f34/

changes discussed on IRC

Comment on attachment 599968 [details] [diff] [review] backend v4.1 r=mano!!!

Richard, could you please take a second look? I had to change a couple details (like moving modules importing since using PlacesUtils in the consts). And while here, I don't remember if there's a way to run tps tests and check they pass through Try or locally?

Comment on attachment 599994 [details] [diff] [review] Sync v4.1 Review of attachment 599994 [details] [diff] [review]: ----------------------------------------------------------------- Code looks fine with fixes below, and discussed on IRC. TPS error: TEST-UNEXPECTED-FAIL | test_sync.js | [phase2] Exception caught: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIAnnotationService.getItemAnnotation] Stack trace: resource://tps/bookmarks.jsm:866 < resource://tps/tps.jsm:409 < Bookmarks__verify()@resource://tps/tps.jsm:882 < resource://tps/tps.jsm:491 < resource://tps/tps.jsm:181 ::: services/sync/modules/engines/bookmarks.js @@ +638,5 @@ > return "folder"; > > case bms.TYPE_BOOKMARK: > let bmkUri = bms.getBookmarkURI(itemId).spec; > + if (bmkUri.search(/^place:/) == 0) { While we're touching this line: bmkUri.indexOf("place:") == 0 ::: services/sync/tps/extensions/tps/modules/bookmarks.jsm @@ +852,5 @@ > let feedURI = Services.io.newURI(this.props.feedUri, null, null); > + let lmFeedURISpec = > + PlacesUtils.annotations.getItemAnnotation(this.props.item_id, > + PlacesUtils.LMANNO_FEEDURI); > + if (feedURI.spec != lmFeedURIspec) { s/URIspec/URISpec

addresses comments and typos. According to IRC discussion passes TPS tests.

Comment on attachment 600032 [details] [diff] [review] Sync v4.2 Review of attachment 600032 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/sync/modules/engines/bookmarks.js @@ +638,5 @@ > return "folder"; > > case bms.TYPE_BOOKMARK: > let bmkUri = bms.getBookmarkURI(itemId).spec; > + if (bmkUri.search(/^place:/) == 0) { Humph. I'll fix this one later!

(In reply to Richard Newman [:rnewman] from comment #93) > > case bms.TYPE_BOOKMARK: > > let bmkUri = bms.getBookmarkURI(itemId).spec; > > + if (bmkUri.search(/^place:/) == 0) { > > Humph. I'll fix this one later! Ah sorry I didn't see we had 2 of those! I can fix it :)

Comment on attachment 599941 [details] [diff] [review] interface v4.1 >diff --git a/toolkit/components/places/mozIAsyncLivemarks.idl b/toolkit/components/places/mozIAsyncLivemarks.idl >+interface nsINavHistoryContainerResultNode; nit: this forward-declaration doesn't seem necessary since the relevant params in this interface are jsval. >+interface mozIAsyncLivemarks : nsISupports >+ * @param aLivemarkInfo >+ * mozILivemarkInfo object containing the required information to >+ * create a new livemark. What is that required information? >+ * @param aLivemarkInfo >+ * mozILivemarkInfo object containing the required information to >+ * fetch the livemark. Either the id or the guid must be valid. "mozILivemarkInfo object containing either an id or a guid of the livemark to remove" (same comment can be used with s/remove/retrieve/ for getLivemark) >+interface mozILivemarkCallback : nsISupports >+ * Invoked when a livemark is requested. s#requested#added/removed/retrieved# ? >+ void getLivemark(in nsresult aStatus, >+ in mozILivemark aLivemark); "getLivemark" is a little confusing for a general purpose callback interface (i.e. when used with removeLivemark). Doesn't matter too much since it's marked |function|, but how about just "callback"? And maybe mention that aLivemark can be null in the removeLivemark case, presumably? >diff --git a/toolkit/components/places/nsILivemarkService.idl b/toolkit/components/places/nsILivemarkService.idl > interface nsILivemarkService : nsISupports >+ * Reloads the livemark with this folder ID, whether or not it's expired. >+ * @param folderID The ID of the folder to be reloaded nit: looks like some stray tabs got left here.

while working on bug 721319 I noticed in some case I don't always correctly add a weak observer in PlacesUtils, so I'll have to make a small additional patch on top of this. Regardless I have to wait for the patch in bug 721319 since it touches the same code and may likely be backported to Aurora. Btw, thanks to everyone involved here for your kindness and availability.

addressed sr comments

unbitrot and changed callback name according to sr

addressed the last comment by Richard

this fixes weak ownership, I found we were using strong ownership when weak was instead requested. We should store the request with the observer.

Comment on attachment 600181 [details] [diff] [review] Sync v5.0 Review of attachment 600181 [details] [diff] [review]: ----------------------------------------------------------------- I love me some accurate flags.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4edcd7c29408 https://hg.mozilla.org/integration/mozilla-inbound/rev/f45eebebc41b https://hg.mozilla.org/integration/mozilla-inbound/rev/08a7bb3f5c92 https://hg.mozilla.org/integration/mozilla-inbound/rev/4814a8d64b8d https://hg.mozilla.org/integration/mozilla-inbound/rev/7925703fbce6

dev-doc-needed for the new interface (and marking the old one deprecated). addons-compat for the deprecation of nsILivemarkService and some of its methods are now no-op (start, stopUpdatingLivemarks, setFeedURI and setSiteURI).

https://hg.mozilla.org/mozilla-central/rev/4edcd7c29408 https://hg.mozilla.org/mozilla-central/rev/f45eebebc41b https://hg.mozilla.org/mozilla-central/rev/08a7bb3f5c92 https://hg.mozilla.org/mozilla-central/rev/4814a8d64b8d https://hg.mozilla.org/mozilla-central/rev/7925703fbce6

Is there a way to disable this feature?, so as to load the livemarks automatically, without the need of going to each livemarks and waiting for it to load ?

(In reply to Girish Sharma from comment #105) > Is there a way to disable this feature?, so as to load the livemarks > automatically, without the need of going to each livemarks and waiting for > it to load ? Until bug 725964 is at least made better, updating livemarks is expensive. We don't want our users to pay that cost they may not need (thus why the livemark load on access). That said, I'm evaluating making a really simple restartless add-on for users willing to pay that price.

(In reply to Marco Bonardo [:mak] from comment #106) > Until bug 725964 is at least made better, updating livemarks is expensive. > We don't want our users to pay that cost they may not need (thus why the > livemark load on access). Load after click is what really looks and feels snappy...

(In reply to dindog from comment #107) > (In reply to Marco Bonardo [:mak] from comment #106) > > Until bug 725964 is at least made better, updating livemarks is expensive. > > We don't want our users to pay that cost they may not need (thus why the > > livemark load on access). > Load after click is what really looks and feels snappy... Only when the Internet speed is really really fast, and the livemark site is connecting properly. Even though I have a fast Internet connection, still I have to wait atleast2-3 seconds for a livemark to populate from site like engadget, mozilla blog etc.

(In reply to dindog from comment #107) > Load after click is what really looks and feels snappy... Surely making streaming videos and online gaming lag for all users is a better option, btw you your complaining in a resolved bug, that is for sure the worst way to report issues.

(In reply to Marco Bonardo [:mak] from comment #109) > (In reply to dindog from comment #107) > > Load after click is what really looks and feels snappy... > > Surely making streaming videos and online gaming lag for all users is a > better option in this case, making an option is a better option IMHO.

Plus one for the above comment. This surely should have an option to enable or disable this setting. Can we have this bug reopened please ? Adding a pref should not be a big task and would easily get a r+ via an inter-diff.

(In reply to Girish Sharma from comment #111) > Can we have this bug reopened please ? no > Adding a pref should not be a big task and would easily get a r+ via an > inter-diff. neither, this is not our usual way to handle follow-up fixes. Please, stop using closed bugs for your issues, file your bug.

Just wondering: Did you forget to remove/change this code in editBookmarksOverlay.xul? http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/editBookmarkOverlay.xul 93 onblur="gEditItemOverlay.onFeedLocationFieldBlur();" 105 onblur="gEditItemOverlay.onSiteLocationFieldBlur();" Should I file a bug on this or is this intentional?

(In reply to Frank Wein [:mcsmurf] from comment #113) > Just wondering: Did you forget to remove/change this code in > editBookmarksOverlay.xul? > http://mxr.mozilla.org/mozilla-central/source/browser/components/places/ > content/editBookmarkOverlay.xul > 93 onblur="gEditItemOverlay.onFeedLocationFieldBlur();" > 105 onblur="gEditItemOverlay.onSiteLocationFieldBlur();" > > Should I file a bug on this or is this intentional? Hi, good catch, please file a bug, if you wish you may also attach a patch and I can review it. thanks.

Comment on attachment 600180 [details] [diff] [review] backend v5.0 >+ function gatherLivemarkUrl(aNode) { >+ try { >+ return PlacesUtils.annotations.getItemAnnotation(aNode.itemId, >+ this.LMANNO_SITEURI); >+ } catch (ex) { >+ return PlacesUtils.annotations.getItemAnnotation(aNode.itemId, >+ this.LMANNO_FEEDURI); >+ } >+ } >+ >+ function isLivemark(aNode) { >+ return PlacesUtils.nodeIsFolder(aNode) && >+ PlacesUtils.annotations.itemHasAnnotation(aNode.itemId, >+ this.LMANNO_FEEDURI); You forgot to switch the constants to use PlacesUtils...

(In reply to neil@parkwaycc.co.uk from comment #117) > You forgot to switch the constants to use PlacesUtils... bugs/patches welcome :)

(In reply to neil@parkwaycc.co.uk from comment #117) > You forgot to switch the constants to use PlacesUtils... bug 758201 will fix that.

For those of you looking for comm-central changeset a87aff0360cc, I accidentally quoted this bug instead of dependent bug 730837. Sorry about that.

(In reply to comment #120) > I accidentally quoted this bug instead of dependent bug 730837. Oops, I did it again, this time for comm-central changeset 9ff708bf4069...