Add Places Bookmarks interfaces to SMILE

RESOLVED FIXED in seamonkey2.1b3

Status

SeaMonkey
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

Trunk
seamonkey2.1b3
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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
Attachment #523889 - Flags: review?(neil)

Comment 2

6 years ago
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?]
Attachment #523889 - Flags: review?(neil) → review+
(Assignee)

Comment 3

6 years ago
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
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b3
(Assignee)

Updated

6 years ago
Attachment #523954 - Attachment is patch: true
Attachment #523954 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

6 years ago
Blocks: 647654
(Assignee)

Comment 4

6 years ago
> [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.
Flags: in-testsuite+
Depends on: 758102
You need to log in before you can comment on or make changes to this bug.