Closed Bug 397073 Opened 13 years ago Closed 13 years ago

Allow runtime overrides by OS version for chrome

Categories

(Toolkit :: Startup and Profile System, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: mconnor, Assigned: mossop)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

We need a way to, at runtime, apply overrides for chrome URLs, such that we can replace, say, Toolbar.png with Toolbar-Vista.png if we're running Windows Vista. Not sure how much this matters for Linux, or how we'd do versioning there.  10.4/10.5 would also be cool on Mac, though its not clear if we'll actually do anything ourselves with it.
Whiteboard: [wanted-firefox3]
From IRC, something akin to:

style chrome://browser/content/ chrome://browser/skin/browser-vista.css OS=WINNT OSVersion>=5.2

Compare against windows version on windows, OSX version on Mac, leave Linux for the time being.
Assignee: dtownsend → nobody
Component: General → XRE Startup
OS: Mac OS X → All
Product: Firefox → Toolkit
QA Contact: general → xre.startup
Hardware: PC → All
"leave Linux" means that if there is an OSVersion property, we shouldn't use the chrome registration line.
Assignee: nobody → dtownsend
Target Milestone: --- → mozilla1.9 M10
Attached patch patch rev 1 (obsolete) — Splinter Review
This adds os and osversion flags to the manifest checks.

os is case insensitively compared against OS_TARGET, so linux, winnt, darwin, ...

osversion is the major and minor version for OSX (10.4 for Tiger, 10.5 for Leopard, previous versions not supported) and Windows (see the table at http://msdn2.microsoft.com/en-us/library/ms724834.aspx). On other operating systems the existence of an osversion flag makes us ignore the manifest line. We can easily add an appropriate version to check against for different OS targets.

The flags are available to all types of manifest declarations.
Attachment #282713 - Flags: review?(benjamin)
Just spotted that we can set a string as being void. Maybe that might be preferable to represent the case where we have no valid osversion?
Comment on attachment 282713 [details] [diff] [review]
patch rev 1

>Index: chrome/src/Makefile.in

>+DEFINES += -DOS_TARGET=\"$(OS_TARGET)\"

Hrm... I know that OS_ARCH is WINNT... is OS_TARGET guaranteed to be WINNT also? They both represent the target OS, oddly enough, and I keep forgetting what the subtle differences between them are.

>Index: chrome/src/nsChromeRegistry.cpp

>+#if defined(XP_WIN)
>+#include "windows.h"
>+#elif defined(XP_MACOSX)
>+#include "gestalt.h"
>+#endif

Nit use #include <brackets> for system headers.

>+ * @param aValue The value that is expected. If this is empty then no
>+ *               comparison will match.

I'm ambivalent about the empty/null question.

>+  nsAutoString osVersion;
>+#if defined(XP_WIN)
>+  OSVERSIONINFO info = { sizeof(OSVERSIONINFO) };
>+  if (GetVersionEx(&info)) {
>+    char *buf = PR_smprintf("%ld.%ld", info.dwMajorVersion, info.dwMinorVersion);
>+    if (buf) {
>+      CopyUTF8toUTF16(buf, osVersion);
>+      PR_smprintf_free(buf);
>+    }
>+  }

Use nsTextFormatter to printf directly into a nsAString instead of doing the smprintf/copy step here and in the mac case.
Attachment #282713 - Flags: review?(benjamin) → review-
Attached patch patch rev 2 (obsolete) — Splinter Review
This addresses the previous comments and pulls the os name from nsIXULRuntime rather than defines. This allows a limited unit test of this feature.
Attachment #282713 - Attachment is obsolete: true
Attachment #284604 - Flags: review?(benjamin)
Attachment #284604 - Flags: review?(benjamin) → review+
Just as a note I will adjust this patch ever so slightly based on the bug 400089 comment 21 which does the same getting of the OSX version number. They are only minor changes though so I will not be re-requesting review.
Whiteboard: [wanted-firefox3] → [wanted-firefox3][has patch][has reviews]
Whiteboard: [wanted-firefox3][has patch][has reviews] → [wanted-firefox3][has patch][has reviews][needs approval]
Comment on attachment 284604 [details] [diff] [review]
patch rev 2

Seeking approval to land for M9. This enables per-OS version theming for the app and allows extensions to better control their functionality based on the OS. Would be good for this to make the beta. Has basic unit tests, doesn't affect content and I think is low risk
Attachment #284604 - Flags: approval1.9?
Status: NEW → ASSIGNED
This is the patch with the minor header changes ready for checkin
Attachment #284604 - Attachment is obsolete: true
Attachment #285479 - Flags: review+
Attachment #285479 - Flags: approval1.9?
Attachment #284604 - Flags: approval1.9?
Flags: wanted1.9+
Whiteboard: [wanted-firefox3][has patch][has reviews][needs approval] → [has patch][has reviews][needs approval]
Attachment #285479 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Whiteboard: [has patch][has reviews][needs approval] → [has patch][has reviews]
For Linux we could probably check against the GTK version library, since GTK is the library that provides native widgets and theming, and almost every time it matches the version number of the entire GNOME environment.

I can do this very easily as a follow up if it is desired.
Checking in chrome/src/nsChromeRegistry.cpp;
/cvsroot/mozilla/chrome/src/nsChromeRegistry.cpp,v  <--  nsChromeRegistry.cpp
new revision: 1.357; previous revision: 1.356
done
RCS file: /cvsroot/mozilla/chrome/test/unit/test_bug397073.js,v
done
Checking in chrome/test/unit/test_bug397073.js;
/cvsroot/mozilla/chrome/test/unit/test_bug397073.js,v  <--  test_bug397073.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/chrome/test/unit/data/test_bug397073.manifest,v
done
Checking in chrome/test/unit/data/test_bug397073.manifest;
/cvsroot/mozilla/chrome/test/unit/data/test_bug397073.manifest,v  <--  test_bug397073.manifest
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Flags: in-testsuite+
Whiteboard: [has patch][has reviews]
Blocks: 405368
Added information about "os" and "osversion" to: http://developer.mozilla.org/en/docs/Chrome_Registration

Let me know if there's anything stupid there.
Note that we are now actually going to start using this over in bug 416531.
Component: XRE Startup → Startup and Profile System
QA Contact: xre.startup → startup
You need to log in before you can comment on or make changes to this bug.