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

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

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.

Comment 1

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

Attachment: patch, first draft, when building with places use places history but non-places bookmarks

Comment 2

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

Attachment: 2nd draft, includes fixes for bugs: 356175, 356453, 356630, 356631

Comment 3

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

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.

Comment 4

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

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?

Comment 5

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

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.

Comment 6

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

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 7

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

Attachment: 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)

Comment 8

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

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.

Comment 9

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

Attachment: updated patch, includes my rough draft of the history sidebar

Comment 10

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

Attachment: get group by (except for date related grouping) working, more cleanup, etc.

Comment 11

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

Attachment: updated version...

Comment 12

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

Attachment: updated version

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)

Comment 13

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

Attachment: updated version

Comment 14

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

Attachment: screen shot of "view | by date and site" working

Comment 15

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

Attachment: update patch with initial implementation of GROUP_BY_DATE (still some todo items)

Comment 16

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

Attachment: base patch, excludes the GROUP_BY_DATE work, updated to trunk (had conflict with browser.js)

Comment 17

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

Attachment: patch for review, cleaned up some comments

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.

Comment 18

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

Attachment: fix mozilla/browser/locales/jar.mn, per dmills

Comment 19

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

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

Comment 20

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

Attachment: add places.css to history-panel.xul, fix jar.mn in themes (for places.css and missing images used by places)

Comment 21

[Dietrich Ayala (: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.
- 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)

Comment 22

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

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

Comment 23

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

Attachment: fix broken "add bookmark", thanks dietrich

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)

Comment 24

[Dietrich Ayala (: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.

Comment 25

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

Comment on attachment 244498 [details] [diff] [review]
fix broken "add bookmark", thanks dietrich

seeking second review from browser peer (gavin)

Comment 26

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

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.

Comment 27

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

Attachment: patch, updated for bit rot (a MOZ_PLACES change made to browser.js moved to nsContextMenu.js)

Comment 28

[:Gavin Sharp [email: [e-mail]]]

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 #?

Comment 29

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

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.

Comment 30

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

Attachment: patch (after addressing gavin's review comments)

Comment 31

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

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