Last Comment Bug 315346 - nsISidebar should be moved to dom/
: nsISidebar should be moved to dom/
Status: RESOLVED FIXED
: fixed1.8.1
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
: Hixie (not reading bugmail)
:
Mentors:
Depends on:
Blocks: 320370 321459
  Show dependency treegraph
 
Reported: 2005-11-06 18:54 PST by Robert O'Callahan (:roc) (email my personal email if necessary)
Modified: 2006-05-02 00:34 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
simple fix (33.09 KB, patch)
2005-11-06 18:56 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
jst: review+
jst: superreview+
Details | Diff | Splinter Review
cvs remove browser/components/sidebar/public and xpfe/components/sidebar/public (18.82 KB, patch)
2005-12-30 04:58 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
benjamin: review+
roc: superreview+
jst: approval‑branch‑1.8.1+
Details | Diff | Splinter Review
patch w/nsISidebar changes for branch (28.34 KB, patch)
2006-05-01 18:44 PDT, Myk Melez [:myk] [@mykmelez]
roc: review+
jst: superreview+
jst: approval‑branch‑1.8.1+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (email my personal email if necessary) 2005-11-06 18:54:12 PST
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.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-11-06 18:56:03 PST
Created attachment 202056 [details] [diff] [review]
simple fix

This patch does that.
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2005-11-07 14:21:01 PST
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...
Comment 3 Darin Fisher 2005-11-07 14:31:41 PST
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.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-05 17:30:10 PST
checked in.
Comment 5 Christian :Biesinger (don't email me, ping me on IRC) 2005-12-16 04:18:42 PST
shouldn't nsISidebar be removed from the old places?
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-12-16 14:10:43 PST
Yeah
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-12-25 17:41:12 PST
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?
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-12-30 04:58:20 PST
Created attachment 207174 [details] [diff] [review]
cvs remove browser/components/sidebar/public and xpfe/components/sidebar/public
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-01-08 15:05:51 PST
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 10 Myk Melez [:myk] [@mykmelez] 2006-04-19 12:06:33 PDT
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?
Comment 11 Myk Melez [:myk] [@mykmelez] 2006-05-01 18:44:17 PDT
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.
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2006-05-01 22:29:30 PDT
Comment on attachment 220467 [details] [diff] [review]
patch w/nsISidebar changes for branch

sr+a=jst
Comment 13 Myk Melez [:myk] [@mykmelez] 2006-05-01 23:00:42 PDT
Landed attachment 220467 [details] [diff] [review] on the branch.

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