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é
:
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 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 [: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 :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 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 [: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 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 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 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 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 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 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 JP Rosevear [:jpr] 2012-06-19 06:44:19 PDT
Reviewed a long while ago, can this just land now?
Comment 12 JP Rosevear [:jpr] 2012-06-19 13:16:59 PDT
Fabrice can we just dupe this to bug 761873 now?
Comment 13 [: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 JP Rosevear [:jpr] 2012-06-19 18:56:42 PDT

*** This bug has been marked as a duplicate of bug 761873 ***
Comment 15 Dão Gottwald [:dao] 2012-08-11 07:30:15 PDT
This still needs to be fixed.
Comment 16 [:fabrice] Fabrice Desré 2012-08-20 10:25:33 PDT
Now that Bug 777710 landed, do we still need that?
Comment 17 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 [: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 [:fabrice] Fabrice Desré 2012-08-20 12:25:56 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/876519ac69eb
Comment 20 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.