397073 - Allow runtime overrides by OS version for chrome

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Mike Connor [:mconnor]]

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.

Comment 1

[Dave Townsend [:mossop]]

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.

Comment 2

[Benjamin Smedberg]

"leave Linux" means that if there is an OSVersion property, we shouldn't use the chrome registration line.

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 1

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.

Comment 4

[Dave Townsend [:mossop]]

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 5

[Benjamin Smedberg]

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.

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This addresses the previous comments and pulls the os name from nsIXULRuntime rather than defines. This allows a limited unit test of this feature.

Comment 7

[Dave Townsend [:mossop]]

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.

Comment 8

[Dave Townsend [:mossop]]

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

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch for checkin

This is the patch with the minor header changes ready for checkin

Comment 10

[Michael Ventnor]

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.

Comment 11

[Reed Loden [:reed]]

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

Comment 12

[Eric Shepherd [:sheppy]]

Added information about "os" and "osversion" to: http://developer.mozilla.org/en/docs/Chrome_Registration

Let me know if there's anything stupid there.

Comment 13

[Alex Faaborg [:faaborg] (Firefox UX)]

Note that we are now actually going to start using this over in bug 416531.