Closed Bug 407843 Opened 17 years ago Closed 16 years ago

There's no point in passing a bms to lms.createLivemarkFolderOnly/lms._createFolder

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: philor, Assigned: philor)

References

()

Details

Attachments

(1 file, 1 obsolete file)

Bug 358946 comment 5 created createLivemarkFolderOnly to avoid kicking off HTTP traffic during first-run import, and passed in a bms to avoid having the first access to the livemark service's own this._bms create another one that would want to import again. Either we fail to test for that case, or it doesn't really exist (any more), because since the checkin for bug 382877 back in August, _createFolder has been using the passed-in bms to create the folder, and then using (and thus creating) this._bms to set it readonly.

I'd do the obvious thing, and s/this._//, but I don't have a clue how to write a test for the badness it's supposed to be causing. Or, I'd do the obvious thing, and take out all the passing of bmses since it's apparently no longer needed, but I don't have a clue what to point at to say "see, that made it unneeded."
Attached patch Fix v.1 (obsolete) — Splinter Review
Meh. It's... code cleanup! and a possible perf win, that's why it doesn't need a test, yeah, that's the ticket...
Assignee: nobody → philringnalda
Status: NEW → ASSIGNED
Attachment #295195 - Flags: review?(dietrich)
Flags: in-testsuite-
Comment on attachment 295195 [details] [diff] [review]
Fix v.1

canceling review request, as bug 382711 will obviate the need for all of this junk, because initiation of the livemark update timer will be externally controllable.
Attachment #295195 - Flags: review?(dietrich)
Depends on: 382711
Morphing a bit, since we were both wrong.

Far as I can see, this really doesn't have much to do with bug 382711 - that only stops us from getting HTTP traffic from the timer deciding to update everything. We still need two separate ways to create a livemark, one for user-created ones which fetches immediately, and one for import which doesn't.

But passing in a bms to createFolder was trying to solve a reentrant service creation problem, that first creating the bms would trigger import, which would create a livemark, which would access the livemark service's bms, which since it wasn't created yet would create it, which would trigger import, which would...

That seems to have been fixed by bug 376008, when you moved import to browser/, and moved the import trigger from creating the bms to the _initPlaces call in nsBrowserGlue.js, so we should clean up createLivemarkFolderOnly/_createFolder to not take a bms, since there's no point in not using this._bms.
Summary: _createFolder breaks createLivemarkFolderOnly's attempt to not reinit the bookmarks service → There's no point in passing a bms to lms.createLivemarkFolderOnly/lms._createFolder
Comment on attachment 295195 [details] [diff] [review]
Fix v.1

r=me.
Attachment #295195 - Flags: review+
Comment on attachment 295195 [details] [diff] [review]
Fix v.1

er sorry. re-reading last comment: yes, proper fix is to remove the passed in bms altogether.
Attachment #295195 - Flags: review+
hm, when migrating on first run, might still have the reentrant service problem.
Attachment #295195 - Attachment is obsolete: true
Attached patch Fix v.2Splinter Review
Well, I don't know. We should do one or the other of these, and this one, taking out the passed in bms, seems to import just fine, and passes tests. I'm willing to believe I just don't know how to trigger the badness, and we don't test it, but I sure don't see it.
Attachment #298656 - Flags: review?(dietrich)
(In reply to comment #6)
> hm, when migrating on first run, might still have the reentrant service
> problem.
> 

That's why it exists, iirc.
Well, note that if we have a problem because of using this._bms, then we've been shipping that problem since last August. That was comment 0. Shouldn't we have seen some sort of pain as a result?
There could have been changes introduced since that make it ok now.

Easy to check. If you run a debug build with a new profile, do you get an assert?
Oh, new profile? That's even easier, I thought I needed to be importing a profile from Fx2. In both cases, no assertions, no fresh warnings or chatter, exactly the same console spew with and without the patch.
(In reply to comment #11)
> Oh, new profile? That's even easier, I thought I needed to be importing a
> profile from Fx2. In both cases, no assertions, no fresh warnings or chatter

Oh good. Hope this is still true: in my day, Places would "import" bookmarks.html when you create new profile, in the process of creating places.sqlite
Comment on attachment 298656 [details] [diff] [review]
Fix v.2

r=me
Attachment #298656 - Flags: review?(dietrich) → review+
Attachment #298656 - Flags: approval1.9?
Attachment #298656 - Flags: approval1.9? → approval1.9+
toolkit/components/places/src/nsLivemarkService.js 1.37
toolkit/components/places/public/nsILivemarkService.idl 1.13
toolkit/components/places/tests/unit/test_exclude_livemarks.js 1.2
toolkit/components/places/tests/bookmarks/test_livemarks.js 1.3
toolkit/components/places/tests/chrome/test_341972a.xul 1.3
toolkit/components/places/tests/chrome/test_341972b.xul 1.3
toolkit/components/places/tests/chrome/test_342484.xul 1.2
browser/components/places/src/nsPlacesImportExportService.cpp 1.41
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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.

Attachment

General

Created:
Updated:
Size: