Last Comment Bug 647571 - Add Places Bookmarks interfaces to SMILE
: Add Places Bookmarks interfaces to SMILE
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b3
Assigned To: Philip Chee
:
Mentors:
Depends on: 758102
Blocks: 647654
  Show dependency treegraph
 
Reported: 2011-04-03 10:00 PDT by Philip Chee
Modified: 2012-06-01 03:17 PDT (History)
1 user (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 SMILEBookmarks. (32.65 KB, patch)
2011-04-03 10:17 PDT, Philip Chee
neil: review+
Details | Diff | Review
Patch v1.0a as checked in [Comment 3] (32.61 KB, patch)
2011-04-04 00:48 PDT, Philip Chee
no flags Details | Diff | Review

Description Philip Chee 2011-04-03 10:00:52 PDT
Now that we are using the Places Bookmarks implementation we should update SMILE to support those and bring us up to feature parity with FUEL.
Comment 1 Philip Chee 2011-04-03 10:17:13 PDT
Created attachment 523889 [details] [diff] [review]
Patch v1.0 SMILEBookmarks.

> -[scriptable, uuid(c9ba8f65-c936-4ac6-a859-8936832b0c12)]
> +[scriptable, uuid(bedc9ea5-5673-44c7-a0af-49f1d37a2ca1)]
>  interface smileIApplication : extIApplication
q.v. [Bug 628586] steelIApplication needs new iid due to changes in parent interface extIApplication.

> +++ b/suite/smile/src/smileApplication.js
> 
> -  var self = this;
> -  gShutdown.push(function() { self._shutdown(); });
> +  gShutdown.push(this._shutdown.bind(this));
I hope I've done the bind correctly.

> -    var self = this;
> -    this._tabbrowser.addEventListener(aType,
> -      this._cleanup[aType] = function(e){ self._event(e); },
> +    this._tabbrowser.tabContainer.addEventListener(aType,
Firefox uses tabContainer due to tabs on top.

> +      this._cleanup[aType] = function(e){ this._event(e); }.bind(this),
>        true);
I hope I've done the bind correctly.

>    _shutdown : function win_shutdown() {
>      for (var type in this._cleanup)
> -      this._tabbrowser.removeEventListener(type, this._cleanup[type], true);
> +      this._tabbrowser.tabContainer.removeEventListener(type, this._cleanup[type], true);
Future proof a bit for when we implement tabs on top/tabs in a toolbar.

>    // for nsIObserver
>    observe: function app_observe(aSubject, aTopic, aData) {
>      // Call the extApplication version of this function first
>      this.__proto__.__proto__.observe.call(this, aSubject, aTopic, aData);
>      if (aTopic == "xpcom-shutdown") {
> +      this._obs.removeObserver(this, "xpcom-shutdown");
q.v. [Bug 506471] FUEL should avoid putting itself in the app-startup category (r=mfinkle).

Passes all the tests in browser_Bookmarks.js
Comment 2 neil@parkwaycc.co.uk 2011-04-03 13:15:37 PDT
Comment on attachment 523889 [details] [diff] [review]
Patch v1.0 SMILEBookmarks.

>+      this._cleanup[aType] = function(e){ this._event(e); }.bind(this),
this._event.bind(this)

r=me with that fixed.

[In theory it's possible to avoid binding this._event N times by doing it once in the constructor. Followup bug perhaps?]
Comment 3 Philip Chee 2011-04-04 00:48:37 PDT
Created attachment 523954 [details] [diff] [review]
Patch v1.0a as checked in [Comment 3]

>>+      this._cleanup[aType] = function(e){ this._event(e); }.bind(this),
> this._event.bind(this)
> 
> r=me with that fixed.
>
Fixed.

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/99b127296682
Comment 4 Philip Chee 2011-04-04 00:57:07 PDT
> [In theory it's possible to avoid binding this._event N times by doing it once
> in the constructor. Followup bug perhaps?]

Filed Bug 647654.

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