Closed
Bug 398914
Opened 18 years ago
Closed 17 years ago
Creating an additional bookmark with "Add a keyword for this search" overwrites the POST_DATA of existing bookmark(s) with the same location
Categories
(Firefox :: Bookmarks & History, defect, P1)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3 beta2
People
(Reporter: bugdickvl, Assigned: dietrich)
References
()
Details
(Keywords: dataloss, regression)
Attachments
(4 files, 7 obsolete files)
124.00 KB,
application/octet-stream
|
Details | |
27.79 KB,
patch
|
Details | Diff | Splinter Review | |
5.09 KB,
patch
|
Details | Diff | Splinter Review | |
1.66 KB,
patch
|
dietrich
:
review+
|
Details | Diff | Splinter Review |
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•18 years ago
|
||
Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100623 Minefield/3.0a9pre
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•18 years ago
|
Updated•18 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Keywords: regression
Assignee | ||
Updated•18 years ago
|
Target Milestone: --- → Firefox 3 M10
Assignee | ||
Updated•17 years ago
|
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → dietrich
Assignee | ||
Comment 2•17 years ago
|
||
make postdata operations bookmark-centric instead of uri-centric.
Attachment #288081 -
Flags: review?(sspitzer)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch][needs review sspitzer]
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Comment 3•17 years ago
|
||
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?
Updated•17 years ago
|
Attachment #288081 -
Flags: review?(sspitzer)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch][needs review sspitzer]
Updated•17 years ago
|
Whiteboard: [need new patch]
Assignee | ||
Comment 4•17 years ago
|
||
Attachment #288081 -
Attachment is obsolete: true
Attachment #290652 -
Flags: review?(sspitzer)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [need new patch]
Assignee | ||
Comment 5•17 years ago
|
||
>
> 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
Whiteboard: [has patch][needs review sspitzer]
Assignee | ||
Comment 6•17 years ago
|
||
Comment 7•17 years ago
|
||
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•17 years ago
|
||
Comment on attachment 290652 [details] [diff] [review]
fix v2
will wait for v2
Attachment #290652 -
Flags: review?(sspitzer)
Updated•17 years ago
|
Whiteboard: [has patch][needs review sspitzer] → [has patch]
Comment 9•17 years ago
|
||
> 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.
Assignee | ||
Comment 10•17 years ago
|
||
(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.
Blocks: 391419
Assignee | ||
Comment 11•17 years ago
|
||
Attachment #290652 -
Attachment is obsolete: true
Attachment #291012 -
Flags: review?(sspitzer)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch] → [has patch][needs review sspitzer]
Assignee | ||
Comment 12•17 years ago
|
||
forgot the new test file in the last patch
Attachment #291012 -
Attachment is obsolete: true
Attachment #291033 -
Flags: review?(sspitzer)
Attachment #291012 -
Flags: review?(sspitzer)
Comment 13•17 years ago
|
||
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!
Attachment #291033 -
Flags: review?(sspitzer) → review+
Updated•17 years ago
|
Whiteboard: [has patch][needs review sspitzer] → [has patch][needs final patch]
Assignee | ||
Comment 14•17 years ago
|
||
> 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.
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch][needs final patch] → [has patch]
Updated•17 years ago
|
Whiteboard: [has patch] → [has patch][needs landing]
Assignee | ||
Comment 16•17 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs landing]
Assignee | ||
Comment 17•17 years ago
|
||
backed out due to Ts regression and test failure.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 18•17 years ago
|
||
note: i disabled the test, but did not cvs remove the file.
Assignee | ||
Comment 19•17 years ago
|
||
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
Assignee | ||
Comment 20•17 years ago
|
||
Attachment #291303 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Assignee | ||
Comment 21•17 years ago
|
||
Assignee | ||
Updated•17 years ago
|
Attachment #291596 -
Flags: review?(sspitzer)
Assignee | ||
Comment 22•17 years ago
|
||
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•17 years ago
|
||
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.
Assignee | ||
Comment 24•17 years ago
|
||
Attachment #291596 -
Attachment is obsolete: true
Attachment #291602 -
Flags: review?(sspitzer)
Attachment #291596 -
Flags: review?(sspitzer)
Comment 25•17 years ago
|
||
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?
Attachment #291602 -
Flags: review?(sspitzer) → review+
Assignee | ||
Comment 26•17 years ago
|
||
added the new test, checked-in.
Attachment #291602 -
Attachment is obsolete: true
Comment 27•17 years ago
|
||
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•17 years ago
|
||
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);
Assignee | ||
Comment 29•17 years ago
|
||
(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•17 years ago
|
||
Assignee | ||
Comment 31•17 years ago
|
||
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•17 years ago
|
||
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.
Assignee | ||
Comment 33•17 years ago
|
||
(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•17 years ago
|
||
(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.
>
Assignee | ||
Updated•17 years ago
|
Attachment #291620 -
Attachment description: still need to test, but ensure that last modified date is bigger → still need to test, but ensure that last modified date is bigger (checked-in)
Attachment #291620 -
Flags: review+
Comment 35•17 years ago
|
||
I also tested this on another machine yesterday (Vista) and got the same results as in Comment 34.
Assignee | ||
Comment 36•17 years ago
|
||
(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•17 years ago
|
||
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•17 years ago
|
||
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
Status: RESOLVED → VERIFIED
Updated•17 years ago
|
Version: unspecified → Trunk
Comment 39•15 years ago
|
||
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•