feat #292 Detecting Mobile Device Properties #292

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@renjurichard

Ready for Code Review !

@piuccio piuccio commented on an outdated diff Jan 8, 2013
src/aria/utils/Device.js
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+Aria.classDefinition({
+ $classpath : 'aria.utils.Device',
+ $dependencies : ['aria.core.Browser'],
+ $singleton : true,
+ $constructor : function () {
+ var navigator = Aria.$global.navigator;
+ var ua = navigator ? navigator.userAgent.toLowerCase() : "";
+ this.ua = ua;
+ var os = navigator.platform.toLowerCase();
+ this.os = os;
+ this.docElement = Aria.$window.document.documentElement;
@piuccio piuccio commented on an outdated diff Jan 8, 2013
src/aria/utils/Device.js
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+Aria.classDefinition({
+ $classpath : 'aria.utils.Device',
+ $dependencies : ['aria.core.Browser'],
+ $singleton : true,
+ $constructor : function () {
+ var navigator = Aria.$global.navigator;
+ var ua = navigator ? navigator.userAgent.toLowerCase() : "";
+ this.ua = ua;
@piuccio
piuccio Jan 8, 2013

missing JsDoc

@piuccio piuccio commented on an outdated diff Jan 8, 2013
src/aria/utils/Device.js
+ var ua = navigator ? navigator.userAgent.toLowerCase() : "";
+ this.ua = ua;
+ var os = navigator.platform.toLowerCase();
+ this.os = os;
+ this.docElement = Aria.$window.document.documentElement;
+ },
+
+ $prototype : {
+
+ /**
+ * Sets the user agent
+ * @param {String} User Agents string
+ * @public
+ */
+
+ setUA : function (userAgent) {
@piuccio
piuccio Jan 8, 2013

Why do you add a setter for a public variable?
If it's just for testing then you don't need a setter at all

@piuccio piuccio commented on an outdated diff Jan 8, 2013
src/aria/utils/Device.js
+ */
+
+ setUA : function (userAgent) {
+ this.ua = userAgent;
+ },
+
+ /**
+ * Checks whether it is a Mobile Device including a Tablet
+ * @public
+ */
+ isDevice : function () {
+ var _isDevice = /(android|bb\d+|meego).+mobile|avantgo|bada\/|blackberry|blazer|compal|elaine|fennec|hiptop|iemobile|ip(hone|od)|iris|kindle|lge |maemo|midp|mmp|netfront|opera m(ob|in)i|palm( os)?|phone|p(ixi|re)\/|plucker|pocket|psp|series(4|6)0|symbian|treo|up\.(browser|link)|vodafone|wap|windows (ce|phone)|xda|xiino/i.test(this.ua)
+ || /1207|6310|6590|3gso|4thp|50[1-6]i|770s|802s|a wa|abac|ac(er|oo|s\-)|ai(ko|rn)|al(av|ca|co)|amoi|an(ex|ny|yw)|aptu|ar(ch|go)|as(te|us)|attw|au(di|\-m|r |s )|avan|be(ck|ll|nq)|bi(lb|rd)|bl(ac|az)|br(e|v)w|bumb|bw\-(n|u)|c55\/|capi|ccwa|cdm\-|cell|chtm|cldc|cmd\-|co(mp|nd)|craw|da(it|ll|ng)|dbte|dc\-s|devi|dica|dmob|do(c|p)o|ds(12|\-d)|el(49|ai)|em(l2|ul)|er(ic|k0)|esl8|ez([4-7]0|os|wa|ze)|fetc|fly(\-|_)|g1 u|g560|gene|gf\-5|g\-mo|go(\.w|od)|gr(ad|un)|haie|hcit|hd\-(m|p|t)|hei\-|hi(pt|ta)|hp( i|ip)|hs\-c|ht(c(\-| |_|a|g|p|s|t)|tp)|hu(aw|tc)|i\-(20|go|ma)|i230|iac( |\-|\/)|ibro|idea|ig01|ikom|im1k|inno|ipaq|iris|ja(t|v)a|jbro|jemu|jigs|kddi|keji|kgt( |\/)|klon|kpt |kwc\-|kyo(c|k)|le(no|xi)|lg( g|\/(k|l|u)|50|54|\-[a-w])|libw|lynx|m1\-w|m3ga|m50\/|ma(te|ui|xo)|mc(01|21|ca)|m\-cr|me(rc|ri)|mi(o8|oa|ts)|mmef|mo(01|02|bi|de|do|t(\-| |o|v)|zz)|mt(50|p1|v )|mwbp|mywa|n10[0-2]|n20[2-3]|n30(0|2)|n50(0|2|5)|n7(0(0|1)|10)|ne((c|m)\-|on|tf|wf|wg|wt)|nok(6|i)|nzph|o2im|op(ti|wv)|oran|owg1|p800|pan(a|d|t)|pdxg|pg(13|\-([1-8]|c))|phil|pire|pl(ay|uc)|pn\-2|po(ck|rt|se)|prox|psio|pt\-g|qa\-a|qc(07|12|21|32|60|\-[2-7]|i\-)|qtek|r380|r600|raks|rim9|ro(ve|zo)|s55\/|sa(ge|ma|mm|ms|ny|va)|sc(01|h\-|oo|p\-)|sdk\/|se(c(\-|0|1)|47|mc|nd|ri)|sgh\-|shar|sie(\-|m)|sk\-0|sl(45|id)|sm(al|ar|b3|it|t5)|so(ft|ny)|sp(01|h\-|v\-|v )|sy(01|mb)|t2(18|50)|t6(00|10|18)|ta(gt|lk)|tcl\-|tdg\-|tel(i|m)|tim\-|t\-mo|to(pl|sh)|ts(70|m\-|m3|m5)|tx\-9|up(\.b|g1|si)|utst|v400|v750|veri|vi(rg|te)|vk(40|5[0-3]|\-v)|vm40|voda|vulc|vx(52|53|60|61|70|80|81|83|85|98)|w3c(\-| )|webc|whit|wi(g |nc|nw)|wmlb|wonu|x700|yas\-|your|zeto|zte\-/i.test(this.ua.substr(0, 4));
+
+ if (_isDevice === true) {
+ return true;
@piuccio
piuccio Jan 8, 2013

By doing this you evaluate the regular expression every time you call the method.

You should choose between two options:

  1. Do everything in the constructor
  2. Do as you did now, but rewrite the function
isDevice : function () {
   // whatever logic you like
   if (previousComputationIsTrue) {
      this.isDevice = Aria.returnTrue;
   } else {
      this.isDevice = Aria.returnFalse;
   }
   return this.isDevice();
}

By doing this you'll execute the regexp only once.

Moreover, never call an internal variable _something, the underscore is just useless and make things less readable.

It would be nice to have a performance test to understand whether there is a loss doing everything in the constructor. The drawback of solution (2) is extra code -> bigger file.

@piuccio piuccio commented on an outdated diff Jan 8, 2013
test/aria/utils/DeviceTest.js
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+(function () {
+ /**
+ * @class test.aria.core.UATest
+ * @extends extends
+ */
+ var classDefinition = {
+ $classpath : 'test.aria.utils.DeviceTest',
+ $dependencies : ['aria.utils.Device'],
+ $extends : "aria.jsunit.TestCase",
+ $constructor : function () {
+
@piuccio
piuccio Jan 8, 2013

empty $constructor and $destructor, you can remove them

@piuccio piuccio commented on an outdated diff Jan 8, 2013
test/aria/utils/DeviceTest.js
+
+ },
+
+ testMobileUserAgents : function () {
+ for (var i = 0; i < this.MobileUserAgents.length; i++) {
+ this.userAgent.ua = "";
+ this.userAgent.ua = this.MobileUserAgents[i];
+ this.assertTrue(this.userAgent.isMobile() === true, "This is not a Mobile device");
+ }
+ },
+
+ testTabletUserAgents : function () {
+ for (var i = 0; i < this.TabletUserAgents.length; i++) {
+ this.userAgent.ua = "";
+ this.userAgent.ua = this.TabletUserAgents[i];
+ this.assertTrue(this.userAgent.isTablet() === true, "This is not a tablet device");
@piuccio
piuccio Jan 8, 2013

You're testing only isTablet and isMobile, what about all the other methods?

@piuccio piuccio commented on an outdated diff Jan 8, 2013
src/aria/utils/Device.js
+ return true;
+ }
+
+ },
+
+ /**
+ * private function - To check whether the style property is supported by the browser
+ * @param {String} CSS Property
+ * @private
+ */
+ _isStyleSupported : function (propName) {
+
+ var prefixes = ['Moz', 'Webkit', 'Khtml', 'O', 'Ms'];
+ var _cache = {};
+
+ var element = element || Aria.$window.document.documentElement;
@piuccio
piuccio Jan 8, 2013

Where is element defined?
This method probably doesn't work, please add a test for it

@piuccio piuccio referenced this pull request Jan 10, 2013
Closed

Mobile Device properties #291

@piuccio piuccio added a commit that referenced this pull request Jan 29, 2013
renju richard feat #292 Mobile Device Properties 935733c
@piuccio
piuccio commented Jan 29, 2013

Integrated with some changes in 935733c

@piuccio piuccio closed this Jan 29, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment