Closed
Bug 337636
Opened 19 years ago
Closed 19 years ago
Get bookmarks working on suiterunner.
Categories
(SeaMonkey :: Bookmarks & History, defect)
SeaMonkey
Bookmarks & History
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: standard8, Assigned: standard8)
References
Details
Attachments
(1 file, 3 obsolete files)
12.50 KB,
patch
|
neil
:
review+
jag+mozilla
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•19 years ago
|
||
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•19 years ago
|
||
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.
Assignee | ||
Comment 3•19 years ago
|
||
Modified to remove the redundant code and tidy a few things up.
Attachment #221747 -
Attachment is obsolete: true
Attachment #221814 -
Flags: superreview?(neil)
Attachment #221814 -
Flags: review?(neil)
Comment 4•19 years ago
|
||
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.
Attachment #221814 -
Flags: superreview?(neil)
Attachment #221814 -
Flags: superreview-
Attachment #221814 -
Flags: review?(neil)
Attachment #221814 -
Flags: review+
Comment 5•19 years ago
|
||
Comment on attachment 221814 [details] [diff] [review]
Second version
oh, and you should add the new makefile to allmakefiles.sh too
Assignee | ||
Comment 6•19 years ago
|
||
Revised according to Neil's comments and added the allmakefiles.sh change that I'd previously forgotten. carrying forward r+, requesting sr.
Attachment #221814 -
Attachment is obsolete: true
Attachment #221892 -
Flags: superreview?(neil)
Attachment #221892 -
Flags: review+
Comment 7•19 years ago
|
||
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•19 years ago
|
||
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.
Attachment #221892 -
Flags: superreview?(neil) → superreview?(jag)
Comment 9•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
+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).
Assignee | ||
Comment 14•19 years ago
|
||
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 ;-)
Attachment #221892 -
Attachment is obsolete: true
Attachment #222071 -
Flags: superreview?(jag)
Attachment #222071 -
Flags: review?(neil)
Attachment #221892 -
Flags: superreview?(jag)
Comment 15•19 years ago
|
||
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•19 years ago
|
||
(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•19 years ago
|
||
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•19 years ago
|
||
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!)
Attachment #222071 -
Flags: review?(neil) → review+
Comment 19•19 years ago
|
||
Ah, right, HasMoreElements().
And yes, if you return instead of break then you can just null out mNext at the end of the function.
Updated•19 years ago
|
Attachment #222071 -
Flags: superreview?(jag) → superreview+
Assignee | ||
Comment 20•19 years ago
|
||
Patch checked in with nits addressed.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•