nsISidebar should be moved to dom/

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

({fixed1.8.1})

Trunk
fixed1.8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

The nsISidebar interface defines a public-facing DOM interface (window.sidebar). It doesn't make sense to have it forked in browser/ and xpfe/ since if they diverge, we'd break compatibility with Web sites.

Furthermore, XULrunner embedders might want to implement this interface. Ephiphany does, for example. Rather than copy it for every embedder, the right thing to do is move it somewhere that's part of XULRunner. I claim dom/ is the right place.
Created attachment 202056 [details] [diff] [review]
simple fix

This patch does that.
Attachment #202056 - Flags: superreview?(jst)
Attachment #202056 - Flags: review?(jst)
Comment on attachment 202056 [details] [diff] [review]
simple fix

r+sr=jst, but the nsIPermission changes are unrealted to the nsISidebar changes here, right? If not, you should get darin to approve that before landing that change...

Updated

12 years ago
Attachment #202056 - Flags: superreview?(jst)
Attachment #202056 - Flags: superreview+
Attachment #202056 - Flags: review?(jst)
Attachment #202056 - Flags: review+

Comment 3

12 years ago
Please get some cvs admin to copy the file in the repository to preserve cvs history :)

jst: the nsIPermission thing is covered in a separate bug.
checked in.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
shouldn't nsISidebar be removed from the old places?
Yeah

Updated

12 years ago
Blocks: 320370
This caused bug 320370 and bug 321459 since the installer manifests weren't updated correctly. For some, dom_sidebar.xpt was added but sidebar.xpt wasn't removed (shouldn't it be?), and for others dom_sidebar.xpt wasn't added at all.

Seems as though the old files still need to be removed, too. Is http://gavinsharp.com/tmp/tmp.diff correct?
Blocks: 321459
Created attachment 207174 [details] [diff] [review]
cvs remove browser/components/sidebar/public and xpfe/components/sidebar/public
Attachment #207174 - Flags: superreview?(roc)
Attachment #207174 - Flags: review?(benjamin)
Attachment #207174 - Flags: review?(benjamin) → review+
Attachment #207174 - Flags: superreview?(roc) → superreview+
Landed attachment 207174 [details] [diff] [review] on the trunk.

mozilla/allmakefiles.sh; new revision: 1.599;
mozilla/browser/components/sidebar/public/Makefile.in; delete;
mozilla/browser/components/sidebar/public/nsISidebar.idl; delete;
mozilla/xpfe/components/sidebar/public/.cvsignore; delete;
mozilla/xpfe/components/sidebar/public/Makefile.in; delete;
mozilla/xpfe/components/sidebar/public/nsISidebar.idl; delete;
Comment on attachment 207174 [details] [diff] [review]
cvs remove browser/components/sidebar/public and xpfe/components/sidebar/public

Shouldn't this be on the branch too, given that mozilla/browser/ changes are supposed to be synced between trunk and branch?
Attachment #207174 - Flags: approval-branch-1.8.1?(peterv)
Attachment #207174 - Flags: approval-branch-1.8.1?(peterv) → approval-branch-1.8.1?(jst)

Updated

11 years ago
Attachment #207174 - Flags: approval-branch-1.8.1?(jst) → approval-branch-1.8.1+
Created attachment 220467 [details] [diff] [review]
patch w/nsISidebar changes for branch

Hmm, looks like the previous patches don't apply cleanly to the branch.  Here's a patch containing the nsISidebar changes for the branch.
Attachment #220467 - Flags: superreview?(jst)
Attachment #220467 - Flags: review?(roc)
Attachment #220467 - Flags: approval-branch-1.8.1?(jst)
Attachment #220467 - Flags: review?(roc) → review+
Comment on attachment 220467 [details] [diff] [review]
patch w/nsISidebar changes for branch

sr+a=jst
Attachment #220467 - Flags: superreview?(jst)
Attachment #220467 - Flags: superreview+
Attachment #220467 - Flags: approval-branch-1.8.1?(jst)
Attachment #220467 - Flags: approval-branch-1.8.1+
Landed attachment 220467 [details] [diff] [review] on the branch.

Updated

11 years ago
Keywords: fixed1.8.1
You need to log in before you can comment on or make changes to this bug.