[patch] history manager rewrite

RESOLVED FIXED in Camino0.8

Status

Camino Graveyard
History
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: S Woodside, Assigned: Mike Pinkerton (not reading bugmail))

Tracking

unspecified
Camino0.8
PowerPC
Mac OS X

Details

Attachments

(2 attachments, 9 obsolete attachments)

45.04 KB, patch
Details | Diff | Splinter Review
4.96 KB, application/x-gzip
Josh Aas
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details
(Reporter)

Description

14 years ago
I'm doing a major rewrite of the history manager. The UI will be a flat listing,
by day. (rather than the current listing by day then by site).
(Reporter)

Comment 1

14 years ago
Created attachment 136908 [details] [diff] [review]
history patch + debugging

This patches RDFOutlineViewDataSource and HistoryDataSource. You'll also need
some new files. There's still a bunch of debugging printfs (commented out
though) in here. But it implements the described functionality in the initial
summary.

I'd still like to get better performance.
(Reporter)

Comment 2

14 years ago
Created attachment 136909 [details]
new files

files needed to go with "history patch + debugging"... you need to add these to
the Chimera target for it to work.

Comment 3

14 years ago
Simon, perhaps you could add the possibility to do a search on history.
Something like the search feature in Mail would be nice, then the user would be
able to select each individual root folder or all (the folders in the left most
coulmn). That way we could easily let the user determine what they want to search.
(Reporter)

Updated

14 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 4

14 years ago
Created attachment 137154 [details] [diff] [review]
new patch (nodebug)

This patch has the new history implementation. You'll also need the new files
HistoryItem.{h,mm} and RDFItem.{h,mm} that I'll upload in a moment.

Following the pattern set by Dave Haas in the bookmarks manager rewrite, I have
implemented RDFItem and HistoryItem classes. HistoryItem is a subclass of
RDFItem. RDFItem provides general support for any RDF node being displayed in
an outline view. HistoryItem provides history-specific services (that where i
implemented the flattened history view).

I have pulled out a lot of the functionality of RDFDataSource and
HistoryDataSource that belonged better in RDFItem and HistoryItem.
RDFDataSource implements a general-purpose data source for outline views to
allow them to display Gecko RDF nodes. HistoryDataSource subclasses it and adds
the connection to nsGlobalHistory as the gecko RDF data source and works with
HistoryItems.

I added the ability to flatten the history list. It displays a flat list per
day. It's sorted with the most recent items first.
(Reporter)

Updated

14 years ago
Attachment #136908 - Attachment is obsolete: true
Attachment #136909 - Attachment is obsolete: true
(Reporter)

Comment 5

14 years ago
Created attachment 137155 [details]
 new files #2

To install: extract the files. HistoryItem.{h,mm} go in src/history and
RDFItem.{h,mm} go in src/extensions. Add all four files to the project to the
Chimera target. apply the patch and rebuild and it should work with the new
flat history manager :-)
(Reporter)

Updated

14 years ago
Attachment #137154 - Flags: review?
(Assignee)

Comment 6

14 years ago
so here's a question for thought...why not try to write this on top of the stub
history impl that conrad was writing? flesh it out to include autocomplete then
put this work on top of that? won't that fix the performance that you're worried
about with the current crappy history impl?
(Reporter)

Comment 7

14 years ago
I never looked into that. what's the url / file location?
(Assignee)

Comment 8

14 years ago
mozilla/embedding/list/nsEmbedGlobalHistory.cpp (i guess you missed it on irc
when i pasted it to you last week).
(Reporter)

Comment 9

14 years ago
Based on reading browsing the bonsai logs for that file, and looking at bug
70714 I don't think nsEmbedGlobalHistory does what we need. It doesn't store any
dates for example. It's mainly to provide visited-link functionality for gecko
embedders (like K-Meleon that runs in libraries).

Probably the best for mozilla would be to fix the performance in
nsGlobalHistory.cpp. But if you think the performance in this patch is
*sufficient* then I wouldn't wait for that code to be fixed...
(Reporter)

Updated

14 years ago
Attachment #137154 - Flags: review?(qa-mozilla)
Attachment #137154 - Flags: review?(haasd)
Attachment #137154 - Flags: review?

Comment 10

14 years ago
After fixing the compile error (see below) this patch seems to work fine on 10.3.2.

Things to fix in next version of patch:

Necessary:
- compile problem on line 43 of RDFItem.mm ('#import "GecoaRDFUtils.h"' needs to
be removed)

Picky:
- at the bottom of HistoryItem.mm, make sure to put a newline between methods.
- RDFItem.h and HistoryItem.h have extra newlines on the bottom
- moz code formatting (as I saw on the site) puts the open block bracket on the
same line as the keyword "if" and same for "else if" and "else". I'm not too
familiar with what people normally do though...
(Assignee)

Updated

14 years ago
Target Milestone: --- → Camino0.8
(Reporter)

Comment 11

14 years ago
Created attachment 138003 [details]
new files #3

new files addressing josha's comments
Attachment #137155 - Attachment is obsolete: true
(Reporter)

Updated

14 years ago
Attachment #138003 - Flags: review?(josha)
(Reporter)

Updated

14 years ago
Attachment #137154 - Flags: review?(qa-mozilla)
Attachment #137154 - Flags: review?(josha)
Attachment #137154 - Flags: review?(haasd)
Attachment #137154 - Flags: review?

Comment 12

14 years ago
Just got a chance to test the patch with code from 12/27/03 and it screws up the
date display. It displays "0" where it should say "Today" and then it shows me
an integer for the other days in my history - like "3" where it should say "3
days ago". I applied the patch and added the 4 files you posted. I think that is
what I'm supposed to do, right?
(Reporter)

Comment 13

14 years ago
I have no control over what it displays there. Im just displaying the vaslue
that is passed to me by nsGlobalHistory. Sometimes I get numbers and sometimes I
get Today, Yesterday, etc. It's not a regression, the old implementation also
has this problem.

Any fix for that should be to nsGlobalHistory in Mozilla I think.

Comment 14

14 years ago
Problem seems to have fixed itself. Looks like you're right about this being
nsGlobalHistory's fault.

r=josha@mac.com
(Reporter)

Comment 15

14 years ago
Comment on attachment 138003 [details]
new files #3

adding r+ for josha.

2nd r?
Attachment #138003 - Flags: review?(josha)
Attachment #138003 - Flags: review?
Attachment #138003 - Flags: review+
(Assignee)

Comment 16

14 years ago
if i delete a history item by hitting backspace, it relaods the entire pane and
closes the open folders. doesn't seem too friendly :(

feels fast though :)
(Assignee)

Comment 17

14 years ago
-- (void) ensureDataSourceLoaded;
+- (void) lazyLoad;
 - (void) cleanupDataSource;

Not. to better fit with the verb-centric naming already in this file, how about
|-loadLazily| instead? |lazyLoad| just doesn't fit with the current naming scheme

+    [[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(shutdown:)
+        name:XPCOMShutDownNotificationName object:nil];

if the outlineViewDataSource adds itself as an observer, shouldn't it remove
itself in dealloc or cleanup?

+- (void)setRootRDFItem:(RDFItem*)item;
+{
+  if( mRootRDFItem )
+    [mRootRDFItem release];
+  mRootRDFItem = [item retain];
+}

what if |item| is |mRootRDFItem|. calling release will cause it to be destroyed.
better to autorelease or use a temp. Also, the |if| is not needed. The runtime
does it for you. This is done in a couple of places.

+  if( [aItem isKindOfClass:[RDFItem class]] ) {
+    RDFItem * rdfItem = aItem;
+    return [rdfItem isExpandable];

rather than checking |isKindOfClass:|, how about just |respondsToSelector:|?
just wondering. What you have is probably faster.

+  if( count > 1 ) {
+    // not sure what to do for multiple drag. just cancel for now.
+    return NO;
+  }

should we file a follow-on bug for this? seems like a shortcoming to only be
able to drag a single item. users won't understand why.

more to come from looking at individual files....

(Assignee)

Comment 18

14 years ago
heh. i meant "Nit" not "Not" :D
(Assignee)

Comment 19

14 years ago
in RDFItem.mm:

  if( mChildNodes)
    [mChildNodes release];

no need to do the if, the runtime will do it for you.

  RDFItem * newChild = [RDFItem alloc];
  return [newChild initWithRDFResource:aRDFResource RDFDataSource:aRDFDataSource];

why break this into two lines?

  if( mChildNodes )
    return [mChildNodes count];

  // Since gecko RDF enumerator->HasMoreElements
  // is so slow, we may as well build the cache now
  // because the extra time it takes is tiny
  if( !mChildNodes )
    [self buildChildCache];

obviously if you get past the first if, the second will succeed. rework the logic.

in HistoryItem.h:

- (id)newChildWithRDFResource:(nsIRDFResource*)aRDFResource
RDFDataSource:(nsIRDFDataSource*)aRDFDataSource;
- (int)numChildren;

no need to put these in the interface, they're inherited.


in HistoryItem.mm

@interface HistoryItem (Private)

@end

what's this for?

  nsCOMPtr<nsIRDFDataSource> histDataSource = do_QueryInterface(historyService);

i guess this shouldn't ever fail, but should probably check. how about rewriting as:

  nsCOMPtr<nsIBrowserHistory> historyService =
do_GetService("@mozilla.org/browser/global-history;1");
  nsCOMPtr<nsIRDFDataSource> histDataSource = do_QueryInterface(historyService);
  if( !histDataSource )
    return;

just more robust, that's all.

  NSComparisonResult result = [[aItem date] caseInsensitiveCompare:[self date]];
  return result;

nit. why break this into multiple statements?

looking good.

Comment 20

14 years ago
Created attachment 138527 [details] [diff] [review]
updated patch

This patch is updated for the current trunk code and it fixes Pinkerton's
comments.

Updated

14 years ago
Attachment #137154 - Attachment is obsolete: true

Comment 21

14 years ago
Created attachment 138528 [details]
new files #4

new files taking into account Pinkerton's comments
Attachment #138003 - Attachment is obsolete: true

Updated

14 years ago
Attachment #138527 - Flags: superreview?
Attachment #138527 - Flags: review?

Updated

14 years ago
Attachment #138528 - Flags: superreview?
Attachment #138528 - Flags: review?
patch applies and compiles.
patch works well - I kind like the new presentation.

I need to read the code before I can r+. Will try tomorow.
(Reporter)

Comment 23

14 years ago
ummm... I just fixed pinkerton's comment #16 in my own local copy and now I see
josh aas posted a new patch that apparently already does that?
(Reporter)

Updated

14 years ago
Blocks: 230340
(Reporter)

Comment 24

14 years ago
re: comment #17

> Not. to better fit with the verb-centric naming already in this file, how about
> |-loadLazily| instead? |lazyLoad| just doesn't fit with the current naming scheme

"load" is a verb...

> if the outlineViewDataSource adds itself as an observer, shouldn't it remove
> itself in dealloc or cleanup?

err.. yeah. To be honest I don't think that the observer is even doing anything
right now. Once the history view is loaded in a window it's static and message
from gecko are never acted upon. I might try to fix that too.

> what if |item| is |mRootRDFItem|. calling release will cause it to be destroyed.

oops..

> Also, the |if| is not needed. The runtime
> does it for you. This is done in a couple of places.

I  thought that sneding a message a nil object would generate a runtime error.

> rather than checking |isKindOfClass:|, how about just |respondsToSelector:|?
> just wondering. What you have is probably faster.

maybe... seems cleaner this way.

> should we file a follow-on bug for this? seems like a shortcoming to only be
> able to drag a single item. users won't understand why.

done (bug 230340)
No longer blocks: 230340
(Reporter)

Comment 25

14 years ago
re comment #19

  RDFItem * newChild = [RDFItem alloc];
  return [newChild initWithRDFResource:aRDFResource RDFDataSource:aRDFDataSource];

> why break this into two lines?

my new patch changes this to make more sense. in the new one I have 
RDFItem* newChild = [self newChild];

where the subclass for HistoryItem overrides |newChild| to return a HistoryItem
instead of an RDFItem.

  NSComparisonResult result = [[aItem date] caseInsensitiveCompare:[self date]];
  return result;

> nit. why break this into multiple statements?

I think it was for debugging purposes.

I'll fix the other stuff.
(Reporter)

Comment 26

14 years ago
A currently visible history view will not reflect updates in real time as
surfing goes on in other windows. Each history view in each tab/window is
separately instantiated and once they are created, they are static at that
point. Although surfing in a window will apparently destroy the history view and
so if you view history, surf, then view again it will be updated.

I don't think it's critical but should probably be fixed at some point in the
future.

Comment 27

14 years ago
Re Comment #26

It should be fixed when we start to allow users to open the BM manager in a new
window or tab (Bug 215235). Then it will be essential.
(Reporter)

Comment 28

14 years ago
Created attachment 138572 [details] [diff] [review]
patch 5

.
(Reporter)

Comment 29

14 years ago
Created attachment 138573 [details]
updated files #5

This patch and files fix the problem with deleting history items. You can now
delete one or more (multiple selection) history items, including folders for
days, and the outline view updates automatically without collapsing the
folders. Also I fixed a bunch of code -related issues noted by pinkerton. (I
changed lazyLoad to loadLazily too).

Josh says to ignore his patch.
Attachment #138527 - Attachment is obsolete: true
Attachment #138528 - Attachment is obsolete: true
(Reporter)

Updated

14 years ago
Attachment #137154 - Flags: review?(josha)
Attachment #137154 - Flags: review?
(Reporter)

Updated

14 years ago
Attachment #138003 - Flags: review?
(Reporter)

Updated

14 years ago
Attachment #138527 - Flags: superreview?
Attachment #138527 - Flags: review?
(Reporter)

Updated

14 years ago
Attachment #138528 - Flags: superreview?
Attachment #138528 - Flags: review?
(Reporter)

Comment 30

14 years ago
Comment on attachment 138573 [details]
updated files #5

r? for files and patch 5
Attachment #138573 - Flags: review?
(Assignee)

Comment 31

14 years ago
>> Not. to better fit with the verb-centric naming already in this file, how about
>> |-loadLazily| instead? |lazyLoad| just doesn't fit with the current naming scheme
> 
> "load" is a verb...

yes, but all the other methods start with a verb. |lazyLoad| does not.

>> Also, the |if| is not needed. The runtime
>> does it for you. This is done in a couple of places.
> 
> I  thought that sneding a message a nil object would generate a runtime error.

No, it is not a runtime error. It fails silently and the result of the
expression is false or nil.

Comment 32

14 years ago
My biggest problem now is that since you can select and delete multiple items,
it follows that you should be able to select multiple items and then right click
and do "open in tabs" and have all of them open. The multiple selection actions
don't seem consistent. Select three items and then right-click. If you hit
delete, it acts on all selected items. If you hit one of the open options, it
acts on only one. I don't think it should behave that way.

As for the code itself, mostly nits...

+- (RDFItem *)rootRDFItem;
+{
+  return mRootRDFItem;
+}
+
+- (void)setRootRDFItem:(RDFItem*)item;
+{
+  if( mRootRDFItem )
+    [mRootRDFItem autorelease];
+  mRootRDFItem = [item retain];
+}
...
+  if( mRootHistoryItem )
+    [mRootHistoryItem autorelease];

The checks before autoreleaseing should be unnecessary - the runtime should do
that for you. Also, don't put ';' at the end of implementation method
signatures. This is done in a few places.

NSString* url = [item url];

Near the bottom of the patch this line doesn't appear to be necessary. I think
you can just use [item url] in the one place you use it. I understand that other
places you do this are for the sake of casting, but this doesn't seem to be.

RDFItem.h
- how is Ben Goodger the original author of this new file? Didn't you write it?
- This comment seems to have a spelling mistake:
  // RDFItems make up rows of an RDF outline view

RDFItem.mm
- there is an extra newline above this method: - (void)invalidateCache;
- the mChildNodes check in dealloc isn't necessary
- a bunch of methods including newChild have a semicolon ending their signature
- why doesn't newChild just return an RDFItem instead of id so that you don't
need to do a variable declaration before you initialize?

HistoryItem.h
- (int)numChildren is inherited from RDFItem. No need to put it in HistoryItem's
public interface.

HistoryItem.mm
- a couple times there are extra newlines between methods
- same semicolon thing
- "  if( mGrandChildNodes ) [mGrandChildNodes autorelease];"
  That looks weird and it is unnecessary anyway.
- maybe add an integrity check after:
    nsCOMPtr<nsIRDFDataSource> histDataSource = do_QueryInterface(historyService);
- I don't know how I feel about this style:
  - (bool)nativeIsExpandable;
  {   return [super isExpandable]; }
(Reporter)

Comment 33

14 years ago
regarding multiple items, the patch for bug 225915 could be used to make that
work in history as well (maybe it will just work automatically). I don't want to
address that in this patch though, better to do it in a new patch after I think.
(Reporter)

Comment 34

14 years ago
Created attachment 138733 [details] [diff] [review]
take 6
Attachment #138572 - Attachment is obsolete: true
(Reporter)

Comment 35

14 years ago
Created attachment 138734 [details]
files for #6

> The checks before autoreleaseing should be unnecessary - the runtime should
do
> that for you.

fixed.

> Also, don't put ';' at the end of implementation method signatures.

I did that on purpose. It makes it easier to copy the declarations over to the
interface and it's legal ObjC syntax.

> how is Ben Goodger the original author of this new file? Didn't you write it?


I don't know what the protocol is here. Pinkerton?
Some of the code in RDFItem and to a lesser extent, in HistoryItem, was
originally written by Ben. I factored it out and made heavy changes.
I'll go with what pink thinks.

>  This comment seems to have a spelling mistake:
>   // RDFItems make up rows of an RDF outline view

what's the mistake?


> why doesn't newChild just return an RDFItem instead of id so that you don't
> need to do a variable declaration before you initialize?


Well.... it's the type that's alloc'd that really matters, but it doesn't
really matter so I changed it.

> - a couple times there are extra newlines between methods

is that bad?

the rest is fixed.
Attachment #138573 - Attachment is obsolete: true
(Reporter)

Updated

14 years ago
Attachment #138573 - Flags: review?
(Reporter)

Updated

14 years ago
Attachment #138734 - Flags: review?

Comment 36

14 years ago
>  This comment seems to have a spelling mistake:
>   // RDFItems make up rows of an RDF outline view

sorry - I meant to write it out but I forgot. Anyway, I though that the first
word should not be plural. Now I've changed my mind.

>> - a couple times there are extra newlines between methods
> is that bad?

Its a nit like I said most of my comments were. Its not bad but it is
unnecessary. Nits are things that could be fixed but don't really need to be if
you don't want to.

>> Also, don't put ';' at the end of implementation method signatures.
> I did that on purpose. It makes it easier to copy the declarations over to the
interface and it's legal ObjC syntax.

In other patches we usually remove them because thats the style that is normal.
 For me, its visually annoying, though I understand what you're saying. I don't
expect the semicolon to be there, so when I see it my reaction is to stop and
make sure your code is what I think it is. Its inconsistent style-wise with most
of the code in Camino. For ease of reading, I think style should be consistent
when possible and reasonable.

Updated

14 years ago
Attachment #138734 - Flags: review? → review+

Updated

14 years ago
Attachment #138003 - Flags: review+
(Reporter)

Updated

14 years ago
Attachment #138734 - Flags: review?
(Assignee)

Comment 37

14 years ago
Comment on attachment 138734 [details]
files for #6

sr=pink
Attachment #138734 - Flags: review? → superreview+
(Assignee)

Comment 38

14 years ago
landed
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.