[Webkit-unassigned] [Bug 41171] JSC bindings for Image Resizer API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=41171


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #59681|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #2 from David Levin <levin at chromium.org>  2010-06-24 17:08:23 PST ---
(From update of attachment 59681)
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-callbacks.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-types.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.)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.