Closed Bug 726062 Opened 8 years ago Closed 8 years ago

B2G UA is wrong, better fix

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: gal, Assigned: fabrice)

References

Details

Attachments

(1 file, 1 obsolete file)

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) {
    ...

?
Blocks: 725602
No longer depends on: 725602
Summary: B2G UA is wrong → B2G UA is wrong, better fix
Attached patch patch (obsolete) — Splinter Review
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
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+
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+
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
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+
Reviewed a long while ago, can this just land now?
blocking-basecamp: --- → ?
Fabrice can we just dupe this to bug 761873 now?
(In reply to JP Rosevear [:jpr] from comment #12)
> Fabrice can we just dupe this to bug 761873 now?

sure, please do.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 761873
blocking-basecamp: ? → +
blocking-kilimanjaro: --- → +
This still needs to be fixed.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Attachment #596113 - Attachment is obsolete: true
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.
Attached patch patchSplinter Review
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)
Attachment #653445 - Flags: review?(gal) → review+
Component: General → Networking: HTTP
Product: Boot2Gecko → Core
https://hg.mozilla.org/mozilla-central/rev/876519ac69eb
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.