71756 - Implement SH public interfaces as described in the APiReview notes

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect, P2)

Description

[Radha on family leave (not reading bugmail)]

Re-factor nsISHistory and nsISHEntry as per discussions posted at
http://www.mozilla.org/projects/embedding/apiReviewNotes.html and n.p.m.embedding

Comment 1

[Radha on family leave (not reading bugmail)]

Attachment: Initial patch

Comment 2

[Radha on family leave (not reading bugmail)]

I have a initial set of diffs. The diffs don't build on linux.  The diffs have 
the following set of changes:

1) A new nsIHistoryentry interface
2) nsISHistory refactored in to nsISHistory and nsISHistoryInternal and the 
methods divided among the two.
3) Changes to nsSHistory.cpp, nsSHEntry.cpp, nsWebShell.cpp, nsDocShell.cpp, 
nsHistory.cpp due to the change in intercaces. 
4) Removed the unused child enumerator in nsISHContainer.idl  and nsSHEntry.cpp
5) Added a enumerator to go thro' the complete historylist to nsSHistory.cpp as 
part of the specifications
6) Removed some unsed methods in nsSHistory (GetNextEntry(), GetPrevEntry())
7) Removed a unused function UpdateCurrentSessionHistory() in nsDocShell.cpp

Back/forward/Go functionalities work.  Things to be done:
1) The flag that indicates frameset navigation in nsIHistoryEntry is 
yet to be implemented
2) Write some test cases or modify nsSHistory::printHistory() to use the newly 
added enumerator. May be this can be an exercise for our QA team. 
3) I noticed few tab problems. Take care of that
4) Get it to build and run in linux.
5) Other things that the reviewers may notice.

Please note that these are just initial set of changes. The basic things work.

Comment 3

[Radha on family leave (not reading bugmail)]

Attachment: Another patch

Comment 4

[Radha on family leave (not reading bugmail)]

The latest patch takes care of  most of the TBDs in the previous patch.

1) Builds on linux/solaris
2) Subframe navigation flag working
3) Tab problems fixed.
4) Leaving the QA of the SHEnumerator to David Epstein. Discussed the same with
him.

Can I get a review/super review?  It is a long patch.Please bear with it and
try to provide a review. Thanks,

Comment 5

[Conrad Carlen (not reading bugmail)]

I'm not an expert on session history so forgive my superficial comments. Here's
one: The session history enumerator descends from nsIEnumerator. See this
comment: http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsIEnumerator.idl#36.
Shouldn't it use nsISimpleEnumerator?

Comment 6

[Radha on family leave (not reading bugmail)]

Conrad, you are right, I will change it over to nsISimpleEnumerator. Most of the 
changes in the patch are mundane ones, ie., changing nsISHistory to 
nsISHistoryInternal or changing nsISHEntry to nsIHistoryEntry.

Comment 7

[Radha on family leave (not reading bugmail)]

Attachment: Uses nsISimpleEnumerator instead of nsIEnumerator

Comment 8

[Radha on family leave (not reading bugmail)]

Comments from rpotts (in a email) with some suggestions. Shall take care of the 
suggestions:

(r/sr=rpotts) -- which ever you need :-)

Comment 9

[Judson Valeski]

/cvsroot/mozilla/xpfe/components/shistory/public/MANIFEST_IDL,v -
* "\ No newline at end of file" - make sure there are newlines at the end of this.

nsIHistoryEntry -
* "//readonly attribute boolean isSubFrame;" - The review notes indicate that we
need this attribute. Why is it commented out?
* Can you add the "@status FROZEN" and "@version 1.0" tokens to the interface
comment (after we resolve the isSubFrame() attribute)? We're now using these to
indicate interface status. "@status UNDER_REVIEW" is used when an interface is
in flux, but we want to make it public.

nsISHEntry.idl -
There are methods which are commented out here. We should just remove them if
they're not used.

Comment 10

[Radha on family leave (not reading bugmail)]

fixed last night

Comment 11

[Michael Dunn]

Correction: Changing QA contact for the Embed API bugs to David Epstein.

Comment 12

[David Epstein]

verified:
1. nsIHistoryEntry.idl exists.
2. nsISHistoryInternal iface spun off from nsIHistoryEntry.
3. no child enumerator in nsISHContainer.idl or nsSHEntry.cpp (there are child 
counters).
4. history enumerators in nsSHistory.cpp. nsSHEnumerator implementations
5. GetNextEntry(), GetPrevEntry() removed from nsSHistory.cpp. 
6. UpdateCurrentSessionHistory() removed from nsDocShell.cpp
7. flag in nsIHistoryEntry.idl for subframe nav.
8. printHistory() method in nsSHistory.cpp. Didn't see any tests with enumerator 
but QA will take care of this.