767313 - Merge the Tabs category into the General category in in-content preferences

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 1

[Siddhartha Dugar [:sdrocking]]

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).

Comment 2

[Manish Goregaokar [:manishearth]]

I think I'll try this out.

Comment 3

[Manish Goregaokar [:manishearth]]

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?

Comment 4

[Manish Goregaokar [:manishearth]]

Attachment: Patch 0.1

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)

Comment 5

[Josh Matthews [:jdm]]

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

Comment 6

[Manish Goregaokar [:manishearth]]

Attachment: Patch 1.1 (in-content)

(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. :)

Comment 7

[Manish Goregaokar [:manishearth]]

Attachment: Patch 1.2 (in-content)

Forgot to rm the old tabs files.

Comment 8

[Josh Matthews [:jdm]]

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.

Comment 9

[Manish Goregaokar [:manishearth]]

(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 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 11

[Manish Goregaokar [:manishearth]]

Attachment: Patch 1.3

(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.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 801369 [details] [diff] [review]
Patch 1.3

Review of attachment 801369 [details] [diff] [review]:
-----------------------------------------------------------------

Good to go!

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 15

[Manish Goregaokar [:manishearth]]

Thanks! :D Should I mark this as resolved?

Comment 16

[u412933]

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.

Comment 17

[Manish Goregaokar [:manishearth]]

(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.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df6621bc6442

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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.

Comment 21

[Manish Goregaokar [:manishearth]]

Ah, I see, thanks :)

Comment 22

[u412933]

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.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

No UI-review? Tsk tsk!

Comment 24

[Zhenshuo Fang (:fang) - Firefox UX Team]

Comment on attachment 801369 [details] [diff] [review]
Patch 1.3

There's one minor alignment issue, I filed bug 917057 
Otherwise looks great!

Comment 25

[Mike Kaply [:mkaply]]

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

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 27

[Camelia Badau [:cbadau], Desktop Test Engineering]

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