406974 - FUEL: no easy history listeners

Status: RESOLVED WONTFIX

(Firefox :: General, defect)

Description

[Tom Aratyn]

I'd like to add easy history event listeners to fuel however I'd like to solicit some comments before I start writting code.

To fit in with the FUEL api, here's how I imagine it should work:

browserTabInst.events.addEventListener('historyEventName', aListener);

To accomplish this here's my plan:
* browsertab will now have a sessionshistorylistener which listens to the browsertab's browser object for the history events (probably lazy loaded)
* when a history of event listener fires, the listener will get it's owning browsertab's events list and dispatch the appropriate event (Events takes it from there)

My main questions are:
* Is this plan good (or is there something more obvious which I'm missing)?
* Is there a naming scheme for events?
* These events can get more than one attribute and often not a string (see http://lxr.mozilla.org/seamonkey/source/docshell/shistory/public/nsISHistoryListener.idl) but EventItem requires a string in the MDC documentation. What's a good solution here? serialize the objects using JSON? Or can I pass a hash/Object instead of a string (not really cause then I break the IDL http://lxr.mozilla.org/seamonkey/source/browser/fuel/public/fuelIApplication.idl#36).

Here's a list of the new events and my proposed names/ids
HistoryNewEntry
HistoryGoBack
HistoryGoForward
HistoryReload
HistoryGoToIndex
History Purge

Comment 1

[Tom Aratyn]

the event names are based directly on http://lxr.mozilla.org/seamonkey/source/docshell/shistory/public/nsISHistoryListener.idl

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I was originally thinking of something like:

browserTab.history

and

browserTab.history.events (with the conventional FUEL event object)

We could also then have a means to retrieve the history (nsISHistory) itself
for a BrowserTab. Therefore, we would be adding a fuelIBrowserHistory instance
to fuelIBrowserTab. fuelIBrowserHistory should at least have a mean to retrieve
the history and have a fuelIEvents instance for listening for history events.
The events types and names you suggest are fine. We can lazy instaciate the
"history.events" member as well, reducing its affect on performance.

Comment 3

[Tom Aratyn]

That sounds like a good idea to have a history object. 

Initially I was thinking of having the events be on the browsertab to have one place for the events but it is a good idea to give session history access per BrowserTab.

I've made up a basic fuleISessionHistory.idl that I'd like to get reviewed and then I'll start implementing in code.

Comment 4

[Tom Aratyn]

Attachment: fuelISessionHistory.idl

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 292086 [details]
fuelISessionHistory.idl 

>
>interface nsIHistoryEntry;
>interface nsISHistoryListener;
>interface nsISimpleEnumerator;

Let's try to not use these in the interface

>
>[scriptable, uuid(b5dbba45-4bd9-4a5c-8ab4-d3e6e448f7fc)]
>interface fuelISessionHistory: nsISupports
>{
>  /**
>   * A readonly property of the interface that returns 
>   * the number of toplevel documents currently available
>   * in session history.
>   */
>   readonly attribute long count;
>
>  /**
>   * A readonly property of the interface that returns 
>   * the index of the current document in session history.
>   */
>   readonly attribute long index;

I don't like this name. Let's find a more JS friendly name.
Maybe currentPage

>
>  /**
>   * A readonly property of the interface that returns 
>   * the index of the last document that started to load.
>   */
>   readonly attribute long requestedIndex;

I don't like this name. Let's find a more JS friendly name.
Maybe lastPage

>
>  /**
>   * A read/write property of the interface, used to Get/Set
>   * the maximum number of toplevel documents, session history 
>   * can hold for each instance. 
>   */
>   attribute long maxLength;

I don't like this name. Let's find a more JS friendly name.
Maybe size

>
>  /**
>   * Called to obtain handle to the history entry at a
>   * given index.
>   *
>   * @param index             The index value whose entry is requested.
>   * @param modifyIndex       A boolean flag that indicates if the current
>   *                          index of session history should be modified 
>   *                          to the parameter index.
>   *
>   * @return                  <code>NS_OK</code> history entry for 
>   *                          the index is obtained successfully.
>   *                          <code>NS_ERROR_FAILURE</code> Error in obtaining
>   *                          history entry for the given index.
>   */
>   nsIHistoryEntry getEntryAtIndex(in long index, in boolean modifyIndex);

Ok, let's remove this method of accessing history items and add a simple readonly Array getter. The Array would be generated each time its requested (no caching). You could probably use the simple enumerator to implement this getter.

readonly nsIVariant pages;

>
>
>  /**
>   * Called to purge older documents from history.
>   * Documents can be removed from session history for various 
>   * reasons. For example to  control memory usage of the browser, to 
>   * prevent users from loading documents from history, to erase evidence of
>   * prior page loads etc...
>   *
>   * @param numEntries        The number of toplevel documents to be
>   *                          purged from history. During purge operation,
>   *                          the latest documents are maintained and older 
>   *                          'numEntries' documents are removed from history.
>   * @throws                  <code>NS_SUCCESS_LOSS_OF_INSIGNIFICANT_DATA</code> Purge was vetod.
>   * @throws                  <code>NS_ERROR_FAILURE</code> numEntries is
>   *                          invalid or out of bounds with the size of history.
>   *                          
>   */
>   void PurgeHistory(in long numEntries);

Lets not throw here. Do some checks in the implementation, catch any exception and return a bool for success.

I also don't like the name. Again, need something more JS friendly. Maybe removePages(numPages)

>
>  /**
>   * Called to obtain a enumerator for all the  documents stored in 
>   * session history. The enumerator object thus returned by this method
>   * can be traversed using nsISimpleEnumerator. 
>   *
>   * @note  To access individual history entries of the enumerator, perform the
>   *        following steps:
>   *        1) Call nsISHistory->GetSHistoryEnumerator() to obtain handle 
>   *           the nsISimpleEnumerator object.
>   *        2) Use nsISimpleEnumerator->GetNext() on the object returned
>   *           by step #1 to obtain handle to the next object in the list. 
>   *           The object returned by this step is of type nsISupports.
>   *        3) Perform a QueryInterface on the object returned by step #2 
>   *           to nsIHistoryEntry.
>   *        4) Use nsIHistoryEntry to access properties of each history entry. 
>   *
>   * @see nsISimpleEnumerator
>   * @see nsIHistoryEntry
>   * @see QueryInterface()
>   * @see do_QueryInterface()
>   */
>   readonly attribute nsISimpleEnumerator SHistoryEnumerator;

Remove this. We'll depend on the Array method above.

Basically, we want this to look and feel more like a JS object than an XPCOM object. Even if that means it is not as performant as the XPCOM version. FUEL is about convenience over performance (if a choice has to be made)

Comment 6

[Samer Ziadeh]

is the size attribute necessary? we have an attribute that returns the number of documents and another one that returns an array of documents.

Comment 7

[Tom Aratyn]

Size is pretty necessary. It's not meant as a counter (count does that and it could be replaced by pages.length) but as a limit of how many pages you care about.

Comment 8

[Samer Ziadeh]

Attachment: Include fuelIBrowserHistory interface

I added the interface to fuelIApplication.idl file,  the history interface starts at around line 366. I didn't add the interface in a separate file because I thought it would be out of context or irrelevant :/

Comment 9

[Tom Aratyn]

maybe drop count, because you can get that via pages.length.

getPage() can't return void ;)

Also do you need getPage and pages array?

Comment 10

[Tom Aratyn]

Regardless of things that may need to be fixed, good work! Great to see you getting some code up. 

Comment 11

[Samer Ziadeh]

=D
I was thinking about the count and pages.length, but wasn't sure.
/me changes it

Comment 12

[Samer Ziadeh]

Attachment: updated fuelIBrowserHistory

I added a few functions to the idl (onGet, on...)
the interface is around line 365

Comment 13

[Tom Aratyn]

Samer, either put up a diff or throw it in a separate file. Diff is probably best.  Scrolling is annoying.

Comment 14

[Samer Ziadeh]

Attachment: idl implementation

I implemented the function from the idl interface.
around line 181
I put comments in there "just in case" for the revision. I will remove them in the end

Comment 15

[Samer Ziadeh]

Tom i saw your comment too late for the second attachement >.< sorry

Comment 16

[Tom Aratyn]

no prob. Just put up the diffs now :).

Comment 17

[Samer Ziadeh]

Attachment: updated fuelIBrowserHistory

I added a few functions to the idl (onGet, on...)

Comment 18

[Samer Ziadeh]

Attachment: idl implementation

I implemented the function from the idl interface.
I put comments in there "just in case" for the revision. I will remove them in
the end

Comment 19

[Tom Aratyn]

So I've had a bit of a day and I may very well be missing something but why does the fuelIBrowserHistory expose those on* functions?

Also, it doesn't seem to be able to add simple history listeners (which was the point of this whole thing, wasn't it?).

Finally, Samer - it's time to face up to it and accept this bug ;).

Comment 20

[David Humphrey (:humph)]

Assigning to Samer (not sure he has EditBugs).

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 322591 [details] [diff] [review]
idl implementation

>Index: browser/fuel/src/fuelApplication.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/fuel/src/fuelApplication.js,v
>retrieving revision 1.29
>diff -u -8 -p -r1.29 fuelApplication.js
>--- browser/fuel/src/fuelApplication.js	3 Apr 2008 03:32:42 -0000	1.29
>+++ browser/fuel/src/fuelApplication.js	27 May 2008 01:27:34 -0000
>@@ -172,16 +172,167 @@ Window.prototype = {
>     this._tabbrowser = null;
>     this._events = null;
>   },
> 
>   QueryInterface : XPCOMUtils.generateQI([Ci.fuelIWindow])
> };
> 
> 
>+
>+//=================================================
>+// History implementation
>+function BrowserHistory(aBrowserTab) {
>+  this._count = 0;
>+  this._currentPage = 0;
>+  this._lastPage = -1;
>+  this._size = 0;

You really do not want to cahce this data. Instead, use the nsISHistory interface to access them. Removes these and see below.

>+  this._pages = [];
>+  this._events = new Events();
>+  this._tab = aBrowserTab;

Add a data member to access the session history interface:

this._history = this._tab.sessionHistory;

Add code to register this call as a nsISHistoryListener here. Something like:

this._history.addSHistoryListener(this);

>+
>+  var self = this;
>+  gShutdown.push(function() { self._shutdown(); });
>+}
>+
>+BrowserHistory.prototype = {
>+  get count() {
>+    return this._pages.length;

should be: return this._history.count;

Same for the rest of the "pass-thru" data members. See nsISHistory.idl for details

>+  },
>+
>+  get currentPage() {
>+    return this._currentPage;

use: this._history.index;

>+  },
>+
>+  get lastPage() {
>+    return this._lastPage;
>+  },
>+
>+  get size() {
>+    return this._size;

use: this._history.maxSize;

>+  },
>+
>+  set size(aSize) {
>+    this._size = aSize;

use: this._history.maxSize;

>+  },
>+
>+  get pages() {
>+    return this._pages;
>+  },
>+
>+  get events() {
>+    return this._events;
>+  },
>+
>+  get tab() {
>+    return this._tab;
>+  },

remove the "tab" property

>+
>+  get: function bh_get(aPage) {
>+    if (aPage > this.count || aPage < 0)
>+      return;
>+
>+    return this.pages[aPage];

use: return this._history.getEntryAtIndex(aPage, false);

>+  },
>+
>+  remove: function bh_remove(aPages) {
>+    if (aPages > this.count || aPages < 0)
>+      return;
>+
>+    var tmp = [];
>+    for(var i = aPages; i < this.count; i++) {
>+      tmp.push(this._pages[i]);
>+    }
>+
>+    this._pages = tmp;

use: this._history.PurgeHistory(aPages);

>+  },
>+
>+  /*
>+   * NOTE: remove comments after revision
>+   */
>+  onNewEntry: function bh_one(aURI) {

"OnHistoryNewEntry" is the true name of this callback function


use: this._events.dispatch("HistoryNew", aURI.spec);

NOTE: we will not use this function to create and manage the this._pages data member. In fact, we should not use this._pages at all. See nsISHistory and the SHistoryEnumerator property. We will build the array on-the-fly and return it from the pages getter method.

>+
>+    /*
>+     * Check if the index is in the middle of the list
>+     * (ie: backward and/or forward navigation happened).
>+     * If so clear the array of pages from the current
>+     * index until the end.
>+     */
>+    if (this.currentPage != this.size - 1) {
>+      // copy pages over to tmp
>+      var tmp = this._pages.slice();
>+
>+      // reset pages
>+      this._pages = null;
>+      this._pages = [];
>+
>+      for (var i = 0; i < this.currentPage; i++) {
>+        this._pages.push(tmp[i]);
>+      }
>+    }
>+
>+    /*
>+     * Check if the current number of pages is at the last
>+     * index. If so then shift down 'numpages - 1' pages
>+     * and add the new entry at the end.
>+     */
>+    else if (this.count == this.size) {
>+      for (var i = 0; i < this.count - 1; i++) {
>+        this._pages[i] = this._pages[i + 1];
>+      }
>+    }
>+
>+    /*
>+     * If the last page didn't finish loading,
>+     * overwrite it's index with the new page
>+     * and update the index
>+     */
>+    var lp = this.lastPage;
>+    if (lp != -1) {
>+      this._pages[lp] = aURI;
>+      this._currentPage = lp;
>+    }
>+
>+    /*
>+     * else add the new page to the end of the document
>+     * and update the index
>+     */
>+    else {
>+      this._currentPage = this._pages.push(aURI) - 1;
>+    }
>+  },
>+
>+  onGet: function bh_og(aIndex) {
>+    if (aIndex < 0 || aIndex > this.count)
>+      return;
>+
>+    this._currentPage = aIndex;
>+  },
>+
>+  onGoBack: function bh_ogb() {

"OnHistoryGoBack" is the true name of this callback function and it has params

>+    this.onGet(this.currentPage - 1);

use: this._events.dispatch("HistoryBack", aURI.spec);

>+  },
>+
>+  onGoForward: function bh_ogf() {

"OnHistoryGoForward" is the true name of this callback function and it has params

>+    this.onGet(this.currentPage + 1);

use: this._events.dispatch("HistoryForward", aURI.spec);

NOTE: you are missing several callbacks from the nsISHistoryListener interface and the ones you have are not the right names and/or signatures. See nsISHistoryListener for the interface methods

>+  },
>+
>+  _shutdown: function bh_shutdown() {

Add code to remove this class as a listener:

this._tab.sessionHistory.removeSHistoryListener(this);

>+    this._count = null;
>+    this._currentPage = null;
>+    this._lastPage = null;
>+    this._size = null;
>+    this._pages = null;
>+    this._events = null;
>+    this._tab = null;
>+  },
>+
>+  QueryInterface : XPCOMUtils.generateQI([Ci.fuelIBrowserHistory])

add Ci.nsISHistoryListener to the array

>+};
>+
> //=================================================
> // BrowserTab implementation
> function BrowserTab(aWindow, aBrowser) {
>   this._window = aWindow;
>   this._tabbrowser = aWindow.getBrowser();
>   this._browser = aBrowser;
>   this._events = new Events();
>   this._cleanup = {};


Hey, you need to add:
1. a data member for the BrowserHistory
2. a getter to BrowserTab to return the history

r-, but you're getting better. Clean this up for another review.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 322590 [details] [diff] [review]
updated fuelIBrowserHistory

>Index: browser/fuel/public/fuelIApplication.idl
>===================================================================
>RCS file: /cvsroot/mozilla/browser/fuel/public/fuelIApplication.idl,v
>retrieving revision 1.13
>diff -u -8 -p -r1.13 fuelIApplication.idl
>--- browser/fuel/public/fuelIApplication.idl	25 Mar 2008 00:38:03 -0000	1.13
>+++ browser/fuel/public/fuelIApplication.idl	27 May 2008 01:27:58 -0000
>@@ -39,16 +39,17 @@
> #include "extIApplication.idl"
> 
> interface nsIVariant;
> interface nsIURI;
> interface nsIDOMHTMLDocument;
> 
> interface fuelIBookmarkFolder;
> interface fuelIBrowserTab;
>+interface fuelIBrowserHistory;
> 
> /**
>  * Interface representing a collection of annotations associated
>  * with a bookmark or bookmark folder.
>  */
> [scriptable, uuid(335c9292-91a1-4ca0-ad0b-07d5f63ed6cd)]
> interface fuelIAnnotations : nsISupports
> {
>@@ -321,16 +322,21 @@ interface fuelIBrowserTab : nsISupports
> 
>   /**
>    * The events object for the browser tab.
>    * supports: "load"
>    */
>   readonly attribute extIEvents events;
> 
>   /**
>+   * The session history event listener
>+   */
>+  readonly attribute fuelIBrowserHistory history;
>+
>+  /**
>    * Load a new URI into this browser tab.
>    * @param   aURI
>    *          The uri to load into the browser tab
>    */
>   void load(in nsIURI aURI);
> 
>   /**
>    * Give focus to this browser tab, and bring it to the front.
>@@ -352,16 +358,115 @@ interface fuelIBrowserTab : nsISupports
> 
>   /**
>    * Move this browser tab to the last tab within the window.
>    */
>   void moveToEnd();
> };
> 
> /**
>+ * Interface representing a browser tab.
>+ */
>+[scriptable, uuid(828e7d1-3fa0-478b-bfc2-6d32923f0d58)]
>+interface fuelIBrowserHistory : nsISupports
>+{
>+  /**
>+   * A readonly property of the interface that returns
>+   * the number of toplevel documents currently available
>+   * in session history
>+   */
>+  readonly attribute long count;
>+
>+  /**
>+   * A readonly property of the interface that returns
>+   * the index of the current document in session history
>+   */
>+  readonly attribute long currentPage;
>+
>+  /**
>+   * A readonly property of the interface that returns
>+   * the index of the last document that started to load
>+   * and hasn't finished yet. When document is finished
>+   * loading a value of -1 is returned
>+   */
>+  readonly attribute long lastPage;
>+
>+  /**
>+   * A read/write property of the interface, used to get/set
>+   * the number of toplevel documents session history can
>+   * can hold for each instance
>+   */
>+  attribute long size;
>+
>+  /**
>+   * A readonly attribute of the interface that returns
>+   * an array of the toplevel documents stored in session
>+   * history
>+   */
>+  readonly attribute nsIVariant pages;
>+
>+  /**
>+   * A readonly attribute of the interface that returns
>+   * a handle to register listeners for the session
>+   * history
>+   */
>+  readonly attribute extIEvents events;
>+
>+  /**
>+   * A readonly attribute of the interface that return
>+   * a handle to the current tab
>+   */
>+  readonly attribute fuelIBrowserTab tab;
>+

I'm not sure we want a back reference to the owning tab. We don't use this pattern for any other objects. BrowserTab doesn't hold a window back reference for example. Let's remove it for now.

>+
>+  /**
>+   * Called when a new document is added to the session history.
>+   *
>+   * @param aURI    The URI of the document to be added to session history.
>+   */
>+  void onNewEntry(in nsIURI aURI);
>+
>+  /**
>+   * Called when a document is fetched from session history.
>+   *
>+   * @param aPage    The index of the page to get from session history.
>+   */
>+  void onGet(in long aPage);
>+
>+  /**
>+   * Called when moving back one level in session history.
>+   */
>+  void onGoBack();
>+
>+  /**
>+   * Called when moving forward one level in session history.
>+   */
>+  void onGoForward();

These are all part of the nsISHistoryListener interface, right? We want these to be part of the implementation, not the interface. Users won't use these methods, they'll use the | events | property to register event listeners for the "events" instead.

Remove them from the IDL

Comment 23

[Samer Ziadeh]

Attachment: revised js

Comment 24

[Samer Ziadeh]

Comment on attachment 322594 [details] [diff] [review]
revised js

agh forgot to to fix typo
this._history.addHistoryListener
i forgot the S

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 322594 [details] [diff] [review]
revised js

>Index: browser/fuel/src/fuelApplication.js
>===================================================================

>+  get pages() {
>+    return this._pages;

Use the nsISHistory.SHistoryEnumerator to fill the array and return it every time

>+  onHistoryNewEntry: function bh_one(aURI) {
>+    this._events.dispatch("HistoryNew", aURI.spec);
>+  },
>+
>+  onHistoryGotoIndex: function bh_og(aIndex) {
>+    if (aIndex < 0 || aIndex > this.count)
>+      return;
>+
>+    this._events.dispatch("HistoryGoto", aURI.spec);

not aURI.spec -> aIndex

>+  },
>+
>+  onHistoryGoBack: function bh_ogb(aURI) {
>+    this._events.dispatch("HistoryBack", aURI.spec);
>+  },
>+
>+  onHistoryGoForward: function bh_ogf(aURI) {
>+    this._events.dispatch("HistoryForward", aURI.spec);
>+  },

All of these callback methods start with a captial "O", as decalred in the IDL, so you need to use it too.

> //=================================================
> // BrowserTab implementation
> function BrowserTab(aWindow, aBrowser) {
>   this._window = aWindow;
>   this._tabbrowser = aWindow.getBrowser();
>   this._browser = aBrowser;
>   this._events = new Events();
>   this._cleanup = {};

Remember to make a BrowserHistory in here and return it from the | history | getter!

Comment 26

[Samer Ziadeh]

Attachment: fuelIApplication.idl

Comment 27

[Samer Ziadeh]

Attachment: fuelIApplication.js

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 322600 [details] [diff] [review]
fuelIApplication.js

>Index: browser/fuel/src/fuelApplication.js
>===================================================================

>+  get pages() {
>+    return this._history.SHistoryEnumerator;

Almost. I want you to loop over the enumerator using the nsISimpleEnumerator and fill an Array. Then return the Array. We don't just want to return the enumerator.

> //=================================================
> // BrowserTab implementation
> function BrowserTab(aWindow, aBrowser) {
>   this._window = aWindow;
>   this._tabbrowser = aWindow.getBrowser();
>   this._browser = aBrowser;
>   this._events = new Events();
>   this._cleanup = {};
>+  this._sessionHistory = new BrowserHistory(this);

this._history should be good enough and not too long

Also, you need to add the getter for history!

get history() {
  return this._history;
}

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

Samer, your code is looking good and is far enough along that you should have some unit tests too. Add a browser_History.js file, modeled after the other test files.

Comment 30

[Samer Ziadeh]

Attachment: fuelIApplication.js

I'm using this code, when I run this command from my extension I get that error.
It doesn't make sense, the spellings are fine and they're pointing to each other.

btw, the way I declared the BrowserHistory within the browsertab doesn't that send the program in a loop, since BrowserHistory._history = BrowserTab.history and BrowserTab.history == BrowserHistory._history;

from the extension I called:
      alert(typeof Application.activeWindow.activeTab);

And this is the errors I'm getting from the error console:

Error: this._history is undefined
Source File: file:///Users/samer/mozilla/trunk/mozilla/objdir-ff-debug-tests/dist/MinefieldDebug.app/Contents/MacOS/components/fuelApplication.js
Line: 187

Error: uncaught exception: [Exception... "'[JavaScript Error: "this._history is undefined" {file: "file:///Users/samer/mozilla/trunk/mozilla/objdir-ff-debug-tests/dist/MinefieldDebug.app/Contents/MacOS/components/fuelApplication.js" line: 187}]' when calling method: [fuelIWindow::activeTab]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://testext/content/overlay.js :: anonymous :: line 9"  data: yes]

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 322782 [details] [diff] [review]
fuelIApplication.js

>Index: browser/fuel/src/fuelApplication.js
>===================================================================

> 
>+
>+//=================================================
>+// History implementation
>+function BrowserHistory(aBrowserTab) {
>+  this._events = new Events();
>+  this._history = aBrowserTab.history;

this._history = aBrowserTab._browser.sessionHistory;

Try that instead.

Comment 32

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 322599 [details] [diff] [review]
fuelIApplication.idl

this looks good, but we need all parts working before we can land anything

Comment 33

[Samer Ziadeh]

Attachment: fuelIApplication.js

I'm working on the test file for that, I tested out the properties/attributes of the function and they worked fine, but only after I commented out the line to register the history listener :/

Comment 34

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 324871 [details] [diff] [review]
fuelIApplication.js

>Index: browser/fuel/src/fuelApplication.js
>===================================================================

>+
>+//=================================================
>+// History implementation
>+function BrowserHistory(aBrowserTab) {
>+  this._events = new Events();
>+  this._history = aBrowserTab.history;

should be:
    this._history = aBrowserTab._browser.sessionHistory;

Comment 35

[Samer Ziadeh]

Attachment: fuelApplication.js

i uploaded the wrong file last time, i had a typo in my diff command, sorry

Comment 36

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 324877 [details] [diff] [review]
fuelApplication.js

>Index: browser/fuel/src/fuelApplication.js
>===================================================================

>+  OnHistoryNewEntry: function bh_one(aURI) {
>+    this._events.dispatch("HistoryNew", aURI.spec);
>+  },
>+
>+  OnHistoryGotoIndex: function bh_og(aURI) {

boolean OnHistoryGotoIndex(aIndex, aGotoURI);

>+    this._events.dispatch("HistoryGoto", aURI.spec);
>+  },
>+
>+  OnHistoryGoBack: function bh_ogb(aURI) {
>+    this._events.dispatch("HistoryBack", aURI.spec);
>+  },
>+
>+  OnHistoryGoForward: function bh_ogf(aURI) {
>+    this._events.dispatch("HistoryForward", aURI.spec);
>+  },
>+
>+  OnHistoryPurge: function bh_ohp(aPages) {
>+    this._events.dispatch("HistoryPurge", aPages);
>+  },
>+
>+  OnHistoryReload: function bh_ohr(aURI) {

 boolean OnHistoryReload(aReloadURI, aReloadFlags);

>+    this._events.dispatch("HistoryReload", aURI);
>+  },

I also noticed that all the listener callbacks should return | true | so the action can continue (as per the nsISHistoryListener IDL)

Comment 37

[Samer Ziadeh]

Attachment: browser_History.js

The history.size can return the size but I get an error when I try to change it's value.

I called this property:
Application.activeWindow.activeTab.history.size = 3

and got these 2 errors:

Error: [Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: file:///Users/samer/mozilla/trunk/mozilla/objdir-ff-debug-tests/dist/MinefieldDebug.app/Contents/MacOS/components/fuelApplication.js :: anonymous :: line 327"  data: no]
Source File: file:///Users/samer/mozilla/trunk/mozilla/objdir-ff-debug-tests/dist/MinefieldDebug.app/Contents/MacOS/components/fuelApplication.js
Line: 327

Error: uncaught exception: [Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: file:///Users/samer/mozilla/trunk/mozilla/objdir-ff-debug-tests/dist/MinefieldDebug.app/Contents/MacOS/components/fuelApplication.js :: anonymous :: line 327"  data: no]

=====

Line 327 from fuelApplication.js is:
  set size(aSize) {
    this._history.maxSize = aSize;
  },


For the page loading I wrote a comment in there, I'll find a different way to load the pages one at a time.
I didn't know how to test for pages, and get()

Comment 38

[Samer Ziadeh]

Attachment: fuelApplication.js

updated it to include the 'return's  for the OnHistory... functions

Comment 39

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #37)
> Created an attachment (id=325439) [details]
> browser_History.js
> 
> The history.size can return the size but I get an error when I try to change
> it's value.
> 
> I called this property:
> Application.activeWindow.activeTab.history.size = 3
> 
> and got these 2 errors:
> 
<snip>
> 
> =====
> 
> Line 327 from fuelApplication.js is:
>   set size(aSize) {
>     this._history.maxSize = aSize;
>   },

this._history.maxLength is the right property of nsISHistory

Comment 40

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 325441 [details] [diff] [review]
fuelApplication.js

>Index: browser/fuel/src/fuelApplication.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/fuel/src/fuelApplication.js,v
>retrieving revision 1.29
>diff -u -8 -p -r1.29 fuelApplication.js
>--- browser/fuel/src/fuelApplication.js	3 Apr 2008 03:32:42 -0000	1.29

>+// History implementation
>+function BrowserHistory(aBrowserTab) {
>+  this._events = new Events();
>+  this._history = null;

probably not needed :)

>+  this._history = aBrowserTab._browser.sessionHistory;
>+
>+  // This line causes the program to crash
>+  //this._history.addSHistoryListener(this);

you need to uncomment this to get your tests working

>+
>+  var self = this;
>+  gShutdown.push(function() { self._shutdown(); });
>+}
>+
>+BrowserHistory.prototype = {
>+  get count() {
>+    return this._history.count;
>+  },
>+
>+  get currentPage() {
>+    return this._history.index;
>+  },
>+
>+  get lastPage() {
>+    return this._history.lastPage;

nsISHistory.lastPage doesn't extist. I think you wanted:

      return this._history.requestedIndex;

>+  },
>+
>+  get size() {
>+    return this._history.maxSize;

      return this._history.maxLength;

>+  },
>+
>+  set size(aSize) {
>+    this._history.maxSize = aSize;

      this._history.maxLength = aSize;

>+
>+  OnHistoryGotoIndex: function bh_og(aIndex, aURI) {
>+    this._events.dispatch("HistoryGoto", aIndex, aURI.spec);

You can't pass 2 params into event.dispatch, only 1. Use the aURI.spec for now.

>+
>+  OnHistoryReload: function bh_ohr(aURI) {

OnHistoryReload(aURI, aFlags) - but only pass the aURI to the dispatch
[read the nsISHistoryListener IDL file!]


r-, let's get these small typos fixed up and some tests working ASAP

Comment 41

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 325439 [details]
browser_History.js

>
>   // NOTE: There is a problem with set size
>   //       it doesn't allow the size to be set
>   //       but it can be read just fine
>   //tab.history.size = 5;
>   //is(tab.history.size, 5, "Check history size");

this can be fixed in fuelApplication.js

>
>   is(tab.history.count, count, "Check history count");
>
>   // Load a few pages
>   // NOTE: this doesn't work as it should
>   //       all the pages are sent at once
>   //       so the history only catches the last one
>   tab.load(url('http://www.mozilla.org'));
>   tab.load(url('http://www.google.com'));
>   tab.load(url('http://cs.senecac.on.ca'));
>   tab.load(url('http://zenit.senecac.on.ca/wiki'));

you will likely need to wait until each page loads, then add the next one.

Comment 42

[Samer Ziadeh]

Attachment: browser_History.js

Comment 43

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 326207 [details]
browser_History.js

>const Ci = Components.interfaces;
>const Cc = Components.classes;
>
>function url(spec) {
>   var ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
>   return ios.newURI(spec, null, null);
>}
>
>function test() {
>   var tab = Application.activeWindow.activeTab;

Remove this line and get 'tab' from below

>   var count = 0;
>
>   // start with a new tab
>   Application.activeWindow.open(url('about:blank'));

I think you want:
    var tab = Application.activeWindow.open(url('about:blank'));

cause you seem to want to use the new blank tab for the tests, but you are actually using the original active tab.

>
>   tab.history.size = 5;
>   is(tab.history.size, 5, "Check history size");
>
>   is(tab.history.count, count, "Check history count");
>
>   // Load a few pages
>   var URLs = [];
>   URLs.push('http://www.mozilla.org');
>   URLs.push('http://www.google.com');
>   URLs.push('http://cs.senecac.on.ca');
>   URLs.push('http://zenit.senecac.on.ca/wiki');

At some point we will need to make simple HTML files to use instead of these real sites. We don't use real sites in our tests - they can go down and break our tests.


Looks good. Make the 'tab' change and then we need to get the callbacks from nsISHistoryListener working too.

Comment 44

[Samer Ziadeh]

I think I found what's causing the program to fail. I think it has to do with BrowserHistory not having enough privilages to add a listener to SHistory
Based on the comments in that function:
http://mxr.mozilla.org/seamonkey/source/docshell/shistory/src/nsSHistory.cpp#594

the error returned was:
###!!! ASSERTION: Oops!  You're asking for a weak reference to an object that doesn't support that.: 'factoryPtr', file nsWeakReference.cpp, line 109

http://mxr.mozilla.org/firefox/source/xpcom/glue/nsWeakReference.cpp#109

The QI is getting the right interface names to add, I don't know what else could be the problem :/

Comment 45

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #44)
> I think I found what's causing the program to fail. I think it has to do with
> BrowserHistory not having enough privilages to add a listener to SHistory
> Based on the comments in that function:
> http://mxr.mozilla.org/seamonkey/source/docshell/shistory/src/nsSHistory.cpp#594
> 
> the error returned was:
> ###!!! ASSERTION: Oops!  You're asking for a weak reference to an object that
> doesn't support that.: 'factoryPtr', file nsWeakReference.cpp, line 109
> 
> http://mxr.mozilla.org/firefox/source/xpcom/glue/nsWeakReference.cpp#109
> 
> The QI is getting the right interface names to add, I don't know what else
> could be the problem :/
> 

Well if it is failing the QI for weak reference then you need to add nsIWeakReference to the BrowserHistory QI:

   QueryInterface : XPCOMUtils.generateQI([Ci.fuelIBrowserHistory, Ci.nsISHistoryListener, Ci.nsIWeakReference])

Comment 46

[Samer Ziadeh]

Attachment: browser_History.js

Comment 47

[Samer Ziadeh]

Attachment: fuelApplication.js

Comment 48

[Samer Ziadeh]

Attachment: browser_History.js

Comment 49

[Samer Ziadeh]

Attachment: browser_History.js

Comment 50

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 330303 [details]
browser_History.js

I think you attached the wrong file :)

Comment 51

[Samer Ziadeh]

Attachment: browser_History.js

oops sorry about that

Comment 52

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 330343 [details]
browser_History.js

>   tab.events.addListener("load", function() {
>      setTimeout( (urlIndex < URLs.length) ? doNewEntry : doneLoading ? null : doReload, 500)
>   });

500 seems kind of long for the timeout. Can you pass with 50 instead?
(I mean for everywhere you use 500, not just here)

>   
>   tab.history.events.addListener("HistoryNew", function(aURI) {
>      count++;
>      currentPage++;

Using the url index can you check the aURI param too? use aURI.spec to compare as a string. Do that wherever you can.

>      
>      is(tab.history.count, count, "check history new entry");
>      is(tab.history.currentPage, currentPage, "check history new entry");
>   });


>   
>   tab.history.events.addListener("HistoryGoto", function(aIndex, aURI) {
>      setTimeout(function() {
>         is(tab.history.currentPage, currentPage, "check history go to");
>      }, 500);
>      
>      setTimeout(doGoForward, 1000);

Put the doGoForward timeout in the above anonymous function


>   function doReload() {

Ok, so doReload is called after the 3 URLs are loaded. Could you check the tab.history.pages array to make sure the top (I think it would be the top) 3 items in array are the 3 loaded URLs

>      doneLoading = true;
>      currentPage++;
>      count++;

I didn't realize that currentPage would be changed during a reload.

Looking very good. Let's try to get a few more checks (described above) tested and get this landed. r- until we get the additional tests, but great job.

Comment 53

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 329273 [details] [diff] [review]
fuelApplication.js

>Index: browser/fuel/src/fuelApplication.js
>===================================================================
>+
>+  get pages() {
>+    var ar = [];
>+    var en = this._history.SHistoryEnumerator;
>+
>+    while (en.hasMoreElements()) {
>+      ar.push(en.getNext());
>+    }
>+
>+    return ar;
>+  },
>+

>+
>+  get: function bh_get(aPage) {
>+    if (aPage > this.count || aPage < 0)
>+      return;
>+
>+    return this._history.getEntryAtIndex(aPage, false);
>+  },

My only feedback on these functions are the fact they return nsIHistoryEntry objects. Futhermore, "pages" returns an nsISupports which needs to be QueryInterfaced into an nsIHistoryEntry.

nsIHistoryEntry isn't _bad_, so I think "get" is ok. On the other hand, we should probably add a QueryInterface to the "pages" code like this:

>+  get pages() {
>+    var ar = [];
>+    var en = this._history.SHistoryEnumerator;
>+
>+    while (en.hasMoreElements()) {
        let entry = en.getNext();
        entry.QueryInterface(Ci.nsIHistoryEntry);
        ar.push(entry);
>+    }
>+
>+    return ar;
>+  },

That way anyone calling "pages" will not need to QI the items in the array themselves.

r+ with that change _and_ make sure the tests pass :)

Comment 54

[Samer Ziadeh]

Attachment: browser_History.js

the doGoto timeout had to be 115, but i put 150 to be safe for the tests to work properly the rest works fine at 50

Comment 55

[Samer Ziadeh]

Attachment: fuelApplication.js

Comment 56

[Samer Ziadeh]

Attachment: fuelApplication.js

the diff didn't work last time,

Comment 57

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 330658 [details]
browser_History.js

Looks good

Comment 58

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 330695 [details] [diff] [review]
fuelApplication.js

Also good

Comment 59

[Samer Ziadeh]

Do I still need to do anything with those?

Comment 60

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 322599 [details] [diff] [review]
fuelIApplication.idl

Asking gavin for a second review before landing

Comment 61

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 330658 [details]
browser_History.js

Asking gavin for a second review before landing

Comment 62

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 330695 [details] [diff] [review]
fuelApplication.js

Asking gavin for a second review before landing

Comment 63

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: combined patch with some fixes

I combined the separate patches into one patch and tested it. I fixed 2 issues and converted some "setTimeout" calls to "executeSoon" calls. All tests pass.

pulling my r+ forward, but this might be easier now for gavin

Comment 64

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

Comment on attachment 345841 [details] [diff] [review]
combined patch with some fixes

Sorry that I never got to this, but I'm assuming we're not really interested in pursuing this further at this point? WONTFIX?