[Webkit-unassigned] [Bug 40018] Image Resizer Patch 1: Adding basic classes to perform the resizing

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Sterling Swigart <sswigart at google.com>  2010-06-03 16:05:30 PST ---
(In reply to comment #4)
> (From update of attachment 57604 [details])
> One generic note: It's better to add the new files to all build files in the same patch, not only to xcodeproj.

Oh, I didn't know about the other ones, but I followed 39083 and added references to the new files in (hopefully) all of the necessary places.

> It also would be nice to add FIXME into places that will be implemented later.

I added these where applicable.

> 
> WebCore/html/AsyncImageResizer.h:45
>  +  enum MimeType {
> This adds a MimeType enum to WebCore namespace. It's a big namespace, so it would be better to have the enum scoped inside of AsyncImageResizer class. In this case, it could be named OutputType and have values JPEG and PNG (caps since they are abbreviations).

The enums are now part of the AsyncImageResizer class. Referring to them from outside the class is quite a mouthful now, but I can deal with that.

> 
> WebCore/html/AsyncImageResizer.h:52
>  +      AsyncImageResizer(CachedImage*, MimeType, float width, float height, ScriptValue successCallback, ScriptValue errorCallback, float quality, bool preserveAspectRatio, bool rotateExif);
> If width and height are in pixels, then we should use IntSize instead of them both. The parameter could be named more descriptive, perhaps 'desiredBounds', which would convey that it's not necessarily the size of the resulting image, because aspect ration could dictate one of the sizes to be different.

Good call. Done.

> 
> Also, the bool parameters usually are represented as enums with 2 nicely named values, for the better readability in callsites. For example, 'enum AspectRatioOption { PreserveAspectRatio, IgnoreAspectRatio }'.

Done.

> 
> WebCore/html/AsyncImageResizer.h:60
>  +      void imageChanged(CachedImage*, const IntRect* = 0);
> virtual overrides usually retain 'virtual' keyword. 

Done.

> 
> WebCore/html/AsyncImageResizer.cpp:39
>  +  
> This empty line is not needed.

Deleted.

> 
> WebCore/html/AsyncImageResizer.cpp:49
>  +      m_thread = ImageResizerThread::create(this, mimeType, width, height, quality, preserveAspectRatio, rotateExif);
> This constructor gets a raw pointer to CachedImage. It seems it needs CachedImage to be alive even after constructor exits. Perhaps the code you are going to write in the future will call getBlobAsync right away, but this is not obvious from the code now. I think it is better to make it a client of CachedImage right here, this will keep CachedImage alive.

That's a good point. Now, constructing AsyncImageResizer is all that needs to be done to do the resizing.

> 
> WebCore/html/AsyncImageResizer.cpp:56
>  +  void AsyncImageResizer::getBlobAsync()
> 'getFoo' normally should return Foo. It's hard to tell what's better name is since this method is not used and incomplete. Maybe remove it for now, moving addClient to the constructor?

I removed this function based on your last comment.

> 
> WebCore/html/ImageResizerThread.h:44
>  +  void returnBlob(void* imageResizerThread);
> This function is not in the patch, so this could be removed.

It is now called returnBlobOrError, and it is now functional based on your request below. It will call the correct callback function in AsyncImageResizer.

> 
> WebCore/html/ImageResizerThread.h:58
>  +      // Threading attributes
> Comments should end with a '.'.

Check.

> 
> WebCore/html/ImageResizerThread.h:59
>  +      Mutex m_threadCreationMutex;
> If we don't really use this mutex now, lets remove it and add when actual code that uses it is added.

This is used to protect references to m_threadID. The other threading classes do the same thing. You don't think I need it?

> 
> WebCore/html/ImageResizerThread.h:65
>  +      bool m_error;
> These fields are not used in this patch, lets not to add them.

Re-added in the struct CallbackInfo, which is now used and filled.

> 
> WebCore/html/ImageResizerThread.h:67
>  +      // Image attributes
> '.'

Check.

> 
> WebCore/html/ImageResizerThread.h:62
>  +      // Results and callback information
> '.' at the end.

Check.

> 
> WebCore/html/ImageResizerThread.cpp:44
>  +  void returnBlobOrError(void* /*imageResizerThread*/)
> It seems this patch's idea is to add a resizer class and a thread that gets started, does the job (except the job code will be added later), then posts callback to the resizer and exits. It seems the patch does all that except actually calling resizer back when thread terminates. It'd be nice to complete this piece of functionality, even if the callback on the resizer object will be empty for now.

I completed this part. Since this function isn't in the ImageResizerThread class, it can't access its fields unless I expose a number of getters, so I store all the callback information in the new struct CallbackInfo.

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