Closed Bug 386392 Opened 12 years ago Closed 12 years ago

Drop pre places/places-bookmarks support from browser/

Categories

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

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: mano, Assigned: mano)

References

Details

Attachments

(4 files, 5 obsolete files)

Priority: -- → P2
Target Milestone: --- → Firefox 3 beta1
Attached patch part 1 (obsolete) — Splinter Review
To be done separately:
  * cvs removes for browser/components/bookmarks & browser/components/history
  * cvs removes for some files in browser/locales
  * cvs remove the old microsumarries interface file.
  * --enable-place-bookmarks build option removal
  * MOZ_PLACES_BOOKMARKS->MOZ_PLACES in the xpfe search service (gavin:
   what's that about anyway?)
Assignee: nobody → mano
Status: NEW → ASSIGNED
Attachment #270405 - Flags: review?(gavin.sharp)
wasn't the plan to keep non-places for alice's performance tests?
* Do we have a tinderbox building non-places/non-places-bookmarks?

* Which performance tests? Ts/Txul/Tp regressions are filed as we break them on the default configuration.

Maintaining three different build modes for browser/ does not make a good balance here IMO, esp. as in-browser window UI based on places is hooked up.

Either way, this has to be done before l10n freeze.
I'm not sure the perf benchmarking on/off is going to be particularly useful, especially if it means we need to maintain other features against the old code.  We should get talos benchmarks for on/off now, and any deltas need to be made up before ship.

If we're at least on par for perf in the general case, or have clearly identified cases where we need to vastly improve perf, I think we're ready to purge the old code.
Note that you'll probably have to do the on/off benchmarks with static build instead of libxul builds, because the non-places code doesn't build with libxul.

(In reply to comment #4)
>  We should get talos benchmarks for on/off now, and any deltas need to be made
> up before ship.

This is done. Last week Alice did 20 runs w/ each of the 3 configurations (no places, places, places bookmarks). Old bookmarks can go away now.
Attached patch part 1, updated to tip (obsolete) — Splinter Review
Attachment #270405 - Attachment is obsolete: true
Attachment #271817 - Flags: review?(gavin.sharp)
Attachment #270405 - Flags: review?(gavin.sharp)
Comment on attachment 271817 [details] [diff] [review]
part 1, updated to tip

>Index: browser/base/content/browser.xul
>-#include browser-gotreehack.inc

We can remove this file too now, right? Both references to it are being removed.

>Index: browser/components/Makefile.in
>Index: browser/components/build/Makefile.in

Indentation looks wrong in these files.

Also don't forget to remove nsIMicrosummaryServiceNonPlaces.

grep -r --exclude=".#*" --exclude="*.orig" --exclude="*.rej" "MOZ_PLACES" browser/ tells me that you forgot a few in places/content, need to fix before landing.

Want to file a bug about making --disable-places fail when building Firefox? (or do it here?). We should also post to the newsgroups/devnews about this.

We need to just remove the ifdefs in xpfe/components/search, we shouldn't build that directory anymore, so they aren't needed. Can file a new bug for that.
Attachment #271817 - Flags: review?(gavin.sharp) → review+
Attachment #272680 - Flags: review? → review?(benjamin)
Attachment #272680 - Flags: review?(benjamin) → review+
Attached patch confuse them (obsolete) — Splinter Review
toolkit/ patch coming.
Attachment #272680 - Attachment is obsolete: true
Attachment #272688 - Flags: review?(gavin.sharp)
Attached patch toolkit changes (obsolete) — Splinter Review
Attachment #272689 - Flags: review?(gavin.sharp)
Attachment #272689 - Flags: review?(gavin.sharp) → review+
Attachment #272688 - Flags: review?(gavin.sharp) → review+
Attached patch for checkinSplinter Review
Attachment #271817 - Attachment is obsolete: true
Attachment #272688 - Attachment is obsolete: true
Attachment #272689 - Attachment is obsolete: true
mozilla/configure.in 1.1850
mozilla/browser/build.mk 1.5
mozilla/browser/confvars.sh 1.6
mozilla/browser/base/content/browser-doctype.inc 1.12
mozilla/browser/base/content/browser-gotreehack.inc delete
mozilla/browser/base/content/browser-menubar.inc 1.116
mozilla/browser/base/content/browser-sets.inc 1.99
mozilla/browser/base/content/browser.js 1.815
mozilla/browser/base/content/browser.xul 1.350
mozilla/browser/base/content/browserMountPoints.inc 1.5
mozilla/browser/base/content/global-scripts.inc 1.14
mozilla/browser/base/content/macBrowserOverlay.xul 1.16
mozilla/browser/base/content/nsContextMenu.js 1.18
mozilla/browser/base/content/web-panels.xul 1.21
mozilla/browser/components/Makefile.in 1.60
mozilla/browser/components/nsBrowserGlue.js 1.32
mozilla/browser/components/build/Makefile.in 1.57
mozilla/browser/components/build/nsModule.cpp 1.51
mozilla/browser/components/dirprovider/nsBrowserDirectoryProvider.cpp 1.18
mozilla/browser/components/feeds/src/FeedConverter.js 1.26
mozilla/browser/components/feeds/src/FeedWriter.js 1.43
mozilla/browser/components/microsummaries/public/Makefile.in 1.3;
mozilla/browser/components/microsummaries/public/nsIMicrosummaryServiceNonPlaces.idl delete
mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js 1.67
mozilla/browser/components/migration/src/Makefile.in 1.34
mozilla/browser/components/migration/src/nsBrowserProfileMigratorUtils.cpp 1.25;
mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp 1.62
mozilla/browser/components/migration/src/nsIEProfileMigrator.h 1.13
mozilla/browser/components/migration/src/nsOperaProfileMigrator.cpp 1.67
mozilla/browser/components/migration/src/nsOperaProfileMigrator.h 1.20
mozilla/browser/components/migration/src/nsSafariProfileMigrator.cpp 1.42
mozilla/browser/components/migration/src/nsSafariProfileMigrator.h 1.19
mozilla/browser/components/places/Makefile.in 1.8
mozilla/browser/components/places/content/controller.js 1.170
mozilla/browser/components/places/content/history-panel.js 1.20
mozilla/browser/components/places/content/history-panel.xul 1.11
mozilla/browser/components/places/content/placesOverlay.xul 1.14
mozilla/browser/components/places/tests/Makefile.in 1.9
mozilla/browser/components/preferences/main.js 1.9
mozilla/browser/components/search/content/engineManager.js 1.17
mozilla/browser/components/sidebar/src/nsSidebar.js 1.29
mozilla/browser/locales/jar.mn 1.68
mozilla/browser/themes/pinstripe/browser/browser.css 1.56
mozilla/browser/themes/pinstripe/browser/jar.mn 1.44
mozilla/browser/themes/winstripe/browser/browser.css 1.66
mozilla/browser/themes/winstripe/browser/jar.mn 1.39
mozilla/toolkit/components/places/src/Makefile.in 1.26
mozilla/toolkit/components/places/src/nsNavBookmarks.cpp 1.109
mozilla/toolkit/components/places/tests/Makefile.in 1.2
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071717 Minefield/3.0a7pre ID:2007071717

bookmarks in sidebar is broken. (cannot display them in sidebar.)
caused by this check-in ?
Attached patch two moreSplinter Review
so we can have a bookmarks sidebar, and pick bookmarks as home pages
Attachment #272725 - Flags: review?(gavin.sharp)
Attachment #272725 - Flags: review?(gavin.sharp) → review+
browser/components/places/jar.mn 1.37
browser/components/preferences/jar.mn 1.18
Blocks: 383833
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Version: unspecified → Trunk
Near as I can tell, this covers all of components/bookmarks/'s few remaining deadcode friends, too.
Attachment #284815 - Flags: review?(mano)
At the very least, the unused strings part of this ought to be blocking whatever turns out to be the string freeze milestone.
Flags: blocking-firefox3?
Comment on attachment 284815 [details] [diff] [review]
rm components/bookmarks/

r=mano excluding the browser/themes files. I would like to remove those only once we move over all the images used wisely by places' jar.mns...
Attachment #284815 - Flags: review?(mano)
Attachment #284815 - Flags: review+
Attachment #284815 - Flags: approval1.9?
Comment on attachment 284815 [details] [diff] [review]
rm components/bookmarks/

this can wait until after beta,
Attachment #284815 - Flags: approvalM9-
Attachment #284815 - Flags: approval1.9?
Attachment #284815 - Flags: approval1.9+
Not a blocker, either, though it's got approval to land :)
Flags: blocking-firefox3? → blocking-firefox3-
Target Milestone: Firefox 3 M9 → Firefox 3 M10
browser/components/bookmarks/.cvsignore delete
browser/components/bookmarks/jar.mn delete
browser/components/bookmarks/Makefile.in delete
browser/components/bookmarks/content/addBookmark2.js delete
browser/components/bookmarks/content/addBookmark2.xul delete
browser/components/bookmarks/content/addBookmark.xul delete
browser/components/bookmarks/content/addBookmark.js delete
browser/components/bookmarks/content/bookmarks.css delete
browser/components/bookmarks/content/bookmarks.js delete
browser/components/bookmarks/content/bookmarksManager.js delete
browser/components/bookmarks/content/bookmarksManager.xul delete
browser/components/bookmarks/content/bookmarksMenu.js delete
browser/components/bookmarks/content/bookmarksPanel.js delete
browser/components/bookmarks/content/bookmarksPanel.xul delete
browser/components/bookmarks/content/bookmarksProperties.js delete
browser/components/bookmarks/content/bookmarksProperties.xul delete
browser/components/bookmarks/content/bookmarksTree.xml delete
browser/components/bookmarks/content/microsummaryPicker.js delete
browser/components/bookmarks/content/selectBookmark.js delete
browser/components/bookmarks/content/selectBookmark.xul delete
browser/components/bookmarks/public/.cvsignore delete
browser/components/bookmarks/public/Makefile.in delete
browser/components/bookmarks/public/nsIBookmarksService.idl delete
browser/components/bookmarks/public/nsIBookmarkTransactionManager.idl delete
browser/components/bookmarks/src/.cvsignore delete
browser/components/bookmarks/src/Makefile.in delete
browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp delete
browser/components/bookmarks/src/nsBookmarksService.cpp delete
browser/components/bookmarks/src/nsBookmarksService.h delete
browser/components/bookmarks/src/nsBookmarkTransactionManager.js delete
browser/components/bookmarks/src/nsForwardProxyDataSource.cpp delete
browser/components/bookmarks/src/nsForwardProxyDataSource.h delete
browser/locales/en-US/chrome/browser/bookmarks/addBookmark.dtd delete
browser/locales/en-US/chrome/browser/bookmarks/bookmarks.dtd delete
browser/locales/en-US/chrome/browser/bookmarks/bookmarks.properties delete
browser/locales/en-US/chrome/browser/bookmarks/bookmarksProperties.dtd delete
philor, you scrub the removed Makefiles out of mozilla/browser/makefiles.sh, right ?
If by that you mean "you will scrub ... once I remind you?" then "yes" :)

browser/makefiles.sh 1.4
Considerably easier than bookmarks - places still uses the same .dtd, and the .properties lives in toolkit, so only the .js and .xul are dead.
Attachment #289327 - Flags: review?(mano)
Comment on attachment 289327 [details] [diff] [review]
rm components/history/

r=mano
Attachment #289327 - Flags: review?(mano)
Attachment #289327 - Flags: review+
Attachment #289327 - Flags: approval1.9?
Attachment #289327 - Flags: approval1.9? → approval1.9+
browser/components/history/.cvsignore delete
browser/components/history/jar.mn delete
browser/components/history/Makefile.in delete
browser/components/history/content/history.js delete
browser/components/history/content/history-panel.xul delete
browser/makefiles.sh 1.5
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Depends on: 381365
Target Milestone: Firefox 3 beta3 → ---
The theme files wound up picking up bugs of their own (bug 382095, bug 424613, possibly even more I've forgotten about), so in hindsight this has been fixed for months.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
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.