Closed Bug 825527 Opened 7 years ago Closed 7 years ago

Convert HTMLImageElement to WebIDL

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: ehsan, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 7 obsolete files)

30.37 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.46 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
16.37 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
8.18 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
15.57 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
We need this for putting canvases in workers.
What has this to do with workers?
Indeed.  We're not going to have HTMLImageElement in workers, so it seems like all we need to do is map it to JSObject* in the worker descriptor, no?
We're going to need ImageBitmap, and if we want to do that in WebIDL, the cleanest way would be to move a bunch of HTML elements to WebIDL, including HTMLImageElement.
OK.  It's not clear to me why _that_ is, but don't let me stop you from doing HTMLImageElement.  ;)  It's a bit of work to do all the bits like nsIImageLoadingContent right...
Oh, and we'll need to add [NamedConstructor] support for this, probably.  Though we might be able to get away without doing that yet, if we really really have to...
Hmm, how does the current constructor for Image() work?  I'm not sure if we can just keep that work the way that it currently does and comment out the NamedConstructor stuff from the IDL for now?
The current constructor uses various classinfo bits and ends up creating image elements in weird ways; see the logic in nsHTMLImageElement.

We can probably keep the existing constructor, but it'll violate the spec in that Image.prototype will not be the same as HTMLImageElement.prototype.  On the other hand, we already have that problem, so it's probably not completely fatal.
(In reply to comment #7)
> The current constructor uses various classinfo bits and ends up creating image
> elements in weird ways; see the logic in nsHTMLImageElement.
> 
> We can probably keep the existing constructor, but it'll violate the spec in
> that Image.prototype will not be the same as HTMLImageElement.prototype.  On
> the other hand, we already have that problem, so it's probably not completely
> fatal.

I see, ok.  Filed bug 825628 in order to implement NamedConstructor.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #696764 - Flags: review?(bzbarsky)
Attachment #696765 - Flags: review?(bzbarsky)
Comment on attachment 696765 [details] [diff] [review]
Part 2: Move HTMLImageElement to WebIDL bindings

I haven't read through this in detail, but this will break all sorts of UI, and fail tests, because it will break all consumers of nsIImageLoadingContent on images.

You need to expose that API on HTMLImageElement with [ChromeOnly] bits.
Attachment #696765 - Flags: review?(bzbarsky) → review-
Comment on attachment 696764 [details] [diff] [review]
Part 1: Rename nsHTMLImageElement to mozilla::dom::HTMLImageElement

This needs to be an hg move, not an hg delete and hg add.

Ideally, I recommend doing an hg mv from nsHTMLImageElement.cpp to HTMLImageElement.cpp, then an hg cp from HTMLImageElement.cpp to HTMLImageElement.h, then deleting the no-longer-needed parts from the .h and .cpp so as to preserve blame as much as possible.  Or whatever the git equivalent is, of course, but the upshot should be the same.
Attachment #696764 - Flags: review?(bzbarsky) → review-
I ran the tests in content/html/content, and here are some test failures:

 5:16.31 21265 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug389797.html | <img> does not QI to imgINotificationObserver - got false, expected true
 5:16.31 21267 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug389797.html | <img> does not QI to nsIImageLoadingContent - got false, expected true

I'm not sure if this is really supposed to work with WebIDL bindings.  Boris?

 5:16.31 25573 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug586763.html | Reflected attribute img.hspace should default to 0 - got undefined, expected 0
 5:16.31 25574 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug586763.html | Reflected attribute img.vspace should default to 0 - got undefined, expected 0

Hmm, http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/html/nsIDOMHTMLImageElement.idl supports a whole bunch of extra properties.  What should we do about them?

 5:16.31 29272 ERROR TEST-UNEXPECTED-PASS | /tests/content/html/content/test/test_bug664299.html | null should have been stringified to 'null' - null should equal null
 5:16.31 29273 ERROR TEST-UNEXPECTED-PASS | /tests/content/html/content/test/test_bug664299.html | null should have been stringified to 'null' - null should equal null

This patch fixes this test: https://gist.github.com/4422031 but I'm not sure what the right thing to do is here, since the null stringification is only fixed for images with my patches.
Sorry, forgot to use the correct git diff flags when generating this patch...
Attachment #696764 - Attachment is obsolete: true
Attachment #696767 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky (:bz) from comment #11)
> Comment on attachment 696765 [details] [diff] [review]
> Part 2: Move HTMLImageElement to WebIDL bindings
> 
> I haven't read through this in detail, but this will break all sorts of UI,
> and fail tests, because it will break all consumers of
> nsIImageLoadingContent on images.
> 
> You need to expose that API on HTMLImageElement with [ChromeOnly] bits.

I'm not really sure what that would look like...  The current js code seems to be expecting to QI image elements to nsIImageLoadingContent.  We can't make that work with the WebIDL binding, can we?  Or are you talking about mirroring all nsIImageLoadingContent methods on HTMLImageElement?
> I'm not sure if this is really supposed to work with WebIDL bindings.  Boris?

Yeah, with WebIDL bindings QI will happily QI to any interface.

Why is the test testing non-QIing to that interface?

> a whole bunch of extra properties.  What should we do about them?

Are they the ones in the "partial interface HTMLImageElement" under http://www.whatwg.org/specs/web-apps/current-work/multipage/obsolete.html#other-elements,-attributes-and-apis (the "obsolete APIs" section of the spec, which everyone always forgets about)?

> This patch fixes this test: https://gist.github.com/4422031

See the changes to this file in https://bug824327.bugzilla.mozilla.org/attachment.cgi?id=696244 and the corresponding discussion in bug 824907.

> I'm not really sure what that would look like... 

You just need to expose the constants, attributes, methods, all as [ChromeOnly].

> We can't make that work with the WebIDL binding, can we? 

WebIDL stuff will always QI to everything and return the same exact object.

> Or are you talking about mirroring all nsIImageLoadingContent methods on
> HTMLImageElement?

Yes, exactly.
(In reply to Boris Zbarsky (:bz) from comment #16)
> > I'm not sure if this is really supposed to work with WebIDL bindings.  Boris?
> 
> Yeah, with WebIDL bindings QI will happily QI to any interface.
> 
> Why is the test testing non-QIing to that interface?

Hmm, hard to say.  This was added in bug 389797 by, ehm, you.  ;-)

> > a whole bunch of extra properties.  What should we do about them?
> 
> Are they the ones in the "partial interface HTMLImageElement" under
> http://www.whatwg.org/specs/web-apps/current-work/multipage/obsolete.
> html#other-elements,-attributes-and-apis (the "obsolete APIs" section of the
> spec, which everyone always forgets about)?

Precisely.  :-)  Should I use a hack similar to the one in HTMLHeadingElement.webidl to work around partial interface?

> > This patch fixes this test: https://gist.github.com/4422031
> 
> See the changes to this file in
> https://bug824327.bugzilla.mozilla.org/attachment.cgi?id=696244 and the
> corresponding discussion in bug 824907.

I see.  Great!

> > I'm not really sure what that would look like... 
> 
> You just need to expose the constants, attributes, methods, all as
> [ChromeOnly].

Ah, what a pain.  :(  Do you object to me doing that in a separate patch to keep things cleaner?

> > We can't make that work with the WebIDL binding, can we? 
> 
> WebIDL stuff will always QI to everything and return the same exact object.

Huh!  I definitely would not expect that.  Why is that, out of curiosity?
> Hmm, hard to say.  This was added in bug 389797 by, ehm, you.  ;-)

OK.  I'll try to look into it, but not on a day I'm officially not working (so on Wednesday).

> Should I use a hack similar to the one in
> HTMLHeadingElement.webidl to work around partial interface?

For now, yes.

> Do you object to me doing that in a separate patch to
> keep things cleaner?

No, but I think at that point we should have the following sequence of patches:

1)  Rename
2)  Add per-spec WebIDL API, but don't actually add the WebIDL file to WebIDL.mk yet and
    don't SetIsDOMBinding().
3)  Add the [ChromeOnly] bits.
4)  Flip to using the WebIDL binding.

so that we don't have broken changesets in the tree.

> Huh!  I definitely would not expect that.  Why is that, out of curiosity?

Because we're trying to phase out QI altogether for WebIDL stuff, because it's nonsensical: WebIDL objects may not even be XPCOM objects!  So for now we did the simplest thing that doesn't break code and will work on trying to remove it later...
> OK.  I'll try to look into it

Ah, so the actual test is basically testing that

  (new Image()) instanceof Components.interfaces.imgINotificationObserver

tests true and similar for nsIImageLoadingContent.  That should still be working.  Why isn't it?  Step through nsJSIID::HasInstance and see what happens, perhaps?  It's possible that we used to test true for non-classinfo interfaces but no longer do with WebIDL bindings (arguably a bug) or something like that...  Peter?
Flags: needinfo?(peterv)
Right now we only handle instanceof on interfaces exposed through classinfo. We might need to fix that now (I think this is only a problem for elements, they're the only DOM objects that don't have CLASSINFO_INTERFACES_ONLY).
Flags: needinfo?(peterv)
Hmm.  And the problem is that we need to not expose random non-classinfo interfaces for non-element things, thus raising the question of how to tell what interfaces to expose on which objects (because QI can't tell)?

I suppose one possibility would be to just add nsIImageLoadingContent and whatnot to the classinfo for HTMLImageElement, since that won't flatten those methods onto the WebIDL object or anything (which is the only reason those elements don't have that interface in the classinfo).  But long-term we might want a better solution, since we want to stop doing classinfo for WebIDL stuff, right?
Flags: needinfo?(peterv)
Attachment #696765 - Attachment is obsolete: true
Attachment #697115 - Flags: review?(bzbarsky)
Attachment #697117 - Flags: review?(bzbarsky)
Comment on attachment 696767 [details] [diff] [review]
Part 1: Rename nsHTMLImageElement to mozilla::dom::HTMLImageElement

>+++ b/content/html/content/src/HTMLImageElement.cpp
>+HTMLImageElement::ParseAttribute(int32_t aNamespaceID,
>                                    nsIAtom* aAttribute,
>                                    const nsAString& aValue,
>                                    nsAttrValue& aResult)

Please fix indent.

>+HTMLImageElement::GetAttributeChangeHint(const nsIAtom* aAttribute,
>                                            int32_t aModType) const

Likewise.

If you're up for it, it would be nice to actually wrap the main body of the file in the mozilla and dom namespaces and remove the "using" declarations. That requires that the DOMCI_NDOE_DATA and NS_NewHTMLImageElement happen outside the namespaces, though.

>+++ b/content/html/content/src/HTMLImageElement.h
>+#ifndef HTMLImageElement_h

I think we've been using mozilla_dom_HTMLImageElement_h for the include guard for this stuff.

r=me
Attachment #696767 - Flags: review?(bzbarsky) → review+
Comment on attachment 697115 [details] [diff] [review]
Part 2: Move HTMLImageElement to WebIDL bindings

>+++ b/content/canvas/src/CanvasRenderingContext2D.cpp

The changes to this file look like they should be in the "turn it on" patch, not in this one.  Does this compile?

>+++ b/content/canvas/src/WebGLContext.h
>     void TexSubImage2D(WebGLenum target, WebGLint level,
>                        WebGLint xoffset, WebGLint yoffset, WebGLenum format,
>-                       WebGLenum type, ElementType* elt, ErrorResult& rv) {
>+                       WebGLenum type, ElementType& elt, ErrorResult& rv) {

Shouldn't that be a const ref?  Otherwise if callers pass in a pointer this function can now modify the value pointed to by the pointer...

>+++ b/content/html/content/src/HTMLImageElement.h
>+  bool IsMap()

The isMap attribute should be [SetterThrows] in the IDL and probably should just use GetBoolAttr/SetHTMLBoolAttr instead of calling the XPCOM versions.

>+  uint32_t Width()
>+  uint32_t Height()

For these, I'd rather we made these the canonical implementations (still inlined is fine if desired) and made the XPCOM versions call these.

Also, these should also be [SetterThrows] and we should add a SetHTMLUnsignedIntAttr along the lines of SetHTMLIntAttr.  The implementation can look identical, I think, looking at what nsGenericHTMLElement::SetUnsignedIntAttr does.

The various string-valued mapped attributes here (alt, src, crossOrigin, useMap, name, align, longDesc, border) should also be [SetterThrows] and need setters using SetHTMLAttr.  You can either add comments saying the XPCOM getters are OK, or if desired define non-virtual inline getters taking "nsString&" (as opposed to "nsAString&") and have those call GetHTMLAttr() directly.

>+  uint32_t NaturalWidth()
>+  uint32_t NaturalHeight()
>+  bool Complete()

Again, I'd prefer that the WebIDL implementation be the canonical one and the XPCOM one forward to it.

>+  int32_t Hspace()
>+  int32_t Vspace()

These also need [SetterThrows] and should use GetIntAttr/SetHTMLIntAttr instead of calling the XPCOM methods..

>+++ b/content/html/content/test/reflect.js
>+  // Elements on new bindings should take the first branch
>+  if ((element.localName == "textarea" && idlAttr == "wrap") ||
>+     (Object.prototype.toString.call(Object.getPrototypeOf(element)).substring(8, 11) != "XPC")) {

Peter has convinced me that the approach in bug 824907 is a better one.  Can we do that here instead, please?

>+++ b/dom/bindings/Bindings.conf
>+++ b/dom/webidl/CanvasRenderingContext2D.webidl
>+++ b/dom/webidl/WebGLRenderingContext.webidl

The changes in these files seem like they should be in part 4.

>+++ b/dom/webidl/HTMLImageElement.webidl
>+ * http://www.whatwg.org/specs/web-apps/current-work/multipage/embedded-content-1.html#htmlimageelement
>+ * http://www.whatwg.org/specs/web-apps/current-work/multipage/obsolete.html#other-elements,-attributes-and-apis

Drop the bit between "current-work/" and "#" in both of those, please; I don't terribly trust the organization of the multipage version to not change.

>+[NamedConstructor=Image(),
>+ NamedConstructor=Image(unsigned long width),
>+ NamedConstructor=Image(unsigned long width, unsigned long height)]

Worth raising a spec issue about whether this should instead be a single constructor with two optional arguments.

I'd like to see an updated patch here.
Attachment #697115 - Flags: review?(bzbarsky) → review-
Comment on attachment 697116 [details] [diff] [review]
Part 3: Mirror the nsIImageLoadingContent API in HTMLImageElement for Chrome callers

>    Bug 825527 - Part 3: Mirror the nsIImageLoadingContent API in HTMLImageElement for Chrome callers

s/Chrome/chrome/ and should mention imgINotificationObserver in here too

>+++ b/content/html/content/src/HTMLImageElement.h
>+  nsIURI* CurrentURI() const

No, this is wrong.  First of all, this function can return null (so should return "URI?" in the IDL, and be called GetCurrentURI.  Second, it returns something different from mCurrentURI in many cases.

I'd rather we had a non-XPCOM GetCurrentURI on nsImageLoadingContent and rewrote the XPCOM getter to call it.

In fact, I would argue that all the nsIImageLoadingContent WebIDL stuff should be on nsImageLoadingContent, so we only have to implement it once.  And as before, we should make the WebIDL versions the canonical ones and make the XPCOM ones call them; once all subclasses of nsIImageLoadingContent are on WebIDL, we can then remove all the non-constant bits of this interface.

If you do that, note that the IsCallerChrome() checks that can cause things to throw are only needed in the XPCOM versions of the methods, since for WebIDL those methods are only exposed to chrome.  Same thing for the null-checks of the aObserver argument to add/removeObserver.

>+  using nsImageLoadingContent::LoadingEnabled;

Why is that needed?

>+++ b/content/html/content/test/test_bug389797.html
>+HTML_TAG("img", "Image", [ "imgINotificationObserver",
>+                               "nsIImageLoadingContent" ], []);

So I just checked, and it looks like imgINotificationObserver is not actually used in js files in m-c or c-c, or anywhere in the addons mxr.  So I think we should just remove it from the list here, not add its stuff to the WebIDL interface, and not expose it in classinfo.

Also, the changes to this file should go in part 4, I think.

>+++ b/dom/base/nsDOMClassInfo.cpp
>+    DOM_CLASSINFO_MAP_ENTRY(imgINotificationObserver)

And so should these.

>+++ b/dom/bindings/Bindings.conf
>+addExternalIface('nsIChannel', nativeType='nsIChannel')

This is already present in this file, as 'MozChannel'.  Just use that in your WebIDL, please.

>+addExternalIface('nsIURI', nativeType='nsIURI')

Already present in this file as 'URI'.

>+++ b/dom/webidl/HTMLImageElement.webidl
>+  imgIRequest getRequest(long aRequestType);

This needs to return "imgIRequest?".

>+  readonly attribute nsIURI currentURI;

This needs to return "URI?".

>+  nsIStreamListener loadImageWithChannel(nsIChannel aChannel);

The implementation seems to return null in some cases (though those cases look slightly bogus to me).  Should probably be returning "nsIStreamListener?".

Again, would like to see an updated patch.
Attachment #697116 - Flags: review?(bzbarsky) → review-
Comment on attachment 697117 [details] [diff] [review]
Part 4: Turn on the WebIDL bindings for HTMLImageElement

r=me, but note the comments on previous patches about things that should move into this patch.
Attachment #697117 - Flags: review?(bzbarsky) → review+
Attached patch Fix some testsSplinter Review
Attachment #697302 - Flags: review?(bzbarsky)
(Filed spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=20554)
Attachment #697115 - Attachment is obsolete: true
Attachment #697324 - Flags: review?(bzbarsky)
Attachment #697116 - Attachment is obsolete: true
Attachment #697328 - Flags: review?(bzbarsky)
Comment on attachment 697302 [details] [diff] [review]
Fix some tests

r=me, though you may not need those enablePrivilege calls anymore either with this change.  Worth checking.
Attachment #697302 - Flags: review?(bzbarsky) → review+
Comment on attachment 697324 [details] [diff] [review]
Part 2: Move HTMLImageElement to WebIDL bindings

>+++ b/content/canvas/src/WebGLContext.h
>+                    ElementType& elt, ErrorResult& rv) {

const here too?

r=me with that.  Thanks!
Attachment #697324 - Flags: review?(bzbarsky) → review+
Comment on attachment 697329 [details] [diff] [review]
Use the correct prototype chain for instanceof HTMLImageElement checks in our UI code

Oh, bah.  Sorry to make you go through this; I should have caught this....

What you want to do is add 'hasInstanceInterface': 'nsIDOMHTMLImageElement' to the Bindings.conf entry for HTMLImageElement.  That will make the instanceof crap work for now.  We can obviously fix our UI, but that doesn't help with extensions.  :(
Attachment #697329 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky (:bz) from comment #36)
> Comment on attachment 697329 [details] [diff] [review]
> Use the correct prototype chain for instanceof HTMLImageElement checks in
> our UI code
> 
> Oh, bah.  Sorry to make you go through this; I should have caught this....
> 
> What you want to do is add 'hasInstanceInterface': 'nsIDOMHTMLImageElement'
> to the Bindings.conf entry for HTMLImageElement.  That will make the
> instanceof crap work for now.  We can obviously fix our UI, but that doesn't
> help with extensions.  :(

Ah, good to know!
Attachment #697117 - Attachment is obsolete: true
Attachment #697329 - Attachment is obsolete: true
Attachment #697333 - Flags: review?(bzbarsky)
Comment on attachment 697328 [details] [diff] [review]
Part 3: Mirror the nsIImageLoadingContent API in HTMLImageElement for Chrome callers

>+++ b/content/base/src/nsImageLoadingContent.cpp
>+already_AddRefed<imgIRequest>
>+nsImageLoadingContent::GetRequest(int32_t aRequestType)

This is changing the behavior when an unknown type is passed in.  Instead, we should mark this as [Throws] in the IDL and .Throw(NS_ERROR_UNEXPECTED) on the ErrorResult in the default case of the switch.

>+nsImageLoadingContent::GetRequest(int32_t aRequestType,
>+  return (*aRequest) ? NS_OK : NS_ERROR_UNEXPECTED;

And here, return the .ErrorCode() of the ErrorResult.  That will fix a but this is introducing where asking for a pending request when there isn't one throws instead of returning null.

>+nsImageLoadingContent::GetRequestType(imgIRequest* aRequest)

Again, this should be throwing as before if aRequest isn't one of our requests, right?

>+already_AddRefed<nsIURI>
>+nsImageLoadingContent::GetCurrentURI()
>+    NS_EnsureSafeToReturn(mCurrentURI, getter_AddRefs(uri));

We used to throw if this failed... Not sure it can do it other than on OOM, but probably better to preserve the behavior here.

>+++ b/content/base/src/nsImageLoadingContent.h
>+  // Web IDL binding methods

I'd really prefer these to come in the same order as the IDL.

Please document that the XPCOM SetLoadingEnabled, AddObserver, RemoveObserver, ForceImageState are OK for us because they never throw when called via WebIDL bindings.

On the other hand, the XPCOM impl of ForceReload is not OK.  It can throw, but right now that exception would be ignored.  It needs to be [Throws] in the IDL.
Attachment #697328 - Flags: review?(bzbarsky) → review-
Comment on attachment 697333 [details] [diff] [review]
Part 4: Turn on the WebIDL bindings for HTMLImageElement

r=me
Attachment #697333 - Flags: review?(bzbarsky) → review+
Comment on attachment 697338 [details] [diff] [review]
Part 3: Mirror the nsIImageLoadingContent API in HTMLImageElement for Chrome callers

r=me
Attachment #697338 - Flags: review?(bzbarsky) → review+
Comment on attachment 697333 [details] [diff] [review]
Part 4: Turn on the WebIDL bindings for HTMLImageElement

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

This should remove the quickstubs for nsIDOMHTMLImageElement.
Flags: needinfo?(peterv)
This added a todo for the align attribute on HTMLImageElement in reflect.js, which seems really wrong given that the WebIDL binding has the right default stringification. Seems like we don't have a test that hits that or how does it work?
(In reply to comment #46)
> This added a todo for the align attribute on HTMLImageElement in reflect.js,
> which seems really wrong given that the WebIDL binding has the right default
> stringification. Seems like we don't have a test that hits that or how does it
> work?

You're right.  There was no test examining it.  Added one in bug 827126.
Duplicate of this bug: 668823
Depends on: 859640
I have learned from Bug 864367, Bug 878509 
 and a SUMO forum post (https://support.mozilla.org/ja/questions/957874) that the deprecated lowsrc, x and y attributes have been dropped by this migration. Was it intended? If so, I'll update articles on MDN.
OS: Mac OS X → All
Hardware: x86 → All
Depends on: 878509
Definitely not intentional.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.