There's not much in the Tabs category, it should be merged into the General category before in-content prefs are enabled by default (temporary until the reorganisation). That cuts down on the number of categories without making one of the categories too big.

Totally agree with this. Also, some options like "Open new windows in a new tab instead" and "Always show the tab bar" may be removed (as long as they are accessible from about:config).

I think I'll try this out.

Just checking if I've got this right -- this is asking for the content of the "Tabs" preference page to be moved to a subsection (fieldset) in the general tab, right? This will probably involve: - Merging tabs.js and tabs.xul into main.js/main.xul - Removing any references to this in any makefiles - If there are any tests involving tabs, fix them. In addition to that, should I make the relevant changes to the in-content folder as well?

I integrated tabs.* into main.*, and made the necessary changes to jar.mn and preferences.xul. I have tested it, and run most of the mochitests too. I haven't made changes to the in-content folder, as mentioned before, should I do that as well? (Also, sorry if I messed up with the r?, I'm not sure how this works. I just chose the top suggested reviewer)

From the description, it really sounds like you should only be making the changes to the in-content folder, not the parent.

(In reply to Josh Matthews [:jdm] from comment #5) > From the description, it really sounds like you should only be making the > changes to the in-content folder, not the parent. Done. I'm leaving both patches out there till we know what :Unfocused was asking for, feel free to obsolete. :)

Forgot to rm the old tabs files.

Comment on attachment 801249 [details] [diff] [review] Patch 1.2 (in-content) I don't actually know anything more than what I've read in this bug.

(In reply to Josh Matthews [:jdm] from comment #8) > Comment on attachment 801249 [details] [diff] [review] > Patch 1.2 (in-content) > > I don't actually know anything more than what I've read in this bug. Ah, sorry about that. I had feedback?d you as you're the one who suggested that the change be made to the in-content folder (so I assumed you knew more about it). Sorry :)

Comment on attachment 801249 [details] [diff] [review] Patch 1.2 (in-content) Review of attachment 801249 [details] [diff] [review]: ----------------------------------------------------------------- Yep, I had meant only the in-content preferences - that's the new one going forward. And I can review this :) Looking good! Basically done, just a couple of small things to clean up. ::: browser/components/preferences/in-content/landing.xul @@ -14,5 @@ > </button> > > - <button label="&paneTabs.title;" class="landingButton" > - oncommand="gotoPref('paneTabs');"> > - <image class="landingButton-icon" type="tabs"/> Also need to remove the related CSS rules in /browser/themes/*/preferences/in-content/preferences.css that set the image for this. ::: browser/components/preferences/in-content/main.js @@ +12,5 @@ > * Initialization of this. > */ > init: function () > { > + //this._pane = document.getElementById("paneMain"); Yea, safe to remove this - it's leftover from the dialog-based preferences. @@ +27,5 @@ > Components.classes["@mozilla.org/observer-service;1"] > .getService(Components.interfaces.nsIObserverService) > .notifyObservers(window, "main-pane-loaded", null); > + > + //Functionality for "Show tabs in taskbar" on Win XP Nit: The line above this has trailing whitespace. Ditto on line 494. @@ +492,5 @@ > + }, > + // TABS > + > + /* > + * Preferences: Nit: Since we don't actually have code to deal with these preferences in this file, may as well just put it in main.xul, right above the related <preference>s. ::: browser/components/preferences/in-content/main.xul @@ +190,5 @@ > </groupbox> > + > +<!-- Tab preferences --> > +<groupbox data-category="paneGeneral" hidden="true"> > + <caption label="&tabsGroup.label;"/> Nit: This block is indented using tabs, should only indent using spaces.

(In reply to Blair McBride [:Unfocused] from comment #10) > Yep, I had meant only the in-content preferences - that's the new one going > forward. And I can review this :) Alright, thanks. I'm still new to this review thing :) > Nit: Since we don't actually have code to deal with these preferences in > this file, may as well just put it in main.xul, right above the related > <preference>s. I made the change, though I'm not too sure of it. The comment was there in the old tabs.js file and it seems to be the norm across the other files as well.

(In reply to Manish Goregaokar [:manishearth] from comment #11) > I made the change, though I'm not too sure of it. The comment was there in > the old tabs.js file and it seems to be the norm across the other files as > well. Yea, I was just concerned about it having some context.

Comment on attachment 801369 [details] [diff] [review] Patch 1.3 Review of attachment 801369 [details] [diff] [review]: ----------------------------------------------------------------- Good to go!

Landed on the fx-team branch, which gets merged to mozilla-central almost daily: https://hg.mozilla.org/integration/fx-team/rev/df6621bc6442

Thanks! :D Should I mark this as resolved?

Should it be marked as a change that affects to add-ons? I have seen many tabs-related extensions that add its own entries to the tabs pane of the classic preferences window: at least a button for to open its own prefwindow. I don't know with the in-content about:preferences, though.

(In reply to Marco from comment #16) > Should it be marked as a change that affects to add-ons? I have seen many > tabs-related extensions that add its own entries to the tabs pane of the > classic preferences window: at least a button for to open its own > prefwindow. I don't know with the in-content about:preferences, though. I *think* that most extensions don't deal with in content yet (so they'll add the pref to the popup window), but I'm not sure.

(In reply to Marco from comment #16) > Should it be marked as a change that affects to add-ons? I have seen many > tabs-related extensions that add its own entries to the tabs pane of the > classic preferences window: at least a button for to open its own > prefwindow. I don't know with the in-content about:preferences, though. This will eventually affect add-ons, so I think we should add this keyword. Thanks.

(In reply to Manish Goregaokar [:manishearth] from comment #15) > Thanks! :D Should I mark this as resolved? Bugs don't get resolved until they reach mozilla-central - soon as that happens, whoever merged the trees will mark the relevant bugs as RESOLVED FIXED.

Ah, I see, thanks :)

1) The respective new Tabs groupbox needs a id, or to overlay it (again, extensions) will be hard, i think. Need that a followup bug? 2) Maybe I should have asked here too, how it affects to bug 565550.

No UI-review? Tsk tsk!

Comment on attachment 801369 [details] [diff] [review] Patch 1.3 There's one minor alignment issue, I filed bug 917057 Otherwise looks great!

Is this the first case where the in content preferences are diverging from in-content preferences?

(In reply to Mike Kaply (:mkaply) from comment #25) > Is this the first case where the in content preferences are diverging from > in-content preferences? Yes.

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.8.5 using latest Nightly 34.0a1 (buildID: 20140729030202).