The following code: bookmarksFile = dirService.get("profDef", Ci.nsILocalFile); bookmarksFile.append("bookmarks.html"); // Run the import let importer = Cc["@mozilla.org/browser/places/import-export-service;1"]. getService(Ci.nsIPlacesImportExportService); importer.importHTMLFromFile(bookmarksFile, true); ...no longer works, as the bookmarks file is inside omnijar. The nsiLocalFile value comes back as "<install>/defaults/profile/bookmarks.html" but this no longer exists. We need a new API to actually pull the bookmarks.html file out of omnijar. This breaks test modules relying on PlacesAPI.js, including: firefox/testAwesomeBar/testSuggestHistoryBookmarks.js

:dietrich pointed us at: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/PlacesUtils.jsm#1715 and http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/PlacesUtils.jsm#1769 ...for backing up and restoring bookmarks. I'll take the bug on.

See https://bug556644.bugzilla.mozilla.org/attachment.cgi?id=458864 for the new api that was added to read bookmark html files from arbitrary uris.

the PlacesUtils stuff is for json backups only, that are in a profile folder, I think the omnijar change here only affects default bookmarks in bookmarks.html. Btw, looks like you can take it from the resource uri as in comment 2

(In reply to comment #3) > Btw, looks like you can take it from the resource uri as in comment 2 Right, but while we are working on that part we also would like to reset the tags too. So using backup/restore will set everything to the initial state.

Yeah. I don't see any reason (yet) not to use the json backups. It actually seems like a better solution--not just because of tags, but because it'll truly restore the pre-test state. Yes, the pre-test state is the default bookmarks and that isn't -likely- to change. However, it's still really two separate things.

Seeing the test failure every day on the teardownModule for the test testAddBookmarkToMenu, does it make sense to disable the test until this is it fixed? It's counted as a top failure

hum I'm missing something, the code in nsBrowseglue has been updated to handle the import through a resource uri, can't you do the same here?

http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#871 http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#889

Marco, is there a way to trigger the complete reset of Places? That would be much better as only resetting the bookmarks.

deleting all backups, bookmarks.html and places.sqlite before launching browser is probably the best bet, but could not be feasible if file is locked (Windows sometimes hold locks longer)

It's more a question how we can do this during a test-run without having to shutdown Firefox. Can we simply call one of the BrowserGlue functions?

hm no, there has never been the need to reset bookmarks during application run... the only way is restore from json.

oh and the restore clearly won't reset history.

Ok, but restoring the bookmarks will also delete the tags. For clearing the history we can continue in using browserHistory.removeAllPages(). Geo, would you be able to check the latest comments so we can finally get those two tests into a state to not fail anymore? That would be brilliant.

tags? Well json restore preserves tags. restore from html does not, there is nothing really fancy I can suggest here :( We plan to make a new import/export component for after 4.0 that could solve this problem, most likely. But this is not a solution I can give you. If you need tags you have to use json for sure (PlacesUtils methods for now). If you can avoid them you can use importFromURI as in browserGlue.

Henrik: yes, it's on my radar. I'll see whether those two comments get us out of the woods for now.

(In reply to comment #15) > tags? Well json restore preserves tags. restore from html does not, there is > nothing really fancy I can suggest here :( Well, that would work fine. We always have to restore from html in that case. Thanks.

Move of Mozmill Test related project bugs to newly created components. You can filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.

Quick note: the @fileoverview is wrong (and already was), as you'll see in the diff. We should fix that, but let's deal with that in a separate update.

Comment on attachment 492554 [details] [diff] [review] Make restoreDefaultBookmarks() load from omnijar > /** >+* Instance of the observer service to gain access to the observer API nit: there has to be an indentation at the beginning of the line, so the stars are in the same column. >+var observerService = Cc["@mozilla.org/observer-service;1"]. >+ getService(Ci.nsIObserverService); nit: indentation in the second line > function restoreDefaultBookmarks() { >+ // Default bookmarks.html file now lives in jar, get via resource URI Instead of calling out that the default html file lives in a jar file, we should call out the real file name as reference. >+ const BOOKMARKS_RESOURCE = "resource:///defaults/profile/bookmarks.html"; >+ >+ // Observer topics we need to watch to know whether we're finished >+ const TOPIC_RESTORE_SUCCESS = "bookmarks-restore-success"; We agreed on that constants should live in the global scope. That way more in detail tests could even use those. >+ // Wait for it to be finished--the observer above will flip this flag >+ mozmill.utils.waitFor(function () { >+ return importSuccessful; >+ }, "Default bookmarks have finished importing"); Talked with Marco on IRC about the timeout value for this waitFor call. Normally the import should be done in under 5s but for dirty profiles with a lot of bookmarks it can take longer. Given that we want to do more in the future, we could use a timeout of 10s to be on the safe side in the next couple of months, also keeping in mind that people will run our tests on slower machines. > function removeAllHistory() { > const TOPIC_EXPIRATION_FINISHED = "places-expiration-finished"; [..] > // Set up an observer so we get notified when remove completes >- var observerService = Cc["@mozilla.org/observer-service;1"]. >- getService(Ci.nsIObserverService); > let observer = { > observe: function(aSubject, aTopic, aData) { > observerService.removeObserver(this, TOPIC_EXPIRATION_FINISHED); > finishedFlag.state = true; > } > } > observerService.addObserver(observer, TOPIC_EXPIRATION_FINISHED, false); Please file a bug (if not done yet) so we can also fix the removeAllHistory function. Otherwise good work!

> nit: there has to be an indentation at the beginning of the line, so the stars > are in the same column. Done > nit: indentation in the second line Done. > Instead of calling out that the default html file lives in a jar file, we > should call out the real file name as reference. Not actually 100% sure I agree, since the idea is you don't care where it lives anymore (it's abstracted by the resource) but I did change the comment to reflect that it's in omni.jar. > We agreed on that constants should live in the global scope. That way more in > detail tests could even use those. We didn't agree on this, and it's not part of the standards doc. In fact, it's not even consistent with the rest of this file. We did agree not to review on standards issues not reflected in the doc, unless they make code obviously unclear or introduce a bug, so I'm surprised by this one. However, since you feel strongly about it, I've weakened the consts by moving them to global scope, also necessitating name-changes so they wouldn't be overly generic. I've also raised the issue within our team re: whether this should be a standard, since I happen to strongly disagree. > Talked with Marco on IRC about the timeout value for this waitFor call. > Normally the import should be done in under 5s but for dirty profiles with a > lot of bookmarks it can take longer. Given that we want to do more in the > future, we could use a timeout of 10s to be on the safe side in the next couple > of months, also keeping in mind that people will run our tests on slower > machines. Awesome functionality catch, thanks! Done. > Please file a bug (if not done yet) so we can also fix the removeAllHistory > function. Otherwise good work! Yep, already done. https://bugzilla.mozilla.org/show_bug.cgi?id=613661

Comment on attachment 492802 [details] [diff] [review] Make restoreDefaultBookmarks() load from omnijar (v2) Looks good. While having it applied to my local tree, I immediately pushed the patch to default: http://hg.mozilla.org/qa/mozmill-tests/rev/d756280532d0 Anything we should backport? If not please close the bug as fixed.

Nope. I think we're good. Older branches still have the bookmarks as a separate file, AFAIK. Closing it out now.