FUEL: no easy history listeners

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
12 years ago
6 years ago

People

(Reporter: themystic.ca, Assigned: sziadeh)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(status2.0 ?)

Details

(Whiteboard: [patchlove])

Attachments

(1 attachment, 23 obsolete attachments)

12.58 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
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
the event names are based directly on http://lxr.mozilla.org/seamonkey/source/docshell/shistory/public/nsISHistoryListener.idl
Assignee: nobody → themystic.ca
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.
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.
Posted file fuelISessionHistory.idl (obsolete) —
Attachment #292086 - Attachment mime type: text/x-idl → text/plain
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)
Attachment #292086 - Flags: review-
is the size attribute necessary? we have an attribute that returns the number of documents and another one that returns an array of documents.
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.

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 :/
Attachment #321953 - Attachment mime type: application/octet-stream → text/plain
maybe drop count, because you can get that via pages.length.

getPage() can't return void ;)

Also do you need getPage and pages array?
Regardless of things that may need to be fixed, good work! Great to see you getting some code up. 
=D
I was thinking about the count and pages.length, but wasn't sure.
/me changes it
Posted file updated fuelIBrowserHistory (obsolete) —
I added a few functions to the idl (onGet, on...)
the interface is around line 365
Attachment #321953 - Attachment is obsolete: true
Samer, either put up a diff or throw it in a separate file. Diff is probably best.  Scrolling is annoying.
Posted file idl implementation (obsolete) —
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
Tom i saw your comment too late for the second attachement >.< sorry
no prob. Just put up the diffs now :).
Posted patch updated fuelIBrowserHistory (obsolete) — Splinter Review
I added a few functions to the idl (onGet, on...)
Attachment #322588 - Attachment is obsolete: true
Posted patch idl implementation (obsolete) — Splinter Review
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
Attachment #322589 - Attachment is obsolete: true
Attachment #322590 - Attachment is patch: true
Attachment #322591 - Attachment is patch: true
Attachment #322590 - Flags: review?(mark.finkle)
Attachment #322591 - Flags: review?(mark.finkle)
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 ;).
Assigning to Samer (not sure he has EditBugs).
Assignee: themystic.ca → sziadeh
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.
Attachment #322591 - Flags: review?(mark.finkle) → review-
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
Attachment #322590 - Flags: review?(mark.finkle) → review-
Posted patch revised js (obsolete) — Splinter Review
Attachment #322591 - Attachment is obsolete: true
Attachment #322594 - Flags: review?(mark.finkle)
Comment on attachment 322594 [details] [diff] [review]
revised js

agh forgot to to fix typo
this._history.addHistoryListener
i forgot the S
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!
Attachment #322594 - Flags: review?(mark.finkle) → review-
Posted patch fuelIApplication.idl (obsolete) — Splinter Review
Attachment #322590 - Attachment is obsolete: true
Attachment #322599 - Flags: review?(mark.finkle)
Posted patch fuelIApplication.js (obsolete) — Splinter Review
Attachment #322594 - Attachment is obsolete: true
Attachment #322600 - Flags: review?(mark.finkle)
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;
}
Attachment #322600 - Flags: review?(mark.finkle) → review-
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.
Posted patch fuelIApplication.js (obsolete) — Splinter Review
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]
Attachment #322600 - Attachment is obsolete: true
Attachment #322782 - Flags: review?(mark.finkle)
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.
Attachment #322782 - Flags: review?(mark.finkle) → review-
Comment on attachment 322599 [details] [diff] [review]
fuelIApplication.idl

this looks good, but we need all parts working before we can land anything
Attachment #322599 - Flags: review?(mark.finkle) → review+
Posted patch fuelIApplication.js (obsolete) — Splinter Review
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 :/
Attachment #322782 - Attachment is obsolete: true
Attachment #324871 - Flags: review?(mark.finkle)
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;
Attachment #324871 - Flags: review?(mark.finkle) → review-
Posted patch fuelApplication.js (obsolete) — Splinter Review
i uploaded the wrong file last time, i had a typo in my diff command, sorry
Attachment #324871 - Attachment is obsolete: true
Attachment #324877 - Flags: review?(mark.finkle)
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)
Attachment #324877 - Flags: review?(mark.finkle) → review-
Posted file browser_History.js (obsolete) —
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()
Attachment #325439 - Flags: review?(mark.finkle)
Posted patch fuelApplication.js (obsolete) — Splinter Review
updated it to include the 'return's  for the OnHistory... functions
Attachment #324877 - Attachment is obsolete: true
Attachment #325441 - Flags: review?(mark.finkle)
(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 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
Attachment #325441 - Flags: review?(mark.finkle) → review-
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.
Attachment #325439 - Flags: review?(mark.finkle) → review-
Posted file browser_History.js (obsolete) —
Attachment #325439 - Attachment is obsolete: true
Attachment #326207 - Flags: review?(mark.finkle)
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.
Attachment #326207 - Flags: review?(mark.finkle) → review+
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 :/
(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])
Posted file browser_History.js (obsolete) —
Attachment #326207 - Attachment is obsolete: true
Attachment #328246 - Flags: review?(mark.finkle)
Attachment #328246 - Attachment mime type: application/x-javascript → text/plain
Posted patch fuelApplication.js (obsolete) — Splinter Review
Attachment #325441 - Attachment is obsolete: true
Attachment #329273 - Flags: review?(mark.finkle)
Posted file browser_History.js (obsolete) —
Attachment #328246 - Attachment is obsolete: true
Attachment #329274 - Flags: review?(mark.finkle)
Attachment #328246 - Flags: review?(mark.finkle)
Attachment #329274 - Attachment mime type: application/x-javascript → text/plain
Posted file browser_History.js (obsolete) —
Attachment #329274 - Attachment is obsolete: true
Attachment #330303 - Flags: review?(mark.finkle)
Attachment #329274 - Flags: review?(mark.finkle)
Comment on attachment 330303 [details]
browser_History.js

I think you attached the wrong file :)
Attachment #330303 - Flags: review?(mark.finkle) → review-
Posted file browser_History.js (obsolete) —
oops sorry about that
Attachment #330303 - Attachment is obsolete: true
Attachment #330343 - Flags: review?(mark.finkle)
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.
Attachment #330343 - Flags: review?(mark.finkle) → review-
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 :)
Attachment #329273 - Flags: review?(mark.finkle) → review+
Posted file browser_History.js (obsolete) —
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
Attachment #330343 - Attachment is obsolete: true
Attachment #330658 - Flags: review?(mark.finkle)
Posted patch fuelApplication.js (obsolete) — Splinter Review
Attachment #329273 - Attachment is obsolete: true
Attachment #330659 - Flags: review?(mark.finkle)
Posted patch fuelApplication.js (obsolete) — Splinter Review
the diff didn't work last time,
Attachment #330659 - Attachment is obsolete: true
Attachment #330695 - Flags: review?(mark.finkle)
Attachment #330659 - Flags: review?(mark.finkle)
Comment on attachment 330658 [details]
browser_History.js

Looks good
Attachment #330658 - Flags: review?(mark.finkle) → review+
Comment on attachment 330695 [details] [diff] [review]
fuelApplication.js

Also good
Attachment #330695 - Flags: review?(mark.finkle) → review+
Do I still need to do anything with those?
Attachment #292086 - Attachment is obsolete: true
Attachment #322599 - Flags: review?(gavin.sharp)
Comment on attachment 322599 [details] [diff] [review]
fuelIApplication.idl

Asking gavin for a second review before landing
Comment on attachment 330658 [details]
browser_History.js

Asking gavin for a second review before landing
Attachment #330658 - Flags: review?(gavin.sharp)
Comment on attachment 330695 [details] [diff] [review]
fuelApplication.js

Asking gavin for a second review before landing
Attachment #330695 - Flags: review?(gavin.sharp)
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
Attachment #345841 - Flags: review+
Attachment #322599 - Attachment is obsolete: true
Attachment #322599 - Flags: review?(gavin.sharp)
Attachment #330658 - Attachment is obsolete: true
Attachment #330658 - Flags: review?(gavin.sharp)
Attachment #330695 - Attachment is obsolete: true
Attachment #330695 - Flags: review?(gavin.sharp)
Attachment #345841 - Flags: review?(gavin.sharp)
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?
Attachment #345841 - Flags: review?(gavin.sharp)
status2.0: --- → ?
Hardware: x86 → All
Whiteboard: [patchlove]
Version: unspecified → Trunk
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.