Fixing a bug where c.g.g.dom.client.Style can throw a JS exception if a style property is set to...

Fixing a bug where c.g.g.dom.client.Style can throw a JS exception if a style
property is set to a numeric type (eg: zIndex = 1).  Most browsers automatically
coerce the value to a String, but IE6-7 does not.  We now coerce the returned
value to a string if it is a numeric type.  This doesn't come up often because
you have to manually set the value to a numeric type using a native method (our
API always sets the value as a string).

Issue: 5548

[skybrian]

LGTM

http://gwt-code-reviews.appspot.com/1709803/diff/1/user/src/com/google/gwt/do...
File user/src/com/google/gwt/dom/client/Style.java (right):

http://gwt-code-reviews.appspot.com/1709803/diff/1/user/src/com/google/gwt/do...
user/src/com/google/gwt/dom/client/Style.java:1764: return typeof(prop) ==
"number" ? "" + prop : prop;
Is the type check cheaper than just writing:
  return "" + prop;

[cromwellian]

http://gwt-code-reviews.appspot.com/1709803/diff/1/user/src/com/google/gwt/do...
File user/src/com/google/gwt/dom/client/Style.java (right):

http://gwt-code-reviews.appspot.com/1709803/diff/1/user/src/com/google/gwt/do...
user/src/com/google/gwt/dom/client/Style.java:1764: return typeof(prop) ==
"number" ? "" + prop : prop;
I'm pretty sure it's cheaper, the original will prevent inlining because it
introduces a local var.

We want it to compile to something like:
""+element[prop]

If this is IE only, it's too bad we can't put it only in that permutation.

On 2012/05/10 23:32:03, skybrian wrote:
> Is the type check cheaper than just writing:
>   return "" + prop;

[jlabanca]

http://gwt-code-reviews.appspot.com/1709803/diff/1/user/src/com/google/gwt/do...
File user/src/com/google/gwt/dom/client/Style.java (right):

http://gwt-code-reviews.appspot.com/1709803/diff/1/user/src/com/google/gwt/do...
user/src/com/google/gwt/dom/client/Style.java:1764: return typeof(prop) ==
"number" ? "" + prop : prop;
The type check isn't an optimization per se; we're checking to make sure prop
isn't null or undefined, which would result in the string "null" or "undefined".
 We also can't just check "this[name]?", because the numeric 0 == false.

I moved the implementation out to DOMImpl so that we only do the typeof check in
IE.  And even then, we now only use the typeof check for zIndex, because setting
the other properties to a numeric value is an error anyway.

This leaves the possibility that a user could set a non-standard style property
to a numeric value (which cannot be done with the Style API), then try to read
it as a String using Style.getProperty().  That will still be an error, but its
an obscure use case, and it doesn't make sense to penalize the 99.999% use case.

http://gwt-code-reviews.appspot.com/1709803/diff/5001/user/src/com/google/gwt...
File user/src/com/google/gwt/dom/client/DOMImpl.java (right):

http://gwt-code-reviews.appspot.com/1709803/diff/5001/user/src/com/google/gwt...
user/src/com/google/gwt/dom/client/DOMImpl.java:299: return style[name];
For non-IE browsers, nothing has changed.  FF and Chrome coerce zIndex to a
String automatically.

http://gwt-code-reviews.appspot.com/1709803/diff/5001/user/src/com/google/gwt...
File user/src/com/google/gwt/dom/client/DOMImplIE9.java (right):

http://gwt-code-reviews.appspot.com/1709803/diff/5001/user/src/com/google/gwt...
user/src/com/google/gwt/dom/client/DOMImplIE9.java:43: return
typeof(style[name]) == "number" ? "" + style[name] : style[name];
I verified that even IE9 can store zIndex as a numeric value.

http://gwt-code-reviews.appspot.com/1709803/diff/5001/user/src/com/google/gwt...
File user/src/com/google/gwt/dom/client/Style.java (right):

http://gwt-code-reviews.appspot.com/1709803/diff/5001/user/src/com/google/gwt...
user/src/com/google/gwt/dom/client/Style.java:1354: public final String
getProperty(String name) {
If the user calls getProperty(String) with a non-standard property name (ex.
"myProperty"), and that property happens to have a numeric value that was set
through some other native method (because Style does not have a numeric setter),
you'll get the same error.  However, that is a very rare use case.

http://gwt-code-reviews.appspot.com/1709803/diff/5001/user/src/com/google/gwt...
user/src/com/google/gwt/dom/client/Style.java:1412: return
DOMImpl.impl.getNumericStyleProperty(this, STYLE_Z_INDEX);
We only pay the extra cost of the typeof check for zIndex, and only on IE.  I
verified that IE does not allow you to set other properties (ex. "top", "left",
"margin") to a numeric value.  It just ignores the command.

If we find other properties that can be set to a numeric value, we can defer to
the same getNumericStyleProperty() method.

[skybrian]

LGTM

[jlabanca]

committed as 10980