Closed Bug 993344 Opened 7 years ago Closed 7 years ago

The "Show tab previews in the Windows taskbar" option doesn't appear in Tabs section for in-content preferences

Categories

(Firefox :: Preferences, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- affected
firefox30 --- affected
firefox31 --- verified

People

(Reporter: cbadau, Assigned: Paenglab)

References

Details

Attachments

(1 file, 2 obsolete files)

Reproducible on the latest Beta (BuildID:20140407135746)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20130205 Firefox/29.0
Reproducible on the latest Aurora (BuildID:20140407004002)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20130205 Firefox/30.0
Reproducible on the latest Nightly (BuildID: 20140407030203) 
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0

Steps to reproduce: 
1. Open about:preferences. 
2. Go to General tab, on Tabs section.
3. Observe the options displayed in this section. 

Actual results: The "Show tab previews in the Windows taskbar" option (which is visible under Tools->Options->Tabs) doesn't appear in Tabs section. 

Expected results: All the options displayed under Tools -> Options -> Tabs appear in Tabs section (from General tab).

Notes: This issue is not a regression.
OS: All → Windows 7
Hardware: All → x86
Hardware: x86 → x86_64
Blocks: 738796
Attached patch inContentShowTab.patch (obsolete) — Splinter Review
Made the version selector similar to the prefwindow's selector.
http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/preferences/tabs.js#41

Also changed the misleading comment from Win XP to Windows 7 and up and added the prefs name with description.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8404071 - Flags: review?(mak77)
Comment on attachment 8404071 [details] [diff] [review]
inContentShowTab.patch

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

the fix looks ok, the surrounding may get some additional fixes. I'll take a second look at the fixed patch.
Thanks.

::: browser/components/preferences/in-content/main.js
@@ +22,5 @@
>                .getService(Components.interfaces.nsIObserverService)
>                .notifyObservers(window, "main-pane-loaded", null);
>  
> +#ifdef XP_WIN
> +    //Functionality for "Show tabs in taskbar" on Windows 7 and up

please add space after // and end comment with a period, like:

// Show "Show tabs in taskbar" option on Windows 7 and up.

@@ +23,5 @@
>                .notifyObservers(window, "main-pane-loaded", null);
>  
> +#ifdef XP_WIN
> +    //Functionality for "Show tabs in taskbar" on Windows 7 and up
> +    

trailing spaces here

@@ +26,5 @@
> +    //Functionality for "Show tabs in taskbar" on Windows 7 and up
> +    
> +    /* browser.taskbar.previews.enable
> +     * - true if tabs are to be shown in the Windows 7 taskbar
> +     */

This comment should be moved below, after // TABS, check what we do in case of DOWNLOADS, just down the same file:

// DOWNLOADS

/*
 * Preferences:
 *
 * browser.download.useDownloadDir - bool
...

you should basically do the same for // TABS, bonus points if you also add documentation for the other TABS options

@@ +33,5 @@
>        let sysInfo = Cc["@mozilla.org/system-info;1"].
>                      getService(Ci.nsIPropertyBag2);
>        let ver = parseFloat(sysInfo.getProperty("version"));
>        let showTabsInTaskbar = document.getElementById("showTabsInTaskbar");
> +      showTabsInTaskbar.hidden = ver < 6.1;

The fix looks ok, the bug was introduced when tabs options were merged back into General, someone forgot to remove the history.state check.
Attachment #8404071 - Flags: review?(mak77) → feedback+
Attached patch inContentShowTab.patch (obsolete) — Splinter Review
Addressed comments.
Attachment #8404071 - Attachment is obsolete: true
Attachment #8404610 - Flags: review?(mak77)
Comment on attachment 8404610 [details] [diff] [review]
inContentShowTab.patch

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

Just some wording may be improved. r=me with that.

::: browser/components/preferences/in-content/main.js
@@ +442,5 @@
>        option.removeAttribute("disabled");
>        startupPref.updateElements(); // select the correct index in the startup menulist
>      }
>    },
>    // TABS

please add a newline before // TABS

@@ +448,5 @@
> +  /*
> +   * Preferences:
> +   *
> +   * browser.link.open_newwindow - int
> +   *   Determines where pages which would open in a new window are opened.

Determines where links targeting new windows should open.

@@ +450,5 @@
> +   *
> +   * browser.link.open_newwindow - int
> +   *   Determines where pages which would open in a new window are opened.
> +   *   Values:
> +   *     1 - Opens such links in the most recent window or tab.

Open in the current window or tab.

@@ +451,5 @@
> +   * browser.link.open_newwindow - int
> +   *   Determines where pages which would open in a new window are opened.
> +   *   Values:
> +   *     1 - Opens such links in the most recent window or tab.
> +   *     2 - Opens such links in a new window.

Open in a new window.

@@ +452,5 @@
> +   *   Determines where pages which would open in a new window are opened.
> +   *   Values:
> +   *     1 - Opens such links in the most recent window or tab.
> +   *     2 - Opens such links in a new window.
> +   *     3 - Opens such links in a new tab.

Open in a new tab in the most recent window.

@@ +455,5 @@
> +   *     2 - Opens such links in a new window.
> +   *     3 - Opens such links in a new tab.
> +   * browser.tabs.loadInBackground - bool
> +   *   True - If display should switch to a new tab which has been opened from
> +   *          a link, false if display shouldn't switch.

Whether browser should switch to a new tab opened from a link.

@@ +458,5 @@
> +   *   True - If display should switch to a new tab which has been opened from
> +   *          a link, false if display shouldn't switch.
> +   * browser.tabs.warnOnClose - bool
> +   *  True - If when closing a window with multiple tabs the user is warned and
> +   *         allowed to cancel the action, false to just close the window.

Whether the user should be warned when trying to close a window with multiple tabs, allowing to cancel the action.

@@ +461,5 @@
> +   *  True - If when closing a window with multiple tabs the user is warned and
> +   *         allowed to cancel the action, false to just close the window.
> +   * browser.tabs.warnOnOpen - bool
> +   *  True - If the user should be warned if he attempts to open a lot of tabs
> +   *         at once (e.g. a large folder of bookmarks).

Whether the user should be warned when trying to open a lot of tabs at once (e.g. a large folder of bookmarks), allowing to cancel the action.
Attachment #8404610 - Flags: review?(mak77) → review+
Patch for check-in addressed the comments.
Attachment #8404610 - Attachment is obsolete: true
Attachment #8404958 - Flags: review+
Keywords: checkin-needed
(In reply to Camelia Badau, QA [:cbadau] from comment #0)
> Notes: This issue is not a regression.

Based on the patch, this looks like a regression from bug 767313? The patch in that bug changed between bug 767313 comment 4 and bug 767313 comment 6 to include the code removed in this patch, without explanation. I wonder if Manish remembers why...
https://hg.mozilla.org/mozilla-central/rev/33cb06129f7b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Verified fixed on Windows 7 x86_64 using Nightly 31.0a1 (buildID: 20140428030203) and the latest Nightly 32.0a1 (buildID: 20140501030202).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.