371798 - query result nodes must update titles on bookmark id not URI

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dietrich Ayala (:dietrich)]

From https://bugzilla.mozilla.org/show_bug.cgi?id=360133#c15:

> Now I'm searching for child nodes by bookmark id instead of URI
> where appropriate.

Yes, that's exactly what I'd like to say! And finally we got ready to
fight against another URI-base enemy "ChangeTitles(...)".

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp&rev=1.79&mark=2868-2874#2868

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: fix v1 - for folder nodes, update titles by bookmark id instead of uri

Comment 2

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

Comment on attachment 258271 [details] [diff] [review]
fix v1 - for folder nodes, update titles by bookmark id instead of uri

r=sspitzer

Comment 3

[Robert Sayre]

do we have a test for this?

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: test v1

wip test. haven't run it yet, as mochitest isn't running the tests in browser/components/places/tests/chrome.

rob, there's a test in there already, but in the mochitest output, the dirs are shown, but no tests run. any idea why?

Comment 5

[Robert Sayre]

(In reply to comment #4)
> Created an attachment (id=258333) [details]
> test v1
> 
> wip test. haven't run it yet, as mochitest isn't running the tests in
> browser/components/places/tests/chrome.

perl runtests.pl --chrome

> 
> rob, there's a test in there already, but in the mochitest output, the dirs are
> shown, but no tests run. any idea why?

It copies non-test resources for the add_livemark test onto the http server's web root. As there are more tests added to the directory, it should make more sense.

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 258333 [details] [diff] [review]
test v1


>+var toolbar = document.getElementById("bookmarksBarContent");

rob, any idea why i wouldn't be able to access nodes in chrome? the above statement works fine in core and extensions, but not in mochitest.

Comment 7

[Robert Sayre]

(In reply to comment #6)
> (From update of attachment 258333 [details] [diff] [review])
> 
> >+var toolbar = document.getElementById("bookmarksBarContent");
> 
> rob, any idea why i wouldn't be able to access nodes in chrome? the above
> statement works fine in core and extensions, but not in mochitest.

That code is looking for the toolbar inside the content document--the one that corresponds to the XUL test file. You need to grab the top level window and ask for its XUL doc.

Comment 8

[Dietrich Ayala (:dietrich)]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 258333 [details] [diff] [review] [details])
> > 
> > >+var toolbar = document.getElementById("bookmarksBarContent");
> > 
> > rob, any idea why i wouldn't be able to access nodes in chrome? the above
> > statement works fine in core and extensions, but not in mochitest.
> 
> That code is looking for the toolbar inside the content document--the one that
> corresponds to the XUL test file. You need to grab the top level window and ask
> for its XUL doc.
> 

ah, ok i see. it'd be great if there was an option to run the chrome test as an overlay, or something that would allow it to run in the same environment as core browser code.

Comment 9

[Asaf Romano (gone)]

Please do not make API tests depend on the browser UI.

Comment 10

[Dietrich Ayala (:dietrich)]

hm, yeah i guess it's more of an end-to-end test. not sure how i'd isolate this change in a test.

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: test v2

Comment 12

[Robert Sayre]

Comment on attachment 259964 [details] [diff] [review]
test v2

looks good to me

Comment 13

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/tests/chrome/Makefile.in;
/cvsroot/mozilla/toolkit/components/places/tests/chrome/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/chrome/test_371798.xul,v
done
Checking in toolkit/components/places/tests/chrome/test_371798.xul;
/cvsroot/mozilla/toolkit/components/places/tests/chrome/test_371798.xul,v  <--  test_371798.xul
initial revision: 1.1
done

Comment 14

[Dietrich Ayala (:dietrich)]

Attachment: test v3

race condition in the test (test can execute before the query result has been live-updated).

this makes the test run after the bookmarks service has sent out notifications.

Comment 15

[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