317631 - Drag & Drop for Places

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: first round, nowhere near done.

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: more...

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: just the changes to navhistoryresult & idl

make navhistoryresult pass tree notifications through to client using an interface similar to nsIXULTreeBuilderObserver (but not located in a file that introduces RDF dependencies). 

When we check this in, we should move this file into the tree/ area of the codebase and have both this and the other code in template builder use _that_.

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 204258 [details] [diff] [review]
just the changes to navhistoryresult & idl

>--- src/nsNavHistoryResult.cpp	23 Nov 2005 23:03:10 -0000	1.7
>+++ src/nsNavHistoryResult.cpp	27 Nov 2005 02:18:35 -0000
>@@ -611,16 +609,37 @@ nsNavHistoryResult::GetSourceQueries(PRU
>+NS_IMETHODIMP 
>+nsNavHistoryResult::AddObserver(nsINavHistoryResultViewObserver* aObserver)
>+{
>+  NS_ENSURE_ARG_POINTER(aObserver);
>+  return mObservers.AppendObject(aObserver);
>+}

AppendObject returns a boolean (true for success, false for out of memory).  You need to convert to the correct nsresult.  Also, this should probably protect against adding the same observer twice.

>+
>+NS_IMETHODIMP
>+nsNavHistoryResult::RemoveObserver(nsINavHistoryResultViewObserver* aObserver)
>+{
>+  NS_ENSURE_ARG_POINTER(aObserver);

Just do:

mObservers.RemoveObject(aObserver);

>@@ -1097,28 +1116,44 @@ NS_IMETHODIMP nsNavHistoryResult::IsSort
> NS_IMETHODIMP nsNavHistoryResult::CanDrop(PRInt32 index, PRInt32 orientation,
>                                           PRBool *_retval)
> {
>-  *_retval = PR_FALSE;
>+  PRInt32 count = mObservers.Count();
>+  nsINavHistoryResultViewObserver* observer = nsnull;
>+  for (PRInt32 i = 0; i < count; ++i) {
>+    observer = (nsINavHistoryResultViewObserver*)mObservers[i];
>+    if (observer) {

- You shouldn't need to cast the value of mObservers[i].  If you do, something's wrong.
- You protect against adding a null observer, so just assert that observer is non-null.
- There's an assumption here that CanDrop doesn't alter the observer list.  That should probably at least be documented.

Same comments apply for the other pass-through methods.

r=me with those fixed.

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch for checkin, review comments applied

also bring forward review+

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: attach the right file!

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

I'm going to say this "works" - additional things should be filed as separate bugs. 

Comment 12

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv