398914 - Creating an additional bookmark with "Add a keyword for this search" overwrites the POST_DATA of existing bookmark(s) with the same location

Status: VERIFIED FIXED

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

Description

[dickvl]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100705 Minefield/3.0a9pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100705 Minefield/3.0a9pre

If I create more then one keyword bookmark by right clicking a search field on a website with other options on that page selected (e.g. forum to search) then the POST_DATA of other bookmarks created before on that page and thus have the same url are all set to the POST_DATA of the last one created.
That also happens if I import a set of bookmarks created with a Firefox version that doesn't have places like Firefox 2 or Gran Paradiso a4.
It seems that both the url and the POST_DATA are only saved once.
If I export the bookmarks then both bookmarks have the same POST_DATA field.


Reproducible: Always

Steps to Reproduce:
1. Open: http://forums.mozillazine.org/search.php
2. Select the 'Firefox Support' forum from the drop down list of Forum
3. Right click the the search field to the right of "Search for Keywords" (top field on the right)
4. Select "Add a keyword for this search" and fill both the name and keyword with 'fsup' and save that Bookmark

Repeat the above steps and select the Firefox General forum from the list and save that bookmark as 'fgen'.
Do a search with both keywords for a common word like 'welcome': fsup welcome and fgen welcome

Actual Results:  
With both keywords I get the same results: both from Firefox General


Expected Results:  
Results from Firefox Support with: fsup welcome
Results from Firefox General with:  fgen welcome


Firefox should store the correct POST_DATA from that form for each bookmark as was done in bookmarks.html before places.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100623 Minefield/3.0a9pre

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

make postdata operations bookmark-centric instead of uri-centric.

Comment 3

[(not reading, please use [e-mail] instead)]

is that the whole fix?  a few things feel like there are missing from this patch, like:

1)  the code that tries to get the post data for a uri, that would need to be fixed.  

mozilla\browser\base\content\browser.js(1831):    aPostDataRef.value = PlacesUtils.getPostDataForURI(shortcutURI);

And, how will this work:

  try {
    var shortcutURI = PlacesUtils.bookmarks.getURIForKeyword(keyword);
    shortcutURL = shortcutURI.spec;
    aPostDataRef.value = PlacesUtils.getPostDataForURI(shortcutURI);
  } catch(ex) {}

we need to take the keyword, get all the bookmark that have that keyword (I think we can have multiple bookmarks with the same keyword) and then get the post data for the bookmark.

in the multiple case, which bookmark do we use?

3) do we need to worry about the import export code?

  C:\builds\trunk\mozilla\browser\components\places\src\nsPlacesImportExportService.cpp(128):#define POST_DATA_ANNO NS_LITERAL_CSTRING("URIProperties/POSTData")
  C:\builds\trunk\mozilla\browser\components\places\src\nsPlacesImportExportService.cpp(952):      mAnnotationService->SetPageAnnotationString(frame.mPreviousLink, POST_DATA_ANNO,
  C:\builds\trunk\mozilla\browser\components\places\src\nsPlacesImportExportService.cpp(1895):  rv = mAnnotationService->PageHasAnnotation(pageURI, POST_DATA_ANNO,
  C:\builds\trunk\mozilla\browser\components\places\src\nsPlacesImportExportService.cpp(1900):    rv = mAnnotationService->GetPageAnnotationString(pageURI, POST_DATA_ANNO,

4) finally, what about people who have previously migrated from fx 2 -> fx 3 beta, and the keywords are on uris not bookmarks?

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: fix v2

Comment 5

[Dietrich Ayala (:dietrich)]

> 
> we need to take the keyword, get all the bookmark that have that keyword (I
> think we can have multiple bookmarks with the same keyword) and then get the
> post data for the bookmark.
> 
> in the multiple case, which bookmark do we use?

first bookmarked, as in fx2

> 
> 3) do we need to worry about the import export code?
> 

fixed

> 4) finally, what about people who have previously migrated from fx 2 -> fx 3
> beta, and the keywords are on uris not bookmarks?
> 

migration path added

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: places.sqlite w/ the STR pre-populated

Comment 7

[(not reading, please use [e-mail] instead)]

Comment on attachment 290652 [details] [diff] [review]
fix v2

looks good.

some comments that will require a v2 (and a couple spin off bugs?)

1)

+// XXX to be removed after beta 2

can you log a spin of bug (tm to fx 3 at the lastest) to remove this code so we don't forget?


2)

+/**
+ * Various migration tasks.
+ */

whoo-hoo, as I also have code to add here for another bug!

+    const oldPostDataAnno = "URIProperties/POSTData";
+    const newPostDataAnno = "bookmarkProperties/POSTData";

I think you don't need newPostDataAnno.

Instead you can use the POST_DATA_ANNO const from utils.js (as we do this with DESCRIPTION_ANNO already)

3)

looking at nsAnnotationService::GetPagesWithAnnotationCOMArray, I think we are safe, but I'll ask anyways:

with a new profile, without any annotations, I think all this code will execute cleanly, and not throw.  

But can you makes sure that nothing bad happens with a new profile (without any annotations)?

4)

I think you can move this code out of the inner most for loop, as uri doesn't change, right?

+          // We can't know which URI+keyword combo this postdata was for, but
+          // it's very likely that if this URI is bookmarked and has a keyword
+          // *and* the URI has postdata, then this bookmark was using the
+          // postdata. Propagate the annotation to all bookmarks for this URI
+          // just to be safe.
+          var postData = annosvc.getPageAnnotation(uri, oldPostDataAnno);

5)


     shortcutURL = shortcutURI.spec;
-    aPostDataRef.value = PlacesUtils.getPostDataForURI(shortcutURI);
+    if (shortcutURI) {
+      var bookmarksForURI = PlacesUtils.bookmarks.getBookmarkIdsForURI(shortcutURI, {});
+      for (let i = 0; i < bookmarksForURI.length; i++) {
+        let bookmark = bookmarksForURI[i];
+        let kw = PlacesUtils.bookmarks.getKeywordForBookmark(bookmark);
+        if (kw == keyword) {
+          aPostDataRef.value = PlacesUtils.getPostDataForBookmark(bookmark);
+          break;
+        }

Based on our discussion, I think that "first one wins" is correct.  Can you add a comment?.

But, this code makes assumptions on how getBookmarkIdsForURI() works, right?

I think we might want to make this explicit in your code, and first order by creation date (ascending).

[This could be a spin off bug.]

6)

you updated the nsIPlacesTransactionsService interface without bumping the uuid.

7)

+  // Testing edit Post Data

Can you extend your test to make sure we can have two different keywords for the same uri with different post data?

8)

To confirm, copy/paste/undo/move/etc all work appropriately, right?

Can you list your tests (and then we'll add the litmus flag to get that added to the fft's for places.

Comment 8

[(not reading, please use [e-mail] instead)]

Comment on attachment 290652 [details] [diff] [review]
fix v2

will wait for v2

Comment 9

[(not reading, please use [e-mail] instead)]

> Based on our discussion, I think that "first one wins" is correct.  Can you add
> a comment?.
>
> But, this code makes assumptions on how getBookmarkIdsForURI() works, right?
>
> I think we might want to make this explicit in your code, and first order by
> creation date (ascending).

actually, we don't want "first one wins".  

The query behind the scenes is:

141   // mDBFindURIBookmarks
142   rv = dbConn->CreateStatement(NS_LITERAL_CSTRING(
143       "SELECT a.id "
144       "FROM moz_bookmarks a, moz_places h "
145       "WHERE h.url = ?1 AND a.fk = h.id and a.type = ?2 "
146       "ORDER BY MAX(COALESCE(a.lastModified, 0), a.dateAdded) DESC"),
147     getter_AddRefs(mDBFindURIBookmarks));
148   NS_ENSURE_SUCCESS(rv, rv);

this is correct, as it gives us the last modified (or created) first.

discussing with dietrich, is what we want when going from keyword to {uri + postdata}

if we have two keywords for the same uri with different postdata, the last bookmark modified (or created) should win.

so, the code is correct as is, but we should add a comment to mDBFindURIBookmarks to be careful not to modify the "order by ..." part, as we have (at least) this code that depends on it.

Comment 10

[Dietrich Ayala (:dietrich)]

(In reply to comment #7)
> 
> 1)
> 
> +// XXX to be removed after beta 2
> 
> can you log a spin of bug (tm to fx 3 at the lastest) to remove this code so we
> don't forget?
> 

making bug 391419 dependent on this.

> 
> 2)
...
> +    const newPostDataAnno = "bookmarkProperties/POSTData";
> 
> I think you don't need newPostDataAnno.
> 
> Instead you can use the POST_DATA_ANNO const from utils.js (as we do this with
> DESCRIPTION_ANNO already)
> 

fixed

> 3)
...
> But can you makes sure that nothing bad happens with a new profile (without any
> annotations)?

works

> 
> 4)
> 
> I think you can move this code out of the inner most for loop, as uri doesn't
> change, right?

fixed

> 
> 6)
> 
> you updated the nsIPlacesTransactionsService interface without bumping the
> uuid.

fixed

> 
> 7)
> 
> +  // Testing edit Post Data
> 
> Can you extend your test to make sure we can have two different keywords for
> the same uri with different post data?
> 

done. also added tests for the other conflict scenarios we discussed in mt view this week.

> 8)
> 
> To confirm, copy/paste/undo/move/etc all work appropriately, right?
> 
> Can you list your tests (and then we'll add the litmus flag to get that added
> to the fft's for places.
> 

the existing litmus tests for these actions should be sufficient. they should be cloned, and executed w/ a postdata keyword search instead of a regular bookmark.

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: fix v3

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: fix v3

forgot the new test file in the last patch

Comment 13

[(not reading, please use [e-mail] instead)]

Comment on attachment 291033 [details] [diff] [review]
fix v3

r=sspitzer, with a few comments:

1)


So we call your placesMigrationTasks() from browser.js, and if it were to throw we'd have big problems.

I was going to suggest that we also may want to use try / catch internal to placesMigrationTasks(), so that if we throw while processing a particular uri, we could still continue on a process others.  or is that going overboard?

I'm going to be in this code (after you land) the "places" folder migration code, so perhaps this can wait until then?

2)
 
-  try {
-    var shortcutURI = PlacesUtils.bookmarks.getURIForKeyword(keyword);
-    shortcutURL = shortcutURI.spec;
-    aPostDataRef.value = PlacesUtils.getPostDataForURI(shortcutURI);
-  } catch(ex) {}
+  [shortcutURL, aPostDataRef.value] =
+    PlacesUtils.getURLAndPostDataForKeyword(keyword);
 
   if (!shortcutURL)
     return aURL;
 
   var postData = "";
   if (aPostDataRef && aPostDataRef.value)
     postData = unescape(aPostDataRef.value);
  
   /**
+   * Get the URI (and any associated POST data) for a given keyword.
+   * @param aKeyword string keyword
+   * @returns an array containing a string URL and a string of POST data
+   */
+  getURLAndPostDataForKeyword: function PU_getURLAndPostDataForKeyword(aKeyword) {
+    var url = null, postdata = null;
+    var uri = this.bookmarks.getURIForKeyword(aKeyword);
+    if (uri) {
+      url = uri.spec;
+      var bookmarks = this.bookmarks.getBookmarkIdsForURI(uri, {});
+      for (let i = 0; i < bookmarks.length; i++) {
+        var bookmark = bookmarks[i];
+        var kw = this.bookmarks.getKeywordForBookmark(bookmark);
+        if (kw == aKeyword) {
+          postdata = this.getPostDataForBookmark(bookmark);
+          break;
+        }
+      }
+    }
+    return [url, postdata];
+  },
+

You've remove a try / catch, which kind of makes me nervous.  (Any idea if that was to allow getPostDataForURI() to fail if there was no post  data, and we'd proceed on, using the keyword?)

I realize this code has changed, and we shouldn't throw (as far as I can see), but any objection to adding the try / catch inside getURLAndPostDataForKeyword()?

3)
 
 const NS_APP_USER_PROFILE_50_DIR = "ProfD";
-const Ci = Components.interfaces;
-const Cc = Components.classes;
-const Cr = Components.results;
+var Ci = Components.interfaces;
+var Cc = Components.classes;
+var Cr = Components.results;

why this change?  Because of this change:

+var loader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
+             getService(Ci.mozIJSSubScriptLoader);
+loader.loadSubScript("chrome://global/content/debug.js");
+loader.loadSubScript("chrome://browser/content/places/utils.js");

 
4)  Index: browser/components/places/tests/unit/test_398914.js

the new test looks great, thanks!

Comment 14

[Dietrich Ayala (:dietrich)]

> 3)
> 
>  const NS_APP_USER_PROFILE_50_DIR = "ProfD";
> -const Ci = Components.interfaces;
> -const Cc = Components.classes;
> -const Cr = Components.results;
> +var Ci = Components.interfaces;
> +var Cc = Components.classes;
> +var Cr = Components.results;
> 
> why this change?  Because of this change:
> 
> +var loader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
> +             getService(Ci.mozIJSSubScriptLoader);
> +loader.loadSubScript("chrome://global/content/debug.js");
> +loader.loadSubScript("chrome://browser/content/places/utils.js");
> 
> 

yes. utils.js defines it's own, so we need to allow these to be overwritten. i've filed bug 406371 to look at solutions for these in utils.js.

Comment 15

[Dietrich Ayala (:dietrich)]

Attachment: fix v3.1 (fixes seth's comments)

for checkin

Comment 16

[Dietrich Ayala (:dietrich)]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.234; previous revision: 1.233
done
Checking in browser/base/content/browser-places.js;
/cvsroot/mozilla/browser/base/content/browser-places.js,v  <--  browser-places.js
new revision: 1.70; previous revision: 1.69
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.906; previous revision: 1.905
done
Checking in browser/components/places/content/bookmarkProperties.js;
/cvsroot/mozilla/browser/components/places/content/bookmarkProperties.js,v  <--  bookmarkProperties.js
new revision: 1.66; previous revision: 1.65
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.85; previous revision: 1.84
done
Checking in browser/components/places/public/nsIPlacesTransactionsService.idl;
/cvsroot/mozilla/browser/components/places/public/nsIPlacesTransactionsService.idl,v  <--  nsIPlacesTransactionsService.idl
new revision: 1.6; previous revision: 1.5
done
Checking in browser/components/places/src/nsPlacesImportExportService.cpp;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v  <--  nsPlacesImportExportService.cpp
new revision: 1.37; previous revision: 1.36
done
Checking in browser/components/places/src/nsPlacesTransactionsService.js;
/cvsroot/mozilla/browser/components/places/src/nsPlacesTransactionsService.js,v  <--  nsPlacesTransactionsService.js
new revision: 1.8; previous revision: 1.7
done
Checking in browser/components/places/tests/unit/head_bookmarks.js;
/cvsroot/mozilla/browser/components/places/tests/unit/head_bookmarks.js,v  <--  head_bookmarks.js
new revision: 1.11; previous revision: 1.10
done
RCS file: /cvsroot/mozilla/browser/components/places/tests/unit/test_398914.js,v
done
Checking in browser/components/places/tests/unit/test_398914.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_398914.js,v  <--  test_398914.js
initial revision: 1.1
done
Checking in browser/components/places/tests/unit/test_bookmarks_html.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_bookmarks_html.js,v  <--  test_bookmarks_html.js
new revision: 1.16; previous revision: 1.15
done
Checking in browser/components/places/tests/unit/test_placesTxn.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_placesTxn.js,v  <--  test_placesTxn.js
new revision: 1.9; previous revision: 1.8
done
Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavBookmarks.cpp
new revision: 1.132; previous revision: 1.131
done

Comment 17

[Dietrich Ayala (:dietrich)]

backed out due to Ts regression and test failure.

Comment 18

[Dietrich Ayala (:dietrich)]

note: i disabled the test, but did not cvs remove the file.

Comment 19

[Dietrich Ayala (:dietrich)]

Ts problem was a different bug, attempting re-landing.

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.236; previous revision: 1.235
done
Checking in browser/base/content/browser-places.js;
/cvsroot/mozilla/browser/base/content/browser-places.js,v  <--  browser-places.js
new revision: 1.72; previous revision: 1.71
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.908; previous revision: 1.907
done
Checking in browser/components/places/content/bookmarkProperties.js;
/cvsroot/mozilla/browser/components/places/content/bookmarkProperties.js,v  <--  bookmarkProperties.js
new revision: 1.68; previous revision: 1.67
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.90; previous revision: 1.89
done
Checking in browser/components/places/public/nsIPlacesTransactionsService.idl;
/cvsroot/mozilla/browser/components/places/public/nsIPlacesTransactionsService.idl,v  <--  nsIPlacesTransactionsService.idl
new revision: 1.8; previous revision: 1.7
done
Checking in browser/components/places/src/nsPlacesImportExportService.cpp;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v  <--  nsPlacesImportExportService.cpp
new revision: 1.39; previous revision: 1.38
done
Checking in browser/components/places/src/nsPlacesTransactionsService.js;
/cvsroot/mozilla/browser/components/places/src/nsPlacesTransactionsService.js,v  <--  nsPlacesTransactionsService.js
new revision: 1.10; previous revision: 1.9
done
Checking in browser/components/places/tests/unit/head_bookmarks.js;
/cvsroot/mozilla/browser/components/places/tests/unit/head_bookmarks.js,v  <--  head_bookmarks.js
new revision: 1.13; previous revision: 1.12
done
Checking in browser/components/places/tests/unit/test_398914.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_398914.js,v  <--  test_398914.js
new revision: 1.3; previous revision: 1.2
done
Checking in browser/components/places/tests/unit/test_bookmarks_html.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_bookmarks_html.js,v  <--  test_bookmarks_html.js
new revision: 1.18; previous revision: 1.17
done
Checking in browser/components/places/tests/unit/test_placesTxn.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_placesTxn.js,v  <--  test_placesTxn.js
new revision: 1.11; previous revision: 1.10
done
Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavBookmarks.cpp
new revision: 1.134; previous revision: 1.133
done

Comment 20

[Dietrich Ayala (:dietrich)]

Attachment: as checked in

Comment 21

[Dietrich Ayala (:dietrich)]

Attachment: test fix

Comment 22

[Dietrich Ayala (:dietrich)]

Checking in browser/components/places/tests/unit/test_398914.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_398914.js,v  <--  test_398914.js
new revision: 1.4; previous revision: 1.3
done

disabling the test for a bit.

Comment 23

[(not reading, please use [e-mail] instead)]

the test failure was on windows tinderbox, which rings a bell.

from irc, dietrich and I think:

	sspitzerMsgMe	  var bm1 = bmsvc.insertBookmark(testFolderId, testURI, -1, "blah");
	sspitzerMsgMe	  bmsvc.setKeywordForBookmark(bm1, "foo");
	sspitzerMsgMe	  PlacesUtils.setPostDataForBookmark(bm1, "pdata1");
	sspitzerMsgMe	  var bm2 = bmsvc.insertBookmark(testFolderId, testURI, -1, "blah");
	sspitzerMsgMe	  bmsvc.setKeywordForBookmark(bm2, "foo");
	sspitzerMsgMe	  PlacesUtils.setPostDataForBookmark(bm2, "pdata2");

given the granularity of PRNow() on windows, these bm1 / bm2 might have the same creation date / last modified date.  (we've seen issues like this before.)


looking at our query:

	sspitzerMsgMe	141   // mDBFindURIBookmarks
	sspitzerMsgMe	142   // NOTE: Do not modify the ORDER BY segment of the query, as certain
	sspitzerMsgMe	143   // features depend on it. See bug 398914 for an example.
	sspitzerMsgMe	144   rv = dbConn->CreateStatement(NS_LITERAL_CSTRING(
	sspitzerMsgMe	145       "SELECT a.id "
	sspitzerMsgMe	146       "FROM moz_bookmarks a, moz_places h "
	sspitzerMsgMe	147       "WHERE h.url = ?1 AND a.fk = h.id and a.type = ?2 "
	sspitzerMsgMe	148       "ORDER BY MAX(COALESCE(a.lastModified, 0), a.dateAdded) DESC"),
	sspitzerMsgMe	149     getter_AddRefs(mDBFindURIBookmarks));
	sspitzerMsgMe	150   NS_ENSURE_SUCCESS(rv, rv);

we should improve this query, so that we consistently break ties by ordering by a.id

	sspitzerMsgMe	ORDER BY MAX(COALESCE(a.lastModified, 0), a.dateAdded) DESC, a.id DESC

ties can happen in the real world (import and dynamically added bookmarks from add ons, chrome, same code as a unit test.)

additionally, when we re-enable the test, I recommend we also add some code to dump out the creation and modified times, as well as check that

bm1.creation <= bm2.creation and bm1.modified <= bm2.modified

(note, <= not <, as they could be equal)

if we do this and the test still fails, something else is up.

Comment 24

[Dietrich Ayala (:dietrich)]

Attachment: test fix v2

Comment 25

[(not reading, please use [e-mail] instead)]

Comment on attachment 291602 [details] [diff] [review]
test fix v2

r=sspitzer, looks good.

in theory we could also test the query by having two bookmarks with the same uri, same creation date, modified date, and the one with the bigger id should be the first when calling getBookmarkIdsForURI()

can I convince you to write a test for that?

Comment 26

[Dietrich Ayala (:dietrich)]

Attachment: test fix v3

added the new test, checked-in.

Comment 27

[(not reading, please use [e-mail] instead)]

hey dietrich, I should have been more precise.

I think your previous version of the patch was better, as we could have confirmed (by looking at tinderbox logs) if this was indeed the orange

as for the supplimental test, your new patch looks good, except that we don't need the logging and, since you set all the dates to testDate, it should be:

+  do_check_equal(bm1da, bm2da);
+  do_check_equal(bm1lm, bm2lm);

so, can you go back to your last patch (so that we can confirm what was going on with tinderbox) and have an additional, separate test for getBookmarkIdsForURI() to ensure for the query change?

also note, to be extra careful, we should do:

+  var ids = bmsvc.getBookmarkIdsForURI(testURI, {});
+  do_check_eq(ids[0], bm2);
+  do_check_eq(ids[1], bm1);

none of this is m10 blocker worthy, of course.

Comment 28

[(not reading, please use [e-mail] instead)]

my mistake, I see now that your test from v2 is still there (thanks dietrich)

but I'd still recommend:

1)

+  do_check_equal(bm1da, bm2da);
+  do_check_equal(bm1lm, bm2lm);

2)

+  do_check_eq(ids[1], bm1);

Comment 29

[Dietrich Ayala (:dietrich)]

(In reply to comment #28)
> my mistake, I see now that your test from v2 is still there (thanks dietrich)
> 
> but I'd still recommend:
> 
> 1)
> 
> +  do_check_equal(bm1da, bm2da);
> +  do_check_equal(bm1lm, bm2lm);
> 
> 2)
> 
> +  do_check_eq(ids[1], bm1);
> 

passed locally, so added and checked-in, r=sspitzer on irc

Comment 30

[(not reading, please use [e-mail] instead)]

Attachment: still need to test, but ensure that last modified date is bigger (checked-in)

Comment 31

[Dietrich Ayala (:dietrich)]

Comment on attachment 291620 [details] [diff] [review]
still need to test, but ensure that last modified date is bigger (checked-in)

Checking in browser/components/places/tests/unit/test_398914.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_398914.js,v  <--  test_398914.js
new revision: 1.7; previous revision: 1.6
done

r=me. passed locally, checking it in.

Comment 32

[Marcia Knous [:marcia]]

seth or dietrich: I tried verifying this using Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9b2pre) Gecko/2007120604 Minefield/3.0b2pre following the STR in Comment 0. I get the same set of results when I use the keywords. I used a new profile, and the only thing I did that is not mentioned is Comment 0 is switch the dropdown back to Forum: All available.

Comment 33

[Dietrich Ayala (:dietrich)]

(In reply to comment #32)
> keywords. I used a new profile, and the only thing I did that is not mentioned
> is Comment 0 is switch the dropdown back to Forum: All available.
> 

Hey Marcia. Do you mean that you created both keywords with "All available" selected?

The key to testing this is to create each keyword w/ a different support forum selected.

Comment 34

[Marcia Knous [:marcia]]

(In reply to comment #33)

Dietrich: No, the keywords were created exactly as described in Comment 0, using the General for fgen and Support for fsup. The only time I changed the dropdown to All Available is when I executed the keyword search.

> Hey Marcia. Do you mean that you created both keywords with "All available"
> selected?
> 
> The key to testing this is to create each keyword w/ a different support forum
> selected.
> 

Comment 35

[Marcia Knous [:marcia]]

I also tested this on another machine yesterday (Vista) and got the same results as in Comment 34.

Comment 36

[Dietrich Ayala (:dietrich)]

(In reply to comment #34)
> (In reply to comment #33)
> The only time I changed the
> dropdown to All Available is when I executed the keyword search.
> 

hm, the dropdown shouldn't be a part of executing the keyword search, i'm confused about what you mean here.

Comment 37

[Carsten Book [:Tomcat]]

works for me in  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
rv:1.9b2pre) Gecko/2007120704 Minefield/3.0b2pre with the steps to reproduce
from comment #0

Comment 38

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2007121904 Minefield/3.0b3pre and the equivalent Win XP build

Comment 39

[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