Blob creation methods for ImageBitmap.

Blob creation methods for ImageBitmap.

Error handling for non-image blobs will be done in a future patch, once
promises have been implemented.
 
BUG=260984, 166658
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156101

[arbesfeld, 2013-07-26 15:33:01]

PTAL. Thanks in advance.

[Justin Novosad, 2013-07-26 18:15:58]

https://codereview.chromium.org/20748002/diff/1/LayoutTests/fast/canvas/canva...
File LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage.html (right):

https://codereview.chromium.org/20748002/diff/1/LayoutTests/fast/canvas/canva...
LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage.html:144:
eventSender.mouseUp();
This is a weird indirect way to get a blob.  Since the object of this test has
nothing to do with drag events, you should use an XmlHttpRequest to fetch a blob

https://codereview.chromium.org/20748002/diff/1/LayoutTests/fast/canvas/canva...
File LayoutTests/fast/canvas/canvas-createImageBitmap-invalid-args.html (right):

https://codereview.chromium.org/20748002/diff/1/LayoutTests/fast/canvas/canva...
LayoutTests/fast/canvas/canvas-createImageBitmap-invalid-args.html:72:
eventSender.mouseUp();
Same here. Use XmlHttpRequest

https://codereview.chromium.org/20748002/diff/1/Source/core/page/DOMWindow.h
File Source/core/page/DOMWindow.h (right):

https://codereview.chromium.org/20748002/diff/1/Source/core/page/DOMWindow.h#...
Source/core/page/DOMWindow.h:421: HashSet<RefPtr<ImageBitmap> >
m_pendingImageBitmaps;
Before we start adding data members to Window, we should first break-out the
creatImageBitmap factories into a separate class because we will also need to
expose this in worker global scopes.

https://codereview.chromium.org/20748002/diff/1/Source/core/page/ImageBitmap.cpp
File Source/core/page/ImageBitmap.cpp (right):

https://codereview.chromium.org/20748002/diff/1/Source/core/page/ImageBitmap....
Source/core/page/ImageBitmap.cpp:90: m_loader = adoptPtr(new
FileReaderLoader(FileReaderLoader::ReadAsDataURL, this));
This loading functionality should not live in ImageBitmap. The class where we
put the factories should take care of this, and only construct the image bitmap
once the data is ready.  The imageBitmap class should only have data members to
store the data that needs to persist in the image bitmap.

[arbesfeld, 2013-08-06 17:41:01]

New patch for review.

[Justin Novosad, 2013-08-07 19:35:12]

https://codereview.chromium.org/20748002/diff/8001/Source/core/page/DOMWindow.h
File Source/core/page/DOMWindow.h (right):

https://codereview.chromium.org/20748002/diff/8001/Source/core/page/DOMWindow...
Source/core/page/DOMWindow.h:248: void loadBlobAsync(ImageBitmapLoader*, Blob*);
Why does this need to live in DOM window? it does not seem to interact with the
rest of this class in any way.

https://codereview.chromium.org/20748002/diff/8001/Source/core/page/ImageBitm...
File Source/core/page/ImageBitmapFactories.cpp (right):

https://codereview.chromium.org/20748002/diff/8001/Source/core/page/ImageBitm...
Source/core/page/ImageBitmapFactories.cpp:250: ImageBitmapLoader* loader = new
ImageBitmapLoader(eventTarget, resolver, 0);
AFAICT, we you are leaking the loader here.

[arbesfeld, 2013-08-07 21:18:48]

On 2013/08/07 19:35:12, junov wrote:
>
https://codereview.chromium.org/20748002/diff/8001/Source/core/page/DOMWindow.h
> File Source/core/page/DOMWindow.h (right):
> 
>
https://codereview.chromium.org/20748002/diff/8001/Source/core/page/DOMWindow...
> Source/core/page/DOMWindow.h:248: void loadBlobAsync(ImageBitmapLoader*,
Blob*);
> Why does this need to live in DOM window? it does not seem to interact with
the
> rest of this class in any way.
> 
>
https://codereview.chromium.org/20748002/diff/8001/Source/core/page/ImageBitm...
> File Source/core/page/ImageBitmapFactories.cpp (right):
> 
>
https://codereview.chromium.org/20748002/diff/8001/Source/core/page/ImageBitm...
> Source/core/page/ImageBitmapFactories.cpp:250: ImageBitmapLoader* loader = new
> ImageBitmapLoader(eventTarget, resolver, 0);
> AFAICT, we you are leaking the loader here.

Something has to hold the ref to the ImageBitmapLoader. While ideally it would
probably be the Promise object itself, for now I am having the DOMWindow hold a
HashSet of pending loaders. Since this was confusing, I moved the methods into a
class that is in ImageBitmapFactories (which is then extended by the DOMWindow).

Unless I am missing something, the .idl "implements" only lets you use static
methods. This is making the code messy since there is both a
"ImageBitmapFactories" namespace that has static methods and an
"ImageBitmapFactory" class that has class variables and methods. We could
probably avoid this but it would eventually require code duplication in
WorkerGlobalScope and Window.

[Justin Novosad, 2013-08-08 15:50:33]

On 2013/08/07 21:18:48, arbesfeld wrote:

> Unless I am missing something, the .idl "implements" only lets you use static
> methods.

Does it have to be a namespace, or can it be a class with static methods.  If
possible, it may be worth considering merging ImageBitmapFactories and
ImageBitmapFactory.

[arbesfeld, 2013-08-08 17:42:39]

Ah, of course. Done.

[Justin Novosad, 2013-08-08 19:05:54]

This is looking tremendously better than the previous iteration.

You need to remove the pattern.png file.  I committed it this morning so if you
just sync and rebase your branch, the file will be there.

Something to think about, but you don't have to take care of it right away in
this patch: You have a lot of calls to EventTarget::toDOMWindow in
ImageBitmapFactories. You will need to change those calls when you expose
ImageBitmapFactories in workers. For accessing the non-static methods of
ImageBitmapFactories, you may want to implement an
EventTarget::toImageBitmapFactories method.  Not sure what hoops you'll have to
jump through for the security origin checks though.

LGTM, but me is no owner

[arbesfeld, 2013-08-12 14:14:32]

Still need an OWNER. Stephen?

[Stephen White, 2013-08-12 14:53:24]

On 2013/08/12 14:14:32, arbesfeld wrote:
> Still need an OWNER. Stephen?

This looks ok to me, but I'd like someone more familiar with DOMWindow.h to take
a look at those changes. It seems a little circuitous to pull in the IDL
bindings in via inheritance (not new to this change, but I'm thinking of folks
trying to find these functions later), and adding a new base class does increase
the memory footprint of DOMWindow itself slightly. Not sure if that's a big
deal.

[do-not-use, 2013-08-12 15:02:19]

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/DOMWindow.h
File Source/core/page/DOMWindow.h (right):

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/DOMWindo...
Source/core/page/DOMWindow.h:83: class DOMWindow : public RefCounted<DOMWindow>,
public ScriptWrappable, public EventTarget, public FrameDestructionObserver,
public ImageBitmapFactories, public Supplementable<DOMWindow> {
Why do we need to subclass ImageBitmapFactories?

ImageBitmapFactories is a partial interface (using implements statement) so we
should not need to have Window inherit ImageBitmapFactories.

ImageBitmapFactories static functions should already take the DOMWindow in
argument.

[arbesfeld, 2013-08-12 15:08:14]

On 2013/08/12 15:02:19, Christophe Dumez wrote:
>
https://codereview.chromium.org/20748002/diff/29001/Source/core/page/DOMWindow.h
> File Source/core/page/DOMWindow.h (right):
> 
>
https://codereview.chromium.org/20748002/diff/29001/Source/core/page/DOMWindo...
> Source/core/page/DOMWindow.h:83: class DOMWindow : public
RefCounted<DOMWindow>,
> public ScriptWrappable, public EventTarget, public FrameDestructionObserver,
> public ImageBitmapFactories, public Supplementable<DOMWindow> {
> Why do we need to subclass ImageBitmapFactories?
> 
> ImageBitmapFactories is a partial interface (using implements statement) so we
> should not need to have Window inherit ImageBitmapFactories.
> 
> ImageBitmapFactories static functions should already take the DOMWindow in
> argument.

We also require some member variables for asynchronous loading of ImageBitmaps.
We need the DOMWindow to hold onto these loaders objects, unless there is
another object that can hold the reference?

[do-not-use, 2013-08-12 15:15:44]

On 2013/08/12 15:08:14, arbesfeld wrote:
> On 2013/08/12 15:02:19, Christophe Dumez wrote:
> >
>
https://codereview.chromium.org/20748002/diff/29001/Source/core/page/DOMWindow.h
> > File Source/core/page/DOMWindow.h (right):
> > 
> >
>
https://codereview.chromium.org/20748002/diff/29001/Source/core/page/DOMWindo...
> > Source/core/page/DOMWindow.h:83: class DOMWindow : public
> RefCounted<DOMWindow>,
> > public ScriptWrappable, public EventTarget, public FrameDestructionObserver,
> > public ImageBitmapFactories, public Supplementable<DOMWindow> {
> > Why do we need to subclass ImageBitmapFactories?
> > 
> > ImageBitmapFactories is a partial interface (using implements statement) so
we
> > should not need to have Window inherit ImageBitmapFactories.
> > 
> > ImageBitmapFactories static functions should already take the DOMWindow in
> > argument.
> 
> We also require some member variables for asynchronous loading of
ImageBitmaps.
> We need the DOMWindow to hold onto these loaders objects, unless there is
> another object that can hold the reference?

Can't we use Supplementable like for other partial interfaces? See for e.g.
DOMWindowCrypto.

[do-not-use, 2013-08-12 15:15:59]

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
File Source/core/page/ImageBitmap.cpp (right):

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.cpp:171: RefPtr<ImageBitmap>
imageBitmap(adoptRef(new ImageBitmap(image, normalizedCropRect)));
nit: we don't really need this local variable, we can return directly.

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
File Source/core/page/ImageBitmapFactories.cpp (right):

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
Source/core/page/ImageBitmapFactories.cpp:289: RefPtr<ImageBitmap>
nullImageBitmap;
It looks like we don't really need this variable. Can't we simply pass 0 (or
nullptr) to the reject() call below?

[do-not-use, 2013-08-12 15:36:14]

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
File Source/core/page/ImageBitmapFactories.h (right):

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
Source/core/page/ImageBitmapFactories.h:80: static PassRefPtr<ImageBitmapLoader>
create(ImageBitmapFactories* factory, PassRefPtr<ScriptPromiseResolver>
resolver, IntRect* cropRect)
It is confusing to take an IntRect raw pointer in argument and take ownership of
it.

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
Source/core/page/ImageBitmapFactories.h:82: RefPtr<ImageBitmapLoader>
imageBitmapLoader = adoptRef(new ImageBitmapLoader(factory, resolver,
cropRect));
nit: We don't really need this local variable.

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
Source/core/page/ImageBitmapFactories.h:96: virtual void didStartLoading() { }
Missing OVERRIDE (below as well)

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
Source/core/page/ImageBitmapFactories.h:105: IntRect* m_cropRect;
Why does m_cropRect need to be a pointer? Do we need to distinguish NULL and
IntRect() ? Also, it seems to be leaking currently.

[arbesfeld, 2013-08-12 15:49:39]

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
File Source/core/page/ImageBitmap.cpp (right):

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.cpp:171: RefPtr<ImageBitmap>
imageBitmap(adoptRef(new ImageBitmap(image, normalizedCropRect)));
On 2013/08/12 15:15:59, Christophe Dumez wrote:
> nit: we don't really need this local variable, we can return directly.

Done.

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
File Source/core/page/ImageBitmapFactories.h (right):

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
Source/core/page/ImageBitmapFactories.h:82: RefPtr<ImageBitmapLoader>
imageBitmapLoader = adoptRef(new ImageBitmapLoader(factory, resolver,
cropRect));
On 2013/08/12 15:36:15, Christophe Dumez wrote:
> nit: We don't really need this local variable.

Done.

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
Source/core/page/ImageBitmapFactories.h:96: virtual void didStartLoading() { }
On 2013/08/12 15:36:15, Christophe Dumez wrote:
> Missing OVERRIDE (below as well)

Done.

https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
Source/core/page/ImageBitmapFactories.h:105: IntRect* m_cropRect;
On 2013/08/12 15:36:15, Christophe Dumez wrote:
> Why does m_cropRect need to be a pointer? Do we need to distinguish NULL and
> IntRect() ? Also, it seems to be leaking currently.

ImageBitmaps created with no x/y/w/h arguments are not cropped, so I use a NULL
IntRect to represent this case. An alternative would be that we use IntRect()
for the uncropped case. Is that more clear? The reason I avoided this was
because an ImageBitmap created with an IntRect(0, 0, 0, 0) would normally throw
an error, so it might be confusing as to why we are throwing an exception for
one overload and using the IntRect() for the other.

[do-not-use, 2013-08-12 15:56:49]

On 2013/08/12 15:49:39, arbesfeld wrote:
>
https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
> File Source/core/page/ImageBitmap.cpp (right):
> 
>
https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
> Source/core/page/ImageBitmap.cpp:171: RefPtr<ImageBitmap>
> imageBitmap(adoptRef(new ImageBitmap(image, normalizedCropRect)));
> On 2013/08/12 15:15:59, Christophe Dumez wrote:
> > nit: we don't really need this local variable, we can return directly.
> 
> Done.
> 
>
https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
> File Source/core/page/ImageBitmapFactories.h (right):
> 
>
https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
> Source/core/page/ImageBitmapFactories.h:82: RefPtr<ImageBitmapLoader>
> imageBitmapLoader = adoptRef(new ImageBitmapLoader(factory, resolver,
> cropRect));
> On 2013/08/12 15:36:15, Christophe Dumez wrote:
> > nit: We don't really need this local variable.
> 
> Done.
> 
>
https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
> Source/core/page/ImageBitmapFactories.h:96: virtual void didStartLoading() { }
> On 2013/08/12 15:36:15, Christophe Dumez wrote:
> > Missing OVERRIDE (below as well)
> 
> Done.
> 
>
https://codereview.chromium.org/20748002/diff/29001/Source/core/page/ImageBit...
> Source/core/page/ImageBitmapFactories.h:105: IntRect* m_cropRect;
> On 2013/08/12 15:36:15, Christophe Dumez wrote:
> > Why does m_cropRect need to be a pointer? Do we need to distinguish NULL and
> > IntRect() ? Also, it seems to be leaking currently.
> 
> ImageBitmaps created with no x/y/w/h arguments are not cropped, so I use a
NULL
> IntRect to represent this case. An alternative would be that we use IntRect()
> for the uncropped case. Is that more clear? The reason I avoided this was
> because an ImageBitmap created with an IntRect(0, 0, 0, 0) would normally
throw
> an error, so it might be confusing as to why we are throwing an exception for
> one overload and using the IntRect() for the other.

I don't know this part of the code enough to comment on the ptr vs IntRect
issue. However, if we do want to use a pointer, we should probably use OwnPtr
instead to stop leaking.

[arbesfeld, 2013-08-12 16:07:57]

I didn't know about the Supplement class. Since ImageBitmapFactories will also
be implemented by WorkerGlobalScope, is there an intelligent way to use the
Supplement class for both?

[do-not-use, 2013-08-12 17:03:42]

On 2013/08/12 16:07:57, arbesfeld wrote:
> I didn't know about the Supplement class. Since ImageBitmapFactories will also
> be implemented by WorkerGlobalScope, is there an intelligent way to use the
> Supplement class for both?

I don't think there is any example for having a class "supplement" several other
interfaces yet. However, in theory, I don't see anything preventing you from
having ImageBitmapFactories subclass both Supplement<DOMWindow> and
Supplement<WorkerGlobalScope>.

[arbesfeld, 2013-08-12 21:11:10]

Merged with https://codereview.chromium.org/22906002/

ImageBitmapFactories now implements supplementable and lives in the modules/
directory.

[do-not-use, 2013-08-12 21:46:19]

LGTM.

https://codereview.chromium.org/20748002/diff/57001/Source/core/page/ImageBit...
File Source/core/page/ImageBitmap.cpp (right):

https://codereview.chromium.org/20748002/diff/57001/Source/core/page/ImageBit...
Source/core/page/ImageBitmap.cpp:177: RefPtr<ImageBitmap>
imageBitmap(adoptRef(new ImageBitmap(image, normalizedCropRect)));
nit: Useless local variable. We can return adoptRef(new ImageBitmap(image,
normalizedCr opRect)); directly.

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

https://codereview.chromium.org/20748002/diff/57001/Source/modules/imagebitma...
Source/modules/imagebitmap/ImageBitmapFactories.cpp:197:
RefPtr<ImageBitmapLoader> loader =
ImageBitmapFactories::ImageBitmapLoader::create(ImageBitmapFactories::from(eventTarget->toDOMWindow()),
resolver, cropRect);
nit: superfluous 'ImageBitmapFactories::' ?

https://codereview.chromium.org/20748002/diff/57001/Source/modules/imagebitma...
Source/modules/imagebitmap/ImageBitmapFactories.cpp:217:
RefPtr<ImageBitmapLoader> loader =
ImageBitmapFactories::ImageBitmapLoader::create(ImageBitmapFactories::from(eventTarget->toDOMWindow()),
resolver, cropRect);
nit: superfluous 'ImageBitmapFactories::' ?

[arbesfeld, 2013-08-12 21:50:21]

On 2013/08/12 21:46:19, Christophe Dumez wrote:
> LGTM.
> 
>
https://codereview.chromium.org/20748002/diff/57001/Source/core/page/ImageBit...
> File Source/core/page/ImageBitmap.cpp (right):
> 
>
https://codereview.chromium.org/20748002/diff/57001/Source/core/page/ImageBit...
> Source/core/page/ImageBitmap.cpp:177: RefPtr<ImageBitmap>
> imageBitmap(adoptRef(new ImageBitmap(image, normalizedCropRect)));
> nit: Useless local variable. We can return adoptRef(new ImageBitmap(image,
> normalizedCr opRect)); directly.
> 
>
https://codereview.chromium.org/20748002/diff/57001/Source/modules/imagebitma...
> File Source/modules/imagebitmap/ImageBitmapFactories.cpp (right):
> 
>
https://codereview.chromium.org/20748002/diff/57001/Source/modules/imagebitma...
> Source/modules/imagebitmap/ImageBitmapFactories.cpp:197:
> RefPtr<ImageBitmapLoader> loader =
>
ImageBitmapFactories::ImageBitmapLoader::create(ImageBitmapFactories::from(eventTarget->toDOMWindow()),
> resolver, cropRect);
> nit: superfluous 'ImageBitmapFactories::' ?
> 
>
https://codereview.chromium.org/20748002/diff/57001/Source/modules/imagebitma...
> Source/modules/imagebitmap/ImageBitmapFactories.cpp:217:
> RefPtr<ImageBitmapLoader> loader =
>
ImageBitmapFactories::ImageBitmapLoader::create(ImageBitmapFactories::from(eventTarget->toDOMWindow()),
> resolver, cropRect);
> nit: superfluous 'ImageBitmapFactories::' ?

Ok, fixed. Thanks for your help!

[commit-bot: I haz the power, 2013-08-12 21:52:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arbesfeld@chromium.org/20748002/68001

[commit-bot: I haz the power, 2013-08-12 23:40:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arbesfeld@chromium.org/20748002/84001

[arbesfeld, 2013-08-13 20:02:56]

Please review the latest patch. The drawImage test was failing in the Debug
build due to an issue with storing the ImageBitmapLoaders in the HashSet, so we
instead just hold raw pointers in a linked list and delete the
ImageBitmapLoaders once they have completed.

[do-not-use, 2013-08-14 07:55:07]

On 2013/08/13 20:02:56, arbesfeld wrote:
> Please review the latest patch. The drawImage test was failing in the Debug
> build due to an issue with storing the ImageBitmapLoaders in the HashSet, so
we
> instead just hold raw pointers in a linked list and delete the
> ImageBitmapLoaders once they have completed.

I much preferred the previous iteration as it was simpler and there was no
manual memory handling.
What was wrong with using a HashSet of RefPtr? This is quite common in
Blink/WebKit.

[do-not-use, 2013-08-14 08:10:29]

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

https://codereview.chromium.org/20748002/diff/68001/Source/modules/imagebitma...
Source/modules/imagebitmap/ImageBitmapFactories.cpp:197:
RefPtr<ImageBitmapLoader> loader =
ImageBitmapFactories::ImageBitmapLoader::create(from(eventTarget->toDOMWindow()),
resolver, cropRect);
e.g. You would add loader to m_pendingLoaders here, right after creating it,
while you have a RefPtr to it.

https://codereview.chromium.org/20748002/diff/68001/Source/modules/imagebitma...
Source/modules/imagebitmap/ImageBitmapFactories.cpp:297:
m_factory->didStartLoading(this);
I believe the issue was that you were creating a smart pointer from a raw one
without adopting here. Likely you would hit the !m_adoptionIsRequired assertion
in ref().

One solution to this would be to store the loader in the HashSet when it is
created, instead of when it starts loading.

[arbesfeld, 2013-08-14 14:31:45]

On 2013/08/14 08:10:29, Christophe Dumez wrote:
>
https://codereview.chromium.org/20748002/diff/68001/Source/modules/imagebitma...
> File Source/modules/imagebitmap/ImageBitmapFactories.cpp (right):
> 
>
https://codereview.chromium.org/20748002/diff/68001/Source/modules/imagebitma...
> Source/modules/imagebitmap/ImageBitmapFactories.cpp:197:
> RefPtr<ImageBitmapLoader> loader =
>
ImageBitmapFactories::ImageBitmapLoader::create(from(eventTarget->toDOMWindow()),
> resolver, cropRect);
> e.g. You would add loader to m_pendingLoaders here, right after creating it,
> while you have a RefPtr to it.
> 
>
https://codereview.chromium.org/20748002/diff/68001/Source/modules/imagebitma...
> Source/modules/imagebitmap/ImageBitmapFactories.cpp:297:
> m_factory->didStartLoading(this);
> I believe the issue was that you were creating a smart pointer from a raw one
> without adopting here. Likely you would hit the !m_adoptionIsRequired
assertion
> in ref().
> 
> One solution to this would be to store the loader in the HashSet when it is
> created, instead of when it starts loading.

Ah, nice catch, thank you. I did not realize that passing "this" to a PassRefPtr
argument was actually creating a second smart pointer (which was then losing its
ref count when the first went out of scope).

[commit-bot: I haz the power, 2013-08-14 14:36:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arbesfeld@chromium.org/20748002/104001

[Stephen White, 2013-08-14 14:39:13]

https://codereview.chromium.org/20748002/diff/104001/Source/modules/imagebitm...
File Source/modules/imagebitmap/ImageBitmapFactories.cpp (right):

https://codereview.chromium.org/20748002/diff/104001/Source/modules/imagebitm...
Source/modules/imagebitmap/ImageBitmapFactories.cpp:270: provideTo(window,
supplementName(), adoptPtr(supplement));
Nit: kind of a style thing, but I prefer that you immediately adoptRef() what is
returned by new into a local RefPtr, even if you then pass ownership elsewhere.
It's easy to forget to adoptPtr() when constructing a PassRefPtr in the argument
list.

i.e., something like:

RefPtr<ImageBitmapFactories> newFactories = adoptRef(new
ImageBitmapFactories());
provideTo(window, supplementName(), newFactories);
supplement = newFactories;

If you're in the habit of adoptRef(new ...), it's easier to spot potential leaks
from new objects which are not adopted.

[commit-bot: I haz the power, 2013-08-14 15:04:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arbesfeld@chromium.org/20748002/118001

[Stephen White, 2013-08-14 15:16:17]

https://codereview.chromium.org/20748002/diff/104001/Source/modules/imagebitm...
File Source/modules/imagebitmap/ImageBitmapFactories.cpp (right):

https://codereview.chromium.org/20748002/diff/104001/Source/modules/imagebitm...
Source/modules/imagebitmap/ImageBitmapFactories.cpp:270: provideTo(window,
supplementName(), adoptPtr(supplement));
On 2013/08/14 14:39:13, Stephen White wrote:
> Nit: kind of a style thing, but I prefer that you immediately adoptRef() what
is
> returned by new into a local RefPtr, even if you then pass ownership
elsewhere.
> It's easy to forget to adoptPtr() when constructing a PassRefPtr in the
argument
> list.
> 
> i.e., something like:
> 
> RefPtr<ImageBitmapFactories> newFactories = adoptRef(new
> ImageBitmapFactories());
> provideTo(window, supplementName(), newFactories);
> supplement = newFactories;
> 
> If you're in the habit of adoptRef(new ...), it's easier to spot potential
leaks
> from new objects which are not adopted.

Matthew pointed out to me that this is an OwnPtr not a RefPtr, and that
provideTo() wants a PassOwnPtr to the base class, not the derived class. Which
makes this kind of tricky.

I suppose you could do:
OwnPtr<ImageBitmapFactories> newFactories = adoptPtr(new
ImageBitmapFactories());
supplement = newFactories.get();
provideTo(window, supplementName(), newFactories.release());

but feel free to leave it as-is, if that's standard practice in the other
Supplements.

[commit-bot: I haz the power, 2013-08-14 16:09:28]

Change committed as 156101