Closed Bug 356487 Opened 15 years ago Closed 15 years ago

make MOZ_PLACES (if enabled) only build the places based history UI, use MOZ_PLACES_BOOKMARKS to turn off the places bookmarks code

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: moco, Assigned: moco)

References

Details

Attachments

(1 file, 16 obsolete files)

122.46 KB, patch
Details | Diff | Splinter Review
make MOZ_PLACES (if enabled) only build the places based history UI, use MOZ_PLACES_BOOKMARKS to turn off the places bookmarks code 

dietrich asked me for a bug on this.  I'll attach my patch once it is ready for primetime.
so far, the two big problems I have left are:

1) bug #356453, clear private data doesn't appear to clear history
2) bug #356174, implement the history sidebar on top of places.

I still need to test to make sure that:

1)  with --enable-places, nothing else is broken
2)  with --disable-places, I didn't break anything

after I do that, I'll seek a review.
Status: NEW → ASSIGNED
Comment on attachment 242250 [details] [diff] [review]
2nd draft, includes fixes for bugs:  356175, 356453, 356630, 356631

did some smoketesting of places / non-places, and I think this is worthy of initial review (while I work on the two known issues for places-enabled)

dietrich, can you review?
Attachment #242250 - Flags: review?(dietrich)
Comment on attachment 242250 [details] [diff] [review]
2nd draft, includes fixes for bugs:  356175, 356453, 356630, 356631

got some good suggestions from dietrich over aim.
Attachment #242250 - Flags: review?(dietrich) → review-
over aim, dietrich points out:

1) what is MOZ_PLACES_NEED_NEW_UI

it's a flag I'm using to disable any Places history UI that was not in Fx 2.0.  I'll rename to MOZ_PLACES_NEW_HISTORY_UI

2)  I'll remove the bogus ones, where I'm doing

#ifndef MOZ_PLACES_NEED_NEW_UI

3) MOZ_PLACES_BOOKMARKS_OR_NEED_NEW_UI

I need to fix that one, as well.

4) add to the wiki what MOZ_PLACES, MOZ_PLACES_BOOKMARKS and MOZ_PLACES_NEW_HISTORY_UI are for.

Comment on attachment 243721 [details] [diff] [review]
3rd draft, address comment from dietrich, includes fixes for bugs: 356175, 356453, 356630, 356631, and now 357301 (add back history sidebar item to configurable toolbars)

not ready for prime time, missing two new files and not working when places enabled.
Attachment #243721 - Attachment is obsolete: true
Attached patch updated version... (obsolete) — Splinter Review
Attachment #244131 - Attachment is obsolete: true
Attached patch updated version (obsolete) — Splinter Review
dietrich, here's what's not working yet:

1) context menu delete (no bug yet)
2) GROUP_BY_DATE, so "View | By Date" and "View | By Date and Site" (see bug #358784)
3) not getting the "no drop" cursor if you drag into the sidebar (see bug #358838, not this is also a Fx 2 bug)
Attachment #244152 - Attachment is obsolete: true
Attached patch updated version (obsolete) — Splinter Review
Attachment #244156 - Attachment is obsolete: true
this patch excludes the GROUP_BY_DATE patch.  

Additionally, cmd_delete and context menu delete of items from the history sidebar do not work yet.
Attachment #244333 - Flags: review?(dietrich)
Attachment #244328 - Attachment is obsolete: true
Attachment #244333 - Attachment is obsolete: true
Attachment #244374 - Flags: review?(dietrich)
Attachment #244333 - Flags: review?(dietrich)
on a related note, for bug #358784, I need to add:

<?xml-stylesheet href="chrome://browser/skin/places/places.css"?>

to history-panel.xul

Additionally, in mozilla/browser/themes/winstripe/browser/jar.mn and mozilla/browser/themes/pinstripe/browser/jar.mn, I need to move places.css out of MOZ_PLACES_BOOKMARKS and into MOZ_PLACES
To clarify: This the expected behavior and state of things WRT to the build flags after this patch, correct?

- MOZ_PLACES: Use Places for history backend, use old History UI.
- MOZ_PLACES_BOOKMARKS: Use Places for bookmarks UI and backend.
- MOZ_PLACES_NEW_HISTORY_UI: Use Places for history UI and backend.
- Turning on all 3 is effectively the equivalent of an all-Places build (ie: what MOZ_PLACES does now)
dietrich:

>To clarify: This the expected behavior and state of things WRT to the build
>flags after this patch, correct?
>
>- MOZ_PLACES: Use Places for history backend, use old History UI.

yes.

>- MOZ_PLACES_BOOKMARKS: Use Places for bookmarks UI and backend.

yes.

> - MOZ_PLACES_NEW_HISTORY_UI: Use Places for history UI and backend.

to be specific, this is is to turn off places-on-history UI that was not part of the history UI in Fx 2.

> - Turning on all 3 is effectively the equivalent of an all-Places build (ie:
what MOZ_PLACES does now)

Yes, if you applied the patch and turned on all 3 (note, you can't easily do that as there is no configure for MOZ_PLACES_NEW_HISTORY_UI and MOZ_PLACES_BOOKMARKS), you should get MOZ_PLACES + a history sidebar.

Note, I have not attempted this.  I have used MOZ_PLACES_NEW_HISTORY_UI and MOZ_PLACES_BOOKMARKS when auditing the code to separate history-on-places from bookmarks-on-places.

As we go forward with bookmarks-on-places, we'd use MOZ_PLACES_BOOKMARKS as starting points for work that needs to be done.

I imagine we'll end up removing the code turned off by MOZ_PLACES_NEW_HISTORY_UI
from aim, bookmarks were not working, because of this:

+#ifdef MOZ_PLACES_BOOKMARKS
function addBookmarkAs(aBrowser, aBookmarkAllTabs, aIsWebPanel)

should be:

+#ifndef MOZ_PLACES_BOOKMARKS
function addBookmarkAs(aBrowser, aBookmarkAllTabs, aIsWebPanel)
Attachment #244451 - Attachment is obsolete: true
Attachment #244498 - Flags: review?(dietrich)
Attachment #244451 - Flags: review?(dietrich)
Comment on attachment 244498 [details] [diff] [review]
fix broken "add bookmark", thanks dietrich

This looks good to me. All the history and bookmarks UI are in place, and basic bookmarks tests checked out. We should get this checked in, and handle other side effects moving forward.
Attachment #244498 - Flags: review?(dietrich) → review+
Comment on attachment 244498 [details] [diff] [review]
fix broken "add bookmark", thanks dietrich

seeking second review from browser peer (gavin)
Attachment #244498 - Flags: review?(gavin.sharp)
there have been changes to browser.js (for "Bug 356590 - Clean up nsContextMenu and make it a bit more window-agnostic, partly fixes bug 281490. r=gavin.") that require some adjustment for bit rot.

working on it now, but I think gavin can still review the attached patch.
Attachment #244498 - Attachment is obsolete: true
Attachment #244603 - Flags: review?(gavin.sharp)
Attachment #244498 - Flags: review?(gavin.sharp)
Comment on attachment 244603 [details] [diff] [review]
patch, updated for bit rot (a MOZ_PLACES change made to browser.js moved to nsContextMenu.js)

>Index: browser/components/places/content/history-panel.js

>+function HistorySidebarInit()

>+  // XXXBlake we should persist the last search value
>+  // If it's empty, this will do the right thing and 
>+  // just group by the old grouping.
>+  // bug #359073 tracks this RFE
>+  searchHistory(gSearchBox.value);
>+}

Another merge from bug 322077: a gSearchBox.focus() call here is lost.

>+function SetPlace(aSearchString)
>+{
>+  var placeURI = ORGANIZER_ROOT_HISTORY_UNSORTED;
>+
>+  // if there is a search string, root the places tree into
>+  // the history root (sorted by SORT_BY_TITLE_ASCENDING)
>+  // otherwise, root the tree based on gHistoryGrouping
>+  if (aSearchString != "")

style nit: I prefer avoiding explicit comparisons to null/undefined/"" when they aren't needed. I don't really know if other places code follows this same convention.

>+    placeURI += "&sort=" + NHQO.SORT_BY_TITLE_ASCENDING;
>+  else {
>+    if (gHistoryGrouping == "site")
>+      placeURI += "&sort=" + NHQO.SORT_BY_TITLE_ASCENDING;
>+    else if (gHistoryGrouping == "visited") 
>+      placeURI += "&sort=" + NHQO.SORT_BY_VISITCOUNT_DESCENDING;
>+    else if (gHistoryGrouping == "lastvisited")
>+      placeURI += "&sort=" + NHQO.SORT_BY_DATE_DESCENDING;
>+    else if (gHistoryGrouping == "dayandsite") {
>+      /* placeURI += "&group=" + NHQO.GROUP_BY_DAY; */
>+      placeURI += "&group=" + NHQO.GROUP_BY_HOST; 
>+      placeURI += "&sort=" + NHQO.SORT_BY_TITLE_ASCENDING;
>+    }
>+    else {
>+      /* placeURI += "&group=" + NHQO.GROUP_BY_DAY; */
>+      placeURI += "&sort=" + NHQO.SORT_BY_TITLE_ASCENDING;
>+    }
>+  }

Kinda seems like this would be easier to read as a switch to select the right sort constant, then a single assignment, and a special case for the dayandsite "group". Or maybe it's best left as-is, up to you.

>+  // fix for bug #358831
>+  // right button click should not toggle open / closed state
>+  // aEvent.button == 1; -> aEvent.button != 0;

This comment is a little verbose, and isn't really necessary, I think. I'd just remove it.

>Index: browser/components/places/content/history-panel.xul

>+<page id="history-panel" orient="vertical"
>+      xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+      onload="HistorySidebarInit();"
>+      elementtofocus="historyTree">

The elementtofocus stuff was removed in bug 322077, so it should not be readded here.

>+#ifdef XP_MACOSX
>+    <!-- XXX sspitzer fix context menu delete and cmd_delete are not working -->
>+    <key id="key_delete2" keycode="VK_BACK" command="cmd_delete"/>
>+#endif

I don't really understand this comment. Can you clarify, and ideally point to a bug #?
Attachment #244603 - Flags: review?(gavin.sharp) → review+
1)

> Another merge from bug 322077: a gSearchBox.focus() call here is lost.

fixed, thanks.

until places is enabled by default, I'm going to have to watch mozilla/browser/components/history/content/history-panel.xul  and mozilla/browser/components/history/content/history.js for changes.

2)

> style nit: I prefer avoiding explicit comparisons to null/undefined/"" when
> they aren't needed. I don't really know if other places code follows this same
> convention.

I fixed this style nit in two places in history-panel.js, thanks.

3)

> Kinda seems like this would be easier to read as a switch to select the right
> sort constant, then a single assignment, and a special case for the dayandsite
> "group". Or maybe it's best left as-is, up to you.

I fixed it to use a switch statement, but kept the code the same.

4)

>This comment is a little verbose, and isn't really necessary, I think. I'd just
> remove it.

removed, thanks.

5)

> The elementtofocus stuff was removed in bug 322077, so it should not be readded
> here.

removed, thanks.

6)

> I don't really understand this comment. Can you clarify, and ideally point to a
> bug #?

for the new history-sidebar-on-places, context menu delete (or delete key) doesn't work yet.  I've logged bug #359462 to track this impending regression (if you enable places) and I've fixed the comment to refer to it.

I'll attach a final version of the patch.
fix checked in.

Checking in browser/base/content/browser-context.inc;
/cvsroot/mozilla/browser/base/content/browser-context.inc,v  <--  browser-context.inc
new revision: 1.28; previous revision: 1.27
done
Checking in browser/base/content/browser-menubar.inc;
/cvsroot/mozilla/browser/base/content/browser-menubar.inc,v  <--  browser-menubar.inc
new revision: 1.104; previous revision: 1.103
done
Checking in browser/base/content/browser-sets.inc;
/cvsroot/mozilla/browser/base/content/browser-sets.inc,v  <--  browser-sets.inc
new revision: 1.84; previous revision: 1.83
done
Checking in browser/base/content/browser.css;
/cvsroot/mozilla/browser/base/content/browser.css,v  <--  browser.css
new revision: 1.21; previous revision: 1.20
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.732; previous revision: 1.731
done
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.328; previous revision: 1.327
done
Checking in browser/base/content/global-scripts.inc;
/cvsroot/mozilla/browser/base/content/global-scripts.inc,v  <--  global-scripts.inc
new revision: 1.10; previous revision: 1.9
done
Checking in browser/base/content/nsContextMenu.js;
/cvsroot/mozilla/browser/base/content/nsContextMenu.js,v  <--  nsContextMenu.js
new revision: 1.3; previous revision: 1.2
done
Checking in browser/components/Makefile.in;
/cvsroot/mozilla/browser/components/Makefile.in,v  <--  Makefile.in
new revision: 1.59; previous revision: 1.58
done
Checking in browser/components/bookmarks/Makefile.in;
/cvsroot/mozilla/browser/components/bookmarks/Makefile.in,v  <--  Makefile.in
new revision: 1.9; previous revision: 1.8
done
Checking in browser/components/build/Makefile.in;
/cvsroot/mozilla/browser/components/build/Makefile.in,v  <--  Makefile.in
new revision: 1.49; previous revision: 1.48
done
Checking in browser/components/build/nsModule.cpp;
/cvsroot/mozilla/browser/components/build/nsModule.cpp,v  <--  nsModule.cpp
new revision: 1.48; previous revision: 1.47
done
Checking in browser/components/feeds/src/FeedConverter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedConverter.js,v  <--  FeedConverter.js
new revision: 1.21; previous revision: 1.20
done
Checking in browser/components/microsummaries/src/nsMicrosummaryService.js.in;
/cvsroot/mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js.in,v  <--  nsMicrosummaryService.js.in
new revision: 1.52; previous revision: 1.51
done
Checking in browser/components/migration/src/Makefile.in;
/cvsroot/mozilla/browser/components/migration/src/Makefile.in,v  <--  Makefile.in
new revision: 1.26; previous revision: 1.25
done
Checking in browser/components/migration/src/nsBrowserProfileMigratorUtils.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsBrowserProfileMigratorUtils.cpp,v  <--  nsBrowserProfileMigratorUtils.cpp
new revision: 1.17; previous revision: 1.16
done
Checking in browser/components/migration/src/nsIEProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp,v  <--  nsIEProfileMigrator.cpp
new revision: 1.46; previous revision: 1.45
done
Checking in browser/components/migration/src/nsIEProfileMigrator.h;
/cvsroot/mozilla/browser/components/migration/src/nsIEProfileMigrator.h,v  <--  nsIEProfileMigrator.h
new revision: 1.9; previous revision: 1.8
done
Checking in browser/components/migration/src/nsOperaProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsOperaProfileMigrator.cpp,v  <--  nsOperaProfileMigrator.cpp
new revision: 1.54; previous revision: 1.53
done
Checking in browser/components/migration/src/nsOperaProfileMigrator.h;
/cvsroot/mozilla/browser/components/migration/src/nsOperaProfileMigrator.h,v  <--  nsOperaProfileMigrator.h
new revision: 1.15; previous revision: 1.14
done
Checking in browser/components/migration/src/nsSafariProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsSafariProfileMigrator.cpp,v  <--  nsSafariProfileMigrator.cpp
new revision: 1.29; previous revision: 1.28
done
Checking in browser/components/migration/src/nsSafariProfileMigrator.h;
/cvsroot/mozilla/browser/components/migration/src/nsSafariProfileMigrator.h,v  <--  nsSafariProfileMigrator.h
new revision: 1.14; previous revision: 1.13
done
Checking in browser/components/places/jar.mn;
/cvsroot/mozilla/browser/components/places/jar.mn,v  <--  jar.mn
new revision: 1.28; previous revision: 1.27
done
Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.100; previous revision: 1.99
done
RCS file: /cvsroot/mozilla/browser/components/places/content/history-panel.js,v
done
Checking in browser/components/places/content/history-panel.js;
/cvsroot/mozilla/browser/components/places/content/history-panel.js,v  <--  history-panel.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/browser/components/places/content/history-panel.xul,v
done
Checking in browser/components/places/content/history-panel.xul;
/cvsroot/mozilla/browser/components/places/content/history-panel.xul,v  <--  history-panel.xul
initial revision: 1.1
done
Checking in browser/components/preferences/jar.mn;
/cvsroot/mozilla/browser/components/preferences/jar.mn,v  <--  jar.mn
new revision: 1.16; previous revision: 1.15
done
Checking in browser/components/preferences/main.js;
/cvsroot/mozilla/browser/components/preferences/main.js,v  <--  main.js
new revision: 1.6; previous revision: 1.5
done
Checking in browser/locales/jar.mn;
/cvsroot/mozilla/browser/locales/jar.mn,v  <--  jar.mn
new revision: 1.60; previous revision: 1.59
done
Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.css
new revision: 1.35; previous revision: 1.34
done
Checking in browser/themes/pinstripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/pinstripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.32; previous revision: 1.31
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.48; previous revision: 1.47
done
Checking in browser/themes/winstripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/winstripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.30; previous revision: 1.29
done
Checking in extensions/metrics/src/Makefile.in;
/cvsroot/mozilla/extensions/metrics/src/Makefile.in,v  <--  Makefile.in
new revision: 1.19; previous revision: 1.18
done
Checking in extensions/metrics/src/nsProfileCollector.cpp;
/cvsroot/mozilla/extensions/metrics/src/nsProfileCollector.cpp,v  <--  nsProfileCollector.cpp
new revision: 1.15; previous revision: 1.14
done
Checking in extensions/metrics/src/nsProfileCollector.h;
/cvsroot/mozilla/extensions/metrics/src/nsProfileCollector.h,v  <--  nsProfileCollector.h
new revision: 1.4; previous revision: 1.3
done
Checking in extensions/metrics/src/nsUICommandCollector.cpp;
/cvsroot/mozilla/extensions/metrics/src/nsUICommandCollector.cpp,v  <--  nsUICommandCollector.cpp
new revision: 1.12; previous revision: 1.11
done
Checking in xpfe/components/Makefile.in;
/cvsroot/mozilla/xpfe/components/Makefile.in,v  <--  Makefile.in
new revision: 1.94; previous revision: 1.93
done
Checking in xpfe/components/search/src/nsInternetSearchService.cpp;
/cvsroot/mozilla/xpfe/components/search/src/nsInternetSearchService.cpp,v  <--  nsInternetSearchService.cpp
new revision: 1.252; previous revision: 1.251
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Component: History → Bookmarks & History
QA Contact: history → bookmarks
You need to log in before you can comment on or make changes to this bug.