Redistributors of Firefox should be able to add their vendor name and versions of distributions to the user agent string, but Firefox currently steals vendor and vendorSub. I talked to dbaron and we came up with either adding a vendor2 and vendorSub2 or creating app and appSub for Firefox to use. I prefer the latter to keep consistency across the apps.

This is the patch I added to our firefox RPMs.

Comment on attachment 168878 [details] [diff] [review] Patch >@@ -99,12 +99,22 @@ interface nsIHttpProtocolHandler : nsIPr Rev UUID? >@@ -520,12 +522,14 @@ nsHttpHandler::BuildUserAgent() > mOscpu.Length() + > mLanguage.Length() + > mMisc.Length() + > mProduct.Length() + > mProductSub.Length() + > mProductComment.Length() + >+ mApp.Length() + >+ mAppSub.Length() + > mVendor.Length() + > mVendorSub.Length() + > mVendorComment.Length() + > 22); 24? One other thing: I didn't realize when I was talking to you that the Mozilla and 5.0 were currently called mAppName and mAppVersion. Adding something called mApp and mAppSub seems a little confusing, but maybe it's OK.

(In reply to comment #2) > (From update of attachment 168878 [details] [diff] [review] [edit]) > >@@ -99,12 +99,22 @@ interface nsIHttpProtocolHandler : nsIPr > > Rev UUID? Yeah, should do that. :-\ > > >@@ -520,12 +522,14 @@ nsHttpHandler::BuildUserAgent() > > mOscpu.Length() + > > mLanguage.Length() + > > mMisc.Length() + > > mProduct.Length() + > > mProductSub.Length() + > > mProductComment.Length() + > >+ mApp.Length() + > >+ mAppSub.Length() + > > mVendor.Length() + > > mVendorSub.Length() + > > mVendorComment.Length() + > > 22); > > 24? Oops, right. Good catch. > One other thing: I didn't realize when I was talking to you that the Mozilla > and 5.0 were currently called mAppName and mAppVersion. Adding something > called mApp and mAppSub seems a little confusing, but maybe it's OK. Couldn't think of anything else at the time. product and productSub are already taken by Gecko/BUILDID. Maybe prog/progSub? ::shrug:: Other suggestions welcome.

Comment on attachment 168878 [details] [diff] [review] Patch can these values not be gotten via prefs? i'm not happy to see nsIHttpProtocolHandler changing. it is one of the only ways for an embedder or extension author to find out the Gecko BuildID at runtime. i know of at least one embedder who is forced to use it.

So in a discussion with darin, he pointed out the idea of using enumeration of a tree of preferences to solve this problem. This allows enumeration of an arbitrary number of comments. I suspect the cleanest way to do this would be to get rid of the vendor and vendorSub prefs and add a comment.* tree of prefs, such that Firefox would set the pref: pref("general.useragent.comment.firefox", "Firefox/@APP_VERSION@"); and Fedora could set the pref pref("general.useragent.comment.fedora", "Fedora Core/3"); etc. (Does nsIPrefBranch and the default branch stuff mean this also provides a way to add the string programmatically by adding to the default branch, which would allow an extension to add only when it's being used?)

Seems like a good idea. Does order matter? Firefox potentially can be in the middle of several items. > (Does nsIPrefBranch and the default branch stuff mean this also provides a way > to add the string programmatically by adding to the default branch, which would > allow an extension to add only when it's being used?) Yep, that can be done. I'll work on getting something hacked up.

We should probably be consistent about the order (i.e., given a set of prefs we should always produce the same output), but I don't think it should matter.

NS_STRINGIFY didn't work and probably wasn't tested. I'm not sure whether I want to use .extra. or .vendor., but .extra. seems more generic and also doesn't have any potential issues with having both .vendor and .vendor.* . I probably need to test the app changes here a bit more (like making sure they all compile).

oops, wrong patch

Comment on attachment 179890 [details] [diff] [review] patch >Index: netwerk/protocol/http/src/nsHttpHandler.h >Index: calendar/sunbird/app/Makefile.in > BUILD_ID = $(shell cat $(DEPTH)/config/build_number) > DEFINES += -DBUILD_ID=\"$(BUILD_ID)\" I removed these two lines as well, since I'm using NS_STRINGIFY now, and BUILD_ID is global.

Comment on attachment 179890 [details] [diff] [review] patch >Index: netwerk/protocol/http/src/nsHttpHandler.cpp >+ if (!mExtraUA.IsEmpty()) { >+ mUserAgent += mExtraUA; >+ } nit pick: this code avoids brackets when possible >+ if (MULTI_PREF_CHANGED(UA_PREF("extra."))) { >+ mExtraUA.Truncate(); >+ >+ // Unfortunately, we can't do this using the pref branch. >+ nsCOMPtr<nsIPrefService> service = >+ do_GetService(NS_PREFSERVICE_CONTRACTID); So, you should be able to get back to the nsIPrefService using QueryInterface from the nsIPrefBranch passed to this function. That might be slightly better. >+ PRUint32 extraCount; >+ char **extraItems; >+ branch->GetChildList("", &extraCount, &extraItems); >+ for (char **item = extraItems, **item_end = extraItems + extraCount; >+ item < item_end; ++item) { >+ nsXPIDLCString valStr; >+ branch->GetCharPref(*item, getter_Copies(valStr)); >+ if (!valStr.IsEmpty()) { >+ mExtraUA += NS_LITERAL_CSTRING(" ") + valStr; >+ } nit: remove extra brackets. It looks like extraItems is leaked. also, it's interesting that the order in which these extra values are appended to the UA string is sort of arbitrary, or are we relying on some sorting done by the preferences code? i think it might be nice to ensure that these are always listed in a consistent order even though people shouldn't really care. >Index: xpcom/base/nscore.h >+#define NS_STRINGIFY_HELPER(x_) #x_ >+#define NS_STRINGIFY(x_) NS_STRINGIFY_HELPER(x_) So, I'm curious... why is this needed? >Index: browser/app/profile/firefox.js >+pref("general.useragent.extra.firefox", "Firefox/@APP_VERSION@"); We should make the same change to mozilla/xulrunner/examples/simple/simple-prefs.js before too many people start copying the old firefox way. >Index: mail/app/Makefile.in >+ifdef MOZ_JPROF >+LIBS += -ljprof >+endif this change is unrelated right? r=darin with these issues addressed

Did some version of Fedora Core really ship with those changes to nsIHttpProtocolHandler? /me hopes not...

(In reply to comment #11) > So, you should be able to get back to the nsIPrefService using > QueryInterface from the nsIPrefBranch passed to this function. > That might be slightly better. I tried that first and got a null pointer. > >Index: xpcom/base/nscore.h > > >+#define NS_STRINGIFY_HELPER(x_) #x_ > >+#define NS_STRINGIFY(x_) NS_STRINGIFY_HELPER(x_) > > So, I'm curious... why is this needed? Currently NS_STRINGIFY(BUILD_ID) produces "BUILD_ID".

> I tried that first and got a null pointer. Wow. That's interesting. It probably has to do with when we get nsIPrefBranch via the Observe callback. > > So, I'm curious... why is this needed? > > Currently NS_STRINGIFY(BUILD_ID) produces "BUILD_ID". Yikes. That has surely caused interesting problems for Firefox nightlies :-(

This should address darin's comments. It does a sort by pref name since the prefs code looks like it just uses hashtable enumeration order. I haven't actually tested this and may not be able to for a few hours since XRemote doesn't differentiate between profiles anymore, but it does compile. Do you mind doing r+sr as well?

I didn't have as many browser windows open as I thought, so tested now.

Comment on attachment 179919 [details] [diff] [review] patch r+sr=darin

Comment on attachment 179919 [details] [diff] [review] patch This makes user-agent addition much easier for distributors and reduces the chance that they'll make binary-incomptabile changes to the HTTP code.

Comment on attachment 179919 [details] [diff] [review] patch a=mkaply

So, actually, my calendar checkout was way out of date, since I wasn't pulling it by default, and once I fixed that the calendar changes looked just like the other app changes...

Fix checked in to trunk, 2005-04-07 11:11 -0700. I'll try to document this on http://www.mozilla.org/build/revised-user-agent-strings.html shortly.

Apparently, this change broke Sunbird (at least on Windows XP). I filed bug 289786. See bug 289786 comment 0 and bug 289786 comment 1 for more information. Should this bug be reopened or do we want to fix this in bug 289786?

This has broken at least one well-known detection script (http://www.webreference.com/tools/browser/javascript.html) - and will probably break many more that make the same assumptions (looking at navigator.vendor for the string "Firefox"). Would it be possible to still make navigator.vendor return "Firefox"?

Firefox is not and never was a vendor.

(And any sniffing script that distinguishes Firefox from other Geckos is seriously broken anyway.)

not to mention that this checkin should not have any visible effects to web developers...

(nevermind my last comment. instead, see comment 24)

(In reply to comment #25) > (And any sniffing script that distinguishes Firefox from other Geckos is > seriously broken anyway.) Not exactly. There are some cases where it does not mean bad code. For instance, I have a version of IE Skin for Mozilla, Firefox and Netscape. All three are available on my web site. But if you click on, say, a link for the theme for Netscape using Firefox, you get an alert saying "This theme requires Netscape". This prevents the user from getting a failed-to-install error. Besides, this does not stop the user from downloading them anyway. bamm.gabriana.com/ieskin.html