726062 - B2G UA is wrong, better fix

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Andreas Gal :gal]

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é]

Attachment: 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 (he/him; ⌚ UTC+1/+2)]

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?)]

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é]

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]]

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]

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)]

(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]]

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)]

(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]]

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]]

Reviewed a long while ago, can this just land now?

Comment 12

[JP Rosevear [:jpr]]

Fabrice can we just dupe this to bug 761873 now?

Comment 13

[[:fabrice] Fabrice Desré]

(In reply to JP Rosevear [:jpr] from comment #12)
> Fabrice can we just dupe this to bug 761873 now?

sure, please do.

Comment 15

[Dão Gottwald [:dao]]

This still needs to be fixed.

Comment 16

[[:fabrice] Fabrice Desré]

Now that Bug 777710 landed, do we still need that?

Comment 17

[Dão Gottwald [:dao]]

The code touched in bug 725602 and referred to in comment 0 is still relevant.

Comment 18

[[:fabrice] Fabrice Desré]

Attachment: 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é]

https://hg.mozilla.org/integration/mozilla-inbound/rev/876519ac69eb

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/876519ac69eb