1157518 - Be more specific about generated client records

Status: RESOLVED FIXED

(Firefox for iOS :: Sync, defect)

Description

[Richard Newman [:rnewman]]

We currently call everything either "simulator" or "phone". We can do better.

Comment 1

[Richard Newman [:rnewman]]

To add:

* Real app version.
* Form factor.

To do:

* Manual testing of device, application, appPackage.

Comment 2

[Maurya Talisetti]

Hi Richard,

Can you help me understand this bug?

From what I understand, we want to add additional options for the formfactor (like iphone 6s plus, iphone 6s, iphone 6 plus, iphone 6, etc.). I do see the notes already made in the ClientsSynchronizer.swift. The app version seems to be hard-coded. Do we want to use the current fennec version value here? (possibly from some property file)
Do we also want to test fennec for all these different formfactor values? (what does manual testing refer to?)

Comment 3

[Richard Newman [:rnewman]]

Hi Maurya,

Sorry for the delayed reply!

The formfactor here should be one of:

“phone”, “largetablet”, “smalltablet”, “desktop”, “laptop”, “tv”

-- see http://docs.services.mozilla.com/sync/objectformats.html#id2

This bug is about poking around the OS-provided data to figure out which of those strings we should use: e.g., an iPad mini should be "smalltablet", and an iPhone should be "phone".

And yes, the version should be 'live', probably AppInfo.appVersion.


Testing is a challenge. I'm not sure if the Simulator behaves correctly, but we should be able to manually test on whatever devices are to hand.

Do let us know if you need more pointers!

Comment 4

[Maurya Talisetti]

Hi Richard,

Thanks for the details! I examined the data provided by iOS and found that it doesn't exactly tell us what device we are using. It only tells us if it's a tablet or a phone, etc. Doesn't tell us if it is an iPad Mini for instance.

        let model = UIDevice.currentDevice().model
        var formfactor: String
        switch model {
            case "iPhone":
                formfactor = "phone"
            case "iPad":
                formfactor = "largetablet" // or, "smalltablet" ?
            default:
                formfactor = "phone"
        }

UIDevice.model is the key property here. Apparently, the available values for UIDevice.model is limited: http://stackoverflow.com/a/14411113/640378

UIScreen additionally provides screen sizes. We could use a combination of information provided by UIDevice and UIScreen to decipher what device it is, though it's not so straightforward. There are external libraries like DeviceGuru which seem to be doing this. Are we allowed to use such external libraries? Or, should we try to re-implement similar approaches here?

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Maurya Talisetti from comment #4)
> Hi Richard,
> 
> Thanks for the details! I examined the data provided by iOS and found that
> it doesn't exactly tell us what device we are using. It only tells us if
> it's a tablet or a phone, etc. Doesn't tell us if it is an iPad Mini for
> instance.
> 
>         let model = UIDevice.currentDevice().model
>         var formfactor: String
>         switch model {
>             case "iPhone":
>                 formfactor = "phone"
>             case "iPad":
>                 formfactor = "largetablet" // or, "smalltablet" ?
>             default:
>                 formfactor = "phone"
>         }
> 
> UIDevice.model is the key property here. Apparently, the available values
> for UIDevice.model is limited: http://stackoverflow.com/a/14411113/640378
> 
> UIScreen additionally provides screen sizes. We could use a combination of
> information provided by UIDevice and UIScreen to decipher what device it is,
> though it's not so straightforward. There are external libraries like
> DeviceGuru which seem to be doing this. Are we allowed to use such external
> libraries? Or, should we try to re-implement similar approaches here?

rnewman is PTO right now, so I'll redirect to sleroux.  I'm not sure we'd take an external dependency in this case, but sleroux will have a better handle.

Comment 6

[Stephan Leroux [:sleroux]]

Hey Maurya,

Looks like the best we can do to get the device name/type is by using a system call to grab the hardware name using sysctlbyname() [1]. Passing in "hw.machine" should return back a string that is more descriptive of the device the user is running. Unfortunately this string looks like "iPhone5,2" and "iPad2,3". I would hesistate on pulling in a library like DeviceGuru since all it does it run this sysctlbyname command and filter the string through a giant switch statement. I think we can be a bit smarter by writing our own method that calls it and filters the various hardware names into our own buckets like 'smalltablet', 'phone', etc. I can see this being an extension on UIDevice with a method named formFactorString() that handles all of this logic.

[1] SO Post http://stackoverflow.com/questions/448162/determine-device-iphone-ipod-touch-with-iphone-sdk

Comment 7

[Maurya Talisetti]

Hi Stephan,

I wrote the function formFactorString() as per your suggestions. It looks good for the simulator.

While testing an iPhone 4s physical device, I ran into a couple of "code sign errors". It seems like I need to have a provisioning profile created before I can install the app on the device. Is there a provisioning profile that the Mozilla iOS team uses? Or, should I create my own?

Comment 8

[Stephan Leroux [:sleroux]]

> Is there a provisioning profile that the Mozilla iOS team uses? Or, should I create my own?

When running on device, you'll need to update the bundle identifiers and use your own signing certs. Instructions can be found here: https://github.com/mozilla/firefox-ios/blob/master/BUILDING.md

Comment 9

[Maurya Talisetti]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1926

Stephan,

I tested the change on the simulator and it looks good.

I'm unable to test on my device because apparently a free developer provisioning profile cannot be created while using the "Passes" feature. Would anyone be able to help us test on an actual device?

Comment 10

[Stephan Leroux [:sleroux]]

Comment on attachment 8764159 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1926

I was only able to test it on my iPhone but the form factor string is working correctly. The patch looks good overall - just a few nits about cleaning up the code to make it more readable we when need to come back to it in the future.

Comment 11

[Maurya Talisetti]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1973

Comment 12

[Stephan Leroux [:sleroux]]

Comment on attachment 8766658 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1973

Looks good - just a super small nit comment about set allocation

Comment 13

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I am not in favor of maintaining a list of devices in our code. This means the list will have to be updated all the time when new devices are introduced.

Why do we need bigtablet/smalltablet granularity? Without that detail, the solution in comment #4 looks fine to me. Just ask the device if it is a phone or tablet.

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Stefan Arentz [:st3fan] from comment #13)
> I am not in favor of maintaining a list of devices in our code. This means
> the list will have to be updated all the time when new devices are
> introduced.
> 
> Why do we need bigtablet/smalltablet granularity? Without that detail, the
> solution in comment #4 looks fine to me. Just ask the device if it is a
> phone or tablet.

In theory, because there's a pretty big difference in screen size and density between an iPad mini v2 and an iPad Pro.  (At least, I think there is.)  In practice, no Firefox client adapts on the basis of this information.  It's more notable on Android, where there is no clear delineation between sizes (phone -> phablet -> tablet) and devices lie.  (On Android, a list of devices is not feasible, of course.)

Comment 15

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

(In reply to Nick Alexander :nalexander from comment #14)

> In theory, because there's a pretty big difference in screen size and
> density between an iPad mini v2 and an iPad Pro.

Why is that not just a local concern for the app currently running? On iOS we base our layout on many things. Device orientation, size class, wether you are in split view mode on an iPad. (The above code would say tabletxxlarge for an iPad Pro but the client possibly runs in a part of the screen as wide as an iPhone 5).

Why does the server-side need to know this detail? What is the use case we have for this? Right now I only know one case: showing the right icon in a device list. Do we have ideas about other cases?

I'm making a big deal out of this because it is unclear to me why we need this info. Having to maintain a list of devices is a maintenance burden that I would prefer to avoid if we don't have a good reason.

Comment 16

[Nick Alexander :nalexander [he/him]]

(In reply to Stefan Arentz [:st3fan] from comment #15)
> (In reply to Nick Alexander :nalexander from comment #14)
> 
> > In theory, because there's a pretty big difference in screen size and
> > density between an iPad mini v2 and an iPad Pro.
> 
> Why is that not just a local concern for the app currently running? On iOS
> we base our layout on many things. Device orientation, size class, wether
> you are in split view mode on an iPad. (The above code would say
> tabletxxlarge for an iPad Pro but the client possibly runs in a part of the
> screen as wide as an iPhone 5).
> 
> Why does the server-side need to know this detail? What is the use case we
> have for this? Right now I only know one case: showing the right icon in a
> device list. Do we have ideas about other cases?

I'm not sure the server ever sees this; it's other clients.  But your point is taken: no, I'm not aware of other cases.

> I'm making a big deal out of this because it is unclear to me why we need
> this info. Having to maintain a list of devices is a maintenance burden that
> I would prefer to avoid if we don't have a good reason.

I'm not saying you should!  Just explaining why it was chosen to be so.  All Sync clients have the right to behave inconsistently -- and all of them do :)

Comment 17

[Maurya Talisetti]

I can't seem to recollect where I saw this, and now I'm not able to find the evidence for it in the code, but I thought that the size of the device determines the frequency at which the sync occurs. Is this true?

Either way, should we go with the solution as in comment #4?

Comment 18

[Nick Alexander :nalexander [he/him]]

(In reply to Maurya Talisetti from comment #17)
> I can't seem to recollect where I saw this, and now I'm not able to find the
> evidence for it in the code, but I thought that the size of the device
> determines the frequency at which the sync occurs. Is this true?

I have never heard this.  We do sync much more aggressively on Desktop devices (about every 5 minutes, if there is activity) but we don't differentiate phones from tablets, for example.  What can happen is that the system (Android, depending on version, device characteristics, network status, etc) can sync us more frequently.

Comment 19

[Maurya Talisetti]

Thanks for confirming that, Nick. So, should we revert to the solution in comment #4 since we don't have any different behavior for phones vs tablets?

Comment 20

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I would like to understand if we can group iPads under "tablet".

Comment 21

[Nick Alexander :nalexander [he/him]]

(In reply to Stefan Arentz [:st3fan] from comment #20)
> I would like to understand if we can group iPads under "tablet".

Android differentiates:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/SharedPreferencesClientsDataDelegate.java#106

Desktop does not appear to.  You can make iOS do differently; I'd be fine with making all iPads be "tablet".

Comment 22

[Stephan Leroux [:sleroux]]

> I'm making a big deal out of this because it is unclear to me why we need this info. Having to maintain a list of devices is a maintenance burden that I would prefer to avoid if we don't have a good reason.

If we're okay with lumping all iPads under 'tablet' and phones/iPods under 'phone' then we can can use the user idiom check [1] instead of having these strings hardcoded. This would be ideal since it would avoid the maintenance cost of updating the device list every time a new device appears.

[1] https://developer.apple.com/library/ios/documentation/UIKit/Reference/UIDevice_Class/#//apple_ref/c/tdef/UIUserInterfaceIdiom

Comment 23

[Maurya Talisetti]

We could also use the solution in comment #4 -> UIDevice.currentDevice().model

Btw, I think we need to choose from among the groupings 'smalltablet' and 'largetablet'. There's no grouping 'tablet' as per http://docs.services.mozilla.com/sync/objectformats.html#id2

Comment 24

[Richard Newman [:rnewman]]

Use "tablet" and I'll add it to those docs.

Comment 25

[Richard Newman [:rnewman]]

https://github.com/mozilla-services/docs/commit/8b5aef407b074eec8d51591290b58d11e7fde831

Comment 26

[Maurya Talisetti]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1973

Hi Stephan,

Got rid of the hardcoded device strings. We are now using UIDevice.userInterfaceIdiom to check if it's a phone or a tablet.

Comment 27

[Stephan Leroux [:sleroux]]

Comment on attachment 8774036 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1973

Looks good and works for me! Thanks for iterating on this bug.

Comment 28

[Stephan Leroux [:sleroux]]

master bdabdec9a01d84e0a4859f74dd0cf7595603ab6f