[webkit-reviews] review denied: [Bug 34782] Normalize custom ctors for Image, Option, Audio : [Attachment 49032] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Yaar Schnitman <yaar at chromium.org>'s
request for review:
Bug 34782: Normalize custom ctors for Image, Option, Audio
https://bugs.webkit.org/show_bug.cgi?id=34782

Attachment 49032: Patch
https://bugs.webkit.org/attachment.cgi?id=49032&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +// 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.