Closed
Bug 1072050
Opened 10 years ago
Closed 10 years ago
Add pref for setting device identifier in UA string
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: sicking, Assigned: ethan)
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
On FirefoxOS we have a lot of partners that set the UA string to include a device identifier. As much as we'd like that to stop, it's not going to stop until we've actually solved websites needs for doing UA-string detection, and that still hasn't been solved. So in the meantime we should add a pref specifically for this to ensure that it's done in a consistent and correct way. Ken, could you find someone on your platform teams that could do this?
Flags: needinfo?(kchang)
Reporter | ||
Comment 1•10 years ago
|
||
Gerv, Lawrence, can you provide information about specifically where in the UA-string that the device-model-identifier should go? And if we have any requirements on its syntax, like "no semicolons" that should be enforced in the code.
Comment 2•10 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #0) > On FirefoxOS we have a lot of partners that set the UA string to include a > device identifier. As much as we'd like that to stop, it's not going to stop > until we've actually solved websites needs for doing UA-string detection, > and that still hasn't been solved. > > So in the meantime we should add a pref specifically for this to ensure that > it's done in a consistent and correct way. > > Ken, could you find someone on your platform teams that could do this? Which UA string(Browser, MMS, RTSP, or..) are you talking about? But I think you are talking about all types of UA strings.
Flags: needinfo?(kchang)
Updated•10 years ago
|
Flags: needinfo?(kchang)
Reporter | ||
Comment 3•10 years ago
|
||
I was just talking about the value that we use when we add the "User-Agent" header to all HTTP requests. I don't *think* that affexts MMS or RTSP, but I don't know those protocols well, so I could be wrong.
Comment 4•10 years ago
|
||
Ethan, I remember that you had fixed the similar bug for RTSP. Please help this bug. Thanks. Let's talk with PM about this.
Assignee: nobody → ettseng
Flags: needinfo?(kchang)
Comment 5•10 years ago
|
||
Jonas: we have a UA override whitelist; we should make sure that all partners who are asking for this are made aware that if it's just for their own use or for a small set of sites, that is a better solution (in terms of web compat) than overriding the UA everywhere. The location and requirements for the device model, if it is to be added, are documented here: https://wiki.mozilla.org/B2G/User_Agent/Device_Model_Inclusion_Requirements The code could enforce the location in the string, plus requirement 1). Gerv
Comment 6•10 years ago
|
||
And yes, this should be for web HTTP only, not for MMS, RTSP or anything else. Gerv
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Ken Chang[:ken] from comment #4) > Ethan, I remember that you had fixed the similar bug for RTSP. Please help > this bug. Thanks. Let's talk with PM about this. In bug 1026923, we changed RTSP code to directly reuse the HTTP UA string (from nsIHttpProtocolHandler). Which means UA strings are the same in HTTP and RTSP.
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Gervase Markham [:gerv] from comment #5) > Jonas: we have a UA override whitelist; we should make sure that all > partners who are asking for this are made aware that if it's just for their > own use or for a small set of sites, that is a better solution (in terms of > web compat) than overriding the UA everywhere. > The location and requirements for the device model, if it is to be added, > are documented here: > https://wiki.mozilla.org/B2G/User_Agent/Device_Model_Inclusion_Requirements > The code could enforce the location in the string, plus requirement 1). > Gerv Thanks Gervase for this information. I will try to add a preference for the UA string first.
Reporter | ||
Comment 9•10 years ago
|
||
Gerv: The request we get from most partners aren't of the form "Website X doesn't work without sticking a device identifier in the UA string", but rather "all other browsers do this and we're worried an unknown number of websites will not work unless we do this". I dislike the device identifier in the UA string as much as you do. But after having talked to web developers I get the impression that removing it is starting at the wrong end. Just like removing the OS identifier or indeed the Gecko version identifier. We first need to provide them with usable alternatives.
Comment 10•10 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #9) > Gerv: The request we get from most partners aren't of the form "Website X > doesn't work without sticking a device identifier in the UA string", but > rather "all other browsers do this and we're worried an unknown number of > websites will not work unless we do this". When we looked at this, our compat testing showed exactly the opposite - websites break when you start doing this. Particularly those who made specific accommodation for the standard FxOS UA string. Of course, the situation may be different now; I haven't tested recently. Karl is the man to ask for the latest info. Gerv
Reporter | ||
Comment 11•10 years ago
|
||
Yes, I think there are many websites too that break. I suspect that that situation remains unchanged.
Comment 13•10 years ago
|
||
Comment on attachment 8498615 [details] [diff] [review] bug-1072050-wip.patch Not sure if b2g.useragent.device_id is the right pref. Other user agent prefs are in general.useragent.*. And if the code is going in netwerk/protocol/http/nsHttpHandler.cpp , that implies genericity. But then, perhaps we want to indicate to people that other OSes and browser builds definitely shouldn't set it... Gerv
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Gervase Markham [:gerv] from comment #13) > Comment on attachment 8498615 [details] [diff] [review] > bug-1072050-wip.patch Gerv, thanks for your feedback. :) > Not sure if b2g.useragent.device_id is the right pref. Other user agent > prefs are in general.useragent.*. I also noticed these preferences exist for packaged UA override list. And I wasn't sure if it's adequate to add new preference here. So, what about naming it as "general.useragent.device_id"? > And if the code is going in > netwerk/protocol/http/nsHttpHandler.cpp , that implies genericity. But then, > perhaps we want to indicate to people that other OSes and browser builds > definitely shouldn't set it... Understood. Do you have suggestion of where to specify the device ID for customized UA? Ethan
Comment 15•10 years ago
|
||
I think general.useragent.device_id is probably the right name. Also, remember, we want to enforce the restrictions on its content in code (no semicolons, no spaces, etc.) from https://wiki.mozilla.org/B2G/User_Agent/Device_Model_Inclusion_Requirements . So please add that. Gerv
Assignee | ||
Comment 16•10 years ago
|
||
1. Name the pref as "general.useragent.device_id". 2. Enforce syntax restriction to Device ID.
Attachment #8498615 -
Attachment is obsolete: true
Assignee | ||
Comment 17•10 years ago
|
||
Hi Gerv, Thanks for your reminder. Restrictions were added in the patch. In terms of genericity you mentioned in comment 13, the code of Device ID is wrapped in a "#if defined(ANDROID) || defined(MOZ_B2G)" block, do you think that's enough to indicate this code is not generic?
Flags: needinfo?(gerv)
Assignee | ||
Comment 19•10 years ago
|
||
(In reply to Gervase Markham [:gerv] from comment #18) > :ethan: I'm sure that's fine. Thanks! Could you help review the patch? Otherwise, do you know who can review it?
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 20•10 years ago
|
||
I'm not the guy to review this patch. Check who reviewed previous UA string-editing patches. Gerv
Comment 21•10 years ago
|
||
(In reply to Ethan Tseng [:ethan] from comment #17) > Hi Gerv, > > Thanks for your reminder. Restrictions were added in the patch. > > In terms of genericity you mentioned in comment 13, the code of Device ID is > wrapped in a > "#if defined(ANDROID) || defined(MOZ_B2G)" block, do you think that's enough > to indicate this code > is not generic? That will change Fennec's UA. Do we want that? Also, MOZ_B2G is true on b2g desktop builds, where this does not really make sense. I think you just want MOZ_WIDGET_GONK.
Comment 22•10 years ago
|
||
(In reply to Fabrice DesrΓ© [:fabrice] from comment #21) > That will change Fennec's UA. Do we want that? Surely only if someone sets the pref? If I'm wrong about that, then no, we certainly don't want that to happen. Gerv
Assignee | ||
Comment 23•10 years ago
|
||
Comment on attachment 8499984 [details] [diff] [review] bug-1072050-wip.patch Hi Kyle, Could you help to review my patch?
Attachment #8499984 -
Flags: review?(khuey)
Assignee | ||
Comment 24•10 years ago
|
||
Comment on attachment 8499984 [details] [diff] [review] bug-1072050-wip.patch Cancel the review because of Fabrice's suggestion in comment 21. I'll update the patch soon.
Attachment #8499984 -
Flags: review?(khuey)
Assignee | ||
Comment 25•10 years ago
|
||
Hi Kyle, Could you help to review this patch?
Attachment #8499984 -
Attachment is obsolete: true
Attachment #8504127 -
Flags: review?(khuey)
Comment on attachment 8504127 [details] [diff] [review] Add device ID in UA string Review of attachment 8504127 [details] [diff] [review]: ----------------------------------------------------------------- I assume you've verified that the user agent is unchanged with the default value of the pref. sr? to sicking to make sure this is what he wanted. ::: netwerk/protocol/http/nsHttpHandler.cpp @@ +710,5 @@ > > +#if defined(MOZ_WIDGET_GONK) > + // Device model identifier should be a simple token, which can be composed > + // of letters, numbers, hyphen ("-") and dot ("."). > + // Occurrence of any other character causes the identifier invalid. Any other character means the identifier is invalid and ignored. @@ +725,5 @@ > + } > + } > + if (valid) { > + mDeviceModelId = deviceId; > + } If the device model id is ignored we should print something to logcat.
Attachment #8504127 -
Flags: superreview?(jonas)
Attachment #8504127 -
Flags: review?(khuey)
Attachment #8504127 -
Flags: review+
Reporter | ||
Updated•10 years ago
|
Attachment #8504127 -
Flags: superreview?(jonas) → superreview+
Assignee | ||
Comment 27•10 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #26) > I assume you've verified that the user agent is unchanged with the default > value of the pref. Yes. Confirmed. :) > > + if (valid) { > > + mDeviceModelId = deviceId; > > + } > If the device model id is ignored we should print something to logcat. Sure. I'll add it.
Assignee | ||
Comment 28•10 years ago
|
||
Addressed issues in the review comment.
Attachment #8504127 -
Attachment is obsolete: true
Assignee | ||
Comment 29•10 years ago
|
||
Refresh comment "r=khuey, sr=sicking".
Attachment #8504540 -
Attachment is obsolete: true
Assignee | ||
Comment 30•10 years ago
|
||
The result of Treeherder: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c9d2520a6214
Keywords: checkin-needed
Comment 31•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa03ad915ea5
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fa03ad915ea5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•