Closed Bug 315346 Opened 15 years ago Closed 15 years ago

nsISidebar should be moved to dom/

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

(Keywords: fixed1.8.1)

Attachments

(3 files)

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.
Attached patch simple fixSplinter Review
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...
Attachment #202056 - Flags: superreview?(jst)
Attachment #202056 - Flags: superreview+
Attachment #202056 - Flags: review?(jst)
Attachment #202056 - Flags: review+
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
Closed: 15 years ago
Resolution: --- → FIXED
shouldn't nsISidebar be removed from the old places?
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
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)
Attachment #207174 - Flags: approval-branch-1.8.1?(jst) → approval-branch-1.8.1+
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+
Keywords: fixed1.8.1
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.