Implement ImageBitmapFactories as a supplementary module.

Implement ImageBitmapFactories as a supplementary module.

This class will also be added as a supplement to WorkerGlobalScope in the future.

[arbesfeld, 2013-08-12 18:21:27]

PTAL

[do-not-use, 2013-08-12 18:28:40]

The CL description says that ImageBitmapFactories will be added as a supplement
to WorkerGlobalScope in the future. Therefore, I think keeping the IDL
implements statement is a good idea instead of using a partial interface.

[arbesfeld, 2013-08-12 18:33:32]

On 2013/08/12 18:28:40, Christophe Dumez wrote:
> The CL description says that ImageBitmapFactories will be added as a
supplement
> to WorkerGlobalScope in the future. Therefore, I think keeping the IDL
> implements statement is a good idea instead of using a partial interface.

Ok, done.

[do-not-use, 2013-08-12 18:43:58]

https://codereview.chromium.org/22906002/diff/5001/Source/modules/imagebitmap...
File Source/modules/imagebitmap/ImageBitmapFactories.cpp (right):

https://codereview.chromium.org/22906002/diff/5001/Source/modules/imagebitmap...
Source/modules/imagebitmap/ImageBitmapFactories.cpp:32: #include
"modules/imagebitmap/ImageBitmapFactories.h"
Someone needs to comment about the move to modules/. I do not know what the
rules are for that.

https://codereview.chromium.org/22906002/diff/5001/Source/modules/imagebitmap...
Source/modules/imagebitmap/ImageBitmapFactories.cpp:48:
ImageBitmapFactories::ImageBitmapFactories(DOMWindow* window)
If we stop subclassing DOMWindowProperty then we no longer need this
constructor.

https://codereview.chromium.org/22906002/diff/5001/Source/modules/imagebitmap...
File Source/modules/imagebitmap/ImageBitmapFactories.h (right):

https://codereview.chromium.org/22906002/diff/5001/Source/modules/imagebitmap...
Source/modules/imagebitmap/ImageBitmapFactories.h:50: class ImageBitmapFactories
: public Supplement<DOMWindow>, public DOMWindowProperty {
Do you really need to subclass DOMWindowProperty? ImageBitmapFactories does not
seem to add any property to the Window object.

[arbesfeld, 2013-08-12 19:01:28]

On 2013/08/12 18:43:58, Christophe Dumez wrote:
>
https://codereview.chromium.org/22906002/diff/5001/Source/modules/imagebitmap...
> File Source/modules/imagebitmap/ImageBitmapFactories.cpp (right):
> 
>
https://codereview.chromium.org/22906002/diff/5001/Source/modules/imagebitmap...
> Source/modules/imagebitmap/ImageBitmapFactories.cpp:32: #include
> "modules/imagebitmap/ImageBitmapFactories.h"
> Someone needs to comment about the move to modules/. I do not know what the
> rules are for that.
> 
>
https://codereview.chromium.org/22906002/diff/5001/Source/modules/imagebitmap...
> Source/modules/imagebitmap/ImageBitmapFactories.cpp:48:
> ImageBitmapFactories::ImageBitmapFactories(DOMWindow* window)
> If we stop subclassing DOMWindowProperty then we no longer need this
> constructor.
> 
>
https://codereview.chromium.org/22906002/diff/5001/Source/modules/imagebitmap...
> File Source/modules/imagebitmap/ImageBitmapFactories.h (right):
> 
>
https://codereview.chromium.org/22906002/diff/5001/Source/modules/imagebitmap...
> Source/modules/imagebitmap/ImageBitmapFactories.h:50: class
ImageBitmapFactories
> : public Supplement<DOMWindow>, public DOMWindowProperty {
> Do you really need to subclass DOMWindowProperty? ImageBitmapFactories does
not
> seem to add any property to the Window object.

Ah ok, I was following the pattern of the other Supplements but I see that it is
not actually needed.

[do-not-use, 2013-08-12 20:12:29]

LGTM but someone else needs to approve the move to modules/

[abarth-chromium, 2013-08-12 20:35:59]

lgtm

https://codereview.chromium.org/22906002/diff/12001/Source/modules/imagebitma...
File Source/modules/imagebitmap/ImageBitmapFactories.cpp (right):

https://codereview.chromium.org/22906002/diff/12001/Source/modules/imagebitma...
Source/modules/imagebitmap/ImageBitmapFactories.cpp:225: ImageBitmapFactories*
ImageBitmapFactories::from(DOMWindow* window)
I don't see any callers of this function.  You shouldn't need it.

https://codereview.chromium.org/22906002/diff/12001/Source/modules/imagebitma...
File Source/modules/imagebitmap/ImageBitmapFactories.h (right):

https://codereview.chromium.org/22906002/diff/12001/Source/modules/imagebitma...
Source/modules/imagebitmap/ImageBitmapFactories.h:49: class ImageBitmapFactories
: public Supplement<DOMWindow> {
Why does this need to Supplement<DOMWindow>?  There's no storage in this class. 
You can just leave it the way it was before.

[abarth-chromium, 2013-08-12 20:36:21]

Do you want to add yourself as an OWNER of the modules/imagebitmap ?

[do-not-use, 2013-08-12 20:38:31]

On 2013/08/12 20:35:59, abarth wrote:
> lgtm
> 
>
https://codereview.chromium.org/22906002/diff/12001/Source/modules/imagebitma...
> File Source/modules/imagebitmap/ImageBitmapFactories.cpp (right):
> 
>
https://codereview.chromium.org/22906002/diff/12001/Source/modules/imagebitma...
> Source/modules/imagebitmap/ImageBitmapFactories.cpp:225: ImageBitmapFactories*
> ImageBitmapFactories::from(DOMWindow* window)
> I don't see any callers of this function.  You shouldn't need it.
> 
>
https://codereview.chromium.org/22906002/diff/12001/Source/modules/imagebitma...
> File Source/modules/imagebitmap/ImageBitmapFactories.h (right):
> 
>
https://codereview.chromium.org/22906002/diff/12001/Source/modules/imagebitma...
> Source/modules/imagebitmap/ImageBitmapFactories.h:49: class
ImageBitmapFactories
> : public Supplement<DOMWindow> {
> Why does this need to Supplement<DOMWindow>?  There's no storage in this
class. 
> You can just leave it the way it was before.

This is a preparation patch for:
https://codereview.chromium.org/20748002/

This is why Supplement is used. Please let me know if this is not the right
approach as I advised to use it on the other CL.

[abarth-chromium, 2013-08-12 20:39:18]

On 2013/08/12 20:38:31, Christophe Dumez wrote:
> This is a preparation patch for:
> https://codereview.chromium.org/20748002/
> 
> This is why Supplement is used. Please let me know if this is not the right
> approach as I advised to use it on the other CL.

That's fine.  We can add Supplement in that CL, which is where it's needed.

[arbesfeld, 2013-08-12 20:47:35]

On 2013/08/12 20:39:18, abarth wrote:
> On 2013/08/12 20:38:31, Christophe Dumez wrote:
> > This is a preparation patch for:
> > https://codereview.chromium.org/20748002/
> > 
> > This is why Supplement is used. Please let me know if this is not the right
> > approach as I advised to use it on the other CL.
> 
> That's fine.  We can add Supplement in that CL, which is where it's needed.

I will just merge this CL with https://codereview.chromium.org/20748002/