Closed
Bug 726062
Opened 12 years ago
Closed 12 years ago
B2G UA is wrong, better fix
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: gal, Assigned: fabrice)
References
Details
Attachments
(1 file, 1 obsolete file)
1.09 KB,
patch
|
gal
:
review+
|
Details | Diff | Splinter Review |
As Ms2ger points out this is the right way to fix this: bool isTablet; nsresult rv = infoService->GetPropertyAsBool(NS_LITERAL_STRING("tablet"), &isTablet); if (NS_SUCCEEDED(rv) && tablet) { ... ?
Reporter | ||
Updated•12 years ago
|
Assignee | ||
Comment 1•12 years ago
|
||
Since the "tablet" property is only set when MOZ_WIDGET_ANDROID is defined (se http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsSystemInfo.cpp#181), I special cased MOZ_WIDGET_GONK to always be "Mobile" for now.
Assignee: nobody → fabrice
Assignee | ||
Updated•12 years ago
|
Attachment #596113 -
Flags: review?(Ms2ger)
Comment 2•12 years ago
|
||
Comment on attachment 596113 [details] [diff] [review] patch Looks good to me, but you want a peer, sorry.
Attachment #596113 -
Flags: review?(Ms2ger) → feedback+
Reporter | ||
Updated•12 years ago
|
Attachment #596113 -
Flags: review?(bsmith)
Comment 3•12 years ago
|
||
Comment on attachment 596113 [details] [diff] [review] patch Review of attachment 596113 [details] [diff] [review]: ----------------------------------------------------------------- I do not understand why B2G avoids making a distinction between tablets and non-tablets like Mobile Firefox does. I would expect that B2G would try to be similar to Mobile Firefox as much as possible in this area, and use "Tablet" for (as-yet-mythical?) B2G tablets. So, if it were up to me, I would instead change "#ifdef MOZ_WIDGET_ANDROID" to "#if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GONK)" instead. If that is the wrong policy, then at least change the patch to be "#if defined(MOZ_WIDGET_GONK) ... #elif defined(MOZ_WIDGET_ANDROID)...#endif" to make it the code clearer. The policy around the UA string is a contentious issue (see dev-platform/dev-planning), which Networking has avoided getting involved in. AFAICT, Gerv is the right one to review what UA string to use when.
Attachment #596113 -
Flags: review?(gerv)
Attachment #596113 -
Flags: review?(bsmith)
Attachment #596113 -
Flags: review+
Assignee | ||
Comment 4•12 years ago
|
||
B2G currently has no support to detect whether it runs on a phone or tablet. I agree we should add it and support tablets like Mobile Firefox does. I can change "#ifdef MOZ_WIDGET_ANDROID" to "#if defined(MOZ_WIDGET_ANDROID) || defined(MOZ_WIDGET_GONK)" and ensure that isTablet return false now for gonk.
Comment 5•12 years ago
|
||
I agree with the strategy in comment #4. Mozilla feels that mobile and tablet are different enough to differentiate in the UA; B2G should go with that, so it needs the mechanism to detect which it is. Gerv
Reporter | ||
Comment 6•12 years ago
|
||
When the first tablet is on the radar, we will definitely add a distinction.
Comment 7•12 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #4) > B2G currently has no support to detect whether it runs on a phone or tablet. > I agree we should add it and support tablets like Mobile Firefox does. > > I can change "#ifdef MOZ_WIDGET_ANDROID" to "#if defined(MOZ_WIDGET_ANDROID) > || defined(MOZ_WIDGET_GONK)" and ensure that isTablet return false now for > gonk. This won't work for B2G's desktop build.
Comment 8•12 years ago
|
||
vingtetun: Surely we want B2G's desktop build to be getting Mobile content, because it's for testing? This patch still seems good to me for now. Gerv
Comment 9•12 years ago
|
||
(In reply to Gervase Markham [:gerv] from comment #8) > vingtetun: Surely we want B2G's desktop build to be getting Mobile content, > because it's for testing? Yes. We are building b2g as a standalone desktop application for testin/developing Gaia (the front-end part of b2g). And the backend in this case is not Gonk nor Android, but it could be GTK, or whatever depending on the platform you use.
Comment 10•12 years ago
|
||
Oh, I see. You are not saying we shouldn't send "Mobile", you are saying the criteria are wrong. Got it :-) BTW, can someone re-summarise this bug so it defines the exact problem? Gerv
Updated•12 years ago
|
Attachment #596113 -
Flags: review?(gerv) → review+
Comment 11•12 years ago
|
||
Reviewed a long while ago, can this just land now?
Updated•12 years ago
|
blocking-basecamp: --- → ?
Comment 12•12 years ago
|
||
Fabrice can we just dupe this to bug 761873 now?
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to JP Rosevear [:jpr] from comment #12) > Fabrice can we just dupe this to bug 761873 now? sure, please do.
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Updated•12 years ago
|
blocking-basecamp: ? → +
blocking-kilimanjaro: --- → +
Comment 15•12 years ago
|
||
This still needs to be fixed.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Updated•12 years ago
|
Attachment #596113 -
Attachment is obsolete: true
Assignee | ||
Comment 16•12 years ago
|
||
Now that Bug 777710 landed, do we still need that?
Comment 17•12 years ago
|
||
The code touched in bug 725602 and referred to in comment 0 is still relevant.
Assignee | ||
Comment 18•12 years ago
|
||
Simple fix addressing comment #0. I don't think we want to do more in this bug anymore. If so, assign to someone else.
Attachment #653445 -
Flags: review?(gal)
Reporter | ||
Updated•12 years ago
|
Attachment #653445 -
Flags: review?(gal) → review+
Updated•12 years ago
|
Component: General → Networking: HTTP
Product: Boot2Gecko → Core
Assignee | ||
Comment 19•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/876519ac69eb
Comment 20•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/876519ac69eb
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•