Jon Morton [:jmorton] Reporter
	Description • 10 years ago

ImageBitmap provides more efficient loading and preparing of image resources for rendering in 2D canvases and WebGL.

Spec: http://www.whatwg.org/specs/web-apps/current-work/#imagebitmap

ImageBitmap also needs to be supported as a CanvasImageSource and a TexImageSource for 2d canvas and WebGL respectively.

	Jon Morton [:jmorton] Reporter
	Comment 6 • 10 years ago

These patches implement createImageBitmap with Image/Canvas/Video elements as sources and adds ImageBitmap support to canvas 2d. I still need to add Blob/ImageData/ImageBitmap as sources, support texImage in WebGL, and add ImageBitmap as a transferable.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 7 • 10 years ago

Comment on attachment 8467965 [details] [diff] [review]
Part 3 - Implement Cropping and Surface Preparation

Review of attachment 8467965 [details] [diff] [review]:
-----------------------------------------------------------------

In the future please submit patches with -U8 (8 lines of diff context). Otherwise looks great!

::: dom/canvas/ImageBitmap.cpp
@@ +101,5 @@
> +    }
> +
> +    IntPoint dest(
> +      mCropRect.X() < surfPortion.X() ? surfPortion.X() - mCropRect.X() : 0,
> +      mCropRect.Y() < surfPortion.Y() ? surfPortion.Y() - mCropRect.Y() : 0);

std::max(0, surfPortion.X() - mCropRect.X()) etc

@@ +110,5 @@
> +      return mSurface = nullptr;
> +    }
> +    // Make mCropRect match new surface we've cropped to
> +    mCropRect.MoveTo(0, 0);
> +    target->ClearRect(Rect(mCropRect));

I believe this ClearRect is not necessary.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 8 • 10 years ago

Comment on attachment 8467967 [details] [diff] [review]
Part 4 - ImageBitmap as a CanvasImageSource

Review of attachment 8467967 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +3331,4 @@
>        error.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>        return;
>      }
> +  } else if (!image.IsImageBitmap()) {

Better to move the ImageBitmap case up here so we don't need this !

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 9 • 10 years ago

Comment on attachment 8467961 [details] [diff] [review]
0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch

ehsan is going to take a first pass on these.

	(no longer active)
	Comment 10 • 10 years ago

Comment on attachment 8467961 [details] [diff] [review]
0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch

Review of attachment 8467961 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good in general, please see my comments below.  The most important issue is whether this object needs to be wrapper-cached.

::: dom/base/nsGlobalWindow.h
@@ +51,4 @@
>  #include "mozilla/dom/EventTarget.h"
>  #include "Units.h"
>  #include "nsComponentManagerUtils.h"
> +#include "mozilla/dom/ImageBitmapSource.h"

Please forward-declare ImageBitmapSource.

::: dom/canvas/ImageBitmap.h
@@ +10,5 @@
> +#include "mozilla/Attributes.h"
> +#include "mozilla/ErrorResult.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "mozilla/dom/ImageBitmapSource.h"
> +#include "mozilla/gfx/Rect.h"

I think you should be able to avoid including ErrorResult.h and ImageBitmapSource.h in the header and just forward declare these.  Maybe Rect.h too.

@@ +24,5 @@
> +
> +class Promise;
> +
> +class ImageBitmap MOZ_FINAL : public nsISupports,
> +                              public nsWrapperCache

As far as I can tell, there is no way for the web content to regain access to an ImageBitmap once it has lost its last reference to it.  Based on that, should ImageBitmap be wrapper-cached?  Am I missing something?

@@ +31,5 @@
> +  NS_DECL_ISUPPORTS
> +
> +  JSObject* WrapObject(JSContext* aCx);
> +
> +  ImageBitmap* GetParentObject() { return nullptr; }

Nit: const.

Also, I think it would be much better to return a useful value here.  You can use the global object passed to the Create function below.

::: dom/canvas/ImageBitmapSource.h
@@ +1,1 @@
> +

Nit: please include the license header here.

::: dom/webidl/ImageBitmap.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +// http://www.whatwg.org/specs/web-apps/current-work/#imagebitmap

Nit: please use a "The origin of this IDL file is" template like other webidl files here.

@@ +26,5 @@
> +interface ImageBitmapFactories {
> +  [Throws]
> +  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image);
> +  [Throws]
> +  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image, long sx, long sy, long sw, long sh);

You should use optional here instead of adding two overloads.

@@ +30,5 @@
> +  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image, long sx, long sy, long sw, long sh);
> +};
> +
> +Window implements ImageBitmapFactories;
> +WorkerGlobalScope implements ImageBitmapFactories;

These need to go into Window.webidl and WorkerGlobalScope.webidl respectively.

::: dom/workers/WorkerScope.h
@@ +8,4 @@
>  
>  #include "Workers.h"
>  #include "mozilla/DOMEventTargetHelper.h"
> +#include "mozilla/dom/ImageBitmapSource.h"

Forware-declare please.

	Jon Morton [:jmorton] Reporter
	Comment 11 • 10 years ago

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10)

> Please forward-declare ImageBitmapSource.

This would definitely be the right way, but unfortunately ImageBitmapSource is a typedef of "HTMLImageElementOrHTMLVideoElementOrHTMLCanvasElementOrBlobOrImageDataOrCanvasRenderingContext2DOrImageBitmap", and to forward declare it I'd have to reference the original, 110 character gargantuan name everywhere. My solution was to create the ImageBitmapSource.h file with just the typedef so it could be included anywhere needed. I filed Bug 1045867 to suggest support for renaming union type class names.

> > +
> > +class Promise;
> > +
> > +class ImageBitmap MOZ_FINAL : public nsISupports,
> > +                              public nsWrapperCache
> 
> As far as I can tell, there is no way for the web content to regain access
> to an ImageBitmap once it has lost its last reference to it.  Based on that,
> should ImageBitmap be wrapper-cached?  Am I missing something?

Nope, not missing anything, you're definitely right.

> @@ +26,5 @@
> > +interface ImageBitmapFactories {
> > +  [Throws]
> > +  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image);
> > +  [Throws]
> > +  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image, long sx, long sy, long sw, long sh);
> 
> You should use optional here instead of adding two overloads.

Hmm, that's what I was going to do at first, but:
1. Other functions with similar arguments (in WebGLRenderingContext and CanvasRenderingContext2D) did it this way
2. It seems to me that with optional, there are 5 overloads here (1-5 args), but we really just want these two overloads. Can I make the last four arguments optional as a whole? I didn't see a way to do that anywhere.

> Also, I think it would be much better to return a useful value here.  You
> can use the global object passed to the Create function below.

I think I can just remove this function entirely if I don't use wrappercache.

And OK on everything else!

	(no longer active)
	Comment 12 • 10 years ago

(In reply to Jon Morton [:jmorton] from comment #11)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #10)
> 
> > Please forward-declare ImageBitmapSource.
> 
> This would definitely be the right way, but unfortunately ImageBitmapSource
> is a typedef of
> "HTMLImageElementOrHTMLVideoElementOrHTMLCanvasElementOrBlobOrImageDataOrCanv
> asRenderingContext2DOrImageBitmap", and to forward declare it I'd have to
> reference the original, 110 character gargantuan name everywhere. My
> solution was to create the ImageBitmapSource.h file with just the typedef so
> it could be included anywhere needed. I filed Bug 1045867 to suggest support
> for renaming union type class names.

Ah, right.  OK, it's fine to keep the include for ImageBitmapSource here.

> > @@ +26,5 @@
> > > +interface ImageBitmapFactories {
> > > +  [Throws]
> > > +  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image);
> > > +  [Throws]
> > > +  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image, long sx, long sy, long sw, long sh);
> > 
> > You should use optional here instead of adding two overloads.
> 
> Hmm, that's what I was going to do at first, but:
> 1. Other functions with similar arguments (in WebGLRenderingContext and
> CanvasRenderingContext2D) did it this way
> 2. It seems to me that with optional, there are 5 overloads here (1-5 args),
> but we really just want these two overloads. Can I make the last four
> arguments optional as a whole? I didn't see a way to do that anywhere.

Hmm, you're right on both points.  Actually the IDL in the spec is wrong, since it just sets the second argument to the optional.  But the prose does suggest that the spec actually wants to implement your IDL (i.e. allow calling either with 1 or 5 args.)  I filed <https://www.w3.org/Bugs/Public/show_bug.cgi?id=26562> for that.  Please keep your existing IDL.  :-)

	(no longer active)
	Comment 13 • 10 years ago

Comment on attachment 8467964 [details] [diff] [review]
0002-Bug-1044102-Part-2-Support-HTML-Image-Video-Canvas-E.patch

Review of attachment 8467964 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/ImageBitmap.cpp
@@ +34,5 @@
>    return ImageBitmapBinding::Wrap(aCx, this);
>  }
>  
> +/* static */ already_AddRefed<ImageBitmap>
> +ImageBitmap::CreateFromElement(Element& aElement, ErrorResult& aRv)

Instead of going from a concrete element type to dom::Element and "coming back" to the concrete type inside SurfaceFromElement, it would be better to make this method a template which would call SurfaceFromElement.  That function has all of the overloads that you need here: http://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.h#1775

@@ +129,5 @@
> +  if (!aRv.Failed()) {
> +    MessageLoop::current()->PostTask(
> +      FROM_HERE,
> +      NewRunnableFunction(&FulfillImageBitmapPromise, promise, imageBitmap));
> +  }

You need to reject the promise if an error happens when constructing an ImageBitmap.

::: dom/canvas/ImageBitmap.h
@@ +50,4 @@
>  
>  protected:
>  
> +  ImageBitmap(gfx::SourceSurface* aSurface);

Nit: please mark this as explicit.

	(no longer active)
	Comment 14 • 10 years ago

Comment on attachment 8467968 [details] [diff] [review]
Part 5 - Tests

Review of attachment 8467968 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/test/test_imagebitmap.html
@@ +4,5 @@
> +<script src="/tests/SimpleTest/SimpleTest.js"></script>
> +<link rel="stylesheet" href="/tests/SimpleTest/test.css">
> +<body>
> +
> +<img src="image_anim-gr.gif" id="image" class="resource" />

Nit: XHTML is dead.  please remove the " /".  ;)

@@ +41,5 @@
> +];
> +
> +var canvas, ctx, ctx2, completedImage;
> +
> +function pFail(ex) {

Nit: just call this failed or something.

@@ +51,5 @@
> +
> +  image.src = 'image_rgrg-256x256.png';
> +  var promise = new Promise(function (arg) { resolver = arg; });
> +
> +  var createBitmap = function(def) {

I'm not sure why you don't just do:

function createBitmap(def) {...

here and elsewhere.

@@ +68,5 @@
> +
> +  return promise.then(function() {
> +    TEST_BITMAPS.forEach(function (test) {
> +      if (!test.bitmap) { return; }
> +      ctx.clearRect ( 0, 0, canvas.width, canvas.height );

Nit: no spaces around braces.

	Jon Morton [:jmorton] Reporter
	Comment 15 • 10 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> std::max(0, surfPortion.X() - mCropRect.X()) etc

Indeed, that's much better.

> I believe this ClearRect is not necessary.

Yep.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> Better to move the ImageBitmap case up here so we don't need this !

Yep.

Thanks for the review!

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #13)
> Instead of going from a concrete element type to dom::Element and "coming
> back" to the concrete type inside SurfaceFromElement, it would be better to
> make this method a template which would call SurfaceFromElement.  That
> function has all of the overloads that you need here:
> http://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.
> h#1775

Ah, yes. Good call.

> 
> You need to reject the promise if an error happens when constructing an
> ImageBitmap.
> 

If I'm understanding this comment correctly, then: the bindings have a special case for methods that return a Promise and will automatically reject the promise with any errors that are returned.

And SGTM on the rest!

	(no longer active)
	Comment 16 • 10 years ago

(In reply to Jon Morton [:jmorton] from comment #15)
> > You need to reject the promise if an error happens when constructing an
> > ImageBitmap.
> > 
> 
> If I'm understanding this comment correctly, then: the bindings have a
> special case for methods that return a Promise and will automatically reject
> the promise with any errors that are returned.

OK, then that part looks good.

	Jon Morton [:jmorton] Reporter
	Comment 17 • 10 years ago

It turns out that ImageBitmap does need to be wrappercached, because the promise resolution codepath uses ToJSValue, which only accepts objects which inherit from nsISupports AND WrapperCache: http://mxr.mozilla.org/mozilla-central/source/dom/bindings/ToJSValue.h#141

(thanks khuey)

	(no longer active)
	Comment 18 • 10 years ago

OK fine!

	Tzuhao Kuo [:kaku] Assignee
	Comment 19 • 10 years ago

Hi Jon, 
This is Kaku from Mozilla Taipei Media team. I am wondering if you are still working on this bug?
Thanks,
Kaku

	Jon Morton [:jmorton] Reporter
	Comment 20 • 10 years ago

Hi Kaku,
Sorry about the delay on this. I will try to finish this bug up pretty soon - within the coming few weeks.

	Andrea Marchesini [:baku]
	Comment 21 • 9 years ago

Kaku, why do we have this dependence on bug 1048325?
I don't think we want to implement blob.close() soon, except if there are valid reasons.
Tell me more :)

	Tzuhao Kuo [:kaku] Assignee
	Comment 22 • 9 years ago

(In reply to Andrea Marchesini (:baku) from comment #21)
> Kaku, why do we have this dependence on bug 1048325?
> I don't think we want to implement blob.close() soon, except if there are
> valid reasons.
> Tell me more :)

Since Blob is one of the source type of ImageBitmap and the "isClosed" property should be checked while constructing the ImageBitmap. The statement is quoted as follows:

"If the Blob object has been disabled through the close() method, then return a promise rejected with an InvalidStateError exception and abort these steps."

	Tzuhao Kuo [:kaku] Assignee
	Comment 23 • 9 years ago

(In reply to Andrea Marchesini (:baku) from comment #21)
> Kaku, why do we have this dependence on bug 1048325?
> I don't think we want to implement blob.close() soon, except if there are
> valid reasons.
> Tell me more :)

Tn the File API, there is also statements for checking the isClosed property in the readAsXXX() methods, but we do not implement the checks in Gecko. Should I just skip this statement for now?

	Andrea Marchesini [:baku]
	Comment 24 • 9 years ago

Yes please. Last time I discussed with bent about implementing a close(), we asked me if really this is needed. We have to discuss it more seriously, but for now, please go on. Maybe file a follow-up about this.

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 9 years ago

A followup bug, depending on the bug for close(), would be good, so when we add close() we update all the various places that check isClosed.

	Tzuhao Kuo [:kaku] Assignee
	Comment 26 • 9 years ago

Modified according to comments.

	Tzuhao Kuo [:kaku] Assignee
	Comment 27 • 9 years ago

Modified according to comments.

	Tzuhao Kuo [:kaku] Assignee
	Comment 28 • 9 years ago

Carry r+ and still modified according to comments.

	Tzuhao Kuo [:kaku] Assignee
	Comment 29 • 9 years ago

Carry r+ and still modified according to comments.

	Tzuhao Kuo [:kaku] Assignee
	Comment 30 • 9 years ago

Carry r+ and still modified according to comments.

	Tzuhao Kuo [:kaku] Assignee
	Comment 34 • 9 years ago

These three new patches implement createImageBitmap with ImageData/CanvasRenderingContext2D/ImageBitmap as sources.

Keep working on add Blob as sources, support texImage in WebGL, and add ImageBitmap as a transferable.

	Boris Zbarsky [:bzbarsky]
	Comment 35 • 9 years ago

The ordering of these patches seems to be off.  The patch to implement the right security checks (which doesn't even exist yet?) should come before the patch that actually returns imagebitmaps for HTML elements.

Incidentally, I don't think the spec's use of the entry settings origin entirely makes sense here....

	Tzuhao Kuo [:kaku] Assignee
	Comment 36 • 9 years ago

(In reply to Boris Zbarsky [:bz] from comment #35)
> The ordering of these patches seems to be off.  The patch to implement the
> right security checks (which doesn't even exist yet?) should come before the
> patch that actually returns imagebitmaps for HTML elements.
> 
> Incidentally, I don't think the spec's use of the entry settings origin
> entirely makes sense here....

Hi Boris, 
Thanks for your comment and after some spec reading and code tracing, here is my proposal for the "security check".
May I have your comments?

------------------------------------------------------------
For the HTMLImageElement and HTMLVideoElement cases:
Step1: Get the Principal object from the incoming global object: (which is the entry settings object)
       nsGlobalWindow::mPrincipal
       ...or...
       WorkerGlobalScope::mWokerPrivate::mPrincipal
Step2: Get the Principal object from the HTMLImage(/Video)Element:
       nsLayoutUtils::SurfaceFromElementResult res = nsLayoutUtils::SurfaceFromElement(...);
       nsLayoutUtils::SurfaceFromElementResult::mPrincipal
Step3: Compare these two Principal objects: (where I am not really sure which method should I use...)
       nsIPrincipal::Equals(...) or nsIPrincipal::Subsumes(...)

For the HTMLCanvasElement and CanvasRenderingContext2D cases:
Just checke the HTMLCanvasElement::isWriteOnly flag.
------------------------------------------------------------

Also, could you talk more about your concern to the usage of "entry settings object" on the spec?

	Tzuhao Kuo [:kaku] Assignee
	Comment 37 • 9 years ago

Here are questions raised whiling I was thinking about implementing the blob as input source.
The incoming blob object might comes from two scenarios:

Scenario 1:
If the incoming Blob object is a encoded image file, which might comes from <input> or drag-drop. In this case, I think I can get the InputStream object from the incoming Blob object and then put the InputStream object into imgTools::DecodeImage() to get an imgIContainer object which I can then have a SourceSurface. Is this way feasible?

Scenario 2:
The incoming Blob object contains decoded image raw pixels. This could happen if JavaScript developers use CanvasRenderingContext2D::GetImageData() to get a TypedArray and then construct the Blob object with the underlying ArrayBuffer of the TypedArray. Sould we also support this scenario?

	Boris Zbarsky [:bzbarsky]
	Comment 38 • 9 years ago

> Step1: Get the Principal object from the incoming global object: (which is the entry
> settings object)

No, it's not.  If you actually want the entry settings object, you want GetEntryGlobal().  Now it's not clear to me whther the spec here makes sense at all; I would have assumed that  the incumbent settings object would make more sense than the entry one.  That's something you should talk to bholley about.  In that case you'd use GetIncumbentGlobal().

Note that you can assert in the HTMLImageElement/HTMLVideoElement cases that you're on mainthread, since DOM elements don't exist on workers.

> Step2: Get the Principal object from the HTMLImage(/Video)Element:

Sadly, no.  The spec's concept of "image origin" doesn't match our concept of "principal of the SurfaceFromElementResult".

You should probably file a separate bug on changing our behavior here (or changing the spec; we need to check whether the spec matches reality), but in the meantime you basically want to assume same-origin if res.mCORSUsed.  Please do file that followup bug, though.

>       nsIPrincipal::Equals(...) or nsIPrincipal::Subsumes(...)

Subsumes.

> Also, could you talk more about your concern to the usage of "entry settings object" on
> the spec?

Sure.  The "entry settings object" is "the window where script first got entered".  It's not clear to me why that should be the thing used for the security check as opposed to "the window containing the script that called the function" or "the window the function is defined in".

	Boris Zbarsky [:bzbarsky]
	Comment 39 • 9 years ago

> The spec's concept of "image origin" doesn't match our concept of "principal of the
> SurfaceFromElementResult".

Oh, and I assume you _will_ add tests here for cross-origin images, both using CORS and not, which would have caught the need for the mCORSUsed check.

	Tzuhao Kuo [:kaku] Assignee
	Comment 40 • 9 years ago

(In reply to Boris Zbarsky [:bz] from comment #38)
> > Step1: Get the Principal object from the incoming global object: (which is the entry
> > settings object)
> 
> No, it's not.  If you actually want the entry settings object, you want
> GetEntryGlobal().  Now it's not clear to me whther the spec here makes sense
> at all; I would have assumed that  the incumbent settings object would make
> more sense than the entry one.  That's something you should talk to bholley
> about.  In that case you'd use GetIncumbentGlobal().
> 
I agree with you that incumbent settings object is the better one according to the statements in https://dxr.mozilla.org/mozilla-central/source/dom/base/ScriptSettings.h#78.

> The spec's concept of "image origin" doesn't match our concept
> of "principal of the SurfaceFromElementResult".
I am not really sure what's the difference between spec's concept of origin and our concept of SurfaceFromElementResult, could you explain more? And I have no idea how can I check origin if I cannot use the Principal object, could you give me some hints?

> You should probably file a separate bug on changing our behavior here (or
> changing the spec; we need to check whether the spec matches reality), but
> in the meantime you basically want to assume same-origin if res.mCORSUsed. 
> Please do file that followup bug, though.

(In reply to Boris Zbarsky [:bz] from comment #39)
> Oh, and I assume you _will_ add tests here for cross-origin images, both
> using CORS and not, which would have caught the need for the mCORSUsed check. 

Conceptually, here is my understanding about how to do security checking.

if (res.mCORSUsed) {
    // cross-origin image with CORS approval, ---> PASS
}
else {
    if (CHECK_SAME_ORIGIN(HTMLImage/VideoElement, EntrySettingsObject)) {
        // same-origin ---> PASS
    }
    else {
        // cross-origin ---> FAIL
    }
}

And I need to more information about the "image origin" to design the CHECK_SAME_ORIGIN().

	Boris Zbarsky [:bzbarsky]
	Comment 41 • 9 years ago

> I am not really sure what's the difference between spec's concept of origin and our
> concept of SurfaceFromElementResult, could you explain more?

Sure.  What we store in the SurfaceFromElementResult is two pieces of data: the actual origin of the image and a boolean indicating whether the image passed CORS checks.  If that boolean is set we don't bother performing security checks against the actual origin of the image, exactly as described in your "Conceptually, ..." pseudocode, using the principal in the SurfaceFromElementResult as the image origin.

What the spec does instead is it associates an origin with each image.  If the image passed CORS checks, or was same-origin with the document that started the load to start with and no CORS checks were done, that origin is an alias for the origin of the <img> element's node document.  Otherwise it's a nonce ("unique identifier") origin.

As long as no one uses document.domain, you can't tell apart the spec's behavior and ours.

But if document.domain is used, you can tell them apart.  Say you have two documents, A and B, which are not same-origin but both set document.domain so they have the same effective script origin.  A loads a cross-site image via CORS.  Now say we're sticking with the "entry settings object" bit in the spec and script is entered in document B, which calls a function in document A which tries to create an ImageBitmap from the image.

Per the spec, we'd take the origin of the entry settings object (which is the origin of B) and compare it to the origin of the image (which is the origin of A) and the security check would fail and the image bitmap would not be created.

In our implementation in this situation we don't have "the origin of A" associated with the image in any way.  So we'd apply your pseudocode above and the ImageBitmap creation would succeed.

That's what we need a followup bug on: figuring out what the actual behavior here should be in this situation.  There are similar problems with drawImage on a canvas in terms of the spec not matching what we're actually doing.

Does that help?

	Tzuhao Kuo [:kaku] Assignee
	Comment 42 • 9 years ago

(In reply to Boris Zbarsky [:bz] from comment #41)
> Does that help?

Yes, of course, your explanation has helped a lot to get me on track. Thank you.

	Tzuhao Kuo [:kaku] Assignee
	Comment 43 • 9 years ago

Cancel reviews and implement security/status checks first.

	Tzuhao Kuo [:kaku] Assignee
	Comment 46 • 9 years ago

Carry r+ and modified according to comments.

	Tzuhao Kuo [:kaku] Assignee
	Comment 47 • 9 years ago

Carry r+ and modified according to comments.

	Tzuhao Kuo [:kaku] Assignee
	Comment 48 • 9 years ago

Carry r+ and modified according to comments.

	Tzuhao Kuo [:kaku] Assignee
	Comment 49 • 9 years ago

Bug 1044102 Part 6 - ImageData as ImageBitamp sources, code and test case

	Tzuhao Kuo [:kaku] Assignee
	Comment 50 • 9 years ago

Bug 1044102 Part 7 - CanvasRenderingContext2D as ImageBitamp sources

	Tzuhao Kuo [:kaku] Assignee
	Comment 51 • 9 years ago

Bug 1044102 Part 7 - CanvasRenderingContext2D as ImageBitamp sources

	Tzuhao Kuo [:kaku] Assignee
	Comment 52 • 9 years ago

Bug 1044102 Part 8 - ImageBitmap as ImageBitamp sources

	Tzuhao Kuo [:kaku] Assignee
	Comment 53 • 9 years ago

Bug 1044102 Part 9 - Blob as ImageBitamp sources

	Chia-hung Tai [:ctai :ctai_mozilla :cht]
	Comment 54 • 9 years ago

Comment on attachment 8569718 [details] [diff] [review]
0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch

Review of attachment 8569718 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/ImageBitmap.cpp
@@ +31,5 @@
> +{
> +  return ImageBitmapBinding::Wrap(aCx, this);
> +}
> +
> +/* static */ already_AddRefed<Promise>

ditto: new line for /* static *

::: dom/canvas/ImageBitmap.h
@@ +26,5 @@
> +
> +class Promise;
> +
> +class ImageBitmap MOZ_FINAL : public nsISupports,
> +                              public nsWrapperCache

I think you miss the cycle collection part. 
"Does the object inherit from nsWrapperCache (directly or indirectly)?  If so, it must be cycle collected."
Quoted from http://blog.kylehuey.com/post/27564411715/cycle-collection

	Chia-hung Tai [:ctai :ctai_mozilla :cht]
	Comment 55 • 9 years ago

Comment on attachment 8569719 [details] [diff] [review]
0002-Bug-1044102-Part-2-Support-HTML-Image-Video-Canvas-E.patch

Review of attachment 8569719 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: dom/canvas/ImageBitmap.cpp
@@ +34,5 @@
>    return ImageBitmapBinding::Wrap(aCx, this);
>  }
>  
> +template<class HTMLElementType>
> +/* static */ already_AddRefed<ImageBitmap>

Could you please move /* static */ in front of 	
template<class HTMLElementType>?

	Chia-hung Tai [:ctai :ctai_mozilla :cht]
	Comment 56 • 9 years ago

Comment on attachment 8569724 [details] [diff] [review]
0006-Bug-1044102-Part-5-Add-ImageBitmap-test.patch

Review of attachment 8569724 [details] [diff] [review]:
-----------------------------------------------------------------

Fix some nits.

::: dom/canvas/test/test_imagebitmap.html
@@ +45,5 @@
> +    {'rect': [0, 0, 128, 128],        'draw': [0, 0, 64, 64, 0, 0, 64, 64],       'test': [[0,    0,    [255, 0, 0, 255], 5]]},
> +    {'rect': [128, 0, 128, 128],      'draw': [0, 0, 64, 64, 0, 0, 64, 64],       'test': [[0,    0,    [0, 255, 0, 255], 5]]},
> +    {'rect': [230, 230, 128, 128],    'draw': [0, 0, 128, 128, 0, 0, 128, 128],   'test': [[0,    0,    [255, 0, 0, 255], 5],
> +                                                                                           [100,  100,  [0, 0, 0, 0],     5]]},
> +    {'rect': [-64, -64, 512, 512],    'draw': [0, 0, 128, 128, 0, 0, 128, 128],   'test': [[0,    0,    [0, 0, 0, 0],     5], 

nit: remove the last space

@@ +67,5 @@
> +  function createBitmap(def) {
> +    return createImageBitmap(image, def.rect[0], def.rect[1], def.rect[2], def.rect[3])
> +      .then(function (bitmap) { def.bitmap = bitmap; }, failed);
> +  };
> +  

nit:
remove leading space

	Chia-hung Tai [:ctai :ctai_mozilla :cht]
	Comment 57 • 9 years ago

Comment on attachment 8569720 [details] [diff] [review]
0003-Bug-1044102-Part-2.1-Security-and-status-checks.patch

Review of attachment 8569720 [details] [diff] [review]:
-----------------------------------------------------------------

Be careful of the nullptr check.

::: dom/canvas/ImageBitmap.cpp
@@ +21,5 @@
> +  if (res.mIsWriteOnly) {
> +    return false;
> +  }
> +
> +  if (res.mCORSUsed) {

Why not use early return? I guess this is a method which will be called many times.
if (!res.mCORSUsed) { return true;}

@@ +23,5 @@
> +  }
> +
> +  if (res.mCORSUsed) {
> +    nsIGlobalObject *entrySettingsObject = GetEntryGlobal();
> +    nsIPrincipal *principal = entrySettingsObject->PrincipalOrNull();

You should do nullptr check before use it. |PrincipalOrNull| is not guaranteed to return a non-nullptr.

@@ +25,5 @@
> +  if (res.mCORSUsed) {
> +    nsIGlobalObject *entrySettingsObject = GetEntryGlobal();
> +    nsIPrincipal *principal = entrySettingsObject->PrincipalOrNull();
> +    if (!(principal->Subsumes(res.mPrincipal))) {
> +      return false;

if (principal->Subsumes(res.mPrincipal)) {
  return true; 
}
return false;

Will that better?

@@ +39,5 @@
> +  nsresult rv;
> +
> +  nsCOMPtr<imgIRequest> imgRequest;
> +  rv = aImageEl.GetRequest(nsIImageLoadingContent::CURRENT_REQUEST, getter_AddRefs(imgRequest));
> +  if (!NS_FAILED(rv) && imgRequest) {

Why not use if (NS_SUSSEEDED(rv) && imgRequest) { ?

@@ +42,5 @@
> +  rv = aImageEl.GetRequest(nsIImageLoadingContent::CURRENT_REQUEST, getter_AddRefs(imgRequest));
> +  if (!NS_FAILED(rv) && imgRequest) {
> +    nsCOMPtr<imgIContainer> imgContainer;
> +    rv = imgRequest->GetImage(getter_AddRefs(imgContainer));
> +    if (!NS_FAILED(rv) && imgContainer->GetType() == imgIContainer::TYPE_RASTER) {

ditto.
Is that possible imgContainer a null-ptr?

	Chia-hung Tai [:ctai :ctai_mozilla :cht]
	Comment 58 • 9 years ago

Comment on attachment 8569726 [details] [diff] [review]
0007-Bug-1044102-Part-6-ImageData-as-ImageBitamp-sources-.patch

Review of attachment 8569726 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: dom/canvas/ImageBitmap.cpp
@@ +225,5 @@
>  
> +/* static */ already_AddRefed<ImageBitmap>
> +ImageBitmap::CreateInternal(nsIGlobalObject* aGlobal, ImageData& aImageData, ErrorResult& aRv)
> +{
> +  // TODO: check the data of ImageData is neutered or not

Is there a bug related this TODO?

@@ +235,5 @@
> +
> +  array.ComputeLengthAndData();
> +  uint32_t const imageWidth = aImageData.Width();
> +  uint32_t const imageHeight = aImageData.Height();
> +  uint32_t const imageStride = imageWidth * 4; // 4 channels per pixel of ImageData

This is based on the assumption that the format of ImageData is RGBA. Be careful here once this assumption is not true.

@@ +239,5 @@
> +  uint32_t const imageStride = imageWidth * 4; // 4 channels per pixel of ImageData
> +  uint32_t const dataLength = array.Length();
> +  gfx::IntSize const imageSize(imageWidth, imageHeight);
> +
> +  RefPtr<DataSourceSurface> surface = Factory::CreateDataSourceSurfaceWithStride(imageSize, SurfaceFormat::B8G8R8A8, imageStride);

ditto.

	Chia-hung Tai [:ctai :ctai_mozilla :cht]
	Comment 59 • 9 years ago

Comment on attachment 8569728 [details] [diff] [review]
0008-Bug-1044102-Part-7-CanvasRenderingContext2D-as-Image.patch

Review of attachment 8569728 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: dom/canvas/ImageBitmap.cpp
@@ +263,5 @@
> +    return nullptr;
> +  }
> +
> +  IntSize surfaceSize = surface->GetSize();
> +  if (surfaceSize.width == 0 || surfaceSize.height == 0) {

Is that possible surfaceSize.width < 0?

	Chia-hung Tai [:ctai :ctai_mozilla :cht]
	Comment 60 • 9 years ago

Comment on attachment 8569730 [details] [diff] [review]
0009-Bug-1044102-Part-8-ImageBitmap-as-ImageBitamp-source.patch

Review of attachment 8569730 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: dom/canvas/ImageBitmap.cpp
@@ +275,5 @@
>  
> +/* static */ already_AddRefed<ImageBitmap>
> +ImageBitmap::CreateInternal(nsIGlobalObject* aGlobal, ImageBitmap& aImageBitmap, ErrorResult& aRv)
> +{
> +  RefPtr<DataSourceSurface> dataSurface = aImageBitmap.mSurface->GetDataSurface();

I think you should add NS_ENSURE_TRUE(dataSurface, NS_ERROR_FAILURE) or !dataSurface to make sure everything goes well.

	Chia-hung Tai [:ctai :ctai_mozilla :cht]
	Comment 61 • 9 years ago

Comment on attachment 8569731 [details] [diff] [review]
0010-Bug-1044102-Part-9-Blob-as-ImageBitamp-sources.patch

Review of attachment 8569731 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

	Tzuhao Kuo [:kaku] Assignee
	Comment 62 • 9 years ago

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #54)
> Comment on attachment 8569718 [details] [diff] [review]
> 0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch
> 
> Review of attachment 8569718 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/ImageBitmap.cpp
> @@ +31,5 @@
> > +{
> > +  return ImageBitmapBinding::Wrap(aCx, this);
> > +}
> > +
> > +/* static */ already_AddRefed<Promise>
> 
> ditto: new line for /* static *
OK.
 
> ::: dom/canvas/ImageBitmap.h
> @@ +26,5 @@
> > +
> > +class Promise;
> > +
> > +class ImageBitmap MOZ_FINAL : public nsISupports,
> > +                              public nsWrapperCache
> 
> I think you miss the cycle collection part. 
> "Does the object inherit from nsWrapperCache (directly or indirectly)?  If
> so, it must be cycle collected."
> Quoted from http://blog.kylehuey.com/post/27564411715/cycle-collection
Yes, I will use codes generated from code-generator.

	Tzuhao Kuo [:kaku] Assignee
	Comment 63 • 9 years ago

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #57)
> Comment on attachment 8569720 [details] [diff] [review]
> 0003-Bug-1044102-Part-2.1-Security-and-status-checks.patch
> 
> Review of attachment 8569720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Be careful of the nullptr check.
> 
> ::: dom/canvas/ImageBitmap.cpp
> @@ +21,5 @@
> > +  if (res.mIsWriteOnly) {
> > +    return false;
> > +  }
> > +
> > +  if (res.mCORSUsed) {
> 
> Why not use early return? I guess this is a method which will be called many
> times.
> if (!res.mCORSUsed) { return true;}
>  
> @@ +25,5 @@
> > +  if (res.mCORSUsed) {
> > +    nsIGlobalObject *entrySettingsObject = GetEntryGlobal();
> > +    nsIPrincipal *principal = entrySettingsObject->PrincipalOrNull();
> > +    if (!(principal->Subsumes(res.mPrincipal))) {
> > +      return false;
> 
> if (principal->Subsumes(res.mPrincipal)) {
>   return true; 
> }
> return false;
> 
> Will that better?

These two parts are logical error. It should be "if (!res.mCORSUsed)" and then early returns the false case.

> @@ +23,5 @@
> > +  }
> > +
> > +  if (res.mCORSUsed) {
> > +    nsIGlobalObject *entrySettingsObject = GetEntryGlobal();
> > +    nsIPrincipal *principal = entrySettingsObject->PrincipalOrNull();
> 
> You should do nullptr check before use it. |PrincipalOrNull| is not
> guaranteed to return a non-nullptr.
OK.
 
> @@ +39,5 @@
> > +  nsresult rv;
> > +
> > +  nsCOMPtr<imgIRequest> imgRequest;
> > +  rv = aImageEl.GetRequest(nsIImageLoadingContent::CURRENT_REQUEST, getter_AddRefs(imgRequest));
> > +  if (!NS_FAILED(rv) && imgRequest) {
> 
> Why not use if (NS_SUSSEEDED(rv) && imgRequest) { ?
OK.

> @@ +42,5 @@
> > +  rv = aImageEl.GetRequest(nsIImageLoadingContent::CURRENT_REQUEST, getter_AddRefs(imgRequest));
> > +  if (!NS_FAILED(rv) && imgRequest) {
> > +    nsCOMPtr<imgIContainer> imgContainer;
> > +    rv = imgRequest->GetImage(getter_AddRefs(imgContainer));
> > +    if (!NS_FAILED(rv) && imgContainer->GetType() == imgIContainer::TYPE_RASTER) {
> 
> ditto.
> Is that possible imgContainer a null-ptr?
The "!NS_FAILED(rv)" checks null-ptr ahead. I will also use NS_SUSSEEDED(rv) here.

	Tzuhao Kuo [:kaku] Assignee
	Comment 64 • 9 years ago

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #56)
> Comment on attachment 8569724 [details] [diff] [review]
> 0006-Bug-1044102-Part-5-Add-ImageBitmap-test.patch
> 
> Review of attachment 8569724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Fix some nits.
> 
> ::: dom/canvas/test/test_imagebitmap.html
> @@ +45,5 @@
> > +    {'rect': [0, 0, 128, 128],        'draw': [0, 0, 64, 64, 0, 0, 64, 64],       'test': [[0,    0,    [255, 0, 0, 255], 5]]},
> > +    {'rect': [128, 0, 128, 128],      'draw': [0, 0, 64, 64, 0, 0, 64, 64],       'test': [[0,    0,    [0, 255, 0, 255], 5]]},
> > +    {'rect': [230, 230, 128, 128],    'draw': [0, 0, 128, 128, 0, 0, 128, 128],   'test': [[0,    0,    [255, 0, 0, 255], 5],
> > +                                                                                           [100,  100,  [0, 0, 0, 0],     5]]},
> > +    {'rect': [-64, -64, 512, 512],    'draw': [0, 0, 128, 128, 0, 0, 128, 128],   'test': [[0,    0,    [0, 0, 0, 0],     5], 
> 
> nit: remove the last space
> 
> @@ +67,5 @@
> > +  function createBitmap(def) {
> > +    return createImageBitmap(image, def.rect[0], def.rect[1], def.rect[2], def.rect[3])
> > +      .then(function (bitmap) { def.bitmap = bitmap; }, failed);
> > +  };
> > +  
> 
> nit:
> remove leading space

Ok for both cases.

	Tzuhao Kuo [:kaku] Assignee
	Comment 65 • 9 years ago

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #58)
> Comment on attachment 8569726 [details] [diff] [review]
> 0007-Bug-1044102-Part-6-ImageData-as-ImageBitamp-sources-.patch
> 
> Review of attachment 8569726 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM.
> 
> ::: dom/canvas/ImageBitmap.cpp
> @@ +225,5 @@
> >  
> > +/* static */ already_AddRefed<ImageBitmap>
> > +ImageBitmap::CreateInternal(nsIGlobalObject* aGlobal, ImageData& aImageData, ErrorResult& aRv)
> > +{
> > +  // TODO: check the data of ImageData is neutered or not
> 
> Is there a bug related this TODO?
Sorry that it is a missed part. I will fill it up.
Neutured ImageData (or ArrayBuffer) will be set to zero dimention (length).
Reference bug: bug 1011574

> @@ +235,5 @@
> > +
> > +  array.ComputeLengthAndData();
> > +  uint32_t const imageWidth = aImageData.Width();
> > +  uint32_t const imageHeight = aImageData.Height();
> > +  uint32_t const imageStride = imageWidth * 4; // 4 channels per pixel of ImageData
> 
> This is based on the assumption that the format of ImageData is RGBA. Be
> careful here once this assumption is not true.
> 
> @@ +239,5 @@
> > +  uint32_t const imageStride = imageWidth * 4; // 4 channels per pixel of ImageData
> > +  uint32_t const dataLength = array.Length();
> > +  gfx::IntSize const imageSize(imageWidth, imageHeight);
> > +
> > +  RefPtr<DataSourceSurface> surface = Factory::CreateDataSourceSurfaceWithStride(imageSize, SurfaceFormat::B8G8R8A8, imageStride);
> 
> ditto.
Ok, I will rearrange this part so that it will be more adaptive to other format.

	Tzuhao Kuo [:kaku] Assignee
	Comment 66 • 9 years ago

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #59)
> Comment on attachment 8569728 [details] [diff] [review]
> 0008-Bug-1044102-Part-7-CanvasRenderingContext2D-as-Image.patch
> 
> Review of attachment 8569728 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM
> 
> ::: dom/canvas/ImageBitmap.cpp
> @@ +263,5 @@
> > +    return nullptr;
> > +  }
> > +
> > +  IntSize surfaceSize = surface->GetSize();
> > +  if (surfaceSize.width == 0 || surfaceSize.height == 0) {
> 
> Is that possible surfaceSize.width < 0?

No, it won't be. However, the spec only checke the zero dimention case.

	Tzuhao Kuo [:kaku] Assignee
	Comment 67 • 9 years ago

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #60)
> Comment on attachment 8569730 [details] [diff] [review]
> 0009-Bug-1044102-Part-8-ImageBitmap-as-ImageBitamp-source.patch
> 
> Review of attachment 8569730 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM
> 
> ::: dom/canvas/ImageBitmap.cpp
> @@ +275,5 @@
> >  
> > +/* static */ already_AddRefed<ImageBitmap>
> > +ImageBitmap::CreateInternal(nsIGlobalObject* aGlobal, ImageBitmap& aImageBitmap, ErrorResult& aRv)
> > +{
> > +  RefPtr<DataSourceSurface> dataSurface = aImageBitmap.mSurface->GetDataSurface();
> 
> I think you should add NS_ENSURE_TRUE(dataSurface, NS_ERROR_FAILURE) or
> !dataSurface to make sure everything goes well.

OK!

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #61)
> Comment on attachment 8569731 [details] [diff] [review]
> 0010-Bug-1044102-Part-9-Blob-as-ImageBitamp-sources.patch
> 
> Review of attachment 8569731 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM

Thanks.

	Tzuhao Kuo [:kaku] Assignee
	Comment 68 • 9 years ago

Add the cycle collection codes.

	Tzuhao Kuo [:kaku] Assignee
	Comment 69 • 9 years ago

Fix according to comments.

	Tzuhao Kuo [:kaku] Assignee
	Comment 70 • 9 years ago

Add test casrs for "security checks".

	Chia-hung Tai [:ctai :ctai_mozilla :cht]
	Comment 71 • 9 years ago

Comment on attachment 8573660 [details] [diff] [review]
0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch

Review of attachment 8573660 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Thanks!

	Chia-hung Tai [:ctai :ctai_mozilla :cht]
	Comment 72 • 9 years ago

Comment on attachment 8573665 [details] [diff] [review]
0003-Bug-1044102-Part-2.1-Security-and-status-checks.patch

Review of attachment 8573665 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Thanks!

::: dom/canvas/ImageBitmap.cpp
@@ +127,5 @@
> +
> +  if (aVideoEl.ReadyState() == HTMLMediaElement::HAVE_NOTHING ||
> +      aVideoEl.ReadyState() == HTMLMediaElement::HAVE_METADATA) {
> +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> +        return nullptr;

nit: alignment.

	Chia-hung Tai [:ctai :ctai_mozilla :cht]
	Comment 73 • 9 years ago

Comment on attachment 8573667 [details] [diff] [review]
0011-Bug-1044102-Part-10-Add-test-cases-of-security-check.patch

Review of attachment 8573667 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

	Tzuhao Kuo [:kaku] Assignee
	Comment 75 • 9 years ago

Carry r+.

	Tzuhao Kuo [:kaku] Assignee
	Comment 77 • 9 years ago

Comment on attachment 8573821 [details] [diff] [review]
0002-Bug-1044102-Part-2-Support-HTML-Image-Video-Canvas-E.patch

Sorry, my fault. This patch needs to be reviewed.

	Tzuhao Kuo [:kaku] Assignee
	Comment 78 • 9 years ago

Carry r+.

	Tzuhao Kuo [:kaku] Assignee
	Comment 79 • 9 years ago

Carry r+.

	Tzuhao Kuo [:kaku] Assignee
	Comment 80 • 9 years ago

Carry r+.

	Tzuhao Kuo [:kaku] Assignee
	Comment 81 • 9 years ago

Carry r+.

	Tzuhao Kuo [:kaku] Assignee
	Comment 87 • 9 years ago

Jon mentioned that we need to support ImageBitmap as texImage in WebGL and add ImageBitmap as a transferable. This sounds convincing and should be very useful. However, I cannot find a written spec for these two scenarios now. Do I miss anything?

	Boris Zbarsky [:bzbarsky]
	Comment 88 • 9 years ago

Comment on attachment 8573825 [details] [diff] [review]
0005-Bug-1044102-Part-4-Support-ImageBitmap-as-a-CanvasIm.patch

Why is using nullptr for the CanvasPattern principal OK?  At the very least that deserves a very nice comment... but as far as I can tell, the one consumer of that member can't handle null.

	Boris Zbarsky [:bzbarsky]
	Comment 89 • 9 years ago

Comment on attachment 8573826 [details] [diff] [review]
0006-Bug-1044102-Part-5-Add-ImageBitmap-test.patch

Why do you need SpecialPowers stuff here?

	(no longer active)
	Comment 90 • 9 years ago

I will probably get to these reviews next week.

(In reply to Tzuhao Kuo [:kaku] from comment #87)
> Jon mentioned that we need to support ImageBitmap as texImage in WebGL and
> add ImageBitmap as a transferable. This sounds convincing and should be very
> useful. However, I cannot find a written spec for these two scenarios now.
> Do I miss anything?

I don't think there is a spec for this yet.  I think roc had a write-up for the API, but I can't find it right now.  roc, can you please point to that?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 91 • 9 years ago

Here is the proposal to make ImageBitmap transferable:
https://wiki.whatwg.org/wiki/WorkerCanvas

I don't know of a written proposal to use ImageBitmap with texImage, but it seems a very logical thing to do.

	Tzuhao Kuo [:kaku] Assignee
	Comment 92 • 9 years ago

(In reply to Not doing reviews right now from comment #88)
> Comment on attachment 8573825 [details] [diff] [review]
> 0005-Bug-1044102-Part-4-Support-ImageBitmap-as-a-CanvasIm.patch
> 
> Why is using nullptr for the CanvasPattern principal OK?  At the very least
> that deserves a very nice comment... but as far as I can tell, the one
> consumer of that member can't handle null.

Sorry that this is a missed part when I was handing security. For doing this part right, an successfully created ImageBitmap should be assigned with an origin. However, there is no description on how to assign an origin to an ImageBitmap in the spec. 

It is intuitive to assign the input source's origin to the newly created ImageBitmap, but not every input source type carries an origin, so this idea is not doable in practical. In other way, we could assign the origin of the script who creates this ImageBitmap to the newly created ImageBitmap. (A discussion is needed for using EnteySettingObject or IncumbentSettingObject.)

Is this way ok?

(In reply to Not doing reviews right now from comment #89)
> Comment on attachment 8573826 [details] [diff] [review]
> 0006-Bug-1044102-Part-5-Add-ImageBitmap-test.patch
> 
> Why do you need SpecialPowers stuff here?
No..., I do not use it. I think I could just get rid of it. Thanks.

	Boris Zbarsky [:bzbarsky]
	Comment 93 • 9 years ago

> an successfully created ImageBitmap should be assigned with an origin

Why?

Or to turn this around, what is the actual security model you're trying to create ImageBitmap?  Is it that once you create one you can use it wherever and it never induces tainting (which is what the spec seems like to me)?  If so, it may be sufficient to set mCORSUsed to true, change the assert in DoDrawImageSecurityCheck to only assert aPrincipal if CORSUsed is false, and add some documentation about how mPrincipal can be null if mCORSUsed is true.

If, on the other hand, you want some other security model, I'd like to understand what this other security model is.

	Tzuhao Kuo [:kaku] Assignee
	Comment 94 • 9 years ago

(In reply to Not doing reviews right now from comment #93)
> > an successfully created ImageBitmap should be assigned with an origin
> 
> Why?
> 
> Or to turn this around, what is the actual security model you're trying to
> create ImageBitmap?  Is it that once you create one you can use it wherever
> and it never induces tainting (which is what the spec seems like to me)?  If
> so, it may be sufficient to set mCORSUsed to true, change the assert in
> DoDrawImageSecurityCheck to only assert aPrincipal if CORSUsed is false, and
> add some documentation about how mPrincipal can be null if mCORSUsed is true.
> 
> If, on the other hand, you want some other security model, I'd like to
> understand what this other security model is.

I was thinking about that the ImageBitmap might taint Canvases or CanvasPatterns. But I think you're right that ImageBitmap never taints others since the spec description about "how to do security check in drawImage() and createPattern() (https://html.spec.whatwg.org/#the-image-argument-is-not-origin-clean)" does not include ImageBitmap case. Thanks for your correction.

	Tzuhao Kuo [:kaku] Assignee
	Comment 95 • 9 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #91)
> Here is the proposal to make ImageBitmap transferable:
> https://wiki.whatwg.org/wiki/WorkerCanvas
> 
> I don't know of a written proposal to use ImageBitmap with texImage, but it
> seems a very logical thing to do.

Ok, I will file new bugs for those extensions of ImageBitmap. 
* Support structured cloning for ImageBitmap.
* ImageBitmap as a transferable object.
* ImageBitmp as texImage in WebGL.
Also file bugs for those issues need further discussion/attention.
* Blob.isClosed() is needed but not implemented yet.
* HtmlImage/VideoElement's principal does not match the concept of spec's origin.

	Tzuhao Kuo [:kaku] Assignee
	Comment 97 • 9 years ago

Modified with bz's comment and add EnsureTarget() before calling ImageBitmap::PrepareForDrawTarget().

	Tzuhao Kuo [:kaku] Assignee
	Comment 98 • 9 years ago

Get rid of SpecialPowers. Carry r+.

	Tzuhao Kuo [:kaku] Assignee
	Comment 103 • 9 years ago

Add test cases for:
(1) security checks
(2) ImageBitmap as source of CreatePattern().

	(no longer active)
	Comment 104 • 9 years ago

Tzuhao, I'm sorry but I will be very busy for the next two weeks or so working on a very high priority project, so I'm planning to defer the reviews here until then.  Is that OK, or is this high priority?

Thanks!

	Tzuhao Kuo [:kaku] Assignee
	Comment 105 • 9 years ago

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #104)
It's ok and thanks for offering to review these patches.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 106 • 9 years ago

I'll try to review this stuff tomorrow.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 107 • 9 years ago

(In reply to Tzuhao Kuo [:kaku] from comment #95)
> > I don't know of a written proposal to use ImageBitmap with texImage, but it
> > seems a very logical thing to do.
...

> Also file bugs for those issues need further discussion/attention.
> * HtmlImage/VideoElement's principal does not match the concept of spec's
> origin.

Did you file this bug? I guess we might need a spec bug and Gecko bug.

	Boris Zbarsky [:bzbarsky]
	Comment 108 • 9 years ago

There's a spec bug at https://www.w3.org/Bugs/Public/show_bug.cgi?id=28374

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 109 • 9 years ago

Comment on attachment 8573820 [details] [diff] [review]
0001-Bug-1044102-Part-1-ImageBitmap-createImageBitmap-Web.patch

>+already_AddRefed<Promise>
>+nsGlobalWindow::CreateImageBitmap(const ImageBitmapSource& image,
>+                                  ErrorResult& aRv)
s/image/aImage


>+already_AddRefed<Promise>
>+nsGlobalWindow::CreateImageBitmap(const ImageBitmapSource& image,
>+                                  int32_t sx, int32_t sy, int32_t sw, int32_t sh,
>+                                  ErrorResult& aRv)
aImage, aSx, aSy, aSw, aSh


>+  // ImageBitmapFactories methods
>+  already_AddRefed<mozilla::dom::Promise>
>+  CreateImageBitmap(const mozilla::dom::ImageBitmapSource& image,
>+                    mozilla::ErrorResult& aRv);
>+
>+  already_AddRefed<mozilla::dom::Promise>
>+  CreateImageBitmap(const mozilla::dom::ImageBitmapSource& image,
>+                    int32_t sx, int32_t sy, int32_t sw, int32_t sh,
>+                    mozilla::ErrorResult& aRv);
>+
Same nits here.

>+interface ImageBitmapFactories {
>+  [Throws]
>+  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image);
>+  [Throws]
>+  Promise<ImageBitmap> createImageBitmap(ImageBitmapSource image, long sx, long sy, long sw, long sh);
So for now it I'd like to see these being under some Pref.


>+  // ImageBitmapFactories methods
>+  already_AddRefed<Promise>
>+  CreateImageBitmap(const ImageBitmapSource& image, ErrorResult& aRv);
>+
>+  already_AddRefed<Promise>
>+  CreateImageBitmap(const ImageBitmapSource& image,
>+                    int32_t sx, int32_t sy, int32_t sw, int32_t sh,
>+                    ErrorResult& aRv);
Also here, arguments should be in form aFoo


r+ those fixed, and pref added. We may want to have it enabled first in Nightly/Aurora only, and disabled in beta/release.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 110 • 9 years ago

Comment on attachment 8573821 [details] [diff] [review]
0002-Bug-1044102-Part-2-Support-HTML-Image-Video-Canvas-E.patch


>-ImageBitmap::ImageBitmap(nsIGlobalObject* aGlobal)
>-  : mParent(aGlobal)
>+ImageBitmap::ImageBitmap(nsIGlobalObject* aGlobal, SourceSurface* aSurface)
>+  : mCropRect(0, 0, aSurface->GetSize().width, aSurface->GetSize().height)
Ok, so this is mainthread only ctor, at least for now, given that SourceSurface is expected to be non-null.
Perhaps assert?

>-  aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
>+  nsRefPtr<ImageBitmap> imageBitmap;
>+
>+  if (aSrc.IsHTMLImageElement()) {
>+    MOZ_ASSERT(!NS_IsMainThread());
>+    imageBitmap = CreateInternal(aGlobal, aSrc.GetAsHTMLImageElement(), aRv);
>+  }
>+  else if (aSrc.IsHTMLVideoElement()) {
} else if (...)

>+    MOZ_ASSERT(!NS_IsMainThread());
>+    imageBitmap = CreateInternal(aGlobal, aSrc.GetAsHTMLVideoElement(), aRv);
>+  }
>+  else if (aSrc.IsHTMLCanvasElement()) {
>+    MOZ_ASSERT(!NS_IsMainThread());
>+    imageBitmap = CreateInternal(aGlobal, aSrc.GetAsHTMLCanvasElement(), aRv);
>+  }
>+  else {
} else {


>+  if (!aRv.Failed()) {
>+    MessageLoop::current()->PostTask(
>+      FROM_HERE,
>+      NewRunnableFunction(&FulfillImageBitmapPromise, promise, imageBitmap));
Please use normal XPCOM event loop, not the IPC loop.
So, something like NS_DispatchToMainThread(NS_NewRunnableMethodWithArgs(...))

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 111 • 9 years ago

Comment on attachment 8575804 [details] [diff] [review]
Part-6-ImageData-as-ImageBitamp-sources-.patch

>+ImageBitmap::CreateInternal(nsIGlobalObject* aGlobal, ImageData& aImageData, ErrorResult& aRv)
>+{
>+  // copy data into SourceSurface
>+  dom::Uint8ClampedArray array;
>+  DebugOnly<bool> inited = array.Init(aImageData.GetDataObject());
>+  MOZ_ASSERT(inited);
>+
>+  array.ComputeLengthAndData();
>+  SurfaceFormat const FORMAT = SurfaceFormat::B8G8R8A8;
>+  uint32_t const BYTES_PER_PIXEL = BytesPerPixel(FORMAT);
>+  uint32_t const imageWidth = aImageData.Width();
>+  uint32_t const imageHeight = aImageData.Height();
>+  uint32_t const imageStride = imageWidth * BYTES_PER_PIXEL;
>+  uint32_t const dataLength = array.Length();
>+  gfx::IntSize const imageSize(imageWidth, imageHeight);
>+
>+  // check the ImageData is neutered or not
>+  if (imageWidth == 0 || imageHeight == 0 ||
>+      (imageWidth*imageHeight*BYTES_PER_PIXEL) != dataLength) {
space before and after *

>+  RefPtr<DataSourceSurface> surface =
>+      Factory::CreateDataSourceSurfaceWithStride(imageSize, FORMAT, imageStride);
2 spaces for indentation.

This needs a review from a gfx peer. I don't know if this stuff is safe in worker threads (I think it is).
And we really must have tests for all this stuff run also in workers.

CreateDataSourceSurfaceWithStride may return null, so you need a null check.


>     imageBitmap = CreateInternal(aGlobal, aSrc.GetAsHTMLCanvasElement(), aRv);
>   }
>+  else if (aSrc.IsImageData()) {
>+    imageBitmap = CreateInternal(aGlobal, aSrc.GetAsImageData(), aRv);
>+  }
>   else {
Again,
} else if (...) {
  ...
} else {

r+ assuming all those handled.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 112 • 9 years ago

Comment on attachment 8575803 [details] [diff] [review]
Part-5-Add-ImageBitmap-test.patch

marking this r-, since we really need tests for the worker stuff.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 113 • 9 years ago

Comment on attachment 8575805 [details] [diff] [review]
Part-7-CanvasRenderingContext2D-as-Image.patch

} else if () {

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 114 • 9 years ago

Comment on attachment 8575806 [details] [diff] [review]
Part-8-ImageBitmap-as-ImageBitamp-source.patch

2 spaces for indentation.
CreateDataSourceSurfaceWithStride may return null, so needs null check.

} else if (...) {

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 115 • 9 years ago

Comment on attachment 8575809 [details] [diff] [review]
Part-10-Add-test-cases-of-security-check.patch

We need worker tests too.
It would be good to try to share as much as possible the same tests
for main thread and for workers.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 116 • 9 years ago

Comment on attachment 8575807 [details] [diff] [review]
Part-9-Blob-as-ImageBitamp-sources.patch

>+already_AddRefed<ImageBitmap>
>+ImageBitmap::CreateInternalFromBlobAsync(nsIGlobalObject* aGlobal, File& aBlob, ErrorResult& aRv)
>+{
Please add assertions that this is used only in the main thread.


>+  FileImpl *fileImpl = aBlob.Impl();
FileImpl* fileImpl

>+
>+  // get the internal stream of the blob
>+  nsIInputStream *stream = nullptr;
nsIInputStream* stream


>+  nsresult rv;
>+  rv = fileImpl->GetInternalStream(&stream);
You leak stream here.
Use nsCOMPtr<nsIInputStrea> stream; and then getter_AddRefs when passing it as param.


>+  // get the mine type string of the blob
>+  // the type will be checked in the DecodeImage() method.
>+  nsAutoString mineTypeUTF16;
>+  aBlob.GetType(mineTypeUTF16);

mine? Should be mime



>+  NS_ConvertUTF16toUTF8 mineTypeUTF8(mineTypeUTF16); // NS_ConvertUTF16toUTF8 ---|> nsAutoCString
>+  imgIContainer *imgContainer = nullptr;
>+  rv = aImgTools->DecodeImage(stream, mineTypeUTF8, &imgContainer);
Again, you leak imgContainer.
Please test all these patches in debug build.



>+class CreateImageBitmapFromBlobTask : public nsRunnable
>+{
>+public:
>+  CreateImageBitmapFromBlobTask(Promise* aPromise,
>+                                nsIGlobalObject* aGlobal,
>+                                mozilla::dom::File& aBlob,
>+                                bool aCrop, IntRect const &aCropRect)
>+  :mPromise(aPromise), mGlobalObject(aGlobal), mBlob(&aBlob),
>+   mCrop(aCrop), mCropRect(aCropRect)
>+  {}
>+
>+  NS_IMETHOD Run()
>+  {
>+    ErrorResult error;
>+    nsRefPtr<ImageBitmap> imageBitmap =
>+        ImageBitmap::CreateInternalFromBlobAsync(mGlobalObject, *mBlob, error);
2 spaces for indentation.

>+
>+    // handle errors while creating ImageBitmap
>+    // (1) error occurs during reading of the object
>+    // (2) the image data is not in a supported file format
>+    // (3) the image data is corrupted
>+    // All these three cases should reject promise with null value
>+    if (error.Failed()) {
>+      mPromise->MaybeReject(error);
>+      return error.ErrorCode();
>+    }
>+
>+    if (imageBitmap && mCrop) {
>+      imageBitmap->SetCrop(mCropRect, error);
>+
>+      if (error.Failed()) {
>+        mPromise->MaybeReject(error);
>+        return error.ErrorCode();
>+      }
>+    }
>+
>+    FulfillImageBitmapPromise(mPromise, imageBitmap);
>+    return error.ErrorCode();
>+  }
>+
>+private:
>+  nsRefPtr<Promise> mPromise;
>+  nsCOMPtr<nsIGlobalObject> mGlobalObject;
>+  RefPtr<mozilla::dom::File> mBlob;
>+  bool mCrop;
>+  IntRect mCropRect;
Promise and globalobject must be released and used only in the same thread they are created.

>+  else if (aSrc.IsBlob()) {
>+    // check blob is closed or not. Bug1048325
>+    nsCOMPtr<nsIRunnable> runnable =
>+        new CreateImageBitmapFromBlobTask(promise, aGlobal, aSrc.GetAsBlob(), aCrop, aCropRect);
>+    NS_DispatchToMainThread(runnable);
>+    return promise.forget();
>+  }
This is not safe in workers. You pass worker-only objects to main thread.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 117 • 9 years ago

Comment on attachment 8573822 [details] [diff] [review]
0003-Bug-1044102-Part-2.1-Security-and-status-checks.patch

This should be update to use the normal coding style.
Foo* variableName
and aFoo for argument naming


This uses GetEntryGlobal() and I thought we'd go with incumbent global.
Also, Get*Global methods may return null.

	Tzuhao Kuo [:kaku] Assignee
	Comment 118 • 9 years ago

Thank you for the review!
Sorry for lots of coding style problem and even typos, I will fix them all.
Also will I catch up test cases for worker.

(In reply to Olli Pettay [:smaug] from comment #107)
> > Also file bugs for those issues need further discussion/attention.
> > * HtmlImage/VideoElement's principal does not match the concept of spec's
> > origin.
> 
> Did you file this bug? I guess we might need a spec bug and Gecko bug.
(In reply to Olli Pettay [:smaug] from comment #117)
> This uses GetEntryGlobal() and I thought we'd go with incumbent global.
> Also, Get*Global methods may return null.
I haven't filed all the follow-up bugs and will do it when this bug is getting done.
So, for getting this bug done, I will use the incumbent global.

(In reply to Olli Pettay [:smaug] from comment #116)
> Comment on attachment 8575807 [details] [diff] [review]
> Part-9-Blob-as-ImageBitamp-sources.patch
> 
> >+already_AddRefed<ImageBitmap>
> >+ImageBitmap::CreateInternalFromBlobAsync(nsIGlobalObject* aGlobal, File& aBlob, ErrorResult& aRv)
> >+{
> Please add assertions that this is used only in the main thread.
> 
> >+private:
> >+  nsRefPtr<Promise> mPromise;
> >+  nsCOMPtr<nsIGlobalObject> mGlobalObject;
> >+  RefPtr<mozilla::dom::File> mBlob;
> >+  bool mCrop;
> >+  IntRect mCropRect;
> Promise and globalobject must be released and used only in the same thread
> they are created.
> 
> >+  else if (aSrc.IsBlob()) {
> >+    // check blob is closed or not. Bug1048325
> >+    nsCOMPtr<nsIRunnable> runnable =
> >+        new CreateImageBitmapFromBlobTask(promise, aGlobal, aSrc.GetAsBlob(), aCrop, aCropRect);
> >+    NS_DispatchToMainThread(runnable);
> >+    return promise.forget();
> >+  }
> This is not safe in workers. You pass worker-only objects to main thread.
For the blob-as-source case, I use the mozilla::image::RasetrImage to decode the blob and the implementation now is main-thread only. I will add a assertion in the ImageBitmap::Create() method to prevent be called from worker.

And OK for all others.

	Tzuhao Kuo [:kaku] Assignee
	Comment 119 • 9 years ago

Carry r+.

	Tzuhao Kuo [:kaku] Assignee
	Comment 122 • 9 years ago

Carry r+.

	Tzuhao Kuo [:kaku] Assignee
	Comment 123 • 9 years ago

Carry r+.

	Tzuhao Kuo [:kaku] Assignee
	Comment 124 • 9 years ago

Carry r+.

	Tzuhao Kuo [:kaku] Assignee
	Comment 126 • 9 years ago

Fix coding style problem, types, memory leakage (tried with debug build), main-thready-only assertion, null pointer checks, XPCOM event loop and the setting object issue.

Upload patches which are related to the above issues first and keep writing test cases for worker.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 127 • 9 years ago

Comment on attachment 8595319 [details] [diff] [review]
Part-2-Support-HTML-Image-Video-Canvas-E.patch

>+  NS_IMETHOD Run() {
Nit, { goes to its own line.

>+  if (aSrc.IsHTMLImageElement()) {
>+    MOZ_ASSERT(NS_IsMainThread(), "Create ImageBitmap from HTMLImageElement off main thread.");
"Createing ImageBitmap from HTMLImageElement off the main thread.", or something like that I think. Also elsewhere.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 128 • 9 years ago

Comment on attachment 8595325 [details] [diff] [review]
Part-9-Blob-as-ImageBitamp-sources.patch

We support Blobs on workers, so CreateImageBitmapFromBlobTask etc. need to be able to deal with both main thread and worker threads.
SourceSurface doesn't seem to have threadsafe refcounting, so perhaps you need to
create SourceSurface on the main thread always, and then in case of workers just pass the data it contains
to worker thread.


Nit, aImgTools has 'a' as prefix, but 'a' should be used with arguments only.

Please test this all in debug build and test workers and main thread.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 129 • 9 years ago

Comment on attachment 8595321 [details] [diff] [review]
Part-2.1-Security-and-status-checks.patch

>+CheckNonBitmapSource(HTMLImageElement& aImageEl)
>+{
>+  nsresult rv;
>+
>+  nsCOMPtr<imgIRequest> imgRequest;
>+  rv = aImageEl.GetRequest(nsIImageLoadingContent::CURRENT_REQUEST, getter_AddRefs(imgRequest));
Just declare rv here where you use it first time.

> ImageBitmap::CreateInternal(nsIGlobalObject* aGlobal, HTMLVideoElement& aVideoEl, ErrorResult& aRv)
> {
>-  // TODO: check network state
>-  // TODO: check origin
>-  // TODO: check ready state
>+  if (aVideoEl.NetworkState() == HTMLMediaElement::NETWORK_EMPTY) {
>+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>+    return nullptr;
>+  }
>+
>+  if (aVideoEl.ReadyState() == HTMLMediaElement::HAVE_NOTHING ||
>+      aVideoEl.ReadyState() == HTMLMediaElement::HAVE_METADATA) {
>+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>+    return nullptr;
>+  }
Why you aren't doing the security check for HTMLVideoElement as the spec says?