Bug 41171

Attachment 59681 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3316681

Comment on attachment 59681 [details] JSC bindings and first wave of tests 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.)

Created attachment 59724 [details] JSC bindings

Fixed. Notes: > > WebCore/bindings/js/JSHTMLImageElementCustom.cpp:119 > + resizeOptions->setAspectRatioOption(preserveAspectRatio.toBoolean(exec)); > s/AspectRatioOption/PreserveAspectRatio/ > Like we spoke about, I retained the current naming. > > > WebCore/html/AsyncImageResizer.cpp:61 > + m_callbackInfo = new CallbackInfo(this); > Doesn't CallbackInfo and AsyncImageResizer leak if AsyncImageResizer::notifiyFinished never gets called? > I now inherit from ActiveDOMObject. I made the object unsuspendable; is this okay? I couldn't determine from the code what exactly needed to be done (and why, mostly) to suspend AsyncImageResizer. > WebCore/WebCore.vcproj/WebCore.vcproj:40199 > + > There appear to be some extra changes here -- some blank lines added? > For some reason the git diff command puts random newlines in the vcproj diff. They're not actually in my branch's vcproj file. Anyway, I deleted them.

Created attachment 59727 [details] JSC bindings

Created attachment 59728 [details] JSC bindings

Attachment 59728 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3319688

Comment on attachment 59728 [details] JSC bindings Just some minor clean-up needed mostly around the lifetime/callbacks in AsyncImageResizer as discussed.

Created attachment 59779 [details] JSC bindings

Created attachment 59782 [details] JSC bindings

Comment on attachment 59782 [details] JSC bindings One more change that I can do on landing. I think I must have miscommunicated about what would be ideal here. WebCore/html/AsyncImageResizer.cpp:73 + m_callbackInfo = 0; I think this leaks the pointer that was in m_callbackInfo. The logic should be reversed in this area. And the final if should be if (m_callbackInfo) { resizeError(); delete m_callbackInfo; }

Created attachment 59802 [details] JSC bindings

Comment on attachment 59802 [details] JSC bindings Looks good. I'll land it.

Comment on attachment 59782 [details] JSC bindings Cleared David Levin's review+ from obsolete attachment 59782 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Was this landed?

(In reply to comment #15) > Was this landed? No, I need to do that. Sterling's internship ended, so I need to take over finishing this completely, but I'm currently working on another item in Chrome, so I haven't been in a rush to land this.

Can we just cq+ it? :) It's been a few months. :)

Comment on attachment 59802 [details] JSC bindings This work has been taken over by someone else. I expect that they will take up this patch as part of that work, so r- for now (until they get to it, bring it up to date, etc.).

As a small note, I think there was a small issue in the final patch (perhaps related to comment 11), so even if the same patch comes up, lifetimes, etc. need to be checked carefully.