Bug 34782 – Normalize custom ctors for Image, Option, Audio

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yaar Schnitman]

Normalize custom ctors for Image, Option, Audio

[Yaar Schnitman]

Created attachment 48457 [details]
Patch

[Eric Seidel (no email)]

Attachment 48457 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/250865

[WebKit Review Bot]

Attachment 48457 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/251891

[Darin Adler]

Comment on attachment 48457 [details]
Patch

This is *so* funny. I have a similar patch on my machine that I started working on earlier today. I chose to emphasize the fact that the create functions are for use with JavaScript constructors by using the name createForJSConstructor rather than just create for them. And I did the work for WebKit's built-in JavaScript engine too, not just for V8. As you should.

You're also using some non-standard formatting here. In the WebKit project we do not put each argument of a function on a separate line, even if there are many of them.

The arguments should be "const String&" rather than "String". And we name things "document", not "doc", in new code at least.

And we don't use PassRefPtr for local variables.

[WebKit Review Bot]

Attachment 48457 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/250962

[Yaar Schnitman]

Very funny indeed!

Would you want to pick up from here, I should I continue working on this?

See my comments below:

(In reply to comment #4)
> (From update of attachment 48457 [details])
> I chose to emphasize the fact that the create functions are
> for use with JavaScript constructors by using the name createForJSConstructor
> rather than just create for them. 
That makes sense, but I want the create functions to follow the same naming conventions that already exist for XmlHttpRequest, Worker, etc.

> JavaScript engine too, not just for V8. As you should.
True.

Once these ctors are normalized, we can refactor the code-generators to auto-generated the boiler-plate code around them. I'm thinking about introducing new IDL ext attr's: 'ConstructViaNew', and 'ConstructWith=Document|ScriptExecutionContext', which will help V8/JSC populate the create dispatch. Another thing that will need to be solved is V8 not supporting [optional] parameters. Does JSC support [optional]?

[David Levin]

> diff --git a/WebCore/html/HTMLImageElement.cpp b/WebCore/html/HTMLImageElement.cpp

> +PassRefPtr<HTMLImageElement> HTMLImageElement::create(Document* doc, int width, int height)
> +{
> +    ASSERT(doc);
> +    HTMLImageElement* image = new HTMLImageElement(imgTag, doc);
> +    if (width >= 0)
> +        image->setWidth(width);
> +    if (height >= 0)
> +        image->setHeight(height);
> +    return image;

Shouldn't this be "return adoptRef(image);"

[Darin Adler]

(In reply to comment #7)
> Shouldn't this be "return adoptRef(image);"

No. Some DOM elements still start reference counts at 0. See bug 28068 for details.

[Darin Adler]

(In reply to comment #6)
> Would you want to pick up from here, I should I continue working on this?

I’ll post my partly done patch so you can merge the two. I need the media part of this for another bug fix I’m working on.

> > I chose to emphasize the fact that the create functions are
> > for use with JavaScript constructors by using the name createForJSConstructor
> > rather than just create for them. 
> That makes sense, but I want the create functions to follow the same naming
> conventions that already exist for XmlHttpRequest, Worker, etc.

One difference there is that those functions are only created from JavaScript.

> Once these ctors are normalized, we can refactor the code-generators to
> auto-generated the boiler-plate code around them. I'm thinking about
> introducing new IDL ext attr's: 'ConstructViaNew', and
> 'ConstructWith=Document|ScriptExecutionContext', which will help V8/JSC
> populate the create dispatch. Another thing that will need to be solved is V8
> not supporting [optional] parameters. Does JSC support [optional]?

I don't know.

[Darin Adler]

Created attachment 48655 [details]
work in progress

[Yaar Schnitman]

Created attachment 49032 [details]
Patch

[Yaar Schnitman]

Comment on attachment 49032 [details]
Patch

1. Darin and my patches are merged.
2. I adopted the createForJSConstructor naming.
3. Re: FIXME: always appending a text node to option: I think this is the right behavior for backward compatibility reasons and because option is quite useless without a text node with its content.
4. I liked the use of pointers for optional args, but I think it will make future auto-generation of the ctors too complex. Instead, I use default values. We can rethink this once the code-generation is fixed.
5. Added a layout test.

[Alexey Proskuryakov]

+    // Calling toJS on the document causes the JS document wrapper to be
+    // added to the window object. This is done to ensure that JSDocument::markChildren
+    // will be called, which will cause the audio element to be marked if necessary.
+    // FIXME: The correct way to do this would be to make HTMLMediaElement derive from
+    // ActiveDOMObject and use its interface to keep its wrapper alive. Then we would
+    // remove this code and the special case in isObservableThroughDOM.

Active DOM objects are marked from JSDocument::markChildren, so it seems that we would still need a document wrapper.

[Darin Adler]

Comment on attachment 49032 [details]
Patch

> +// Image tests
> +shouldBeNonNull("new Image()");
> +shouldBe("new Image(100).width", "100");
> +shouldBe("new Image(100,200).height", "200");

It's great to have some additional test coverage here. I'd like to see a lot more things tested on these objects, though. These objects have tons of attributes, and this tests only the ones you can change with arguments. It would be easy to test a few more things, like making sure we have the right type of object and at least covering some of the other key attributes such as ownerDocument and form. You could even use tagName or outerHTML to check we get the right kind of element and possibly check that we get the correct prototype.

There should definitely be a test of width and height values for the cases where they are not set explicitly with constructor arguments.

There should be a test that autobuffer gets set for the new Audio element but not for document.createElement("audio").

> +shouldBeEqualToString("new Option('data', 'value').value", 'value');

Same thing with value, a test where it is not set explicitly.

> +        Normalize custom ctors for Image, Option, Audio
> +        https://bugs.webkit.org/show_bug.cgi?id=34782
> +
> +        Test: fast/js/custom-constructors.html

I'm kind of disappointed at these change logs that are just automatically generated. I guess most people are doing that but I love the per function comments, even if brief.

> - * Copyright (C) 2007, 2008 Apple Inc. All rights reserved.
> + * Copyright (C) 2007, 2010 Apple Inc. All rights reserved.

Copyrights are followed by a list of years of publication. For a project like this where anyone can see things at any time, check-ins are roughly equivalent to publication. We should never remove a year as we are doing here with 2008. Did I do this in my patch? This should list the years that Apple work in this file was published. Lets just add 2010 without removing 2008.

> +    // Calling toJS on the document causes the JS document wrapper to be
> +    // added to the window object. This is done to ensure that JSDocument::markChildren
> +    // will be called, which will cause the audio element to be marked if necessary.
> +    // FIXME: The correct way to do this would be to make HTMLMediaElement derive from
> +    // ActiveDOMObject and use its interface to keep its wrapper alive. Then we would
> +    // remove this code and the special case in isObservableThroughDOM.
> +    toJS(exec, jsConstructor->globalObject(), document);

This change is a bug fix. Ideally we need a test to demonstrate this bug; show that an audio element won't be deallocated while the audio is playing in the case this covers. We can probably live without that test case for now. Geoff Garen might have some advice on how to test this.

> +    int width = -1;
> +    int height = -1;

Why is it OK to use "-1" as a special value here? The old code went out of its way to not have any special values and track whether a value was passed separate from the actual value. I think it's possibly wrong to make negative numbers have a special meaning. I believe this changes behavior for the case where an actual negative number is passed. There should be test cases covering this and a rationale for why it's OK to change the behavior mentioned in the change log at least.

> +    if (args.size() > 0 && !args.at(0).isUndefinedOrNull())
> +        width = args.at(0).toInt32(exec);
> +    if (args.size() > 1 && !args.at(1).isUndefinedOrNull())
> +        height = args.at(1).toInt32(exec);

The code here is doing extra work. The isUndefinedOrNull() check will work without any check of args.size, so you don't need both.

The old code in this function did not have a special case for undefined or null, treating both as "0", not as "no value specified". You changed the behavior but did not add any test cases covering these cases you changed nor did you give any rationale for changing this.

>      }
> -    
> +
>      if (attrName == borderAttr || attrName == alignAttr) {

What's the reason for all this trailing whitespace stripping? This will gratuitously mark various unchanged lines as changed in Subversion history, and is not typically something we do in WebKit patches. Maybe it's something done at Google?

> +    static PassRefPtr<HTMLImageElement> createForJSConstructor(Document*, const int width, const int height);

The "const" modifiers here have no effect and should be omitted.

Everything else looks good.

I'm going to mark this as review- because I don't think the behavior changes for negative numbers and for undefined or null arguments for the image constructor should be combined with the rest of the change. If that behavior should change, I'd like to see a test case covering it and have it done either before or after this refactoring instead of at the same time as the refactoring.

[Darin Adler]

(In reply to comment #13)
> +    // Calling toJS on the document causes the JS document wrapper to be
> +    // added to the window object. This is done to ensure that
> JSDocument::markChildren
> +    // will be called, which will cause the audio element to be marked if
> necessary.
> +    // FIXME: The correct way to do this would be to make HTMLMediaElement
> derive from
> +    // ActiveDOMObject and use its interface to keep its wrapper alive. Then
> we would
> +    // remove this code and the special case in isObservableThroughDOM.
> 
> Active DOM objects are marked from JSDocument::markChildren, so it seems that
> we would still need a document wrapper.

Good point. It seems the FIXME that I wrote that Yaar merged into his patch is wrong. Lets just leave the FIXME part of the comment out.

[Yaar Schnitman]

Created attachment 49044 [details]
Patch

[Yaar Schnitman]

Comment on attachment 49044 [details]
Patch

1. I added more tests.
2. Removed wrong FIXMEs
3. Uses const int* optionals

I also run the new tests against old safari and chromium to verify constructors behave the same. 

BTW, audio.autobuffer is true by default, but I tested anyway.

p.s. Whitespace changes: AFAIK, the latest webkit style rule about whitespace changes is that if I made real edits elsewhere in these files, its ok. I'm using an automated tool to fix whitespace changes, and the new changes are correct.

[WebKit Commit Bot]

Comment on attachment 49044 [details]
Patch

Clearing flags on attachment: 49044

Committed r54999: <http://trac.webkit.org/changeset/54999>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #17)
> BTW, audio.autobuffer is true by default, but I tested anyway.

I suggest removing the unneeded line of code setting autobuffer, then.

    audio->setAutobuffer(true);

If it's true by default that line should not be needed.

> p.s. Whitespace changes: AFAIK, the latest webkit style rule about whitespace
> changes is that if I made real edits elsewhere in these files, its ok. I'm
> using an automated tool to fix whitespace changes, and the new changes are
> correct.

I think this rule is strange, and I don't agree with it; I'm not sure how it became the "latest rule". I don't think it's good to have automated tools making insignificant changes that harm our ability to find real changes in file history and provide no tangible benefit.

[Adam Barth]

> > p.s. Whitespace changes: AFAIK, the latest webkit style rule about whitespace
> > changes is that if I made real edits elsewhere in these files, its ok. I'm
> > using an automated tool to fix whitespace changes, and the new changes are
> > correct.
> 
> I think this rule is strange, and I don't agree with it; I'm not sure how it
> became the "latest rule". I don't think it's good to have automated tools
> making insignificant changes that harm our ability to find real changes in file
> history and provide no tangible benefit.

This might be a bit of a Chromium / WebKit culture clash.  Chromium-land is more aggressive about cleaning up style violations than WebKit-land is.  For folks that work in both code bases, it's easy to transplant cultural norms from one to the other.

[Darin Adler]

(In reply to comment #14)
> What's the reason for all this trailing whitespace stripping? This will
> gratuitously mark various unchanged lines as changed in Subversion history, and
> is not typically something we do in WebKit patches. Maybe it's something done
> at Google?

(In reply to comment #21)
> This might be a bit of a Chromium / WebKit culture clash.

I think it could be, yes. As I said in the comment quoted above.

[Adam Barth]

Ah, I missed that.  I didn't read the whole code review.