Closed Bug 337324 Opened 16 years ago Closed 16 years ago

move Firefox-specific microsummary directory defs to nsBrowserDirectoryProvider

Categories

(Firefox Graveyard :: Microsummaries, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 2 beta1

People

(Reporter: myk, Assigned: myk)

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 2 obsolete files)

Microsummary directories are defined in nsDirectoryServiceDefs, but on IRC bsmedberg says "we have nsBrowserDirectoryProvider for firefox-specific keys,and the #define should be in a firefox-specific place, either in some nsMicroSummaries.h or nsBrowserCompsCID.h".
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 2 beta1
This patch moves the microsummary directories to nsBrowserDirectoryProvider.  For filename consistency with nsDirectoryServiceDefs.h and nsAppDirectoryServiceDefs.h, I put the microsummary defines into the new file nsBrowserDirectoryServiceDefs.h.

I also copied nsAppFileLocationProvider::CloneMozBinDirectory() into nsBrowserDirectoryProvider.  And I fixed a hardcoded reference to the UsrMicsumDefs directory in nsMicrosummaryService.js.in.

Do I still need to provide a Mac OS 9-specific (i.e. XP_MAC) name for the microsummary generators directory, given that Firefox no longer supports that OS?
Attachment #223989 - Flags: review?(benjamin)
Whiteboard: [swag: 0.25d]
Attached patch patch v2 (obsolete) — Splinter Review
myk, I don't think you want the copying-from-defaults behavior of the lower section of browserdirectory, so I remade your patch to shortcut everything in the first set of if/else blocks. You don't need CloneMozBinDirectory, you can safely just use NS_XPCOM_CURRENT_PROCESS_DIR
Attachment #223989 - Attachment is obsolete: true
Attachment #224894 - Flags: review?(myk)
Attachment #223989 - Flags: review?(benjamin)
Attached patch patch v3 Splinter Review
> myk, I don't think you want the copying-from-defaults behavior of the lower
> section of browserdirectory, so I remade your patch to shortcut everything in
> the first set of if/else blocks. You don't need CloneMozBinDirectory, you can
> safely just use NS_XPCOM_CURRENT_PROCESS_DIR

Yeah, that all seems right to me and has the corollary benefit of making the logic simpler.

This patch looks good, except that we probably don't need to do #if defined(XP_MAC), since browser doesn't support Mac OS 9-, and it almost certainly never will.  Also, we don't need to #include nsILocalFile.h now that we aren't adding CloneMozBinDirectory.

Here's an updated patch with these changes.
Attachment #224894 - Attachment is obsolete: true
Attachment #224913 - Flags: review?(benjamin)
Attachment #224894 - Flags: review?(myk)
Attachment #224913 - Flags: review?(benjamin) → review+
Attachment #224913 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #224913 - Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+
Fix checked in to trunk and 1.8 branch.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Keywords: fixed1.8.1
Whiteboard: [swag: 0.25d]
Component: Bookmarks → Microsummaries
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.