[webkit-reviews] review denied: [Bug 41171] JSC bindings for Image Resizer API : [Attachment 59681] JSC bindings and first wave of tests

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Sterling Swigart
<sswigart at google.com>'s request for review:
Bug 41171: JSC bindings for Image Resizer API
https://bugs.webkit.org/show_bug.cgi?id=41171

Attachment 59681: JSC bindings and first wave of tests
https://bugs.webkit.org/attachment.cgi?id=59681&action=review

------- Additional Comments from David Levin <levin at chromium.org>
WebCore/ChangeLog:23
 +	    (WebCore::AsyncImageResizer::start): Client of this class no longer
does any refcounting
Please add "."

WebCore/ChangeLog:29
 +	    (WebCore::AsyncImageResizer::CallbackInfo::CallbackInfo): Now only
stores necesseties.
Typo: necessities

WebCore/ChangeLog:16
 +	    * WebCore.xcodeproj/project.pbxproj: Ditto.
What about the andriod build file?

WebCore/WebCore.vcproj/WebCore.vcproj:40199
 +  
There appear to be some extra changes here -- some blank lines added?



WebCore/bindings/js/JSHTMLImageElementCustom.cpp:119
 +	   
resizeOptions->setAspectRatioOption(preserveAspectRatio.toBoolean(exec));
s/AspectRatioOption/PreserveAspectRatio/

WebCore/bindings/js/JSHTMLImageElementCustom.cpp:156
 +	HTMLImageElement* imp = static_cast<HTMLImageElement*>(impl());
Please avoid abbreviations in WebKit. Why not imageElement instead of imp?

WebCore/bindings/js/JSHTMLImageElementCustom.cpp:138
 +	ExceptionCode ec = 0;
Move this to right before it is used.

WebCore/html/AsyncImageResizer.cpp:49
 +	 new AsyncImageResizer(scriptExecutionContext, cachedImage, outputType,
successCallback, errorCallback, resizeOptions);
extra space here (at the beginning of the line).



WebCore/html/AsyncImageResizer.cpp:61
 +	m_callbackInfo = new CallbackInfo(this);
Doesn't CallbackInfo and AsyncImageResizer leak if
AsyncImageResizer::notifiyFinished never gets called?

WebCore/html/AsyncImageResizer.cpp:78
 +  void AsyncImageResizer::resizeComplete(RefPtr<Blob> blob)
Why not just pass a Blob* here (instead of a RefPtr<Blob>?

WebCore/html/HTMLImageElement.cpp:472
 +	// The taintsCanvas method also allows data URLs.
Ok, but ideally this answers a question that the code reader has. It would be
nice if this comment was a bit more explicit about what question it is
answering.

WebCore/html/HTMLImageElement.h:96
 +  
I'd remove the blank lines in this one instance (for the if/endif).


WebCore/html/ResizeOptions.cpp:47
 +  PassRefPtr<ResizeOptions> ResizeOptions::create() { return adoptRef(new
ResizeOptions()); }
Please make this multi-line.

WebCore/html/ResizeOptions.h:57
 +	void setWidth(int width);
No need to say "width" here as it is apparent due to the function name.

WebCore/html/ResizeOptions.h:60
 +	void setHeight(int height);
Ditto for "height".

WebCore/html/ResizeOptions.cpp:50
 +	: m_width()
Init to a valid value or omit this. (I'd suggest a valid value.)

LayoutTests/http/tests/misc/image-webkitGetImage-remote-origin.html-disabled:28

 +  <img id="testImage"
src="http://localhost:8000/../../fast/dom/HTMLImageElement/resources/blue_rect.
jpg" />
This line looks suspect to me because the http server shouldn't allow files
outside of the directory it is serving. (i.e. this
"http://localhost:8000/../.." shouldn't work).

Please add an image in the http/tests/misc/resources directory.

LayoutTests/fast/dom/HTMLImageElement/webkitGetImage/script-tests/test-callback
s.js:16
 +  // Only the error callback will currently never be called in this patch, so
test that it works for now
Please finish with a ".".

LayoutTests/fast/dom/HTMLImageElement/webkitGetImage/script-tests/argument-type
s.js:4
 +    try {
Formatting for tests isn't as strict but it would be nice to be consistent
within this file. You use 4 spaces elsewhere.

LayoutTests/fast/dom/HTMLImageElement/webkitGetImage/script-tests/image-has-no-
source.js:10
 +		i.webkitGetImage("image/jpeg", emptyFunction);
TABs in the js file. (Not strictly enforced to not have these but it is nice to
avoid.)