Added Full Support for iOS, Kindle and BlackBerry Mobile Devices.

[bhamodi]

This PR updates the mobile browser detection along with some minor formatting updates.

[le717]

The script uses 2-space indentation throughout, can you restore that please?

[bhamodi]

@le717 - Ok, I updated all indentation to be 2-spaces.

[bhamodi]

Also, I just noticed that the README needs to be updated to reflect the new mobile browser detection. I'll send another PR to reflect that tonight. 👍

[le717]

Could you alphabetize this list so it's easier to find the device key? I know it was not before, but now that there are much more in it, it would be best to sort it now.

[bhamodi]

Done! 👍

[le717]

Just some minor things there. As for the readme, it'd be better if you do that in this same PR. Also, can you update the tests with the UAs you used as well?

[bhamodi]

@le717 - Does our definition of webkit only apply to desktop webkit? How about AppleWebKit which is found on most mobile browsers?

[le717]

AppleWebKit is handled in #44.

[le717]

What is browser.bb, and alias to browser.blackberry?

[le717]

Just a few more comments, I'll be running the tests on my machine as soon as I can. :)

[bhamodi]

Regarding the alphabetized UA matcher, it should work fine since they were all connected through || operators.
And the reason that we have to include both browser.bb and browser.blackberry is because browser.blackberry only covers BlackBerry devices which are under BB10. With the start of BB10 OS, they no longer have blackberry in the UA, but rather they use BB.

[le717]

That makes the library a tad fragmented, because if people didn't know that distinction, they would get confused as to why blackberry is not matching BB10. I think it be best to do the same that's been done for IE 11, reassign browser.bb to browser.blackberry if it is true.

[gabceb]

Sorry for my delay, I've been slammed with work. I will take a look at this tomorrow.

[bhamodi]

Sounds good @gabceb 👍
And @le717 - Would it look something like this?

if ( browser.bb ) {
    browser["blackberry"] = true;
}

What else would have to change? Also, since we're talking about combining similar UA together, it is important to note that Silk is a browser on Kindle devices. Do we want to group that together too or no? I would lean towards no because it is a different browser, just available on the same device. We also have to make a decision about the PlayBook which is another BlackBerry product (but it's a tablet and has a unique UA).

[bhamodi]

@gabceb and @le717 - any updates?

[le717]

@bhamodi Sorry, I was suddenly slammed the last part of the week. I'll try to get to your questions sometime tomorrow. :)

[gabceb]

@le717 I've added you as a collaborator for the project. You should be able to push and accept PRs

[le717]

@gabceb I was not expecting that. Thanks! However I would still like it if you would review my PR after I land this one. It's my thought that every PR should be reviewed by at least one other person who did not submit the patch. :)

[le717]

@bhamodi Let me break your comment down:

Would it look something like this? -snip-

See if this works (borrowed from the browser.opr/browser.rv reassignment, which is the same thing going on here):

if ( browser.bb ) {
    var blackberry = "blackberry";

    matched.browser = blackberry;
    browser[blackberry] = true;
}

I'll pull your changes and test this manually as well.

Also, since we're talking about combining similar UA together, it is important to note that Silk is a browser on Kindle devices. Do we want to group that together too or no? I would lean towards no because it is a different browser, just available on the same device.

No, keep that separate. We can run Chome and Firefox on Win, Mac, and Linux. but do we lump them all under browser.chrome? Certainly not.

We also have to make a decision about the PlayBook which is another BlackBerry product (but it's a tablet and has a unique UA).

I also say keep the PlayBook separate, but I feel like I need to explain that in reference to reassigning browser.bb to browser.blackberry.

The PlayBook and Blackberry phone are two different devices. While they are both a Blackberry (if anything by solely name), they are considered two different devices. However, by the same logic, why merge browser.bb and browser.blackberry when they could be considering different devices? BB10 OS is not offered on older hardware, only newer, so that could warrant the different device label. I see it like this: the new Blackberry is not a new product altogether but rather a refreshed continuation of the "old" Blackberry. This is supported by the 10 in the OS's name. Even though it is new, it is still considered a Blackberry, not a completely different device. Most developers will (or rather should) use feature detection to detect exactly what browser version it is, or at the least grab browser.versionNumber and write

if (jQuery.browser.versionNumber >= 10) {
  // New Blackberry
}

On the other hand, the PlayBook is a true tablet, a different device, not a phone like a Blackberry. Sure, there are those "phablet" phones that are borderline tablet, but the PlayBook is a true tablet because it does not have a cellular plan like phones that support voice. Additionally, and here again the "phablet" blurs these lines, but most people do not want to hold some 3 ounce, 7-8" touch screen to their face and talk on the phone with it. Thus it is a different device than the Blackberry "proper" and should be listed separately.

[le717]

@bhamodi Whoops, sorry. Hit the wrong key on my keyboard. Finished the comment.

[le717]

Just tested the code I gave, it works correctly, acting the same as the browser.rv to browser.msie reassignment reference. You should be able use that and revise the tests and readme to use browser.blackberry instead of browser.bb.

[bhamodi]

@le717 - Thanks for the detailed comments! I have implemented everything we talked about. I'll leave the JavaScript minification to you and @gabceb because I'm not sure which minifier you guys have been using. Cheers.

[le717]

Why is casper.exit() added here and in the rest of the next tests when the original tests do not have this statement? Is it required by newer versions of Casper?

[bhamodi]

I don't believe so. I took that from the Android test and used it for the tests I made. Good catch, I think it's only required for the last test case in the file. I'll fix that tonight when I get home. 👍

[le717]

These two lines still reference browser.bb.

[bhamodi]

Yes, I left these intentionally. This is the test for BB10 phones. Are we going to be able to access the version number by using blackberry here? I thought we couldn't because v_10 is not defined for blackberry, only BB.

[le717]

You're right! Sorry, my bad. I got these references to the Casper .bb confused with the plugin .bb for moment.

[le717]

I'm seeing some mega test failures with these changes, but I do not think they are caused by this. I'm getting failures also on master, seemingly caused by c911970. It looks like I may have to fix this on master first, but that will make this unmergable unless you merge with master or submit a revised PR, after I can finally finish this (rather long, sorry about that) review. That OK with you?

[bhamodi]

Ah I see. Would it be ok to just merge this and then you can create a PR that addresses the failing tests? I can help you with that if you need some support!

[le717]

Unfortunately, merging this will only compound the errors. I'm getting four failures on master but eight with this, all probably coming from the same errors in master. See #46 for details if you want. I probably won't able to fix them all this week, so this may not be landed until sometime next week after those are resolved. Very, very sorry about that. :(

[bhamodi]

That's fine @le717. Do what you got to do. 👍
My only comment would be that I think we should release a fix before next week as the issues mentioned in #46 are critical. Good luck, let me know if you need support.

[bhamodi]

@le717 @gabceb - So now that #46 and #48 have been merged, shall I rebase this and prepare it for merging?

[le717]

Actually, because there's been a few other merges going in as well (not to mention fixing the indentation), I think it will be easier for you to start fresh with the detection code, tests, and readme and submit a new PR instead of rebasing this one once #49 has landed.

[bhamodi]

@le717 - Ok, ping me when #49 is merged.

[le717]

@bhamodi Ping! The fixes have landed, and I'd like to merge your changes as well. :)

[bhamodi]

@le717 Thanks! I'll get a new PR fired up ASAP.

[bhamodi]

@le717 You can find the updated PR here: #50 👍