Last Comment Bug 339964 - move tabbrowser.xml out of mozilla/toolkit and into mozilla/browser
: move tabbrowser.xml out of mozilla/toolkit and into mozilla/browser
Status: RESOLVED FIXED
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: x86 All
: -- normal with 1 vote (vote)
: Firefox 3 alpha8
Assigned To: Ryan Flint [:rflint] (ping via IRC for reviews)
:
Mentors:
http://steelgryphon.com/blog/?p=74
Depends on: 392989
Blocks: 294342 327886 387345 393361 468835
  Show dependency treegraph
 
Reported: 2006-06-01 08:25 PDT by (not reading, please use seth@sspitzer.org instead)
Modified: 2008-12-10 06:47 PST (History)
28 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
cvscopies for moving tabbrowser.xml to mozilla/browser (6.50 KB, text/plain)
2007-07-08 13:54 PDT, Stefan [:stefanh]
no flags Details
New version of cvscopies (6.30 KB, text/plain)
2007-07-19 10:41 PDT, Stefan [:stefanh]
no flags Details
Move tabbrowser (357.66 KB, patch)
2007-07-19 10:57 PDT, Stefan [:stefanh]
no flags Details | Diff | Splinter Review
CVS Copies (6.34 KB, text/plain)
2007-08-17 10:28 PDT, Ryan Flint [:rflint] (ping via IRC for reviews)
mconnor: review+
Details
Patch (76.29 KB, patch)
2007-08-22 12:07 PDT, Ryan Flint [:rflint] (ping via IRC for reviews)
mconnor: review+
Details | Diff | Splinter Review

Description (not reading, please use seth@sspitzer.org instead) 2006-06-01 08:25:11 PDT
move tabbrowser.xml out of mozilla/toolkit and into mozilla/browser

see mconnor's blog post http://steelgryphon.com/blog/?p=74 and the comments for why this is a good idea.

see also https://bugzilla.mozilla.org/show_bug.cgi?id=339036#c8
Comment 1 neil@parkwaycc.co.uk 2006-06-01 08:39:24 PDT
(In reply to comment #0)
>see also https://bugzilla.mozilla.org/show_bug.cgi?id=339036#c8
It's not that easy... tabbrowser.xml and browser.xml are also linked.
Comment 2 Mike Connor [:mconnor] 2006-06-01 09:33:23 PDT
That should be fixed too.  <notificationbox/> handled a lot of that, I think.
Comment 3 John Gaunt (redfive) 2006-12-28 15:18:23 PST
I strongly support having some implementation of tabbrowser in the toolkit, however I also think that the code that has, over time, moved from the browser to the tabbrowser should move back so you can get all the full functionality of the browser without having to have tabs (I think this is the case, I remember looking into that a while back).

From a Songbird perspective we are working on adding tabbed browsing to the application and are taking the approach of extending the current binding for tabbrowser and there have been a much smaller number of changes needed to get it working than I would have thought. I was able to get it working with additional XUL widgets by extending, overriding the content and a couple of methods.

If anything I think the tabbrowser should be re-thought and factored to enable people to extend it or create new widgets that use it (by adding children content for instance or moving some content declarations from tabbrowser into a tabbrowser-tab-panel binding). There is SO MUCH stuff in the binding (3000 lines thank you very much) that deals with the bookkeeping of managing multiple tabs (and it's crucial stuff); it seems wasteful to me to force anyone who wants to implement tabbed browsing to force them to write all that code again.
Comment 4 Ian McKellar 2007-04-10 17:56:16 PDT
I've been hacking up tabbrowser to work in Songbird. There weren't that many changes that I needed to make, but enough that I had to make a fork of tabbrowser.xml. Now I'm finding that tabbrowser changes are occurring (see bug 370742 for example) that break my tabbrowser.

We took the approach of having a base tabbrowser.xml which is almost identical to the one in toolkit and sb-tabbrowser.xml which represents all the songbird-specific behavior and content. Would Mozilla be likely to accept patches to make the tabbrowser.xml in toolkit more generic?
Comment 5 Mike Connor [:mconnor] 2007-04-14 21:42:12 PDT
I don't think that moving tabbrowser out of toolkit requires anyone to rewrite from scratch, though it does require maintaining forked code.  Refactoring to be more generic is one path, I'm not sure its the best one, but in the absence of a clear path forward, i'm somewhat open to the idea of things being more generic and extending as a minimal step.
Comment 6 Mike Connor [:mconnor] 2007-04-14 21:46:31 PDT
All that said, I kinda want to gut tabbrowser at some point, since it seems to be fairly bloat-heavy and slow (minimo did their own impl to work around significant perf issues from the current tabbrowser).  That is much harder in toolkit than in browser, since in toolkit there's a promise of some reusability and API stability for consumers.
Comment 7 Mike Connor [:mconnor] 2007-07-05 13:08:41 PDT
Is there a set of "core" tab tasks needed regardless of what one's doing?  Having thought about this a fair bit lately, I don't think that tabbrowser itself is truly a "core" widget, and we should move it to /browser ASAP, and move some subset of items into tabbox based on "generically useful to tab users."  Tabbed browsing, as it stands, is not a generic task, and really isn't well-suited to the toolkit.
Comment 8 Stefan [:stefanh] 2007-07-08 13:54:59 PDT
Created attachment 271450 [details]
cvscopies for moving tabbrowser.xml to mozilla/browser

Those are the files that should be copied over to browser/. I'm just attaching it here for reference until I've tested it. The first 7 lines are the most interesting (the rest are icons etc). If there's a decision on doing this, I could go ahead and proceed. Next step would then be to produce a patch with all the relevant changes. I figured the whole process could be done in two steps.

1) Get cvs copying done
2) Land the (yet to come) patch

Note that this will affect mail/base/content/tabmail.xml, which I think should be sorted out before we're moving on here.
Comment 9 Stefan [:stefanh] 2007-07-08 13:59:30 PDT
CC:ing mscott and bienvenu as tabmail.xml is afaik the only app in the tree that will be affected.
Comment 10 Stefan [:stefanh] 2007-07-08 14:01:02 PDT
(In reply to comment #9)
> CC:ing mscott and bienvenu as tabmail.xml is afaik the only app in the tree
> that will be affected.
> 

..as thunderbird is the only app in the tree that will be affected
Comment 11 Stefan [:stefanh] 2007-07-19 10:41:38 PDT
Created attachment 272988 [details]
New version of cvscopies
Comment 12 Stefan [:stefanh] 2007-07-19 10:57:37 PDT
Created attachment 272991 [details] [diff] [review]
Move tabbrowser

OK, here's the patch that does the move... Ryan tested an earlier version and found some errors. This is an updated version. Originally, I added browser/themes/*stripe/browser/tabbrowser/tabbrowser.css (which was a copy of the toolkit/themes/*stripe/global/browser.css), this patch adds all the tabbrowser style rules to the existing browser.css in browser/themes/*stripe/browser instead. Not sure if that is better, though.

The patch should apply without any problems. It doesn't move the images - but those are listed in attachment #272988 [details].

I'll hand this over to Ryan, since I'm currently not able to build. Note that there will probably be some substantial changes to tabbrowser style rules in global/browser.css when bug 387345 is resolved (I think Ryan will fix that one before the move), so this isn't a final patch.

Note also that the patch is not tested on thunderbird and will most likely break thunderbird's tabmail.
Comment 13 Ryan Flint [:rflint] (ping via IRC for reviews) 2007-08-17 10:28:29 PDT
Created attachment 277106 [details]
CVS Copies

I'll get an updated patch up once we've copied these files over as a couple of the bindings need to be modified once they're in their new locations.
Comment 14 Mike Connor [:mconnor] 2007-08-20 19:54:59 PDT
Comment on attachment 277106 [details]
CVS Copies

let's do this thing
Comment 15 Ryan Flint [:rflint] (ping via IRC for reviews) 2007-08-22 12:07:27 PDT
Created attachment 277758 [details] [diff] [review]
Patch

To make this easier to review, nothing but a move is being done here. There is a bit of style duplication, so the incoming rules from toolkit have been placed so they'll be overridden by the existing browser stuff (I'll clean these issues up as dependencies of 387345).
Comment 16 Mike Connor [:mconnor] 2007-08-22 12:19:33 PDT
Comment on attachment 277758 [details] [diff] [review]
Patch

r=me, but man this is bloaty and needs to die
Comment 17 Ryan Flint [:rflint] (ping via IRC for reviews) 2007-08-22 15:31:29 PDT
mozilla/netwerk/test/jarlist.dat 1.10 
mozilla/browser/base/content/browser.css 1.34 
mozilla/browser/base/content/tabbrowser.xml 1.237 
mozilla/browser/themes/pinstripe/browser/browser.css 1.65
mozilla/browser/themes/pinstripe/browser/jar.mn 1.46
mozilla/browser/themes/winstripe/browser/tabbrowser/tabbrowserBindings.xml 1.12 
mozilla/browser/base/jar.mn 1.114 
mozilla/browser/themes/pinstripe/browser/tabbrowser/tabbrowserBindings.xml 1.21 
mozilla/browser/locales/jar.mn 1.70
mozilla/browser/themes/winstripe/browser/browser.css 1.76 
mozilla/browser/themes/winstripe/browser/jar.mn 1.41 
mozilla/toolkit/content/jar.mn 1.32 
mozilla/toolkit/content/xul.css 1.99 
mozilla/toolkit/locales/jar.mn 1.39 
mozilla/toolkit/themes/pinstripe/global/jar.mn 1.34 
mozilla/toolkit/themes/winstripe/global/globalBindings.xml 1.12 
mozilla/toolkit/themes/winstripe/global/jar.mn 1.34

Leaving open to cvs remove the left over bits once the tree's a bit greener.
Comment 18 Scott MacGregor 2007-08-23 18:20:04 PDT
FYI: I've forked the tabbrowser files into thunderbird to fix tabs in 
thunderbird. The patch is in Bug 393312. It sure is a lot of forked code that's now duplicated in tbird and fx. I wonder if we can't re-factor some of it back into toolkit to make it a lot easier to maintain.
Comment 19 Ian McKellar 2007-08-23 18:31:24 PDT
I'm also maintaining a forked tabbrowser in Songbird too :-(

I'd love to collaborate on a shared tabbrowser for toolkit. What I've done is maintain a slightly hacked firefox tabbrowser.xml (removing some of the more explicit firefox dependencies) and then extended it (in sb-tabbrowser.xml) to implement Songbird specific behavior.

Should we have a bug for 'put tabbrowser.xml back into mozilla/toolkit'?
Comment 20 Mike Connor [:mconnor] 2007-10-01 12:04:15 PDT
we're going to make the Firefox tabbrowser pretty much app-specific, to explicitly avoid the overhead of extending the widget, which is showing to be pretty non-trivial.  If someone wants to build a better tabbrowser-base, that'd be cool by me, in general, but the Firefox one is going to stay put.
Comment 21 Nickolay_Ponomarev 2007-11-07 01:18:13 PST
Added this to:
http://developer.mozilla.org/en/docs/Firefox_3_for_developers
http://developer.mozilla.org/en/docs/Updating_extensions_for_Firefox_3
...and updated the pages in the XUL reference to mention that <tabbrowser> is not available to general XUL apps and extensions.

Please don't forget to mark changes that affect developers as needing documentation updates by adding the keyword "dev-doc-needed".
Comment 22 Richard 2008-03-04 17:10:13 PST
After reading all the comments on this bug and the comments on this blog:
http://steelgryphon.com/blog/?p=74
I got the impression that the consensus was that it was good to move tabbrowser out of toolkit and in its place have a more generic tabbrowser-base in toolkit. It seems like the second part of this just disappeared. If the decision to go ahead with large changes like this is made elsewhere it would make sense to post the summary of that process back here. If someone has started work on a generic tabbrowser could they please post a link here. Thanks.

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