Closed Bug 339964 Opened 14 years ago Closed 13 years ago

move tabbrowser.xml out of mozilla/toolkit and into mozilla/browser

Categories

(Firefox :: Tabbed Browser, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3 alpha8

People

(Reporter: moco, Assigned: rflint)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 3 obsolete files)

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
(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.
That should be fixed too.  <notificationbox/> handled a lot of that, I think.
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.
Blocks: 294342, 327886
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?
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.
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.
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.
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.
CC:ing mscott and bienvenu as tabmail.xml is afaik the only app in the tree that will be affected.
(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
Attached file New version of cvscopies (obsolete) —
Attachment #271450 - Attachment is obsolete: true
Attached patch Move tabbrowser (obsolete) — Splinter Review
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.
Target Milestone: --- → Firefox 3 M8
Attached file 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.
Attachment #272988 - Attachment is obsolete: true
Attachment #277106 - Flags: review?(mconnor)
Comment on attachment 277106 [details]
CVS Copies

let's do this thing
Attachment #277106 - Flags: review?(mconnor) → review+
Attached patch PatchSplinter Review
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).
Assignee: nobody → ryan
Attachment #272991 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #277758 - Flags: review?(mconnor)
Comment on attachment 277758 [details] [diff] [review]
Patch

r=me, but man this is bloaty and needs to die
Attachment #277758 - Flags: review?(mconnor) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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.
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'?
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.
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".
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.
Blocks: 468835
You need to log in before you can comment on or make changes to this bug.