337636 - Get bookmarks working on suiterunner.

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Mark Banner (:standard8)]

Currently bookmarks don't work on suiterunner because the way toolkit defines the directory service providers. We need a clone of nsBrowserDirectoryProvider.cpp to provide the missing entries.

Discussions on irc suggest that suite/profile would be a good place to put it, we could then put migration in suite/profile/migration.

It'll be named as nsSuiteDirectoryProvider.cpp as there are some mailnews items as well that should really go into it, but that'll be a seperate patch/bug.

Comment 1

[Mark Banner (:standard8)]

Attachment: First version

This might be the only version, but I'm still classing it as WIP for now - I need to just read it through a bit more.

Comment 2

[[e-mail]]

We don't explicitly back up the bookmarks file, instead we rely on the safe output stream not clobbering it unti we've written out a new file. Our bookmarks service also dynamically handles the browser.bookmarks.file pref.

Comment 3

[Mark Banner (:standard8)]

Attachment: Second version

Modified to remove the redundant code and tidy a few things up.

Comment 4

[[e-mail]]

Comment on attachment 221814 [details] [diff] [review]
Second version

>-DIRS +=		app
>+DIRS +=	app \
>+	profile \
>+	$(NULL)
Nit: the rest of the file uses double-tab-indent

>+  nsCOMPtr<nsIFile> file;
>+
>+  nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
>+                                       getter_AddRefs(parentDir));
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  rv = parentDir->Clone(getter_AddRefs(file));
Nit: declare file here.

>+  PRBool exists;
>+  if (NS_SUCCEEDED(file->Exists(&exists)) && !exists) {
>+      EnsureProfileFile(leafstr, parentDir, file);
>+  }
Nits: indent should be 2 spaces and you don't need braces.

>+  if (!strcmp(aKey, NS_APP_SEARCH_DIR_LIST)) {
Please convert this to an early return.

>+    nsCOMPtr<nsIProperties> dirSvc
>+      (do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID));
>+    if (!dirSvc)
>+      return NS_ERROR_FAILURE;
Please use the rv version of do_GetService so that you can return it
(don't forget to check the other occurrances of the same pattern).

>+static char const kContractID[] = "@mozilla.org/suite/directory-provider;1";
>...
>+NS_IMPL_NSGETMODULE(SuiteDirProvider, components)
Please move all the module stuff to the end of the file.

>+  defaults->AppendNative(aLeafName);
>+
>+  PRBool exists;
>+  rv = defaults->Exists(&exists);
>+  if (NS_FAILED(rv) || !exists)
>+    return;
>+
>+  defaults->CopyToNative(aParentDir, aLeafName);
You don't need all those checks; CopyToNative will fail, but we don't care.

>+nsSuiteDirectoryProvider::AppendingEnumerator::AppendingEnumerator
>+    (nsISimpleEnumerator* aBase,
>+     char const *const *aAppendList) :
>+  mBase(aBase),
>+  mAppendList(aAppendList)
>+{
>+  // Initialize mNext to begin.
>+  GetNext(nsnull);
>+}
AppendingEnumerator is too complex for what it we need it to do, which is to append "searchplugins" to each file enumerated by the base enumerator. However to retain some flexibility you might want to keep the leaf name as a parameter to the enumerator.

Comment 5

[[e-mail]]

Comment on attachment 221814 [details] [diff] [review]
Second version

oh, and you should add the new makefile to allmakefiles.sh too

Comment 6

[Mark Banner (:standard8)]

Attachment: Third version

Revised according to Neil's comments and added the allmakefiles.sh change that I'd previously forgotten. carrying forward r+, requesting sr.

Comment 7

[[e-mail]]

Comment on attachment 221892 [details] [diff] [review]
Third version

>+DIRS +=	app \
>+		profile \
>+		$(NULL)
[DIRS += \ <- possibly put a tab here somehwere
		app \ etc.]

>+REQUIRES	= \
>+		xpcom \
>+		string \
>+		xulapp \
>+		$(NULL)
>+
>+CPPSRCS = 	nsSuiteDirectoryProvider.cpp \
>+		$(NULL)
Inconsistent again!

>+ * The Original Code is the mozilla.org code.
[... is mozilla.org code]

>+    nsCAutoString                 mLeafName;
[nsDependentCString]

>+  nsresult rv = NS_GetSpecialDirectory(NS_APP_PROFILE_DEFAULTS_50_DIR,
>+                                       getter_AddRefs(defaults));
>+  if (NS_FAILED(rv))
>+    return;
This is the reverse of my previous nit - you could test defaults instead.


>+    nsCOMPtr<nsIFile> nextbase(do_QueryInterface(nextbasesupp));
>+    if (!nextbase)
>+      continue;
>+
>+    nextbase->Clone(getter_AddRefs(mNext));
>+    if (!mNext)
>+      continue;
I don't think you need to clone this any more.

>+  nsCOMPtr<nsICategoryManager> catMan
>+    (do_GetService(NS_CATEGORYMANAGER_CONTRACTID));
>+  if (!catMan)
>+    return NS_ERROR_FAILURE;
rv version?
>+  nsCOMPtr<nsICategoryManager> catMan
>+    (do_GetService(NS_CATEGORYMANAGER_CONTRACTID));
>+  if (!catMan)
>+    return NS_ERROR_FAILURE;

Comment 8

[[e-mail]]

Comment on attachment 221892 [details] [diff] [review]
Third version

>+NS_IMETHODIMP nsSuiteDirectoryProvider::AppendingEnumerator::GetNext(nsISupports* *aResult)
>+{
>+  if (aResult)
>+    NS_ADDREF(*aResult = mNext);
>+
>+  mNext = nsnull;
>+
>+  // Ignore all errors
>+
>+  PRBool more;
>+  while (NS_SUCCEEDED(mBase->HasMoreElements(&more)) && more) {
>+    nsCOMPtr<nsISupports> nextbasesupp;
>+    mBase->GetNext(getter_AddRefs(nextbasesupp));
>+
>+    nsCOMPtr<nsIFile> nextbase(do_QueryInterface(nextbasesupp));
>+    if (!nextbase)
>+      continue;
>+
>+    nextbase->Clone(getter_AddRefs(mNext));
>+    if (!mNext)
>+      continue;
>+
>+    mNext->AppendNative(mLeafName);
>+
>+    PRBool exists;
>+    if (NS_SUCCEEDED(mNext->Exists(&exists)) && exists)
>+      break;
>+
>+    mNext = nsnull;
>+  }
>+
>+  return NS_OK;
jag, do you think that consumers (not just searchplugindirs, but future consumers) expect us to return only existing files? If not, then we could simply get the next dir from the base and append the leaf name.

Comment 9

[jag (Peter Annema)]

Neil:

Since we copied nsBrowserDirectoryProvider.cpp and then modified our copy, shouldn't we leave most of the license text alone other than to maybe update the (C) line and have Mark's name added to the contributors list?


Regarding nsIDirectoryServiceProvider2::getFiles(...), while we just provide locations with no guarantee whether or not the file/dir exists (e.g. what if you need a file location so you can write a (new) file to it) I'd say what I expect is a list of files/directories that one could reasonably expect to logically exist, whether or not they actually exist. Put differently, if I were to iterate across the list and touch/mkdir each file/dir that doesn't exist, we don't end up with files/dirs in unexpected places. Put yet differently, the items in the list need to "make sense".

So if for example only a subset of the XRE_EXTENSIONS_DIRs would be expected to have a subdirectory named "searchplugins", then return a list with only that subset. If "expected" in this case is "because the extension provides it", then "exists" is your test.

Perhaps we should start a wiki on what to expect (read: what the contract is) for various keys to dirSvc->Get().


Mark Banner:

+NS_METHOD
+nsSuiteDirectoryProvider::Unregister(nsIComponentManager* aCompMgr,

For all the other ones you put the NS_METHOD/NS_IMETHODIMP on the same line as the function definition. I personally prefer the two-line style, but as long as they're all the same, I'll leave it up to you which one to pick.


I know you just copied this, but could you fix |leafstr|, |nextbase| and |nextbasesupp| to be |leafStr|, |nextBase| and |nextBaseSupport| respectively? If I missed any, please fix them too.

Comment 10

[[e-mail]]

I prefer NS_IMETHOD on its own line, especially on some of those long lines.
I also prefer nsISupports* aParam or PRBool* or nsISupports** aResult.

I did some digging around and it looks as if at least for this key we guarantee to enumerate existing files. That puts paid to my idea though :-(

[my constructor is inline because it doesn't need to call GetNext]

NS_IMETHODIMP
nsSuiteDirectoryProvider::AppendingEnumerator::HasMoreElements(PRBool* aResult)
{
  return mBase->HasMoreElements(aResult);
}

NS_IMETHODIMP
nsSuiteDirectoryProvider::AppendingEnumerator::GetNext(nsISupports** aResult)
{
  nsresult rv = mBase->GetNext(aResult);
  if (NS_SUCCEEDED(rv)) {
    nsCOMPtr<nsIFile> file(do_QueryInterface(*aResult));
    if (file)
      file->AppendNative(mLeafName);
  }
  return rv;
}

Comment 11

[[e-mail]]

Comment on attachment 221892 [details] [diff] [review]
Third version

>+    nsCOMPtr<nsISupports> nextbasesupp;
Actually I think I'd prefer this to be called nextSupports

Comment 12

[[e-mail]]

Comment on attachment 221892 [details] [diff] [review]
Third version

>+  if (aResult)
>+    NS_ADDREF(*aResult = mNext);
NS_IF_ADDREF, I think, just in case ;-)

Comment 13

[jag (Peter Annema)]

+NS_IMETHODIMP nsSuiteDirectoryProvider::AppendingEnumerator::HasMoreElements(PRBool *aResult)
+{
+  *aResult = mNext ? PR_TRUE : PR_FALSE;

*aResult = mNext != nsnull;

+NS_IMETHODIMP nsSuiteDirectoryProvider::AppendingEnumerator::GetNext(nsISupports* *aResult)
+{
+  if (aResult)
+    NS_ADDREF(*aResult = mNext);

GetNext() should return NS_ERROR_FAILURE if !mNext, according to nsISimpleEnumerator. You could do something like:

  if (aResult) {
    if (!mNext) {
      *aResult = nsnull;
      return NS_ERROR_FAILURE;
    }

    NS_ADDREF(*aResult = mNext);
  }

Or you could move the part that sets mNext out into a new function that both the constructor and GetNext() can call. In that case you can also assert that aResult should never be null (since xpconnect will always provide a pointer, and c++ consumers should too).

Comment 14

[Mark Banner (:standard8)]

Attachment: Fourth version

Ok, I think this includes the fixes for all the various nits from Neil and Jag. Requesting reviews afresh to make sure everyone's happy ;-)

Comment 15

[jag (Peter Annema)]

So, taking a step back... any idea why the original (FF) GetNext() didn't simply call mBase->GetNext() until it finds a "searchplugins" that exists or hits the end? Why this "init" from the constructor and peeking ahead for the next mNext?

Neil: why is that Clone() in GetNext() not needed? Are the nsIFiles in the nsISimpleEnumerator we get from dirSvc->Get() ours and not shared?

Comment 16

[[e-mail]]

(In reply to comment #15)
>Any idea why the original (FF) GetNext() didn't simply call mBase->GetNext()
>until it finds a "searchplugins" that exists or hits the end? Why this "init"
>from the constructor and peeking ahead for the next mNext?
To make HasMoreElements work, you need to have looked for one.
 
>Neil: why is that Clone() in GetNext() not needed? Are the nsIFiles in the
>nsISimpleEnumerator we get from dirSvc->Get() ours and not shared?
I'm pretty sure e.g. nsDirectoryService clones its cached directories.

Comment 17

[[e-mail]]

Comment on attachment 222071 [details] [diff] [review]
Fourth version

>+  char const* leafName = nsnull;
const char* leafName = nsnull;

>+  mNext = nsnull;
>+
>+  // Ignore all errors
>+
>+  PRBool more;
>+  while (NS_SUCCEEDED(mBase->HasMoreElements(&more)) && more) {
>+    nsCOMPtr<nsISupports> nextSupports;
>+    mBase->GetNext(getter_AddRefs(nextSupports));
>+
>+    mNext = do_QueryInterface(nextSupports);
>+    if (!mNext)
>+      continue;
>+
>+    mNext->AppendNative(mLeafName);
>+
>+    PRBool exists;
>+    if (NS_SUCCEEDED(mNext->Exists(&exists)) && exists)
>+      break;
>+
>+    mNext = nsnull;
>+  }
If we change that break to a return, then we can probably get away with clearing mNext one at the end of the function rather than twice as here.

Comment 18

[[e-mail]]

Comment on attachment 222071 [details] [diff] [review]
Fourth version

r=me with the const* fixed (not so sure about the nsnull assignment at this time of night!)

Comment 19

[jag (Peter Annema)]

Ah, right, HasMoreElements().

And yes, if you return instead of break then you can just null out mNext at the end of the function.

Comment 20

[Mark Banner (:standard8)]

Patch checked in with nits addressed.