Closed Bug 719717 Opened 12 years ago Closed 12 years ago

Move the tabs preferences to in-content UI

Categories

(Firefox :: Settings UI, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 15

People

(Reporter: jaws, Assigned: owenccarpenter)

References

Details

(Whiteboard: [testday-20120622])

Attachments

(1 file, 14 obsolete files)

7.31 KB, patch
Unfocused
: review+
jaws
: feedback+
Details | Diff | Splinter Review
As the first step of moving preferences to be within content, the Tabs preferences pane should be moved to an in-content page.

We'll want to keep the same organization of the preferences, but have them match the style of the Add-Ons Manager.

There is inContentUI.css, which tries to provide the common generic
styles for in-content UI like this:
https://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/inContentUI.css

It's currently used by about:addons, about:permissions, and the initial
Sync page. But about:addons is the only one of those pages that's really
complex - so there may be additional things that could move into
inContentUI.css to be shared.
Not sure which version of the addons manager is this, but in one of shorlander's mock-ups there are bread crumbs on top-left of the page, which probably will be removed in future versions since we don't want the page to have too many levels. So we should use pop-ups (if there's more than one level)when moving current content into an in-content page.
(In reply to Zhenshuo Fang (:fang) - Firefox UX Team from comment #1)
> Not sure which version of the addons manager is this, but in one of
> shorlander's mock-ups there are bread crumbs on top-left of the page, which
> probably will be removed in future versions since we don't want the page to
> have too many levels. So we should use pop-ups (if there's more than one
> level)when moving current content into an in-content page.

Is this understanding correct: "The current plan is that any modal dialogs accessed through the preferences will remain a modal dialog in this initial phase."?

If so, I agree that they should remain modal dialogs. Will the back-button be enough for users to reach the base page?
(In reply to Zhenshuo Fang (:fang) - Firefox UX Team from comment #1)
> in one of
> shorlander's mock-ups there are bread crumbs on top-left of the page, which
> probably will be removed in future versions

This is contrary to everything I've been told, and basically makes all the effort put into bug 660726 a waste of time...
For (In reply to Jared Wein [:jaws] from comment #2)
> Is this understanding correct: "The current plan is that any modal dialogs
> accessed through the preferences will remain a modal dialog in this initial
> phase."?
>
> If so, I agree that they should remain modal dialogs. Will the back-button
> be enough for users to reach the base page?

Yes. If it's currently a modal dialog, we don't have to change it into a bread crumb. And the back-button should be enough for now, just like how it works in the add-ons manager.


(In reply to Blair McBride (:Unfocused) from comment #3)
> 
> This is contrary to everything I've been told, and basically makes all the
> effort put into bug 660726 a waste of time...

Oops I didn't know this bug. But actually I think for the "Advanced" section, the bread crumbs make sense in this initial phase. What I was trying to say was we shouldn't take effort to make everything into a bread crumb system, because in the future we want to redesign Preferences so it's more flat and don't have too many levels.
I think the bread crumb system is clearly an advantage and could unify in-content pages. It's clear and simple and could be a way to navigate between the add-ons manager and the preferences for example.
I think one of the long term goal would be having all dialogs in-content (the best examples in browsers today is the "clear history" dialog from Google Chrome).
The javascript does not work, we are trying to figure that out now.
Attachment #591963 - Flags: feedback?(jwein)
Attachment #591963 - Flags: feedback?(bmcbride)
Comment on attachment 591963 [details] [diff] [review]
First Try In-Content UI tabs page

I've provided feedback for this patch over email. Thanks for requesting feedback!
Attachment #591963 - Flags: feedback?(jwein)
Attachment #591963 - Flags: feedback?(bmcbride)
Attached patch In Content tabs UI (obsolete) — Splinter Review
I was not able to make changes to the inContentUI.css file to format this file but I was able to import the files and apply the classes, but not make changes to it.  I added my own .css file to make it look better.
Attachment #593888 - Flags: feedback?(jwein)
Attachment #593888 - Flags: feedback?(bmcbride)
Comment on attachment 593888 [details] [diff] [review]
In Content tabs UI

Overall nit-picky comment: Please convert all tab characters to space characters and make sure that there is a blank newline at the end of files.

I will provide more feedback tomorrow when I have more time to do a proper feedback pass. Thanks for requesting feedback.
A couple more nits:
+.content-prefs{
+  margin-left: 150px;
+  margin-top: 25px;
+  margin-right:150px;
+  margin-bottom:300px;
Make this:
margin: 25px 150px 300px 150px;  
And why these very large margins? 
Please center things, or use 'flex' to proportionally size the spaces.

+  background-color: rgba(255, 255, 255, 0.35);
+  background-image: -moz-linear-gradient(top,
+                                         rgba(255, 255, 255, 0),
+                                         rgba(255, 255, 255, 0.75));
Use shorthand notation.

+.root {
Don't repeat any styling that is already in inContentUI.css

+.centered{
+	margin-left:100px;
+}
See above, this is not the way to center things.
Attachment #591963 - Attachment is obsolete: true
Comment on attachment 593888 [details] [diff] [review]
In Content tabs UI

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

Please read https://wiki.mozilla.org/DevTools/CSSTips for tips on how you should be writing your CSS code.

::: browser/components/about/AboutRedirector.cpp
@@ +106,5 @@
>      nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT |
>      nsIAboutModule::ALLOW_SCRIPT },
>    { "permissions", "chrome://browser/content/preferences/aboutPermissions.xul",
>      nsIAboutModule::ALLOW_SCRIPT },
> +  { "tabs", "chrome://browser/content/preferences/tabs_test.xul",

I was under the impression that we were going to use "preferences" as the name. If using "tabs" is just an intermediate step until a containing page is created, then this is OK for now. Either way, we should rename this file to tabs.xul, instead of tabs_test.xul.

::: browser/components/preferences/jar.mn
@@ +36,5 @@
>  *   content/browser/preferences/security.js
>  *   content/browser/preferences/selectBookmark.xul
>  *   content/browser/preferences/selectBookmark.js
> +*	content/browser/preferences/tabs_test.xul
> +*	content/browser/preferences/tabs_test.css

tabs.css here too, but only if this file is actually necessary.

::: browser/components/preferences/tabs_test.css
@@ +25,5 @@
> +                    url("chrome://global/skin/inContentUI/background-texture.png");
> +}
> +
> +.centered{
> +	margin-left:100px;

The .centered class is used to provide some extra left-margin on the checkboxes within the tabsPrefBox. I'm not sure if a style for this already exists in inContentUI.css, but if anything the name of this class shouldn't be |centered|. More importantly, this style and others within this CSS file will not work for right-to-left locales such as Hebrew and Arabic. Anytime that there are different margin-left and margin-right values, something is generally wrong. In place of using margin-left or margin-right, you should use -moz-margin-start and -moz-margin-end, respectively, as they will correctly adapt to right-to-left locales.

::: browser/components/preferences/tabs_test.xul
@@ +1,3 @@
> +<?xml version="1.0"?>
> +<?xml-stylesheet href="chrome://global/skin/"?>
> +<?xml-stylesheet href="chrome://browser/content/preferences/tabs_test.css" type="text/css"?>

Are you having trouble getting the CSS to load using the /global/skin/ path? The "/global/skin/" is path is created during compile-time based on the target platform (Windows, Linux, Mac OS X). Let's place the CSS file (only if the file is necessary) in /global/skin/preferences. To get files to show up in /global/skin/preferences you will have to place your CSS files in browser/themes/*stripe/preferences/ (winstripe is for Windows, pinstripe is for Mac OS X, and gnomestripe is for Linux).

@@ +11,5 @@
> +%tabsDTD;
> +]>
> +<page
> +    id="Tabs_Window"
> +    title="Tabs Prefences"

This title will need to be localizable, so make sure that the text is added to a DTD file (browser/locale/preferences/tabs.dtd) and referenced the same way as other locale-specific text is, for example |&scope.name;|. Also, I think you meant "Tabs Preferences" ;)
Attachment #593888 - Flags: feedback?(jwein)
(In reply to Jared Wein [:jaws] from comment #11)
> ::: browser/components/preferences/tabs_test.xul
> @@ +1,3 @@
> > +<?xml version="1.0"?>
> > +<?xml-stylesheet href="chrome://global/skin/"?>
> > +<?xml-stylesheet href="chrome://browser/content/preferences/tabs_test.css" type="text/css"?>
> 
> Are you having trouble getting the CSS to load using the /global/skin/ path?
> The "/global/skin/" is path is created during compile-time based on the
> target platform (Windows, Linux, Mac OS X). Let's place the CSS file (only
> if the file is necessary) in /global/skin/preferences. To get files to show
> up in /global/skin/preferences you will have to place your CSS files in
> browser/themes/*stripe/preferences/ (winstripe is for Windows, pinstripe is
> for Mac OS X, and gnomestripe is for Linux).

Sorry, I misspoke a little here. If you need to add a platform-independent CSS file, then you can follow Jon's approach in bug 723328. He has made it such that it is accessible via chrome://browser/content/preferences/.

If the changes are platform-dependent, such as using specific fonts to fit the look-and-feel of the host platform, then those changes should still exist in /browser/themes/*stripe/preferences, and the chrome URL will be chrome://browser/skin/prefrences/*.

Sorry for the confusion, let me know if I wasn't clear.
Jared,

Ok thank you very much for the feedback, I have solved the issue with the margins by using -moz-margin-start, however when I tried to test it by changing my preferred language to Hebrew my Tabs pages remained in English, even though other sites (google) were loading in Hebrew.  I don't know what I need to add in order to import languages into this page.

I think the problem I am having with the inContentUI.css come froma problem I am having when compiling.  I can apply the classes in the inContentUI.css file with no problems but I cannot make changes to the file.  When I compile the toolkit folder I get a fatal error saying "Cannot open include file: 'domstubs.h': no such file or directory".  I think this is why none of my changes are being reflected.

I have not submitted a new patch yet because stylistically this file will look exactly the same as my old file.  Jon and I have been able to get both of our files (Tabs and Privacy) into one file that is currently working but needs some style changes.  Do you want us to resubmit our files separately on each of our bugs or do you want us to submit 1 patch with both pages in one file?
(In reply to MSU Capstone Team from comment #13)
> Jared,
> 
> Ok thank you very much for the feedback, I have solved the issue with the
> margins by using -moz-margin-start, however when I tried to test it by
> changing my preferred language to Hebrew my Tabs pages remained in English,
> even though other sites (google) were loading in Hebrew.  I don't know what
> I need to add in order to import languages into this page.

Getting Hebrew added to your build environment probably won't help unless you can read Hebrew ;)

There is an add-on that you can install which will allow you to change Firefox to run in RTL mode. It is called Force RTL and can be found here: https://addons.mozilla.org/en-US/firefox/addon/force-rtl/

To use it, go to the Tools menu and choose Force RTL Direction (Alt+T -> Force RTL Direction). Do this again to revert back to LTR.

> I think the problem I am having with the inContentUI.css come froma problem
> I am having when compiling.  I can apply the classes in the inContentUI.css
> file with no problems but I cannot make changes to the file.  When I compile
> the toolkit folder I get a fatal error saying "Cannot open include file:
> 'domstubs.h': no such file or directory".  I think this is why none of my
> changes are being reflected.

Can you try pulling the latest source from mozilla-central and reconfiguring?

1) hg qpop -a
2) hg pull -u
3) cd /mozilla-central
4) autoconf-2.13
5) cd js/src
6) autoconf-2.13
7) cd ../../obj-dir
8) ../configure
9) python -O "../build/pymake/make.py" -s -j12
10) Grab a cup of tea and some crumpets since it may be a while ;)

> I have not submitted a new patch yet because stylistically this file will
> look exactly the same as my old file.  Jon and I have been able to get both
> of our files (Tabs and Privacy) into one file that is currently working but
> needs some style changes.  Do you want us to resubmit our files separately
> on each of our bugs or do you want us to submit 1 patch with both pages in
> one file?

I would prefer submitting the files to separately to each of the bugs. Once one of the bugs gets finished, the other patch can be updated to only include the necessary changes.
Ok, I pulled the latest source and reconfigured like you said and I was able to make changes to the inContentUI.css file, so I must've just had a bad build or something.  I also tested the Force RTL and it worked like it should have.  I will submit a patch this evening after class.
Attached patch Tabs with inContentUI.css (obsolete) — Splinter Review
Ok, so I was able to alter the inContentUI.css files and put in an "indent" class that handles the indentation using -moz-margin-start and works in both RTL and LTR situations.  I did not alter the main-content class of the inContentUI.css file so this file may not look the same as it will when implemented in the about:preferences page but the styling of the check boxes and background will be the same, only the inner window will be smaller.  As for the file names, that is just temporary as I did not want to save over the existing tabs.xul file.
Attachment #595581 - Flags: feedback?(jwein)
Attachment #595581 - Flags: feedback?(bmcbride)
Comment on attachment 595581 [details] [diff] [review]
Tabs with inContentUI.css

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

General comments:
* Still need to sort out tabs vs spaces (indents should always be by 2 spaces)
* Need to get on to making it so these prefs can be filtered (for search) in the future

::: browser/components/preferences/tabs_test.xul
@@ +1,3 @@
> +<?xml version="1.0"?>
> +<?xml-stylesheet href="chrome://global/skin/"?>
> +<?xml-stylesheet href="chrome://global/skin/inContentUI.css"?>

This should be loaded via @import in a platform-specific stylesheet for this page, like the previous patch version attempted to do. Reasoning for this is that inContentUI.css is an implementation detail of the theme (albeit, common to our built-in themes) - the page shouldn't need to know about it, and other themes shouldn't be forced to use it.

@@ +4,5 @@
> +
> +
> +
> +
> +<!DOCTYPE overlay [

The document element is <page>, so this DOCTYPE should be for page.

@@ +15,5 @@
> +    id="Tabs_Window"
> +    title="Tabs Prefences"
> +    orient="horizontal"
> +    xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +	xmlns:xhtml="http://www.w3.org/1999/xhtml">

For consistency, the namespace name should be "html" (ie, xmlns:html instead of xmlns:xhtml), even though the namespace is actually XHTML. inContentUI.css, for instance, uses "html" as the namespace name, and having it named two different things leads to confusion.

::: toolkit/themes/winstripe/global/inContentUI.css
@@ +115,5 @@
>    border: 1px solid #C3CEDF;
>    border-radius: 5px;
>  }
>  
> +*|*.indent{

Space before the "{" (same goes for pinstripe and gnomnestripe).

@@ +116,5 @@
>    border-radius: 5px;
>  }
>  
> +*|*.indent{
> +  -moz-margin-start:50px;

Space after the ":" (same goes for pinstripe and gnomnestripe).
Attachment #595581 - Flags: feedback?(bmcbride) → feedback+
Attachment #593888 - Attachment is obsolete: true
Attachment #593888 - Flags: feedback?(bmcbride)
Comment on attachment 595581 [details] [diff] [review]
Tabs with inContentUI.css

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

Basically what Blair said in his feedback pass as well as my recommendations from the previous feedback pass. Keep up the good work!
Attachment #595581 - Flags: feedback?(jwein) → feedback+
Attached patch Tabs importing inContentUI.css (obsolete) — Splinter Review
Ok I believe I have made all the changes you requested and tried to fix all my tabs to 2 spaces but I may have missed a couple.  I saw in your feedback for Jon's bug (Bug 723328) that we should remove todo items labelled <!--XXX-->, there is one in my file, should I remove it?

As far as the search capability goes, it seems that all the language for the preferences are stored in the .dtd files.  Would it be possible for us search these files for entities that match the users query or is that not a good way of going about it.
Attachment #595843 - Flags: feedback?(jwein)
Attachment #595843 - Flags: feedback?(bmcbride)
(In reply to MSU Capstone Team from comment #19)
> I saw in your feedback
> for Jon's bug (Bug 723328) that we should remove todo items labelled
> <!--XXX-->, there is one in my file, should I remove it?

No, that is actually a todo item (or, at least, an explanation of why a hack exists - that would presumably be fixed eventually). So it should stay.
(In reply to MSU Capstone Team from comment #19)
> As far as the search capability goes, it seems that all the language for the
> preferences are stored in the .dtd files.

Correct.

>  Would it be possible for us
> search these files for entities that match the users query

No.

The solution I imagine is to have each preference (or small group of inter-dependent preferences) be inside a container element (eg, a vbox). That container would have an attribute that contains all relevant strings (but of course isn't displayed anywhere). Then when searching, you just need to patch the search string against that attribute on all the container elements.
I have made a bit a research and I found that the new Chrome's Uber Page (landed in Chromium 19 at least for settings, extensions and in-content help/about Chrome) is apparently gonna unify all pages in one (Downloads, Bookmarks and History). It's a pretty interesting idea : in fact someone has already proposed such an approach for Firefox home tab here (I don't think doing for the home tab is the best, but it's already a start) : https://wiki.mozilla.org/Anti-minimalistic_redundant_app_button_and_home_tab
(In reply to Guillaume C. [:GE3K0S] from comment #22)
> I have made a bit a research and I found that the new Chrome's Uber Page
> (landed in Chromium 19 at least for settings, extensions and in-content
> help/about Chrome) is apparently gonna unify all pages in one (Downloads,
> Bookmarks and History). It's a pretty interesting idea : in fact someone has
> already proposed such an approach for Firefox home tab here (I don't think
> doing for the home tab is the best, but it's already a start) :
> https://wiki.mozilla.org/Anti-minimalistic_redundant_app_button_and_home_tab

Indeed - see bug 711157, which is going through review now.
Comment on attachment 595843 [details] [diff] [review]
Tabs importing inContentUI.css

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

(Ugh, forgot to press the submit button on Friday, sorry.)

Immediate next steps:
* Integrate this into the main page
* Prep for search functionality (comment 27)

::: browser/components/preferences/tabs_test.xul
@@ +21,5 @@
> +  <hbox flex="1" id="tabs-prefences" class="main-content">
> +		
> +    <prefpane id="paneTabs"
> +#ifdef XP_WIN
> +	  onpaneload="gTabsPane.init();"

I haven't explicitly tested, but this probably isn't working - see bug 724686 comment 7.
Attachment #595843 - Flags: feedback?(bmcbride) → feedback+
(In reply to Blair McBride (:Unfocused) from comment #21)
> The solution I imagine is to have each preference (or small group of
> inter-dependent preferences) be inside a container element (eg, a vbox).
> That container would have an attribute that contains all relevant strings
> (but of course isn't displayed anywhere). Then when searching, you just need
> to patch the search string against that attribute on all the container
> elements.

Forgot to mention: The sections would filter on a separate attribute, whose value is an ID for the section it's associated with. eg, category="general" or category="advanced-network"
Thanks for the feedback, I tested the functionality of the tabs page and had odd results.  When the page loads the correct settings are displayed but if changes are made they aren't saved.  However if I open the modal preferences box and make changes to the settings in the tabs pane, they will be shown on my in-content tabs page.  I made the change from onpaneload="gTabsPane.init();" to onload="gTabsPane.init();" but the functionality remains the same either way.  I don't understand why this is happening, I haven't explicitly tested this in OSX, but I believe Jon has used an old copy of my tabs pane that was working fine.  I will look into this more and discuss it tonight at our meeting.
Attachment #595843 - Flags: feedback?(jwein) → feedback+
Attached patch in-content tabs pane (obsolete) — Splinter Review
Patch is split of off the old privacy bug patch. Fixed xhtml issue and some indentations.
Attachment #603382 - Flags: feedback?(jwein)
Attachment #603382 - Flags: feedback?(bmcbride)
Comment on attachment 603382 [details] [diff] [review]
in-content tabs pane

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

::: browser/components/preferences/in-content/tabs.xul
@@ +26,5 @@
> +	</preferences>
> +
> +	<!-- XXX flex below is a hack because wrapping checkboxes don't reflow
> +			 properly; see bug 349098 -->
> +	<vbox id="tabPrefsBox" align="start" flex="1">

Please remove the <vbox> containing all the preferences and add either a <vbox> around each preference or an attribute to each <checkbox> to state what category the preference belongs to. This will be used for the search and menu navigation from the landing page.

::: browser/locales/en-US/chrome/browser/preferences/tabs.dtd
@@ +15,4 @@
>  
>  <!ENTITY showTabsInTaskbar.label          "Show tab previews in the Windows taskbar">
>  <!ENTITY showTabsInTaskbar.accesskey      "k">
> +<!ENTITY tabsHeader.label                 "Tabs">
\ No newline at end of file

Can we use &paneTabs.title; here? See http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/preferences.xul#104 for where it is already used.
Attachment #603382 - Flags: feedback?(jwein) → feedback+
Attachment #595581 - Attachment is obsolete: true
Attachment #595843 - Attachment is obsolete: true
Attached patch tabs in-content pane (obsolete) — Splinter Review
Fixed all issues addressed in Jared's feedback. I did not add the vbox's around the preference elements since I figured it would be better for Owen to do that in his patch since I don't know how he is going to want to wrap those.
Attachment #605537 - Flags: feedback?(jwein)
Attachment #605537 - Flags: feedback?(bmcbride)
Attachment #603382 - Attachment is obsolete: true
Attachment #603382 - Flags: feedback?(bmcbride)
Comment on attachment 605537 [details] [diff] [review]
tabs in-content pane

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

::: browser/components/preferences/in-content/preferences.xul
@@ +72,5 @@
>        src="chrome://browser/content/preferences/in-content/main.js"/>
>    <script type="application/javascript"
> +      src="chrome://browser/content/preferences/in-content/privacy.js"/>
> +  <script type="application/javascript"
> +     src="chrome://browser/content/preferences/in-content/tabs.js"/>

This should be added in tabs.xul

::: browser/components/preferences/in-content/tabs.js
@@ +66,5 @@
> +#ifdef XP_WIN
> +  /**
> +   * Initialize any platform-specific UI.
> +   */
> +  init: function () {

You're calling this function unconditionally in home.js, but the function will only exist on Windows (#ifdef XP_WIN).

@@ +68,5 @@
> +   * Initialize any platform-specific UI.
> +   */
> +  init: function () {
> +    const Cc = Components.classes;
> +    const Ci = Components.interfaces;

I've asked these to be included as global constants in bug 733473 - they can removed from here once that is done.

::: browser/components/preferences/in-content/tabs.xul
@@ +6,5 @@
> +    <preference id="browser.link.open_newwindow"     name="browser.link.open_newwindow"     type="int"/>
> +    <preference id="browser.tabs.autoHide"           name="browser.tabs.autoHide"           type="bool" inverted="true"/>
> +    <preference id="browser.tabs.loadInBackground"   name="browser.tabs.loadInBackground"   type="bool" inverted="true"/>
> +    <preference id="browser.tabs.warnOnClose"        name="browser.tabs.warnOnClose"        type="bool"/>
> +    <preference id="browser.tabs.warnOnOpen"         name="browser.tabs.warnOnOpen"         type="bool"/>

Make the indentation/line-wrapping consistant with the other patches.

@@ +18,5 @@
> +                        accesskey="&newWindowsAsTabs.accesskey;"
> +                        preference="browser.link.open_newwindow"
> +                        onsyncfrompreference="return gTabsPane.readLinkTarget();"
> +                        onsynctopreference="return gTabsPane.writeLinkTarget();"
> +                        class="indent"/>

The first character of wrapped lines should line up with the first character of the first attribute on the first line.

::: browser/locales/en-US/chrome/browser/preferences/tabs.dtd
@@ +14,4 @@
>  <!ENTITY switchToNewTabs.accesskey    "s">
>  
>  <!ENTITY showTabsInTaskbar.label          "Show tab previews in the Windows taskbar">
> +<!ENTITY showTabsInTaskbar.accesskey      "k">
\ No newline at end of file

You've removed a newline at the end of this file - we need that kept :) All sorts of tools complain if a file doesn't end in a newline.
Attachment #605537 - Flags: feedback?(bmcbride) → feedback+
Comment on attachment 605537 [details] [diff] [review]
tabs in-content pane

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

I'll clear my feedback request since Blair has provided a pretty thorough feedback pass already :)
Attachment #605537 - Flags: feedback?(jwein)
Attached patch tabs in-content pane (obsolete) — Splinter Review
fixed indentation, tabs init being called only for windows xp, and all other feedback
Attachment #605537 - Attachment is obsolete: true
Attachment #608470 - Flags: feedback?(jwein)
Attachment #608470 - Flags: feedback?(bmcbride)
Comment on attachment 608470 [details] [diff] [review]
tabs in-content pane

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

Blair, is this ready for review?

::: browser/components/preferences/in-content/jar.mn
@@ +4,5 @@
>  *  content/browser/preferences/in-content/preferences.xul
>  *  content/browser/preferences/in-content/main.xul
>  *  content/browser/preferences/in-content/main.js
> +*   content/browser/preferences/in-content/tabs.xul
> +*   content/browser/preferences/in-content/tabs.js

Extra space got added here.
Attachment #608470 - Flags: feedback?(jwein) → feedback+
Attached patch tabs in-content pane (obsolete) — Splinter Review
feedback fixed
Attachment #608470 - Attachment is obsolete: true
Attachment #608470 - Flags: feedback?(bmcbride)
Attachment #608506 - Flags: feedback?(jwein)
Attachment #608506 - Flags: feedback?(bmcbride)
Attachment #608506 - Flags: feedback?(jwein) → feedback+
Attached patch tabs in-content pane (obsolete) — Splinter Review
added consistent title
Attachment #608506 - Attachment is obsolete: true
Attachment #608506 - Flags: feedback?(bmcbride)
Attachment #609059 - Flags: feedback?(jwein)
Attachment #609059 - Flags: feedback?(bmcbride)
Comment on attachment 609059 [details] [diff] [review]
tabs in-content pane

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

No r+ yet, based on review comments in other patches that apply here. Namely the <vbox>s from bug 734013, and fixing up <header>.

::: browser/components/preferences/in-content/tabs.xul
@@ +2,5 @@
> +   - v. 2.0. If a copy of the MPL was not distributed with this file,
> +   - You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<script type="application/javascript"
> +  src="chrome://browser/content/preferences/in-content/tabs.js"/>

Indent wrapped line correctly.
Attachment #609059 - Flags: feedback?(bmcbride) → feedback+
Attached patch tabs in-content pane (obsolete) — Splinter Review
fixed feedback, included search vbox's, and fixed title
Attachment #609059 - Attachment is obsolete: true
Attachment #609059 - Flags: feedback?(jwein)
Attachment #609220 - Flags: feedback?(jwein)
Attachment #609220 - Flags: feedback?(bmcbride)
Comment on attachment 609220 [details] [diff] [review]
tabs in-content pane

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

::: browser/components/preferences/in-content/tabs.xul
@@ +9,5 @@
> +  <preference id="browser.link.open_newwindow"     name="browser.link.open_newwindow"     type="int"/>
> +  <preference id="browser.tabs.autoHide"           name="browser.tabs.autoHide"           type="bool" inverted="true"/>
> +  <preference id="browser.tabs.loadInBackground"   name="browser.tabs.loadInBackground"   type="bool" inverted="true"/>
> +  <preference id="browser.tabs.warnOnClose"        name="browser.tabs.warnOnClose"        type="bool"/>
> +  <preference id="browser.tabs.warnOnOpen"         name="browser.tabs.warnOnOpen"         type="bool"/>

nit: please fix this indentation.
Attachment #609220 - Flags: feedback?(jwein) → feedback+
Attached patch tabs in-content patch (obsolete) — Splinter Review
fixed nits from feedback
Attachment #609220 - Attachment is obsolete: true
Attachment #609220 - Flags: feedback?(bmcbride)
Attachment #610389 - Flags: feedback?(jwein)
Attachment #610389 - Flags: feedback?(bmcbride)
Attachment #610389 - Flags: feedback?(jwein) → feedback+
Comment on attachment 610389 [details] [diff] [review]
tabs in-content patch

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

Just one thing:

::: browser/components/preferences/in-content/tabs.xul
@@ +23,5 @@
> +  <preference id="browser.tabs.warnOnOpen"
> +              name="browser.tabs.warnOnOpen"
> +              type="bool"/>
> +#ifdef XP_WIN
> +  <preference id="browser.taskbar.previews.enable" name="browser.taskbar.previews.enable" type="bool"/>

You missed fixing up the indentation/line wrapping of this.
Attachment #610389 - Flags: feedback?(bmcbride) → feedback+
Attached patch tabs in-content patch (obsolete) — Splinter Review
removed vbox from heading, fixed blairs feedback, removed vbox around 1st child elements and added data-category and hidden to 1st child element instead
Attachment #610389 - Attachment is obsolete: true
Attachment #613228 - Flags: feedback?(jwein)
Attachment #613228 - Flags: feedback?(bmcbride)
Attached patch Tabs with updated data-category (obsolete) — Splinter Review
Attachment #613228 - Flags: feedback?(jwein) → feedback+
Attachment #613228 - Attachment is obsolete: true
Attachment #613228 - Flags: feedback?(bmcbride)
Attachment #614499 - Flags: feedback?(jwein)
Attachment #614499 - Flags: feedback?(bmcbride)
Attachment #614499 - Attachment is obsolete: true
Attachment #614499 - Flags: feedback?(jwein)
Attachment #614499 - Flags: feedback?(bmcbride)
Attachment #614531 - Flags: feedback?(jwein)
Attachment #614531 - Flags: feedback?(bmcbride)
Attachment #614531 - Flags: feedback?(jwein) → feedback+
Attachment #613672 - Attachment is obsolete: true
Attachment #614531 - Flags: feedback?(bmcbride) → review+
https://hg.mozilla.org/mozilla-central/rev/c14a9f3448da
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I verify fixed this bug, tested at Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120621 Firefox/15.0a2.
Status: RESOLVED → VERIFIED
Whiteboard: [testday-20120622]
You need to log in before you can comment on or make changes to this bug.