Revision: 7646
Author: [email protected]
Date: Wed Mar 3 05:43:42 2010
Log: Fixing possible NPE in ImageSrcIE6 when an img element is copied.
http://gwt-code-reviews.appspot.com/150807
http://code.google.com/p/google-web-toolkit/source/detail?r=7646
Added:
/trunk/user/test/com/google/gwt/dom/client/ImageElementTest.java
/trunk/user/test/com/google/gwt/dom/public-test
/trunk/user/test/com/google/gwt/dom/public-test/largeImage0.jpg
/trunk/user/test/com/google/gwt/dom/public-test/largeImage1.jpg
/trunk/user/test/com/google/gwt/dom/public-test/largeImage2.jpg
/trunk/user/test/com/google/gwt/dom/public-test/smallImage0.jpg
/trunk/user/test/com/google/gwt/dom/public-test/smallImage1.jpg
/trunk/user/test/com/google/gwt/dom/public-test/smallImage2.jpg
Modified:
/trunk/user/src/com/google/gwt/dom/client/DOMImplIE6.java
/trunk/user/src/com/google/gwt/dom/client/ImageElement.java
/trunk/user/src/com/google/gwt/dom/client/ImageSrcIE6.java
/trunk/user/src/com/google/gwt/layout/client/LayoutImplIE6.java
/trunk/user/src/com/google/gwt/user/client/ui/impl/ClippedImageImplIE6.java
/trunk/user/test/com/google/gwt/dom/DOMTest.gwt.xml
=======================================
--- /dev/null
+++ /trunk/user/test/com/google/gwt/dom/client/ImageElementTest.java Wed
Mar 3 05:43:42 2010
@@ -0,0 +1,93 @@
+/*
+ * Copyright 2008 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may
not
+ * use this file except in compliance with the License. You may obtain a
copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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.
+ */
+package com.google.gwt.dom.client;
+
+import com.google.gwt.junit.client.GWTTestCase;
+
+/**
+ * Tests for {...@link ImageElement}.
+ */
+public class ImageElementTest extends GWTTestCase {
+
+ @Override
+ public String getModuleName() {
+ return "com.google.gwt.dom.DOMTest";
+ }
+
+ /**
+ * IE6 has a special implementation of {...@link
ImageElement#setSrc(String)}
+ * that prevents the browser from loading uncached images multiple
times. In
+ * order to test this properly, the browser should be set to clear its
cache
+ * on exit.
+ */
+ public void testSetSrc() {
+ // The parent element will actually load the image.
+ ImageElement parent = Document.get().createImageElement();
+ parent.setSrc("largeImage0.jpg");
+ assertTrue(parent.getSrc().endsWith("largeImage0.jpg"));
+
+ // The child element does not have its source set until the parent
loads.
+ ImageElement child = Document.get().createImageElement();
+ child.setSrc("largeImage0.jpg");
+ assertTrue(child.getSrc().endsWith("largeImage0.jpg"));
+
+ child.setSrc("smallImage1.jpg");
+ assertTrue(child.getSrc().endsWith("smallImage1.jpg"));
+ }
+
+ public void testSetSrcCloneParent() {
+ // The parent element will actually load the image.
+ ImageElement parent = Document.get().createImageElement();
+ parent.setSrc("largeImage1.jpg");
+ assertTrue(parent.getSrc().endsWith("largeImage1.jpg"));
+
+ // The child element does not have its source set until the parent
loads.
+ ImageElement child = Document.get().createImageElement();
+ child.setSrc("largeImage1.jpg");
+ assertTrue(child.getSrc().endsWith("largeImage1.jpg"));
+
+ // The parent clone will have its source set. We call setSrc to
convert it
+ // to a child.
+ final ImageElement cloneParent = parent.cloneNode(true).cast();
+ cloneParent.setSrc("largeImage1.jpg");
+ assertTrue(cloneParent.getSrc().endsWith("largeImage1.jpg"));
+
+ cloneParent.setSrc("smallImage1.jpg");
+ assertTrue(cloneParent.getSrc().endsWith("smallImage1.jpg"));
+ }
+
+ public void testSetSrcCloneChild() {
+ // The parent element will actually load the image.
+ ImageElement parent = Document.get().createImageElement();
+ parent.setSrc("largeImage2.jpg");
+ assertTrue(parent.getSrc().endsWith("largeImage2.jpg"));
+
+ // The child element does not have its source set until the parent
loads.
+ ImageElement child = Document.get().createImageElement();
+ child.setSrc("largeImage2.jpg");
+ assertTrue(child.getSrc().endsWith("largeImage2.jpg"));
+
+ // The child clone will not have its source set. We call setSrc to
ensure
+ // it is registered as a child.
+ final ImageElement cloneChild = parent.cloneNode(true).cast();
+ cloneChild.setSrc("largeImage2.jpg");
+ assertTrue(cloneChild.getSrc().endsWith("largeImage2.jpg"));
+
+ cloneChild.setSrc("smallImage2.jpg");
+ assertTrue(cloneChild.getSrc().endsWith("smallImage2.jpg"));
+ }
+
+}
=======================================
--- /dev/null
+++ /trunk/user/test/com/google/gwt/dom/public-test/largeImage0.jpg Wed
Mar 3 05:43:42 2010
Binary file, no diff available.
=======================================
--- /dev/null
+++ /trunk/user/test/com/google/gwt/dom/public-test/largeImage1.jpg Wed
Mar 3 05:43:42 2010
Binary file, no diff available.
=======================================
--- /dev/null
+++ /trunk/user/test/com/google/gwt/dom/public-test/largeImage2.jpg Wed
Mar 3 05:43:42 2010
Binary file, no diff available.
=======================================
--- /dev/null
+++ /trunk/user/test/com/google/gwt/dom/public-test/smallImage0.jpg Wed
Mar 3 05:43:42 2010
Binary file, no diff available.
=======================================
--- /dev/null
+++ /trunk/user/test/com/google/gwt/dom/public-test/smallImage1.jpg Wed
Mar 3 05:43:42 2010
Binary file, no diff available.
=======================================
--- /dev/null
+++ /trunk/user/test/com/google/gwt/dom/public-test/smallImage2.jpg Wed
Mar 3 05:43:42 2010
Binary file, no diff available.
=======================================
--- /trunk/user/src/com/google/gwt/dom/client/DOMImplIE6.java Wed Jan 27
05:45:28 2010
+++ /trunk/user/src/com/google/gwt/dom/client/DOMImplIE6.java Wed Mar 3
05:43:42 2010
@@ -21,6 +21,47 @@
*/
class DOMImplIE6 extends DOMImplTrident {
+ private static boolean isIE6;
+ private static boolean isIE6Detected;
+
+ /**
+ * Check if the browser is IE6 or IE7.
+ *
+ * @return <code>true</code> if the browser is IE6, <code>false</code>
if IE7
+ * or any other browser
+ * @deprecated this method may be removed at any time
+ */
+ @Deprecated
+ static boolean isIE6() {
+ if (!isIE6Detected) {
+ isIE6 = isIE6Impl();
+ isIE6Detected = true;
+ }
+ return isIE6;
+ }
+
+ // Stolen and modified from UsernmAgent.gwt.xml.
+ // TODO(jgw): Get rid of this method, and switch to using soft
permutations
+ // once they land in trunk.
+ private static native boolean isIE6Impl() /*-{
+ function makeVersion(result) {
+ return (parseInt(result[1]) * 1000) + parseInt(result[2]);
+ }
+
+ var ua = navigator.userAgent.toLowerCase();
+ if (ua.indexOf("msie") != -1) {
+ var result = /msie ([0-9]+)\.([0-9]+)/.exec(ua);
+ if (result && result.length == 3) {
+ var v = makeVersion(result);
+ if (v < 7000) {
+ return true;
+ }
+ }
+ }
+
+ return false;
+ }-*/;
+
@Override
public native void cssClearOpacity(Style style) /*-{
style.filter = '';
@@ -59,7 +100,11 @@
*/
@Override
public String imgGetSrc(Element img) {
- return ImageSrcIE6.getImgSrc(img);
+ if (isIE6()) {
+ return ImageSrcIE6.getImgSrc(img);
+ } else {
+ return super.imgGetSrc(img);
+ }
}
/**
@@ -71,7 +116,11 @@
*/
@Override
public void imgSetSrc(Element img, String src) {
- ImageSrcIE6.setImgSrc(img, src);
+ if (isIE6()) {
+ ImageSrcIE6.setImgSrc(img, src);
+ } else {
+ super.imgSetSrc(img, src);
+ }
}
@Override
=======================================
--- /trunk/user/src/com/google/gwt/dom/client/ImageElement.java Fri May 15
13:11:37 2009
+++ /trunk/user/src/com/google/gwt/dom/client/ImageElement.java Wed Mar 3
05:43:42 2010
@@ -112,7 +112,16 @@
}-*/;
/**
+ * <p>
* URI designating the source of this image.
+ * </p>
+ * <p>
+ * If you {...@link #cloneNode(boolean)} an {...@link ImageElement}, or an
element
+ * that contains an {...@link ImageElement}, then you must call
+ * {...@link #setSrc(String)} on the cloned element to ensure it is loaded
+ * properly in IE6. Failure to do so may cause performance problems, or
your
+ * image may not load due to an IE6 specific workaround.
+ * </p>
*
* @see <a
href="http://www.w3.org/TR/1999/REC-html401-19991224/struct/objects.html#adef-src-IMG";>W3C
HTML Specification</a>
*/
=======================================
--- /trunk/user/src/com/google/gwt/dom/client/ImageSrcIE6.java Thu Apr 17
15:10:46 2008
+++ /trunk/user/src/com/google/gwt/dom/client/ImageSrcIE6.java Wed Mar 3
05:43:42 2010
@@ -33,27 +33,29 @@
* loading, it will be removed from this map.
*/
private static JavaScriptObject srcImgMap;
-
+
static {
executeBackgroundImageCacheCommand();
}
-
+
/**
- * Returns the src of the image, or the pending src if the image is
pending.
+ * Returns the src of the image, or the pending src if the image is
pending.
*/
public static native String getImgSrc(Element img) /*-{
return img.__pendingSrc || img.src;
}-*/;
/**
- * Sets the src of the image, queuing up with other requests for the same
- * URL if necessary.
+ * Sets the src of the image, queuing up with other requests for the
same URL
+ * if necessary. If the element is a clone, it may think it is in a
pending
+ * state but will not be updated properly when the image loads, so we
need to
+ * add it to the queue.
*/
public static void setImgSrc(Element img, String src) {
- // Early out for no-op prop set.
- if (getImgSrc(img).equals(src)) {
- return;
- }
+ // We can't early out yet because the img may be a clone that needs to
be
+ // cleaned up and added to the list of children.
+ boolean isSameSource = getImgSrc(img).equals(src);
+
// Lazy init the map
if (srcImgMap == null) {
srcImgMap = JavaScriptObject.createObject();
@@ -63,13 +65,26 @@
if (oldSrc != null) {
// The element is pending; there must be a top node for this src.
Element top = getTop(srcImgMap, oldSrc);
- assert (top != null);
- if (top.equals(img)) {
+ if (top == null) {
+ // This is a clone, so clean it up.
+ cleanupExpandos(img);
+ } else if (top.equals(img)) {
+ if (isSameSource) {
+ // Early out as this is the top element, and therefore not a
clone.
+ return;
+ }
+
// It's a pending parent.
removeTop(srcImgMap, top);
- } else {
+ } else if (removeChild(top, img, isSameSource)) {
// It's a pending child.
- removeChild(top, img);
+ if (isSameSource) {
+ // Early out as this is a known child, and therefore not a clone.
+ return;
+ }
+ } else {
+ // The child wasn't found, so this is a clone.
+ cleanupExpandos(img);
}
}
@@ -95,10 +110,11 @@
/**
* Sets an image as the pending parent for the specified URL.
*/
- private static native void addTop(JavaScriptObject srcImgMap, Element
img, String src) /*-{
+ private static native void addTop(JavaScriptObject srcImgMap, Element
img,
+ String src) /*-{
// No outstanding requests; load the image.
img.src = src;
-
+
// If the image was in cache, the load may have just happened
synchronously.
if (img.complete) {
// We're done
@@ -109,9 +125,9 @@
img.__kids = [];
img.__pendingSrc = src;
srcImgMap[src] = img;
-
+
var _onload = img.onload, _onerror = img.onerror, _onabort =
img.onabort;
-
+
// Same cleanup code matter what state we end up in.
function finish(_originalHandler) {
// Grab a copy of the kids.
@@ -142,7 +158,7 @@
img.onabort = function() {
finish(_onabort);
}
-
+
img.__cleanup = function() {
img.onload = _onload;
img.onerror = _onerror;
@@ -152,6 +168,15 @@
}
}-*/;
+ /**
+ * Cleanup an img element that was created using cloneNode.
+ *
+ * @param img the img element
+ */
+ private static native void cleanupExpandos(Element img) /*-{
+ img.__cleanup = img.__pendingSrc = img.__kids = null;
+ }-*/;
+
private static native void executeBackgroundImageCacheCommand() /*-{
// Fix IE background image refresh bug, present through IE6
// see http://www.mister-pixel.com/#Content__state=is_that_simple
@@ -180,17 +205,29 @@
}-*/;
/**
- * Removes a child image from its pending parent.
+ * Removes a child image from its pending parent. If checkOnly is true,
the
+ * method will only check that the child is a child of the parent.
+ *
+ * @param parent the top element that is loading the source
+ * @param child the child to remove
+ * @param checkOnly if true, only verify that the child is a child of
parent
+ * @return true if the child is found, false if not
*/
- private static native void removeChild(Element parent, Element child)
/*-{
+ private static native boolean removeChild(Element parent, Element child,
+ boolean checkOnly) /*-{
var kids = parent.__kids;
for (var i = 0, c = kids.length; i < c; ++i) {
if (kids[i] === child) {
- kids.splice(i, 1);
- child.__pendingSrc = null;
- return;
+ if (!checkOnly) {
+ kids.splice(i, 1);
+ child.__pendingSrc = null;
+ }
+ return true;
}
}
+
+ // If the child isn't in any kids lists, it must be a clone.
+ return false;
}-*/;
/**
=======================================
--- /trunk/user/src/com/google/gwt/layout/client/LayoutImplIE6.java Mon Jan
25 08:52:17 2010
+++ /trunk/user/src/com/google/gwt/layout/client/LayoutImplIE6.java Wed
Mar 3 05:43:42 2010
@@ -38,30 +38,6 @@
*/
class LayoutImplIE6 extends LayoutImplIE8 {
- private static boolean isIE6 = isIE6();
-
- // Stolen and modified from UserAgent.gwt.xml.
- // TODO(jgw): Get rid of this method, and switch to using soft
permutations
- // once they land in trunk.
- private static native boolean isIE6() /*-{
- function makeVersion(result) {
- return (parseInt(result[1]) * 1000) + parseInt(result[2]);
- }
-
- var ua = navigator.userAgent.toLowerCase();
- if (ua.indexOf("msie") != -1) {
- var result = /msie ([0-9]+)\.([0-9]+)/.exec(ua);
- if (result && result.length == 3) {
- var v = makeVersion(result);
- if (v < 7000) {
- return true;
- }
- }
- }
-
- return false;
- }-*/;
-
private static Element createStyleRuler(Element parent) {
DivElement styleRuler = Document.get().createDivElement();
DivElement styleInner = Document.get().createDivElement();
@@ -83,6 +59,10 @@
});
}
+ private static native boolean isIE6() /*-{
+ return @com.google.gwt.dom.client.DOMImplIE6::isIE6()();
+ }-*/;
+
@SuppressWarnings("unused") // called from JSNI
private static native void measureDecoration(Element elem) /*-{
var ruler = elem.__styleRuler;
@@ -139,7 +119,7 @@
@Override
public Element attachChild(Element parent, Element child, Element
before) {
- if (!isIE6) {
+ if (!isIE6()) {
return super.attachChild(parent, child, before);
}
@@ -165,7 +145,7 @@
@Override
public void fillParent(Element elem) {
- if (!isIE6) {
+ if (!isIE6()) {
super.fillParent(elem);
return;
}
@@ -175,7 +155,7 @@
@Override
public void finalizeLayout(Element parent) {
- if (!isIE6) {
+ if (!isIE6()) {
super.finalizeLayout(parent);
return;
}
@@ -188,13 +168,14 @@
public void initParent(Element parent) {
super.initParent(parent);
- if (isIE6) {
+ if (isIE6()) {
setPropertyElement(parent, "__styleRuler", createStyleRuler(parent));
}
}
+ @Override
public void layout(Layer layer) {
- if (!isIE6) {
+ if (!isIE6()) {
super.layout(layer);
return;
}
@@ -205,7 +186,7 @@
@Override
public void onAttach(Element parent) {
- if (isIE6) {
+ if (isIE6()) {
// No need to re-connect layer refs. This will be taken care of
// automatically in layout().
initResizeHandler(parent);
@@ -215,7 +196,7 @@
@Override
public void onDetach(Element parent) {
- if (isIE6) {
+ if (isIE6()) {
removeLayerRefs(parent);
removeResizeHandler(parent);
removeUnitChangeHandler(relativeRuler);
=======================================
---
/trunk/user/src/com/google/gwt/user/client/ui/impl/ClippedImageImplIE6.java
Fri Nov 20 16:10:50 2009
+++
/trunk/user/src/com/google/gwt/user/client/ui/impl/ClippedImageImplIE6.java
Wed Mar 3 05:43:42 2010
@@ -31,8 +31,6 @@
private static String moduleBaseUrlProtocol =
GWT.getHostPageBaseURL().startsWith("https") ? "https://"; : "http://";;
-
- private static boolean isIE6 = isIE6();
private static native void injectGlobalHandler() /*-{
$wnd.__gwt_transparentImgHandler = function (elem) {
@@ -40,28 +38,10 @@
@com.google.gwt.user.client.DOM::setImgSrc(Lcom/google/gwt/user/client/Element;Ljava/lang/String;)(elem,
@com.google.gwt.core.client.GWT::getModuleBaseURL()() + "clear.cache.gif");
};
}-*/;
-
- // Stolen and modified from UserAgent.gwt.xml.
- // TODO(jgw): Get rid of this method, and switch to using soft
permutations
- // once they land in trunk.
+
private static native boolean isIE6() /*-{
- function makeVersion(result) {
- return (parseInt(result[1]) * 1000) + parseInt(result[2]);
- }
-
- var ua = navigator.userAgent.toLowerCase();
- if (ua.indexOf("msie") != -1) {
- var result = /msie ([0-9]+)\.([0-9]+)/.exec(ua);
- if (result && result.length == 3) {
- var v = makeVersion(result);
- if (v < 7000) {
- return true;
- }
- }
- }
-
- return false;
- }-*/;
+ return @com.google.gwt.dom.client.DOMImplIE6::isIE6()();
+ }-*/;
public ClippedImageImplIE6() {
injectGlobalHandler();
@@ -70,7 +50,7 @@
@Override
public void adjust(Element clipper, String url, int left, int top, int
width,
int height) {
- if (!isIE6) {
+ if (!isIE6()) {
super.adjust(clipper, url, left, top, width, height);
return;
}
@@ -97,7 +77,7 @@
@Override
public Element createStructure(String url, int left, int top, int width,
int height) {
- if (!isIE6) {
+ if (!isIE6()) {
return super.createStructure(url, left, top, width, height);
}
@@ -111,7 +91,7 @@
@Override
public String getHTML(String url, int left, int top, int width, int
height) {
- if (!isIE6) {
+ if (!isIE6()) {
return super.getHTML(url, left, top, width, height);
}
@@ -146,7 +126,7 @@
@Override
public Element getImgElement(Image image) {
- if (!isIE6) {
+ if (!isIE6()) {
return super.getImgElement(image);
}
return image.getElement().getFirstChildElement();
=======================================
--- /trunk/user/test/com/google/gwt/dom/DOMTest.gwt.xml Thu Apr 3 09:07:28
2008
+++ /trunk/user/test/com/google/gwt/dom/DOMTest.gwt.xml Wed Mar 3 05:43:42
2010
@@ -13,4 +13,5 @@
<!-- limitations under the
License. -->
<module>
<inherits name="com.google.gwt.dom.DOM"/>
+ <public path="public-test"/>
</module>
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
