Last Comment Bug 382187 - use places for SeaMonkey history
: use places for SeaMonkey history
Status: RESOLVED FIXED
: fixed-seamonkey2.0
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- normal with 6 votes (vote)
: seamonkey2.0a3
Assigned To: Robert Kaiser
:
Mentors:
: 109758 345055 (view as bug list)
Depends on: 320831 suiterunner 370698 383833 391346 392844 415117 468344 468809 469465 470543 473007 477994
Blocks: 63292 394502 448729 456694 460095 467157 468326 468330 468336 468341 468363 468381 472873 477150 SMPlacesBMarks
  Show dependency treegraph
 
Reported: 2007-05-27 17:40 PDT by Robert Kaiser
Modified: 2010-04-13 10:51 PDT (History)
31 users (show)
kairo: wanted‑seamonkey2.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
backend WIP patch, v1 (8.22 KB, patch)
2007-05-28 04:32 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
use places history backend if MOZ_PLACES is set (6.63 KB, patch)
2007-06-09 05:03 PDT, Robert Kaiser
ted: review+
Details | Diff | Splinter Review
use places history backend if MOZ_PLACES is set, v1.1 (6.61 KB, patch)
2007-06-21 14:25 PDT, Robert Kaiser
kairo: review+
Details | Diff | Splinter Review
use places history backend if --enable-places is set, v1.2 (checked in) (6.34 KB, patch)
2007-08-08 12:52 PDT, Robert Kaiser
kairo: review+
neil: superreview+
Details | Diff | Splinter Review
enabling and UI, WIP (359.74 KB, patch)
2008-10-03 09:27 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
switch UI, v1 (382.01 KB, patch)
2008-10-03 17:03 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
garbled up patch (to ignore) (46.81 KB, patch)
2008-11-13 10:54 PST, Robert Kaiser
no flags Details | Diff | Splinter Review
switch UI, v2 (260.39 KB, patch)
2008-11-13 12:17 PST, Robert Kaiser
no flags Details | Diff | Splinter Review
switch UI, v3 (239.25 KB, patch)
2008-11-20 13:12 PST, Robert Kaiser
no flags Details | Diff | Splinter Review
switch UI, v3.1 (237.13 KB, patch)
2008-11-22 08:21 PST, Robert Kaiser
neil: review+
neil: superreview+
Details | Diff | Splinter Review
Screenshot of assertion failed dialog (71.04 KB, image/png)
2008-11-22 14:26 PST, Stefan [:stefanh]
no flags Details
switch UI, v4 (231.41 KB, patch)
2008-12-06 08:27 PST, Robert Kaiser
neil: review+
kairo: superreview+
Details | Diff | Splinter Review

Description Robert Kaiser 2007-05-27 17:40:50 PDT
(not sure if there's a better component for suite history)

For suiterunner, we should move to places for history, which should be pretty easy, as Mano told me on IRC.

And the basics are easy enough that I managed within the last few hours to get it built and visited sites are working in the browser, so the backend and migration of the old mork history to places is actually there in my patch.

As storage doesn't support RDF, out UI is broken as well though and urlbar autocomplete as well as the history window still need to be fixed.
Comment 1 Robert Kaiser 2007-05-28 04:32:38 PDT
Created attachment 266370 [details] [diff] [review]
backend WIP patch, v1

This is the backend WIP patch I'm working with currently. I think this should basically be everything we need to do with the backend (as I said, it seems to work here).
If people think it's worth landing backend and frontend work separately, this can be reworked so that the different history implementations are enabled depending on whether MOZ_PLACES is set - I didn't do that for now.
Comment 2 Robert Kaiser 2007-06-09 05:03:22 PDT
Created attachment 267795 [details] [diff] [review]
use places history backend if MOZ_PLACES is set

Together with the patch in bug 383833, this makes SeaMonkey build the places history backend if MOZ_PLACES is set.

Note that this patch conflicts with my attachment 267090 [details] [diff] [review] in bug 380786, I'll post a revised patch here if that one will go in before the patch here.

the good thing with this approach is that we can get this patch in and have anyone else than me work on the needed UI changes easily.
Comment 3 Robert Kaiser 2007-06-21 14:25:42 PDT
Created attachment 269286 [details] [diff] [review]
use places history backend if MOZ_PLACES is set, v1.1

This patch is practically identical to the last one, just synced with the trunk, as nsModule.cpp saw a few cleanups and the last patch would probably conflict there now because of some context lines that went away.

Again, this patch does not change anything in normal builds yet, it just adds the necessary ifdefs so that one can do a places-backend-enabled build by just adding MOZ_PLACES=1 to suite/confvars.sh
Comment 4 Robert Kaiser 2007-06-25 05:51:11 PDT
Now that the first step of bug 383833 has been checked in, the 1.1 patch here should really work (for both the default non-places and the MOZ_PLACES cases).
Before that checkin, I suppose the MOZ_PLACES part might have had some problems unless you correctly fixed toolkit Makefiles along with it, but now I think everything should work fine to optionally switch the backend with the 1.1 patch.
Comment 5 Robert Kaiser 2007-07-29 04:47:26 PDT
With the latest checkin for bug 304309, a build with the patch here and MOZ_PLACES=1 even has a working urlbar history, only the history window and sidebar stay empty as the RDF datasource is gone. Bug 321172 should help there though.
I'm still for inclusion of this patch as people doing their own builds can try places history easily with it...
Comment 6 Robert Kaiser 2007-08-08 12:52:35 PDT
Created attachment 275824 [details] [diff] [review]
use places history backend if --enable-places is set, v1.2 (checked in)

I've been informed that the last patch bitrotted a bit, so here's an unbitrotted version.
Comment 7 neil@parkwaycc.co.uk 2007-08-16 13:10:59 PDT
Comment on attachment 275824 [details] [diff] [review]
use places history backend if --enable-places is set, v1.2 (checked in)

(I don't see why you should use MOZ_PLACES when you can use --enable-places)
Comment 8 Robert Kaiser 2007-08-17 06:10:49 PDT
Comment on attachment 275824 [details] [diff] [review]
use places history backend if --enable-places is set, v1.2 (checked in)

Right, --enable-places should be the nice way to turn on MOZ_PLACES ;-)
Checked in this patch.
Comment 9 jag (Peter Annema) 2007-08-18 05:21:17 PDT
Woot!
Comment 10 Robert Kaiser 2007-12-06 06:47:25 PST
We now have mozStorage templates available, so I guess someone who knows a bit how to deal with XUL trees should be able to get the UI working.
The backend works fine, we have some slight glitches with mouse clicks on history autocomplete doing wrong things while keyboard scrolling and pressing enter works, but everything else except the history window and sidebar work fine in --enable-places builds, which I've using for production for a few months now.
We also probably want prefs UI like in bug 402880, as a note.

Who could pick this up and get the UI working?
Comment 11 Igor Velkov 2008-04-03 16:28:23 PDT
why --enable-places did not on for nightly builds?
Comment 12 Justin Wood (:Callek) 2008-04-03 18:03:25 PDT
Igor, --enable-places is not on in nightly builds, because with it enabled none of our related user interface works correctly.
Comment 13 Tony Mechelynck [:tonymec] 2008-05-08 21:56:50 PDT
*** Bug 345055 has been marked as a duplicate of this bug. ***
Comment 14 Robert Kaiser 2008-10-03 09:27:06 PDT
Created attachment 341625 [details] [diff] [review]
enabling and UI, WIP

Here's a first WIP for enabling places history and converting UI. The sidebar panel here is almost 1:1 a copy of Firefoxtrunk, the history window is a wild mix between our old history window and parts of the places organizer.
This still needs cleanup to get rid of non-history-related stuff, and some work to make menu items work, but we get some UI with this again finally :)
Comment 15 Robert Kaiser 2008-10-03 11:15:05 PDT
My WIP work is using the link opening functions from bug 370698, so we depend on that.
Comment 16 Igor Velkov 2008-10-03 13:03:12 PDT
Is it possible on this Big Dig time integrate the "Show Parent Folder" add-on functionality?
Comment 17 Robert Kaiser 2008-10-03 13:25:03 PDT
Igor, not sure what this add-on is but this is not about any "Big Dig time" thing, this is about changing the history (and only history) backend storage to a more modern database while keeping UI very similar to what we had (not 100% though, there are slight changes). Everything else that might build up on this can be done incrementally afterwards.
Comment 18 Robert Kaiser 2008-10-03 17:03:51 PDT
Created attachment 341706 [details] [diff] [review]
switch UI, v1

OK, I've come far enough to call this patch a v1 now and go to reviews. Not sure what parts are still in there that don't deal with history and could be cut out, I hope I caught most of those.
I'll look into the Modern theme in an additional patch, this one only has the default theme (it basically only tree icons anyway).
Comment 19 Caio Tiago Oliveira (asrail) 2008-10-05 20:37:18 PDT
If I leave a space on the end of the input the dropdown menu got empty with a lag depending on places performance - grayed out in the meantime, but with full height (keep flashing while writing a sentence). That works fine on FX.

> +pref("browser.urlbar.restrict.bookmark", "*");

Doesn't make much sense if you won't use places bookmarks.
At least it is not ever working here.


I've seen this warning a lot of times:
 WARNING: Attempting to register an observer twice!: .../comm-central/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp, line 3872

I don't know the code very well, but this warning shouldn't be raised and I'm not used to see it without this patch.


Livemarks and tags will be used by SeaMonkey?
If so, there is a lot of code that won't be used on suite/common/src/nsPlacesTransactionsService.js
, if not all.

Alternatively you could leave the backend unchanged and only not to expose such things on the frontend. As a win, would be fair easier for someone to make a extension to enable tags, places bookmarks and so on.
Comment 20 Robert Kaiser 2008-10-06 05:00:45 PDT
(In reply to comment #19)
> If I leave a space on the end of the input the dropdown menu got empty with a
> lag depending on places performance - grayed out in the meantime, but with full
> height (keep flashing while writing a sentence). That works fine on FX.

Hmm, I never saw that myself, I think.

> > +pref("browser.urlbar.restrict.bookmark", "*");
> 
> Doesn't make much sense if you won't use places bookmarks.
> At least it is not ever working here.

If this is not defined, it will be taken as empty and the bar will always filter for bookmarks by default, i.e. always be empty. I don't like how they coded this feature for filter for stuff by default, but as they did it like that, we need to define filter characters for all parts right away.

> I've seen this warning a lot of times:
>  WARNING: Attempting to register an observer twice!:
> .../comm-central/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,
> line 3872
> 
> I don't know the code very well, but this warning shouldn't be raised and I'm
> not used to see it without this patch.

I also never saw that here... This sounds like toolkit code?

> Livemarks and tags will be used by SeaMonkey?
> If so, there is a lot of code that won't be used on
> suite/common/src/nsPlacesTransactionsService.js
> , if not all.

We're only using history for now, I tried to kill everything for now that was clearly bookmarks stuff, but I missed going through PlacesTransactionsService. I'm waiting for Neil's review comments before doing more work on that though.

> Alternatively you could leave the backend unchanged and only not to expose such
> things on the frontend. As a win, would be fair easier for someone to make a
> extension to enable tags, places bookmarks and so on.

I'd rather not have two bookmarks backends in the product, I'm pretty sure they'd clash, e.g. the places bookmarks backend will automatically import and delete a bookmarks.html when it find it...
Comment 21 Caio Tiago Oliveira (asrail) 2008-10-06 07:32:42 PDT
(In reply to comment #20)
> Hmm, I never saw that myself, I think.

If you type "mozilla " on the urlbar with this patch applied it list some entries?
Here it doesn't work. If I type "mozilla s" it works.

> > > +pref("browser.urlbar.restrict.bookmark", "*");
> > 
> > Doesn't make much sense if you won't use places bookmarks.
> > At least it is not ever working here.
> 
> If this is not defined, it will be taken as empty and the bar will always
> filter for bookmarks by default, i.e. always be empty. I don't like how they
> coded this feature for filter for stuff by default, but as they did it like
> that, we need to define filter characters for all parts right away.

OK. I just though it was possible to completely ignore that keyword, i.e., not filtering for bookmarks at all, on the code.

> > I've seen this warning a lot of times:
> >  WARNING: Attempting to register an observer twice!:
> > .../comm-central/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,
> > line 3872
> > 
> > I don't know the code very well, but this warning shouldn't be raised and I'm
> > not used to see it without this patch.
> 
> I also never saw that here... This sounds like toolkit code?

The code itself is in toolkit, but we should take care with the places we use the service. That's possible we're trying to initialize it twice.

Are you using debug builds and loading a lot of pages?

> > Livemarks and tags will be used by SeaMonkey?
> > If so, there is a lot of code that won't be used on
> > suite/common/src/nsPlacesTransactionsService.js
> > , if not all.
> 
> We're only using history for now, I tried to kill everything for now that was
> clearly bookmarks stuff, but I missed going through PlacesTransactionsService.
> I'm waiting for Neil's review comments before doing more work on that though.
> 
> > Alternatively you could leave the backend unchanged and only not to expose such
> > things on the frontend. As a win, would be fair easier for someone to make a
> > extension to enable tags, places bookmarks and so on.
> 
> I'd rather not have two bookmarks backends in the product, I'm pretty sure
> they'd clash, e.g. the places bookmarks backend will automatically import and
> delete a bookmarks.html when it find it...

OK.
Comment 22 Robert Kaiser 2008-10-06 08:31:37 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > Hmm, I never saw that myself, I think.
> 
> If you type "mozilla " on the urlbar with this patch applied it list some
> entries?
> Here it doesn't work. If I type "mozilla s" it works.

Interesting, you're right. Could we do that in a followup? Could be related to autocomplete or specific to urlbar.

> OK. I just though it was possible to completely ignore that keyword, i.e., not
> filtering for bookmarks at all, on the code.

No, unfortunately that's not possible.

> The code itself is in toolkit, but we should take care with the places we use
> the service. That's possible we're trying to initialize it twice.
> 
> Are you using debug builds and loading a lot of pages?

I've used debug for testing the patch, but not with a lot of pages. I just copied FF code, so I wonder why we'd work differently here.
Comment 23 Caio Tiago Oliveira (asrail) 2008-10-06 11:59:46 PDT
I agree with you it's better to fix minor issues in a follow-up, don't know about Neil.
At least would be interesting to resolve the major issues before this one to be addressed.


Well... there is nsNavHistoryQueryResultNode::FillChildren() and nsNavHistoryResult::RemoveHistoryObserver. If you call FillChildren twice without calling RemoveHistoryObserver, that warning is raised.
You don't call any of them directly, so I don't know where the error could reside.

I'll try to look at the stack when I back home.

Maybe someone that worked on this can help.
Comment 24 Caio Tiago Oliveira (asrail) 2008-10-06 20:31:09 PDT
Opening the history dialog on a package (unpacked tar.bz2) throws this:
Failed to load jar:file:///home/asrail/mozbuilds/suiterunner/chrome/toolkit.jar!/content/global/nsTreeSorting.js
JavaScript error: chrome://communicator/content/history/history.js, line 111: SortInNewDirection is not defined

and it kindly breaks - load old dialog.

Running it  twice also raises:
###!!! ASSERTION: URI mapped to two different specs?: 'uriMapEntry->mDocMapEntry == nsnull', file /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/xpcom/io/nsFastLoadFile.cpp, line 420
WARNING: redundant multiplexed document?: 'docMapEntry->mString == nsnull', file /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/xpcom/io/nsFastLoadFile.cpp, line 1404


While running direct from the dist directory, it works, but:
it raises a lot of warnings:
WARNING: GetDefaultCharsetForLocale: need to add multi locale support: file /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/intl/uconv/src/nsUNIXCharset.cpp, line 189

while opening the history or opening pages. At least on a debug build the performance penalty for those actions is very big - for a big (42M) places database it freezes for a few seconds.

Sometimes the history takes too long that it shows the "unresponsive script" dialog with:
Script: chrome://communicator/content/history/treeView.js:201

as taking too long.

I could manage to get that warning on a clean profile:
Breakpoint 1, nsNavHistoryResult::AddHistoryObserver (this=0x7ff14cb4d400, aNode=0x7ff14cb4c200)
    at /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp:3872
3872       NS_WARNING("Attempting to register an observer twice!");
(gdb) bt
#0  nsNavHistoryResult::AddHistoryObserver (this=0x7ff14cb4d400, aNode=0x7ff14cb4c200)
    at /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp:3872
#1  0x00007ff16b49fe52 in nsNavHistoryQueryResultNode::FillChildren (this=0x7ff14cb4c200)
    at /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp:2399
#2  0x00007ff16b4a010f in nsNavHistoryQueryResultNode::Refresh (this=0x7ff14cb4c200)
    at /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp:2483
#3  0x00007ff16b4a0272 in nsNavHistoryQueryResultNode::OnTitleChanged (this=0x7ff14cb4d4b0, aURI=0x7ff13e511600, 
    aPageTitle=@0x7ff13a7b8048)
    at /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp:2737
#4  0x00007ff16b49ab3f in nsNavHistoryResult::OnTitleChanged (this=<value optimized out>, aURI=0x7ff13e511600, 
    aPageTitle=@0x7fff8a3207d0)
    at /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp:4380
#5  0x00007ff16b47d491 in nsNavHistory::SetPageTitleInternal (this=0x7ff16bdd0800, aURI=0x7ff13e511600, 
    aTitle=@0x7ff14a427358)
    at /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/toolkit/components/places/src/nsNavHistory.cpp:5906
#6  0x00007ff16b483829 in nsNavHistory::CommitLazyMessages (this=0x7ff16bdd0800)
    at /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/toolkit/components/places/src/nsNavHistory.cpp:4847
#7  0x00007ff1815291b3 in nsTimerImpl::Fire (this=0x7ff169ed5760)
    at /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/xpcom/threads/nsTimerImpl.cpp:420
#8  0x00007ff1815292a3 in nsTimerEvent::Run (this=0x7ff13c624d90)
    at /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/xpcom/threads/nsTimerImpl.cpp:512
#9  0x00007ff1815251d9 in nsThread::ProcessNextEvent (this=0x7ff179f1e700, mayWait=1, result=0x7fff8a320a7c)
    at /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/xpcom/threads/nsThread.cpp:510
#10 0x00007ff1814de7cf in NS_ProcessNextEvent_P (thread=0x7ff14cb4d4b0, mayWait=1) at nsThreadUtils.cpp:227
#11 0x00007ff172b9415a in nsBaseAppShell::Run (this=0x7ff179e08d60)
    at /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:170
#12 0x00007ff1719da0ce in nsAppStartup::Run (this=0x7ff175959f10)
    at /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:182
#13 0x00007ff1819d3077 in XRE_main (argc=<value optimized out>, argv=<value optimized out>, aAppData=<value optimized out>)
    at /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/toolkit/xre/nsAppRunner.cpp:3265
#14 0x0000000000400d83 in main (argc=4, argv=0x7fff8a3211e8)
    at /home/asrail/mozbuilds/hg.mozilla.org/comm-central/suite/app/nsSuiteApp.cpp:103



I'm not 100% sure it works everytime, but is working reliably here:
 - load some pages on history - say, mozilla.org, mozillazine.org, seamonkey-project.org and mozdev.org
 - open history
 - search for something that shows results - say, mozilla
 - load any page that changes the results (update the time or title) - say, mozillazine.org

After this, it gets corrupted and loading any page with the history open is broken - say, load flickr.com.

If you close history and open it again everything is fine.


Firefox doesn't suffer from this, neither on the sidebar or the dialog.

Also it looks like SeaMonkey doesn't suffer from this on the history sidebar, only on the history dialog.


PS: re-adding grouping items by date (today, yesterday, 2 days ago, ..., more than 6 days ago) is planed for that dialog?
Comment 25 Robert Kaiser 2008-10-07 07:24:21 PDT
(In reply to comment #24)
> PS: re-adding grouping items by date (today, yesterday, 2 days ago, ..., more
> than 6 days ago) is planed for that dialog?

I'm not able to do this, but I'm happy if anyone will do it as a followup.

I must admit I have no clue even where to start looking at regarding your comments about problems. I'm way too weak on JS foo.
Comment 26 Serge Gautherie (:sgautherie) 2008-10-07 08:47:46 PDT
(In reply to comment #24)
> jar:file:///home/asrail/mozbuilds/suiterunner/chrome/toolkit.jar!/content/global/nsTreeSorting.js
> JavaScript error: chrome://communicator/content/history/history.js, line 111:
> SortInNewDirection is not defined

nsTreeSorting.js was renamed in bug 411828;
and the patch here removes the 5 references to SortInNewDirection() anyway.

Are you sure you tested the latest patch ?

> ###!!! ASSERTION: URI mapped to two different specs?:
> 'uriMapEntry->mDocMapEntry == nsnull', file

I'm quite sure that I've seen this assertion without having ever tried the patch in this bug:
I think this is not related to it, unless you're saying that the already checked in patch caused it ?

> While running direct from the dist directory, it works, but:
> it raises a lot of warnings:
> WARNING: GetDefaultCharsetForLocale: need to add multi locale support: file
> /home/asrail/mozbuilds/hg.mozilla.org/comm-central/mozilla/intl/uconv/src/nsUNIXCharset.cpp,
> line 189

This looks rather like bug 448655.
Comment 27 Caio Tiago Oliveira (asrail) 2008-10-07 10:38:22 PDT
(In reply to comment #26)
> (In reply to comment #24)
> > jar:file:///home/asrail/mozbuilds/suiterunner/chrome/toolkit.jar!/content/global/nsTreeSorting.js
> > JavaScript error: chrome://communicator/content/history/history.js, line 111:
> > SortInNewDirection is not defined
> 
> nsTreeSorting.js was renamed in bug 411828;
> and the patch here removes the 5 references to SortInNewDirection() anyway.
> 
> Are you sure you tested the latest patch ?

Yes, I'm sure.

When I run this directly from the dist directory it works fine, but if I make a tar.bz2 and unpack it raises that error. I think it's some issue with the packaging process.

> > ###!!! ASSERTION: URI mapped to two different specs?:
> > 'uriMapEntry->mDocMapEntry == nsnull', file
> 
> I'm quite sure that I've seen this assertion without having ever tried the
> patch in this bug:

This assertion is related to the fast-load file. There are several causes to raise this. If I erase the fast-load file and run it twice from the unpacked tar.bz2, it raises this.
Comment 28 Serge Gautherie (:sgautherie) 2008-10-07 11:11:16 PDT
(In reply to comment #27)

I know/understand: my point being to file separate bugs for separate issues, unless you're saying that the patch(es) here is causing them.
Comment 29 Robert Kaiser 2008-10-07 12:07:04 PDT
(In reply to comment #27)
> When I run this directly from the dist directory it works fine, but if I make a
> tar.bz2 and unpack it raises that error. I think it's some issue with the
> packaging process.

Now I understand what you mean, and you're right, the transaction service needs to be added to the packages files. I'll fix that locally and include it in the next patch update after I get a first round of review comments from Neil.
For now, don't test this patch on a build created with "make package", test it on (a copy of) dist/bin.

> > > ###!!! ASSERTION: URI mapped to two different specs?:
> > > 'uriMapEntry->mDocMapEntry == nsnull', file
> > 
> > I'm quite sure that I've seen this assertion without having ever tried the
> > patch in this bug:
> 
> This assertion is related to the fast-load file. There are several causes to
> raise this. If I erase the fast-load file and run it twice from the unpacked
> tar.bz2, it raises this.

Does it happen in a build _without_ this patch?
Comment 30 Caio Tiago Oliveira (asrail) 2008-10-07 18:49:40 PDT
(In reply to comment #29)
> > This assertion is related to the fast-load file. There are several causes to
> > raise this. If I erase the fast-load file and run it twice from the unpacked
> > tar.bz2, it raises this.
> 
> Does it happen in a build _without_ this patch?

No, it doesn't. Probably the warning was caused by the wrong files being packaged (right files not packaged).
I.e., probably the first issue caused this warning after the first run.

Serge: yes, this patch caused the bugs I mentioned. So... I can't file separate issues since the issue is not on the main code.
Comment 31 neil@parkwaycc.co.uk 2008-10-10 16:32:00 PDT
OK, so here are the differences I noticed between the old and new UI:
New features:
* Can sort the history manager on columns not displayed
* Can search sidebar and manager inline (used to be a separate manager window)
* Can change the grouping in the sidebar
* Hovering the history sidebar updates the status bar
* Can select multiple entries in the sidebar (possibly not useful)
Regressed labels:
* Open Link in New Window is now Open in a New Window
* Open Link in New Tab is now Open in a New Tab
* Copy Link Location is now Copy
* Title is now Name
* Last Visited is now Visit Date?
Missing features:
* Can't group the history manager
* Sidebar only shows the title (you can't pick or sort columns)
* Open Link in New Window/Bookmark only appears when one link is selected
* Can't delete all history from a site or domain
* No First Visited, Hostname (minor) or Referrer columns
* Useless Added and Last Modified columns
* No Select All context menuitem (minor)
* History manager hover doesn't update the status bar
Comment 32 neil@parkwaycc.co.uk 2008-10-12 13:47:32 PDT
Bookmark This Link doesn't work in the sidebar.
Comment 33 neil@parkwaycc.co.uk 2008-10-12 13:59:37 PDT
I crashed in Places submitting that comment. It was trying to notify some sort of observer that no longer existed. I'm not sure whether it was caused by the main history window or the sidebar as I had had both open.
Comment 34 neil@parkwaycc.co.uk 2008-10-12 14:02:32 PDT
It was the sidebar, since I crashed submitting *that* comment...
Comment 35 neil@parkwaycc.co.uk 2008-10-12 14:14:11 PDT
(In reply to comment #29)
> (In reply to comment #27)
> > When I run this directly from the dist directory it works fine, but if I make a
> > tar.bz2 and unpack it raises that error. I think it's some issue with the
> > packaging process.
> Now I understand what you mean, and you're right, the transaction service needs
> to be added to the packages files.
Aren't you supposed to be removing it? History doesn't use transactions.
Comment 36 neil@parkwaycc.co.uk 2008-11-10 09:49:00 PST
(In reply to comment #31)
> * No First Visited, Hostname (minor) or Referrer columns
Places doesn't seem to support Hostname. It doesn't really support Referrer because it records all redirects, not just link clicks. But it might be possible to provide a First Visited column.
Comment 37 neil@parkwaycc.co.uk 2008-11-12 13:58:18 PST
Comment on attachment 341706 [details] [diff] [review]
switch UI, v1

I haven't looked at the whole patch, just skimmed the new files.

>+function InsertionPoint(aItemId, aIndex, aOrientation, aIsTag,
>+                        aDropNearItemId) {
Bookmarks stuff.

>+    case "cmd_cut":
Not sure cut makes sense, but, I guess it's OK.

>+    case "placesCmd_moveBookmarks":
>+      return this._hasRemovableSelection(true);
Bookmarks stuff.

>+    case "cmd_paste":
>+      return this._canInsert() && this._isClipboardDataPasteable();
Bookmarks stuff.

>+    case "placesCmd_sortBy:name":
Bookmarks stuff.

>+    // All other Places Commands are prefixed with "placesCmd_" ... this 
>+    // filters out other commands that we do _not_ support (see 329587).
>+    const CMD_PREFIX = "placesCmd_";
>+    return (aCommand.substr(0, CMD_PREFIX.length) == CMD_PREFIX);
This could use a regexp...

>+  _canInsert: function PC__canInsert() {
Bookmarks stuff.

>+  rootNodeIsSelected: function PC_rootNodeIsSelected() {
Unused?

>+  _isClipboardDataPasteable: function PC__isClipboardDataPasteable() {
Bookmarks stuff.

>+  _buildSelectionMetadata: function PC__buildSelectionMetadata() {
Hmm, looks overkill for just history...

>+  _assertURINotString: function PC__assertURINotString(value) {
Unused?

>+  sortFolderByName: function PC_sortFolderByName() {
Bookmarks stuff.

>+  _removeRange: function PC__removeRange(range, transactions, removedFolders) {
Bookmarks stuff.

>+      if (queryType == Components.interfaces.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS)
>+        this._removeRowsFromBookmarks(aTxnName);
This is always true.

>+  /**
>+   * Paste Bookmarks and Folders from the clipboard
>+   */
>+  paste: function PC_paste() {
Bookmarks stuff...

>+var PlacesControllerDragHelper = {
Bookmarks stuff.

>diff --git a/suite/common/history/history-panel.js b/suite/common/history/history-panel.js
>new file mode 100644
This shouldn't be a separate file. We'll want to be able to do most of these things in the main history manager too.

>+window.addEventListener("SidebarFocused",
>+                        function()
>+                        gSearchBox.focus(),
>+                        false);
We have a special attribute for this - <page elementtofocus="search-box">

>diff --git a/suite/common/history/menu.xml b/suite/common/history/menu.xml
>new file mode 100755
Bookmarks stuff.

>+hbox[type="places"] {
>+  -moz-binding: url("chrome://communicator/content/history/toolbar.xml#places-bar");
>+  overflow: hidden;
>+}
>+
>+menupopup[type="places"] {
>+  -moz-binding: url("chrome://communicator/content/history/menu.xml#places-menupopup");
>+}
>+
>+menupopup[placespopup="true"] {
>+  -moz-binding: url("chrome://communicator/content/history/menu.xml#places-popup-base");
>+}
Bookmarks stuff.

>diff --git a/suite/common/history/places.js b/suite/common/history/places.js
>new file mode 100755
Some of this stuff looks a bit weird, not sure which bits we need yet.

>diff --git a/suite/common/history/placesOverlay.xul b/suite/common/history/placesOverlay.xul
>new file mode 100644
Could have called this historyTreeOverlay.xul perhaps?

>diff --git a/suite/common/public/nsIPlacesTransactionsService.idl b/suite/common/public/nsIPlacesTransactionsService.idl
>new file mode 100644
Bookmarks stuff.

>diff --git a/suite/common/src/nsPlacesTransactionsService.js b/suite/common/src/nsPlacesTransactionsService.js
>new file mode 100644
Bookmarks stuff.
Comment 38 Robert Kaiser 2008-11-13 10:43:48 PST
(In reply to comment #31)
> Regressed labels:
> * Open Link in New Window is now Open in a New Window
> * Open Link in New Tab is now Open in a New Tab

Corrected in upcoming new patch.

> * Copy Link Location is now Copy

Hmm, we're now using a generic label for this just as for delete.

> * Title is now Name
> * Last Visited is now Visit Date?

Corrected in upcoming new patch.

> Missing features:
> * Can't group the history manager
> * Can't delete all history from a site or domain

Can we/someone do this in a followup?

> * Sidebar only shows the title (you can't pick or sort columns)
> * Open Link in New Window/Bookmark only appears when one link is selected

Not sure how to deal with this in a better way, but if there are ideas, followup?

> * No Select All context menuitem (minor)
> * History manager hover doesn't update the status bar

Shouldn't be too hard to do if wanted, I hope, but I'd also very much like to move this out to followup as I'm not very good at coding, so I'm glad about anything we can get in first, and then about anything someone else can improve step by step...

> * No First Visited, Hostname (minor) or Referrer columns

Not sure how host name or referrer can be done due to missing places support (the latter has been a long-standing open bug there, AFAIK). I actually thought the "Added" column would contain the First Visited date, but it seems as it only lists bookmark adding. The places DB has single visits stored, so as long as they are inside the history cutoff dates, I'm sure we can get them out somehow. Followup please?

> * Useless Added and Last Modified columns

I removed Last Modified in my upcoming new patch, see above for why I left "Added" in - should I remove that as well for now?
Comment 39 Robert Kaiser 2008-11-13 10:46:40 PST
(In reply to comment #37)
> >+    // All other Places Commands are prefixed with "placesCmd_" ... this 
> >+    // filters out other commands that we do _not_ support (see 329587).
> >+    const CMD_PREFIX = "placesCmd_";
> >+    return (aCommand.substr(0, CMD_PREFIX.length) == CMD_PREFIX);
> This could use a regexp...

Haven't looked into this yet.

> >diff --git a/suite/common/history/places.js b/suite/common/history/places.js
> >new file mode 100755
> Some of this stuff looks a bit weird, not sure which bits we need yet.

OK, another iteration of the patch is upcoming, this is largely unchanged for now.

> >diff --git a/suite/common/history/placesOverlay.xul b/suite/common/history/placesOverlay.xul
> >new file mode 100644
> Could have called this historyTreeOverlay.xul perhaps?

Hmm, it doesn't really overlay the tree itself, which that name would imply.

I addressed all other comments in the new patch I'll attach in a minute.
Comment 40 Robert Kaiser 2008-11-13 10:54:59 PST
Created attachment 348014 [details] [diff] [review]
garbled up patch (to ignore)

Here's an update of the patch. It ports a lot of prefs from FF, basically all I saw that seemed to apply to places. If you're trying it with current trunk, you might run into failing UI because toolkit places code exports a "utils.js" module which gets overwritten by a gloda (mailnews) module with the same name in current builds, the gloda people are about to solve this in bug 463278, but arguable, places guys should probably have named the module "placesUtils.js" instead as well. In any case, that module stuff is not the fault of my patch but a conflict of toolkit and mailnews.
Comment 41 Robert Kaiser 2008-11-13 11:36:59 PST
Comment on attachment 348014 [details] [diff] [review]
garbled up patch (to ignore)

oops, something's wrong here....
Comment 42 Robert Kaiser 2008-11-13 12:17:28 PST
Created attachment 348027 [details] [diff] [review]
switch UI, v2

Here's the correct v2 patch, sorry for the glitch with the other one, I had messed up the state of my mq a bit.
Comment 43 neil@parkwaycc.co.uk 2008-11-14 06:00:04 PST
Comment on attachment 348027 [details] [diff] [review]
switch UI, v2

Today I will be reviewing the theme.

>diff --git a/suite/themes/classic/communicator/history/history.png b/suite/themes/classic/communicator/history/history.png
>new file mode 100755
Does this image get used? I've never seen it display.

>+.sidebar-placesTree {
The old history used class="plain" instead to get a reasonable display.

>+.sidebar-placesTreechildren::-moz-tree-cell(leaf) ,
>+.sidebar-placesTreechildren::-moz-tree-image(leaf) {
>+  cursor: pointer;
>+}
The old history achieves this by including sidebarListView.css to add extra rules for sidebars. (You might want to add the underlining rule there too.)

>+/* Trees */
The history sidebar currently manages to use the same rules as bookmarks. It's a shame that toolkit is overriding them. See bug 464916. Anyway, I think you only need rules for four icons: 1. URLs 2/3. Folders. 4. Calendars.

>+#viewButton {
>+  margin-top: 2px;
>+  margin-bottom: 2px;
>+}
>+
>+#viewButton > hbox > dropmarker {
>+  height: auto;
>+  width: auto;
>+  -moz-margin-end: -3px;
>+}
Hmm, this is ugly. Maybe this should be a toolbarbutton instead?

>diff --git a/suite/themes/classic/communicator/history/query.png b/suite/themes/classic/communicator/history/query.png
>new file mode 100755
Does this image get used?
Comment 44 neil@parkwaycc.co.uk 2008-11-15 15:08:30 PST
Comment on attachment 348027 [details] [diff] [review]
switch UI, v2

Today I'm reviewing the XUL and XBL.

I have previously noticed a number of issues with the implementation as it's simply a straight port and I don't expect you to rewrite it, but I think it might be an idea to land the XUL as separate files NPOB and use jar.mn to switch based on --enable-places until the code is ready to go live.

>+#ifdef XP_MACOSX
>+    <key id="key_delete2" keycode="VK_BACK" command="cmd_delete"/>
>+#endif
The history manager doesn't need to ifdef this because of utilityOverlay...

>+    <label value="&find.label;" accesskey="&find.accesskey;" 
>+           control="search-box"/>
>+    <textbox id="search-box" flex="1" type="search"
>+             oncommand="searchHistory(this.value);"/>
Should this use emptytext instead?

>+    <popup id="placesColumnsContext"
>+           onpopupshowing="ViewMenu.fillWithColumns(event, null, null, 'checkbox', null);"
>+           oncommand="ViewMenu.showHideColumn(event.target); event.stopPropagation();"/>
I wonder what the benefit of filling the columns dynamically is.

>+        <menuitem id="orgCut"
>+                  command="cmd_cut"
>+                  label="&cutCmd.label;"
>+                  key="key_cut"
>+                  accesskey="&cutCmd.accesskey;"
If you called it menu_cut it would overlay these attributes from utilityOverlay.

>+                  selection="separator|link|folder|mixed"/>
Does this attribute have any use? The other strange ones I saw were collection and forcehideselection.

>diff --git a/suite/common/history/tree.xml b/suite/common/history/tree.xml
>+      <method name="getBestOptions">
>+      <property name="showRoot">
>+      <property name="onOpenFlatContainer">
>+        <getter><![CDATA[
>+          return this.getAttribute("onopenflatcontainer");
>+      <method name="selectPlaceURI">
>+      <method name="selectNode">
>+      <method name="getRemovableSelectionRanges">
>+      <method name="selectItems">
These seem to be unused or irrelevant.
Comment 45 neil@parkwaycc.co.uk 2008-11-15 15:58:44 PST
Comment on attachment 348027 [details] [diff] [review]
switch UI, v2

>-pref("browser.urlbar.autoFill", false);
>-pref("browser.urlbar.showPopup", true);
>-pref("browser.urlbar.showSearch", true);
>-pref("browser.urlbar.matchOnlyTyped", false);
Any reason to move these?

I had another look at the JS files and found some more apparently unused symbols:

RELOAD_ACTION_*

_hasRemovableSelection (it looks like it will always true)

aTxnName (and its callers)

OrganizerCommand_find and browser.xul are mentioned in an #ifdef but we don't use either anyway.

createMenuItemForNode

cleanPlacesPopup

Additionally:

some functions call PlacesUtils.nodeIsLivemarkContainer which will always be false for us

showHideColumn seems to muck around with splitters, which would be wrong because it breaks the columnpicker.

setSortColumn, getColumnType, sortTypeToColumnType should only need to work on history columns.

modifKey is probably computed wrongly for suite.

convertPRTimeToString code is wrong :-(
Comment 46 Stefan [:stefanh] 2008-11-15 16:00:31 PST
> >+    <label value="&find.label;" accesskey="&find.accesskey;" 
> >+           control="search-box"/>
> >+    <textbox id="search-box" flex="1" type="search"
> >+             oncommand="searchHistory(this.value);"/>
> Should this use emptytext instead?

That would be nice :-)
Comment 47 Justin Wood (:Callek) 2008-11-16 21:36:55 PST
(In reply to comment #44)
> (From update of attachment 348027 [details] [diff] [review])
> >+    <popup id="placesColumnsContext"
> >+           onpopupshowing="ViewMenu.fillWithColumns(event, null, null, 'checkbox', null);"
> >+           oncommand="ViewMenu.showHideColumn(event.target); event.stopPropagation();"/>
> I wonder what the benefit of filling the columns dynamically is.

If I had to guess, extensions
Comment 48 Robert Kaiser 2008-11-20 12:50:43 PST
(In reply to comment #43)
> >diff --git a/suite/themes/classic/communicator/history/history.png b/suite/themes/classic/communicator/history/history.png
> >new file mode 100755
> Does this image get used? I've never seen it display.

> >diff --git a/suite/themes/classic/communicator/history/query.png b/suite/themes/classic/communicator/history/query.png
> >new file mode 100755
> Does this image get used?

treeView.js' getCellProperties function at least has code for assigning the properties those two are shown with. I'm not 100% sure in which situations we'd end up with them though. Removing them along with the rules for showing them.

> >+.sidebar-placesTreechildren::-moz-tree-cell(leaf) ,
> >+.sidebar-placesTreechildren::-moz-tree-image(leaf) {
> >+  cursor: pointer;
> >+}
> The old history achieves this by including sidebarListView.css to add extra
> rules for sidebars. (You might want to add the underlining rule there too.)

But do we want the pointer cursor on non-leaf items? Or is what we want an update of sidebarListView.css with the complete set of rules from here, actually? I'm assuming the latter for now.

> >+/* Trees */
> The history sidebar currently manages to use the same rules as bookmarks. It's
> a shame that toolkit is overriding them. See bug 464916. Anyway, I think you
> only need rules for four icons: 1. URLs 2/3. Folders. 4. Calendars.

I hope I realized what exactly is meant to stay there :)

> >+#viewButton {
> Hmm, this is ugly. Maybe this should be a toolbarbutton instead?

Yes, looks usable for me on Linux, let's see how it works out for others...

(In reply to comment #44)
> I have previously noticed a number of issues with the implementation as it's
> simply a straight port and I don't expect you to rewrite it, but I think it
> might be an idea to land the XUL as separate files NPOB and use jar.mn to
> switch based on --enable-places until the code is ready to go live.

That's a possibility, but as long as it's not in by default, I expect nobody than us two looking at it much and it would really be good to have more widely spread testing. I'd vote for turning this on by default as soon as possible in alpha phase instead of surprising users with changing UI this way in beta.

> >+    <label value="&find.label;" accesskey="&find.accesskey;" 
> >+           control="search-box"/>
> >+    <textbox id="search-box" flex="1" type="search"
> >+             oncommand="searchHistory(this.value);"/>
> Should this use emptytext instead?

Hmm, and have no way any more to get there with a keyboard shortcut?

> >+    <popup id="placesColumnsContext"
> >+           onpopupshowing="ViewMenu.fillWithColumns(event, null, null, 'checkbox', null);"
> >+           oncommand="ViewMenu.showHideColumn(event.target); event.stopPropagation();"/>
> I wonder what the benefit of filling the columns dynamically is.

No idea.

> >+        <menuitem id="orgCut"
> >+                  command="cmd_cut"
> >+                  label="&cutCmd.label;"
> >+                  key="key_cut"
> >+                  accesskey="&cutCmd.accesskey;"
> If you called it menu_cut it would overlay these attributes from
> utilityOverlay.

Hmm, and editMenuOverlay does use the same. I wonder if there's a certain reason why places devs didn't go that way. In any case, I changed all those edit items this way now.

> >+                  selection="separator|link|folder|mixed"/>
> Does this attribute have any use? The other strange ones I saw were collection
> and forcehideselection.

Those are used to determine what items are shown for what selection, see _shouldShowMenuItem on PlacesController (controller.js)

> >diff --git a/suite/common/history/tree.xml b/suite/common/history/tree.xml
> >+      <method name="getBestOptions">
> >+      <property name="showRoot">
> >+      <property name="onOpenFlatContainer">
> >+        <getter><![CDATA[
> >+          return this.getAttribute("onopenflatcontainer");
> >+      <method name="selectPlaceURI">
> >+      <method name="selectNode">
> >+      <method name="getRemovableSelectionRanges">
> >+      <method name="selectItems">
> These seem to be unused or irrelevant.

showRoot is at least used and a bit more work to remove. I start to think that we probably will just do another copy of the places tree when we might be incorporating places bookmarks some time in the future, and if an extension wants to do more with the backend, it probably needs to bring its own implementation as well. Given that, I'll try to get rid of things like that as well. Same for onOpenFlatContainer.

(In reply to comment #45)
> (From update of attachment 348027 [details] [diff] [review])
> >-pref("browser.urlbar.autoFill", false);
> >-pref("browser.urlbar.showPopup", true);
> >-pref("browser.urlbar.showSearch", true);
> >-pref("browser.urlbar.matchOnlyTyped", false);
> Any reason to move these?

Just so that they are grouped with the rest of the prefs that belong to urlbar and history.

> I had another look at the JS files and found some more apparently unused
> symbols:
> 
> aTxnName (and its callers)

I left that in as it 1) would need a careful inspection for callers of PlacesController.remove so nobody calls it with that argument any more, and 2) breaks any extension that tries to use that function. We are probably crippling our places interfaces enough that we won't have easily portable extensions anyways, so maybe I'm thinking over-cautiously here.

> OrganizerCommand_find and browser.xul are mentioned in an #ifdef but we don't
> use either anyway.

The latter is mentioned with cmd_handleBackspace and cmd_handleShiftBackspace which we are defining in navigatorOverlay.xul, so I guess we still need that, just with a corrected comment?

> some functions call PlacesUtils.nodeIsLivemarkContainer which will always be
> false for us

OK, removed those.

> showHideColumn seems to muck around with splitters, which would be wrong
> because it breaks the columnpicker.

Not sure what to do here.

> setSortColumn, getColumnType, sortTypeToColumnType should only need to work on
> history columns.

OK, done.

> modifKey is probably computed wrongly for suite.

Right - but then, we can ask whereToOpenLink() from utilityOverlay, I guess :)

> convertPRTimeToString code is wrong :-(

Hmm, what can I do about that?
Comment 49 Robert Kaiser 2008-11-20 13:12:15 PST
Created attachment 349264 [details] [diff] [review]
switch UI, v3

This new iteration of the patch addresses all comments where I knew what to actually do ;-)
Comment 50 neil@parkwaycc.co.uk 2008-11-21 07:59:49 PST
(In reply to comment #48)
> (In reply to comment #43)
> > The old history achieves this by including sidebarListView.css to add extra
> > rules for sidebars. (You might want to add the underlining rule there too.)
> But do we want the pointer cursor on non-leaf items? Or is what we want an
> update of sidebarListView.css with the complete set of rules from here,
> actually? I'm assuming the latter for now.
Oh, I didn't spot the leaf property, good catch.

> (In reply to comment #44)
> > I have previously noticed a number of issues with the implementation as it's
> > simply a straight port and I don't expect you to rewrite it, but I think it
> > might be an idea to land the XUL as separate files NPOB and use jar.mn to
> > switch based on --enable-places until the code is ready to go live.
> That's a possibility, but as long as it's not in by default, I expect nobody
> than us two looking at it much and it would really be good to have more widely
> spread testing. I'd vote for turning this on by default as soon as possible in
> alpha phase instead of surprising users with changing UI this way in beta.
Ah, well I was hoping to land this and then improve it (at the very least per comment #31 but also see below) before surprising users with it ;-)

> (In reply to comment #45)
> > Any reason to move these?
> Just so that they are grouped with the rest of the prefs that belong to urlbar
> and history.
Ah, makes sense.

> > I had another look at the JS files and found some more apparently unused
> > symbols:
> > aTxnName (and its callers)
> I left that in as it 1) would need a careful inspection for callers of
> PlacesController.remove so nobody calls it with that argument any more, and 2)
> breaks any extension that tries to use that function. We are probably crippling
> our places interfaces enough that we won't have easily portable extensions
> anyways, so maybe I'm thinking over-cautiously here.
Actually, I said callers, but I think we don't actually need to call remove instead we can just call removePagesFromHistory directly.

> > OrganizerCommand_find and browser.xul are mentioned in an #ifdef but we don't
> > use either anyway.
> The latter is mentioned with cmd_handleBackspace and cmd_handleShiftBackspace
> which we are defining in navigatorOverlay.xul, so I guess we still need that,
> just with a corrected comment?
I'm not sure we're actually overlaying that even on the Mac (can you get this tested on a Mac perhaps?)

> > showHideColumn seems to muck around with splitters, which would be wrong
> > because it breaks the columnpicker.
> Not sure what to do here.
Well, I probably want to follow up by rewriting that code too.

> > modifKey is probably computed wrongly for suite.
> Right - but then, we can ask whereToOpenLink() from utilityOverlay, I guess :)
Sounds reasonable, although I haven't read the patch yet.

> > convertPRTimeToString code is wrong :-(
> Hmm, what can I do about that?
More rewriting followup for me?
Comment 51 Stefan [:stefanh] 2008-11-21 08:29:50 PST
(In reply to comment #48)

> > >+    <label value="&find.label;" accesskey="&find.accesskey;" 
> > >+           control="search-box"/>
> > >+    <textbox id="search-box" flex="1" type="search"
> > >+             oncommand="searchHistory(this.value);"/>
> > Should this use emptytext instead?
> 
> Hmm, and have no way any more to get there with a keyboard shortcut?

accel+k should work fine, no? Minefield uses accel+f, I think (cmd+f focuses the search field for me, at least)
Comment 52 Robert Kaiser 2008-11-22 08:16:56 PST
(In reply to comment #50)
> Ah, well I was hoping to land this and then improve it (at the very least per
> comment #31 but also see below) before surprising users with it ;-)

I think the easiest and most clear solution would be to land this on trunk after a2 and improve it with followup patches for a3.

> Actually, I said callers, but I think we don't actually need to call remove
> instead we can just call removePagesFromHistory directly.

I replaces .remove calls with ._removePagesFromHistory calls now in a new iteration of the patch (upcoming in a minute).

> I'm not sure we're actually overlaying that even on the Mac (can you get this
> tested on a Mac perhaps?)

Hmm, this is confusing. We're not overlaying this on the history window, and it actually looks like not even FF is overlaying their version on their places organizer window...

> > > convertPRTimeToString code is wrong :-(
> > Hmm, what can I do about that?
> More rewriting followup for me?

I'm happy with that - as long as I get my work here to a conclusion, the shoes I put on with this code are too large for me ;-)

I also switched to emptytext in the new iteration of the patch, adding a accel+F shortcut for the history window, and removing all accesskeys on the sidebar (the "w" of "View" was clashing with the "Window" menu, and we apparently avoid accesskeys in all other sidebar panels).
Comment 53 Robert Kaiser 2008-11-22 08:21:40 PST
Created attachment 349583 [details] [diff] [review]
switch UI, v3.1

This iteration only has minor updates from the last one, mainly what I mentioned in the last update, additionally merging the two .dtd files (something I've wanted to do for some time now but other changes were more important).
Comment 54 Stefan [:stefanh] 2008-11-22 14:22:51 PST
I've tested attachment #349583 [details] [diff] [review] and I've noticed a couple of assertions:

###!!! ASSERTION: Replacing a node with one back in time or some nonmatching node: 'aNode->mAccessCount >= mChildren[aIndex]->mAccessCount', file /Users/Stefan/comm/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp, line 1499

###!!! ASSERTION: All URI nodes being updated must have parents: 'Not Reached', file /Users/Stefan/comm/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp, line 1659
Comment 55 Stefan [:stefanh] 2008-11-22 14:26:55 PST
Created attachment 349606 [details]
Screenshot of assertion failed dialog

I also got hit by this "assertion-failed" dialog.
Comment 56 Stefan [:stefanh] 2008-11-25 08:43:05 PST
I haven't had any time to investigate more about those assertions. But jftr, when I tested this, I was able to reproduce one of them (the first 2, don't remember which one) when I did the following:

1) Start with no global history
2) Visit a couple of sites
3) Open the history window
4) Delete all history
5) Visit one of the sites that was in the history

Unfortunately, I'm not sure the above mentioned steps are the cause of the assertion :/
Comment 57 Tony Mechelynck [:tonymec] 2008-12-02 15:05:41 PST
I'm setting this to block bug 63292 but I'm not sure it's the right thing to do, or even that the block shouldn't be in the opposite direction. Feel free to check and correct.
Comment 58 neil@parkwaycc.co.uk 2008-12-05 09:10:13 PST
Comment on attachment 349583 [details] [diff] [review]
switch UI, v3.1

Items which I'd like to see fixed have no brackets.
{Items which it would be nice if you could fix have these brackets}
[These are just comments, and not items to fix]

>+  isCommandEnabled: function PC_isCommandEnabled(aCommand) {

>+  supportsCommand: function PC_supportsCommand(aCommand) {
Please move supportsCommand before isCommandEnabled

>+    const CMD_PREFIX = "placesCmd_";
>+    return (aCommand.substr(0, CMD_PREFIX.length) == CMD_PREFIX);
Please change this to a /regexp/.test

>+   *    "bookmark"          node is a bookamrk
>+   *    "livemarkChild"     node is a child of a livemark
>+   *    "tagChild"          node is a child of a tag
{Well, these won't be true ;-)}

>+          if (PlacesUtils.nodeIsBookmark(node)) {
{I guess this won't be true either :-)}

>+      // annotations
{Does this applies to history? If not, more to remove 8-)}

>+   *  5) The "hideifnoinsetionpoint" attribute may be set on a menu-item to
{This is a typo (missing r in insertion) but it's unused anyway...}

>+    var ip = this._view.insertionPoint;
{...as it says later on, there is no insertion point for history queries}

>+    // Set Open Folder/Links In Tabs items enabled state if they're visible
Not sure that we need this, just leave it always enabled?

>+    const kWarnOnOpenPref = "browser.tabs.warnOnOpen";
We don't seem to have a default for this in browser-prefs.js?

>+  _confirmOpenTabs: function(numTabsToOpen) {
{Unused}

>+      if (PlacesUtils.nodeIsHost(node))
>+        bhist.removePagesFromHost(node.title, true);
[I wonder what happens with a date_site query]

>+        var URIslice = URIs.slice(i, Math.max(i + REMOVE_PAGES_CHUNKLEN, URIs.length));
Whoa, this should be Math.min, but in fact slice doesn't care if we pass i + REMOVE_PAGES_CHUNKLEN on its own ;-)
[We should actually improve on this coding in a followup patch]

>+      var placeString = mozURLString = htmlString = unicodeString = "";
This is actually incorrect but we don't even warn about it any more :-(
{Should be one var CSv line, and one chained assignment line}

>+        function generateChunk(type, overrideURI) {
{Not sure what the point of a separate function is here}

>+    catch(e) {
>+      alert(e);
>+    }
{This doesn't look right...}

>+    finally {
>+      if (oldViewer)
>+        result.viewer = oldViewer;
>+    }
[I wish I knew why we twiddle the viewer]

>+  updatePlacesCommand("placesCmd_open");
{It would be nice if goUpdateCommand worked here}

>+    var splitter = column.nextSibling;
Don't show or hide splitters.

>+      if (curChildType == Components.interfaces.nsINavHistoryResultNode.RESULT_TYPE_SEPARATOR) {
{No separators in history}

>+    // the third state to reset the sorting to the natural bookmark order. When
>+    // you are looking at history, that third state has no meaning so we try
>+    // to disallow it.
{This is referring to the now-useless "allowTriState" variable}

>+      <property name="showRoot">
>+        <getter><![CDATA[
>+          return this.getAttribute("showRoot") == "true";
{Also never true...}

>+      <property name="flatList">
>+        <getter><![CDATA[
>+          return this.getAttribute("flatList") == "true";
[I want to eliminate this]

>+      <property name="onOpenFlatContainer">
>+        <getter><![CDATA[
>+          return this.getAttribute("onopenflatcontainer");
{Unused}

>+      <method name="getDragableSelection">
[Does nobody know how to spell any more?]

Also, at least 5 lines had trailing spaces (for some reason git-apply didn't give me its usual summary).
Comment 59 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-12-05 20:26:17 PST
(In reply to comment #58)
> >+        var URIslice = URIs.slice(i, Math.max(i + REMOVE_PAGES_CHUNKLEN, URIs.length));
> Whoa, this should be Math.min, but in fact slice doesn't care if we pass i +
> REMOVE_PAGES_CHUNKLEN on its own ;-)

Eek! I filed bug 468209 on fixing this in the browser/ code this was copied from.
Comment 60 Robert Kaiser 2008-12-06 08:20:48 PST
(In reply to comment #58)
> (From update of attachment 349583 [details] [diff] [review])
> >+   *    "bookmark"          node is a bookamrk
> >+   *    "livemarkChild"     node is a child of a livemark
> >+   *    "tagChild"          node is a child of a tag
> {Well, these won't be true ;-)}

Removed those, along with a part of the code below, and spotted that "day" wasn't listed there, which I also corrected. I'm actually not even sure if "folder" needs to be kept here.

> >+      // annotations
> {Does this applies to history? If not, more to remove 8-)}

Not sure about those...

> >+   *  5) The "hideifnoinsetionpoint" attribute may be set on a menu-item to
> {This is a typo (missing r in insertion) but it's unused anyway...}

Filed bug 468233 for Firefox about this, they used the misspelling all over.

> >+    // Set Open Folder/Links In Tabs items enabled state if they're visible
> Not sure that we need this, just leave it always enabled?

It looks to me as they want to ensure it gets enabled, but I don't see where it would get disabled in the first place. I didn't notice any difference when I commented out the .disabled assignment, so I removed the whole block in the new patch.

> >+    const kWarnOnOpenPref = "browser.tabs.warnOnOpen";
> We don't seem to have a default for this in browser-prefs.js?

Right, added that and browser.tabs.maxOpenBeforeWarn - we really should use the warning when opening more than 15 tabs. Not sure if we have the "open container in tabs" function anywhere yet, but I think we probably should add it on day/host group containers and then we need those prefs.

> >+        var URIslice = URIs.slice(i, Math.max(i + REMOVE_PAGES_CHUNKLEN, URIs.length));
> Whoa, this should be Math.min, but in fact slice doesn't care if we pass i +
> REMOVE_PAGES_CHUNKLEN on its own ;-)
> [We should actually improve on this coding in a followup patch]

I incorporated Marco's fix from bug 468209 for this.

> >+        function generateChunk(type, overrideURI) {
> {Not sure what the point of a separate function is here}

Me neither, we probably should ask places people if there's a special reason and if not remove it in both implementations

> >+    catch(e) {
> >+      alert(e);
> >+    }
> {This doesn't look right...}

Right, changed to a dump() for now if that's OK, I guess this is something we don't expect to actually hit.

> >+    finally {
> >+      if (oldViewer)
> >+        result.viewer = oldViewer;
> >+    }
> [I wish I knew why we twiddle the viewer]

I wish I'd understand more of the code, actually ;-)

> >+  updatePlacesCommand("placesCmd_open");
> {It would be nice if goUpdateCommand worked here}

I suspect it does, it's exactly the same code, just that no browser/ code uses it at all, for whatever reason, even though it's in toolkit. I just tried it in the new patch.

> >+      <property name="showRoot">
> >+        <getter><![CDATA[
> >+          return this.getAttribute("showRoot") == "true";
> {Also never true...}

Eliminated showRoot completely.

> >+      <property name="flatList">
> >+        <getter><![CDATA[
> >+          return this.getAttribute("flatList") == "true";
> [I want to eliminate this]

What is it for? It look like we're using it in the history window and I really want to be able to have a flat history view available, sorted and filtered by whatever means. I personally always have disliked the date/site-grouped views, I'd like not to be forced to them.

> >+      <method name="getDragableSelection">
> [Does nobody know how to spell any more?]

ugh, right. I'd like that to be addressed globally though and have http://mxr.mozilla.org/mozilla-central/search?string=Dragable fixed as well rather than do this only on our side.

> Also, at least 5 lines had trailing spaces (for some reason git-apply didn't
> give me its usual summary).

I searched for all trailing whitespace in the patch and eliminated all cases.
Comment 61 Robert Kaiser 2008-12-06 08:27:57 PST
Created attachment 351691 [details] [diff] [review]
switch UI, v4

I hope this is the last version for this patch, forwarding the sr flag but re-requesting review to get an OK on the changes in this version.
Comment 62 neil@parkwaycc.co.uk 2008-12-06 11:47:03 PST
(In reply to comment #60)
>(In reply to comment #58)
>>(From update of attachment 349583 [details] [diff] [review])
>>>+    const kWarnOnOpenPref = "browser.tabs.warnOnOpen";
>>We don't seem to have a default for this in browser-prefs.js?
>Right, added that and browser.tabs.maxOpenBeforeWarn - we really should use the
>warning when opening more than 15 tabs. Not sure if we have the "open container
>in tabs" function anywhere yet, but I think we probably should add it on
>day/host group containers and then we need those prefs.
We might also want to add these prefs to openUILinkArrayIn() at some point.
Comment 63 Robert Kaiser 2008-12-07 05:27:41 PST
> We might also want to add these prefs to openUILinkArrayIn() at some point.

Filed as bug 468325. We also should file followups for the features we want to implement on top of this functionality, like I just did with bug 468326 for UI prefs.

Pushed the patch as http://hg.mozilla.org/comm-central/rev/28012b9430e5 - this bug should now be FIXED.

Please file regressions and enhancements as new bugs depending on this one.
Comment 64 Tony Mechelynck [:tonymec] 2008-12-07 12:19:40 PST
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20081207 SeaMonkey/2.0a3pre - Build ID: 20081207072048

Verified on Linux (AFAICT):
- The history manager and its menus are different than what they used to be in today's nightly and before.
- The default for browser.history.expire.days is now 180 rather than 9.
- My profile now includes a places.sqlite dated a few minutes ago, and a zero-length places.sqlite-journal which is even more recent.



Partly OT: Beware that going back and forth between versions of SeaMonkey built before and after landing this fix might, in some cirscumstances, result in history dataloss -- see among others bug 468180 comment #5 (the rest of that bug's comments are edifying; they're about the move to signons.sqlite which is more or less parallel to this case).
Comment 65 Sven Grull 2008-12-08 02:52:34 PST
Using a recent trunk build with places support it seems that the old history.dat is not migrated. Is there no code for this migration in general or is there something missing that triggers the migration?
BTW: Is there a possibility to do the migration by hand?
Comment 66 Philip Chee 2008-12-08 03:07:22 PST
In the autocomplete dropdown, the Search Google for "foo" row doesn't have the usual background, margins and the "G" icon. Possibly some missing CSS in classic?
Comment 67 Tony Mechelynck [:tonymec] 2008-12-08 03:48:57 PST
(In reply to comment #65)
> Using a recent trunk build with places support it seems that the old
> history.dat is not migrated. Is there no code for this migration in general or
> is there something missing that triggers the migration?
> BTW: Is there a possibility to do the migration by hand?

IIUC the old history is oly migrated the first time; so (IIUC) the following ought to migrate the history again (but at the cost of losing everything in the "new" history):
  1. Close SeaMonkey completely (and wait for its process to disappear from the Windows task manager, the KDE System Guard, the Gnome System Monitor, etc.; in Windows make sure you are watching the "processes" list, not the "applications" list)
  2. Remove places.history (and possibly places.history-journal) from your profile
  3. Restart SeaMonkey.

At step 2 you may want to move rather than remove (just in case I misunderstood).
Comment 68 Sven Grull 2008-12-08 03:57:32 PST
(In reply to comment #67)
> At step 2 you may want to move rather than remove (just in case I
> misunderstood).

Thanks for the hint. In the meantime I have read in mozilla.dev.apps.seamonkey that I am not the only one that had this problem.
Comment 69 Robert Kaiser 2008-12-08 04:09:17 PST
(In reply to comment #67)
>   2. Remove places.history (and possibly places.history-journal) from your
> profile

s/history/sqlite/ ;-)

And yes, if you already have that file, your history.dat will not be migrated. If you don't have it, it should get migrated.


(In reply to comment #66)
> In the autocomplete dropdown, the Search Google for "foo" row doesn't have the
> usual background, margins and the "G" icon. Possibly some missing CSS in
> classic?

I think that's unrelated, actually, and has been there with or without places. The patch here didn't change anything related to the urlbar itself, it only exchanged the autocomplete provider that it requests history results from.
Comment 70 Tony Mechelynck [:tonymec] 2008-12-08 04:56:11 PST
(In reply to comment #69)
[...]
> s/history/sqlite/ ;-)
[...]
Oops, you're right. Freudian slip? :-(
Comment 71 Robert Kaiser 2009-09-18 06:18:00 PDT
Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query
Comment 72 Robert Kaiser 2010-04-13 10:51:03 PDT
*** Bug 109758 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.