Closed Bug 1157518 Opened 10 years ago Closed 8 years ago

Be more specific about generated client records

Categories

(Firefox for iOS :: Sync, defect)

All
iOS 8
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fxios-v6.0 --- fixed

People

(Reporter: rnewman, Assigned: maurya1985, Mentored)

References

Details

(Whiteboard: [good first bug][lang=swift])

Attachments

(1 file, 2 obsolete files)

We currently call everything either "simulator" or "phone". We can do better.
Blocks: 1097218
To add: * Real app version. * Form factor. To do: * Manual testing of device, application, appPackage.
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?)
Flags: needinfo?(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!
Flags: needinfo?(rnewman)
Assignee: nobody → maurya1985
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?
Flags: needinfo?(rnewman)
Status: NEW → ASSIGNED
(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.
Flags: needinfo?(rnewman) → needinfo?(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
Flags: needinfo?(sleroux)
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?
Flags: needinfo?(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
Flags: needinfo?(sleroux)
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?
Flags: needinfo?(sleroux)
Attachment #8764159 - Flags: review?(sleroux)
Comment on attachment 8764159 [details] 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.
Flags: needinfo?(sleroux)
Attachment #8764159 - Flags: review?(sleroux) → review+
Attachment #8766658 - Flags: review?(sleroux)
Comment on attachment 8766658 [details] Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1973 Looks good - just a super small nit comment about set allocation
Attachment #8766658 - Flags: review?(sleroux) → review+
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 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.)
(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.
(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 :)
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?
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(nalexander)
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?
Flags: needinfo?(nalexander)
I would like to understand if we can group iPads under "tablet".
(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".
Flags: needinfo?(nalexander)
> 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
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
Use "tablet" and I'll add it to those docs.
Flags: needinfo?(rnewman)
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.
Attachment #8764159 - Attachment is obsolete: true
Attachment #8766658 - Attachment is obsolete: true
Attachment #8774036 - Flags: review?(sleroux)
Comment on attachment 8774036 [details] 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.
Attachment #8774036 - Flags: review?(sleroux) → review+
master bdabdec9a01d84e0a4859f74dd0cf7595603ab6f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: