Last Comment Bug 726062 - B2G UA is wrong, better fix
: B2G UA is wrong, better fix
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla17
Assigned To: [:fabrice] Fabrice Desré
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks: 725602
  Show dependency treegraph
 
Reported: 2012-02-10 10:13 PST by Andreas Gal :gal
Modified: 2012-08-21 06:30 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
patch (1.15 KB, patch)
2012-02-10 11:04 PST, [:fabrice] Fabrice Desré
brian: review+
gerv: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
patch (1.09 KB, patch)
2012-08-20 11:15 PDT, [:fabrice] Fabrice Desré
gal: review+
Details | Diff | Splinter Review

Description User image Andreas Gal :gal 2012-02-10 10:13:58 PST
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) {
    ...

?
Comment 1 User image [:fabrice] Fabrice Desré 2012-02-10 11:04:02 PST
Created attachment 596113 [details] [diff] [review]
patch

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.
Comment 2 User image :Ms2ger (⌚ UTC+1/+2) 2012-02-11 12:59:00 PST
Comment on attachment 596113 [details] [diff] [review]
patch

Looks good to me, but you want a peer, sorry.
Comment 3 User image Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-02-11 20:39:21 PST
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.
Comment 4 User image [:fabrice] Fabrice Desré 2012-02-11 20:45:22 PST
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 User image Gervase Markham [:gerv] 2012-02-14 02:57:53 PST
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
Comment 6 User image Andreas Gal :gal 2012-02-14 03:56:10 PST
When the first tablet is on the radar, we will definitely add a distinction.
Comment 7 User image Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-02-22 00:00:19 PST
(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 User image Gervase Markham [:gerv] 2012-02-22 02:28:28 PST
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 User image Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-02-22 02:36:44 PST
(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 User image Gervase Markham [:gerv] 2012-02-22 02:39:54 PST
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
Comment 11 User image JP Rosevear [:jpr] 2012-06-19 06:44:19 PDT
Reviewed a long while ago, can this just land now?
Comment 12 User image JP Rosevear [:jpr] 2012-06-19 13:16:59 PDT
Fabrice can we just dupe this to bug 761873 now?
Comment 13 User image [:fabrice] Fabrice Desré 2012-06-19 13:22:23 PDT
(In reply to JP Rosevear [:jpr] from comment #12)
> Fabrice can we just dupe this to bug 761873 now?

sure, please do.
Comment 14 User image JP Rosevear [:jpr] 2012-06-19 18:56:42 PDT

*** This bug has been marked as a duplicate of bug 761873 ***
Comment 15 User image Dão Gottwald [:dao] 2012-08-11 07:30:15 PDT
This still needs to be fixed.
Comment 16 User image [:fabrice] Fabrice Desré 2012-08-20 10:25:33 PDT
Now that Bug 777710 landed, do we still need that?
Comment 17 User image Dão Gottwald [:dao] 2012-08-20 10:45:08 PDT
The code touched in bug 725602 and referred to in comment 0 is still relevant.
Comment 18 User image [:fabrice] Fabrice Desré 2012-08-20 11:15:15 PDT
Created attachment 653445 [details] [diff] [review]
patch

Simple fix addressing comment #0. I don't think we want to do more in this bug anymore. If so, assign to someone else.
Comment 19 User image [:fabrice] Fabrice Desré 2012-08-20 12:25:56 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/876519ac69eb
Comment 20 User image Ed Morley [:emorley] 2012-08-21 06:30:11 PDT
https://hg.mozilla.org/mozilla-central/rev/876519ac69eb

Note You need to log in before you can comment on or make changes to this bug.