B2G UA is wrong, better fix

RESOLVED FIXED in mozilla17

Status

()

Core
Networking: HTTP
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gal, Assigned: fabrice)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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

6 years ago
Blocks: 725602
No longer depends on: 725602
Summary: B2G UA is wrong → B2G UA is wrong, better fix
(Assignee)

Comment 1

6 years ago
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.
Assignee: nobody → fabrice
(Assignee)

Updated

6 years ago
Attachment #596113 - Flags: review?(Ms2ger)
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

6 years ago
Attachment #596113 - Flags: review?(bsmith)
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

6 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.
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

6 years ago
When the first tablet is on the radar, we will definitely add a distinction.
(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.
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
(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.
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
Attachment #596113 - Flags: review?(gerv) → review+

Comment 11

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

Updated

5 years ago
blocking-basecamp: --- → ?

Comment 12

5 years ago
Fabrice can we just dupe this to bug 761873 now?
(Assignee)

Comment 13

5 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

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 761873
blocking-basecamp: ? → +
blocking-kilimanjaro: --- → +
This still needs to be fixed.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---

Updated

5 years ago
Attachment #596113 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
Now that Bug 777710 landed, do we still need that?
The code touched in bug 725602 and referred to in comment 0 is still relevant.
(Assignee)

Comment 18

5 years ago
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.
Attachment #653445 - Flags: review?(gal)
(Reporter)

Updated

5 years ago
Attachment #653445 - Flags: review?(gal) → review+

Updated

5 years ago
Component: General → Networking: HTTP
Product: Boot2Gecko → Core
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/876519ac69eb
https://hg.mozilla.org/mozilla-central/rev/876519ac69eb
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.