Bug 139179 – HTMLSelectElement add() should support index as second argument

• Component: DOM
• Status: RESOLVED FIXED

[Shivakumar J M]

HTMLSelectElement add() should be as per the HTML specification : http://www.w3.org/html/wg/drafts/html/master/forms.html#the-select-element

[Sam Weinig]

What exactly is not to spec? Do you have a test case that fails?

[Shivakumar J M]

(In reply to comment #1)
> What exactly is not to spec? Do you have a test case that fails?


Dear  Sam,

      As per Spec:    
http://www.w3.org/html/wg/drafts/html/master/forms.html#the-select-element, select.add should support an index as the second optional argument. so calling select.add("option", index) will fail, currently only HTMLElement is supported as second argument. so user can do add operation based on index of item also.
Also its done in:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLSelectElement#add%28%29

Thanks

[Sam Weinig]

(In reply to comment #2)
> (In reply to comment #1)
> > What exactly is not to spec? Do you have a test case that fails?
> 
> 
> Dear  Sam,
> 
>       As per Spec:    
> http://www.w3.org/html/wg/drafts/html/master/forms.html#the-select-element,
> select.add should support an index as the second optional argument. so
> calling select.add("option", index) will fail, currently only HTMLElement is
> supported as second argument. so user can do add operation based on index of
> item also.
> Also its done in:
> https://developer.mozilla.org/en-US/docs/Web/API/HTMLSelectElement#add%28%29
> 
> Thanks

Can you provide a test case that fails as an attachments?

[Shivakumar J M]

Created attachment 243185 [details]
test cases failed for htmlselect add method with index parameter

HTMLSelect::add(select, index), with any index value, the element always adds at the end.

[Shivakumar J M]

Created attachment 243186 [details]
Patch

HTMLSelectElement add() should support index as second argument.

[Darin Adler]

Comment on attachment 243186 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243186&action=review

The tricky part here is the binding, not the C++ function.

We need more test coverage. I don’t see anything here about passing, say, a string that can be converted to a number, or a boolean which can be converted to a number. We also want to cover things like the smallest integer, the largest integer, the numbers just outside the integer range, infinity, etc.

Currently the unusual bindings case is breaking for all the languages other than JavaScript, such as Objective-C, so that needs to be addressed before we can land this.

> Source/WebCore/html/HTMLSelectElement.cpp:237
> +    HTMLElement* beforeElement = downcast<HTMLElement>(options()->item(beforeIndex));
> +
> +    add(element, beforeElement, ec);

I’d leave out the blank line and maybe the local variable entirely.

> Source/WebCore/html/HTMLSelectElement.h:61
>      void add(HTMLElement*, HTMLElement* beforeElement, ExceptionCode&);
>  
> +    void add(HTMLElement*, int beforeIndex, ExceptionCode&);

I’d leave out the blank line and have these be in one paragraph.

> Source/WebCore/html/HTMLSelectElement.idl:48
>      [ObjCLegacyUnnamedParameters, RaisesException] void add([Default=Undefined] optional HTMLElement element,
>                              [Default=Undefined] optional HTMLElement before);
> +
> +    [ObjCLegacyUnnamedParameters, RaisesException] void add([Default=Undefined] optional HTMLElement element,
> +                            [Default=Undefined] optional long before);

It’s strange to have a blank line between the “add” lines and not between this and the previous and next functions. Lets leave out the blank line.

In this new overload, the arguments don’t need to be optional, [Default=Undefined] is also unneeded/wrong and ObjCLegacyUnnamedParameters is incorrect.

[Shivakumar J M]

(In reply to comment #6)
> Comment on attachment 243186 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243186&action=review
> 
> The tricky part here is the binding, not the C++ function.
> 
> We need more test coverage. I don’t see anything here about passing, say, a
> string that can be converted to a number, or a boolean which can be
> converted to a number. We also want to cover things like the smallest
> integer, the largest integer, the numbers just outside the integer range,
> infinity, etc.
> 
> Currently the unusual bindings case is breaking for all the languages other
> than JavaScript, such as Objective-C, so that needs to be addressed before
> we can land this.
> 
> > Source/WebCore/html/HTMLSelectElement.cpp:237
> > +    HTMLElement* beforeElement = downcast<HTMLElement>(options()->item(beforeIndex));
> > +
> > +    add(element, beforeElement, ec);
> 
> I’d leave out the blank line and maybe the local variable entirely.
> 
> > Source/WebCore/html/HTMLSelectElement.h:61
> >      void add(HTMLElement*, HTMLElement* beforeElement, ExceptionCode&);
> >  
> > +    void add(HTMLElement*, int beforeIndex, ExceptionCode&);
> 
> I’d leave out the blank line and have these be in one paragraph.
> 
> > Source/WebCore/html/HTMLSelectElement.idl:48
> >      [ObjCLegacyUnnamedParameters, RaisesException] void add([Default=Undefined] optional HTMLElement element,
> >                              [Default=Undefined] optional HTMLElement before);
> > +
> > +    [ObjCLegacyUnnamedParameters, RaisesException] void add([Default=Undefined] optional HTMLElement element,
> > +                            [Default=Undefined] optional long before);
> 
> It’s strange to have a blank line between the “add” lines and not between
> this and the previous and next functions. Lets leave out the blank line.
> 
> In this new overload, the arguments don’t need to be optional,
> [Default=Undefined] is also unneeded/wrong and ObjCLegacyUnnamedParameters
> is incorrect.


Dear  Darin,

      Thanks for the inputs, will updated the patch and tests.

Thanks

[Shivakumar J M]

Created attachment 243283 [details]
Patch-Updated-Review1

updated patch with review comments.

[Darin Adler]

Comment on attachment 243283 [details]
Patch-Updated-Review1

View in context: https://bugs.webkit.org/attachment.cgi?id=243283&action=review

review- because of the build failures on Mac and GTK

> Source/WebCore/html/HTMLSelectElement.idl:46
> +    [RaisesException] void add(HTMLElement element, long before);

GTK and Mac bindings are still failing to build. To work around it, this line needs to go inside a #if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT block.

> LayoutTests/fast/dom/HTMLSelectElement/select-add.html:44
> +mySelect.add(new Option("X", "Y", false, false), -100);

We should cover the following integers: 2147483647, 2147483648, -2147483648, -2147483649, since these are the edge cases for converting floating point numbers to integers.

> LayoutTests/fast/dom/HTMLSelectElement/select-add.html:52
> +mySelect.add(new Option("X", "Y", false, false), "1");

Besides "1", we should cover false and true, which should convert into 0 and 1 when passed as an integer.

[Shivakumar J M]

Created attachment 243343 [details]
Patch-Updated-Review2

Updated the patch with review comments

[Build Bot]

Comment on attachment 243343 [details]
Patch-Updated-Review2

Attachment 243343 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6725672185626624

New failing tests:
fast/dom/dom-add-optionelement.html
dom/html/level2/html/HTMLSelectElement18.html
fast/dom/incompatible-operations.html
dom/xhtml/level2/html/HTMLSelectElement18.xhtml

[Build Bot]

Created attachment 243344 [details]
Archive of layout-test-results from ews104 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 243343 [details]
Patch-Updated-Review2

Attachment 243343 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6286089799073792

New failing tests:
fast/dom/dom-add-optionelement.html
dom/html/level2/html/HTMLSelectElement18.html
fast/dom/incompatible-operations.html
dom/xhtml/level2/html/HTMLSelectElement18.xhtml

[Build Bot]

Created attachment 243345 [details]
Archive of layout-test-results from ews101 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Shivakumar J M]

Working on Test case fails: dom/html/level2/html/HTMLSelectElement18.html and dom/xhtml/level2/html/HTMLSelectElement18.xhtml. it looks we need to change in HTMLOptionsCollection as well.

[Darin Adler]

Comment on attachment 243343 [details]
Patch-Updated-Review2

Code change looks OK, but the 4 regression tests mentioned above are now failing and need to be updated. Either the tests themselves have to be updated or at least their expected results.

[Shivakumar J M]

(In reply to comment #16)
> Comment on attachment 243343 [details]
> Patch-Updated-Review2
> 
> Code change looks OK, but the 4 regression tests mentioned above are now
> failing and need to be updated. Either the tests themselves have to be
> updated or at least their expected results.


Dear  Darin,

      Out of 4 failed tests, "fast/dom/incompatible-operations.html", modified to work fine in my local setup. 
      But other 3 tests ( dom/html/level2/html/HTMLSelectElement18.html, dom/xhtml/level2/html/HTMLSelectElement18.xhtml, fast/dom/dom-add-optionelement.html) are failing, when 'null' is passed as second argument to add(). Whenever select.add(element1,null) is passed, the new add api is adding the element1 to begining of the list, so but as per spec "If before is omitted, null, or a number out of range, then element will be added at the end of the list". so these is making all 3 tests fail. 

Thanks

[Darin Adler]

(In reply to comment #17)
>       But other 3 tests ( dom/html/level2/html/HTMLSelectElement18.html,
> dom/xhtml/level2/html/HTMLSelectElement18.xhtml,
> fast/dom/dom-add-optionelement.html) are failing, when 'null' is passed as
> second argument to add(). Whenever select.add(element1,null) is passed, the
> new add api is adding the element1 to begining of the list, so but as per
> spec "If before is omitted, null, or a number out of range, then element
> will be added at the end of the list". so these is making all 3 tests fail. 

Yes, thus it sounds like an incompatible change; probably no big deal, but we need to modify the tests to expect the new behavior, and reaffirm that changing this behavior will be OK for web compatibility and even compatibility in in-app use of WebKit. Never trivial, but as I said, probably fine.

The absolute minimum is to correct our tests and expectations as part of the same check-in that changes the behavior.

[Shivakumar J M]

Created attachment 243480 [details]
Patch-Updated-Review3

Updated the patch with tests modifed.

[Shivakumar J M]

Dear  Darin,

      Updated the patch with failed tests, below are the observations:

      1) After adding new add() api to suppourt index as second argument, no need to call add(element1, null), to add element1 at the end of an select list.
so modified the test to not to call null to add element to end of list.
 
      2) When 'null' is passed as second argument to add() api, the new add() api is adding the element1 to begining of the list. i.e before first element in the list. 

      3) But when i run attached manual test cases in Chrome and FireFox with add(element1, null), element1 adds at the end of an select list.


Thanks

[Darin Adler]

Comment on attachment 243480 [details]
Patch-Updated-Review3

So we are now going to behave differently from Chrome and Firefox when calling add(x, null)? That does not sound good.

[Shivakumar J M]

(In reply to comment #21)
> Comment on attachment 243480 [details]
> Patch-Updated-Review3
> 
> So we are now going to behave differently from Chrome and Firefox when
> calling add(x, null)? That does not sound good.


Yes right, the call to add(x, null) is now going to new add() api, which takes index as argument, so it looks 'null' is going as 0 in "options()->item(beforeIndex)", so it gives first element as before element. so x is getting added to beginning of the list. 
Before adding new add() api, the call to add(x, null) was going to old add() api, which takes HTMLElement as second argument, in the insertBefore() call, if before is null, x will be appended to end of list, so x was getting added to the end of list. so in new api, we need to treat null differently, when add(x,null) is called, will try using other datatypes for index argument.

[Shivakumar J M]

We can use Union types as in http://www.w3.org/TR/WebIDL/#idl-union. is it supported ?

[Darin Adler]

(In reply to comment #23)
> We can use Union types as in http://www.w3.org/TR/WebIDL/#idl-union. is it
> supported ?

I don’t think the bindings scripts handle those. Someone could do that project at some point.

In the short term, I’m sure you can find a way to get the null case to be handled the way it was before. It might depend on the order the two overloads are defined in the IDL file. Not sure, might require a little bit of research.

[Chris Dumez]

(In reply to comment #24)
> (In reply to comment #23)
> > We can use Union types as in http://www.w3.org/TR/WebIDL/#idl-union. is it
> > supported ?
> 
> I don’t think the bindings scripts handle those. Someone could do that
> project at some point.

This is correct. The WebKit bindings generator doesn't currently support unions.

> In the short term, I’m sure you can find a way to get the null case to be
> handled the way it was before. It might depend on the order the two
> overloads are defined in the IDL file. Not sure, might require a little bit
> of research.

Yes, the order of the overloads in the IDL file matters. The ones with the most specific argument types should come first (because the bindings generators will adds the argument type checks in the same order as in the IDL, in order to call the right overload function in the implementation).

[Darin Adler]

(In reply to comment #25)
> (In reply to comment #24)
> > In the short term, I’m sure you can find a way to get the null case to be
> > handled the way it was before. It might depend on the order the two
> > overloads are defined in the IDL file. Not sure, might require a little bit
> > of research.
> 
> Yes, the order of the overloads in the IDL file matters. The ones with the
> most specific argument types should come first (because the bindings
> generators will adds the argument type checks in the same order as in the
> IDL, in order to call the right overload function in the implementation).

I think that means that if we put the version that takes the long index first in the IDL file, then null will be handled as before. Please try that.

[Shivakumar J M]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > In the short term, I’m sure you can find a way to get the null case to be
> > > handled the way it was before. It might depend on the order the two
> > > overloads are defined in the IDL file. Not sure, might require a little bit
> > > of research.
> > 
> > Yes, the order of the overloads in the IDL file matters. The ones with the
> > most specific argument types should come first (because the bindings
> > generators will adds the argument type checks in the same order as in the
> > IDL, in order to call the right overload function in the implementation).
> 
> I think that means that if we put the version that takes the long index
> first in the IDL file, then null will be handled as before. Please try that.


Tried by moving new add() api to above the old add() api in IDL file, but did not worked, also tried using implementedAs method then also same results, still add(x,null) adds the element x to begining of the list. 
Now trying to use Custom in add() method and trying to make changes in webcore/bindings/js/JSHTMLSelectElementCustom.cpp, same way as remove()/add() method implemented in HTMLOptionsCollection.cpp.

[Shivakumar J M]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > (In reply to comment #24)
> > > > In the short term, I’m sure you can find a way to get the null case to be
> > > > handled the way it was before. It might depend on the order the two
> > > > overloads are defined in the IDL file. Not sure, might require a little bit
> > > > of research.
> > > 
> > > Yes, the order of the overloads in the IDL file matters. The ones with the
> > > most specific argument types should come first (because the bindings
> > > generators will adds the argument type checks in the same order as in the
> > > IDL, in order to call the right overload function in the implementation).
> > 
> > I think that means that if we put the version that takes the long index
> > first in the IDL file, then null will be handled as before. Please try that.
> 
> 
> Tried by moving new add() api to above the old add() api in IDL file, but
> did not worked, also tried using implementedAs method then also same
> results, still add(x,null) adds the element x to begining of the list. 
> Now trying to use Custom in add() method and trying to make changes in
> webcore/bindings/js/JSHTMLSelectElementCustom.cpp, same way as
> remove()/add() method implemented in HTMLOptionsCollection.cpp.


Dear  Chris and Darin,

      Thanks for the inputs, below are some observations:
      1) After moving new add() api above/below the old add() api in IDL file, there will be 2 add functions add1() and add2() will be generated after build, add1() will resolve to first, add2() will resolve the second. But the call to add(x,null), still matches to new add() api which takes index parameter.
      2) Added 'Custom' in old add() method and tried to call the different add() method, based on parameter in "webcore/bindings/js/JSHTMLSelectElementCustom.cpp,". But for both 'null' and '0', toInt32() gives same value. toHTMLSelectElement() also gives same value.
      3) Tried adding TreatNullAs=NullString for index parameter, but still values same for null or 0 input. Will try some other options to differentiate these values.

Thanks

[Shivakumar J M]

Created attachment 243670 [details]
Patch-Updated-Review4

Updated the patch to use Custom in add() method.

[Shivakumar J M]

After Added 'Custom' in old add() method and tried to call the different add() method, based on parameter in webcore/bindings/js/JSHTMLSelectElementCustom.cpp", now all tests are working fine. Now old case add(x,null), also works fine. Now attached manual test behave same as Chrome and FireFox also.

[Chris Dumez]

Before we decide to use custom code, would you mind trying:
[ObjCLegacyUnnamedParameters, RaisesException] void add([Default=Undefined] optional HTMLElement? element, [Default=Undefined] optional HTMLElement? before);
[RaisesException] void add(HTMLElement element, long before);

The order is important, so are the question marks to indicate that the arguments are nullable.

This generate the following code:

===
EncodedJSValue JSC_HOST_CALL jsHTMLSelectElementPrototypeFunctionAdd(ExecState* exec)
{
    size_t argsCount = exec->argumentCount();
    JSValue arg0(exec->argument(0));
    JSValue arg1(exec->argument(1));
    if (argsCount == 0 || (argsCount == 1 && (arg0.isNull() || (arg0.isObject() && asObject(arg0)->inherits(JSHTMLElement::info())))) || (argsCount == 2 && (arg0.isNull() || (arg0.isObject() && asObject(arg0)->inherits(JSHTMLElement::info()))) && (arg1.isNull() || (arg1.isObject() && asObject(arg1)->inherits(JSHTMLElement::info())))))
        return jsHTMLSelectElementPrototypeFunctionAdd1(exec);
    if ((argsCount == 2 && (arg0.isObject() && asObject(arg0)->inherits(JSHTMLElement::info()))))
        return jsHTMLSelectElementPrototypeFunctionAdd2(exec);
    return throwVMTypeError(exec);
}
===

I did not have much time to look into this but this generated code seems to do what you want, right?

[Chris Dumez]

By the way, what the spec says is that HTMLSelectElement.add() should behave the same way as HTMLOptionsCollection.add():
https://html.spec.whatwg.org/multipage/forms.html#dom-select-add

However, in WebKit, HTMLOptionsCollection.add() only supports an index as second argument (not an HTMLElement).

I think HTMLOptionsCollection.add() should be updated in the same patch to make sure both behave the same way (and according to spec).

[Shivakumar J M]

(In reply to comment #32)
> By the way, what the spec says is that HTMLSelectElement.add() should behave
> the same way as HTMLOptionsCollection.add():
> https://html.spec.whatwg.org/multipage/forms.html#dom-select-add
> 
> However, in WebKit, HTMLOptionsCollection.add() only supports an index as
> second argument (not an HTMLElement).
> 
> I think HTMLOptionsCollection.add() should be updated in the same patch to
> make sure both behave the same way (and according to spec).


Dear  Chris,

      Thanks for the inputs, below are some observations:


      1) Adding question marks to indicate that the arguments are nullable seems to work fine, also added new add() api below the old add() api. But no need to make argument1 as nullable, only second argument is fine as below:
[ObjCLegacyUnnamedParameters, RaisesException] void add([Default=Undefined] optional HTMLElement element, [Default=Undefined] optional HTMLElement? before);
     
     2) With above changes all tests worked fine, but add(element1,Undefined), adds element1 to begining of list, but it should add element1 to end of list. Tried making [Default=Null] for second argument, but did not compile. Default value for second argument should be null.

     3) Yes, right HTMLOptionsCollection also needs to change, but here they are using custom code instead of generated code. so should we remove custom code and make add() api to use generated code ? i.e same as HTMLSelectElement.
Or modify generated code ?. Will try both methods and tests to update patch.

Thanks

[Darin Adler]

Comment on attachment 243670 [details]
Patch-Updated-Review4

View in context: https://bugs.webkit.org/attachment.cgi?id=243670&action=review

Looks pretty good, but enough mistakes that I’ll say review-.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:37
> +    HTMLSelectElement& imp = impl();

I suggest naming this local variable "select" instead of "imp", as is done in the other functions in this file.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:38
> +    HTMLElement* after(JSHTMLElement::toWrapped(exec->argument(0)));

It’s really strange to call this local variable “after”. This argument is the new element we are attempting to add as a child of the select element. I suggest we call it “element” or “new element”. I don’t think the word “after” makes sense for this at all.

We normally prefer "=" syntax to construction syntax for a line like this, at least when it’s hand-written, not script-generated.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:40
> +    if (exec->hadException())
> +        return jsUndefined();

This is unneeded dead code. There is nothing above this that can cause an exception.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:43
> +    if (exec->hadException())
> +        return jsUndefined();

This is unneeded dead code. There is nothing above this that can cause an exception.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:44
> +    ExceptionCode ec = 0;

It’s strange to define this up here when it’s not used until later. I would move the definition down to right where it’s actually used.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:45
> +    int index = exec->argument(1).toInt32(exec);

It would be better to not even do this conversion to an integer if argument 1 turned out to be an HTMLElement. It would probably be easy to structure the code this way by putting this code in the else branch below.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:46
> +    JSValue arg1(exec->argument(1));

Putting argument(1) into a local variable after the function has already used it twice, and then using that local variable only one time, is not good style. We should either omit the local variable entirely, or set it up before the argument is used the first time and use it all three times.

It’s also quite strange to put line of code in between the toInt32 call that might have cause an exception and the line that checks for that exception. Those two lines should be right next to each other.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:59
> +    if (exec->argumentCount() == 1) {
> +        if (after)
> +            imp.add(after, nullptr, ec);
> +    } else {
> +        if (before || arg1.isUndefinedOrNull())
> +            imp.add(after, before, ec);
> +        else
> +            imp.add(after, index, ec);
> +    }

There is no need to check argumentCount here. The code in the "else" will do exactly the same thing without a special case for one argument, since argument 1 will be undefined and before will be nullptr. So please delete the unneeded special case for 1 argument. Also, it’s is not good to have an explicit null check for "after" here in the first branch, but not below. That null check is not needed at all, since HTMLSelectElement::add already checks it, but if we did want a null check I think we’d want it in all cases, not just one.

Do we need a check for extra arguments? I believe some DOM functions raise exceptions when you have unwanted extra arguments. Maybe this is an older function where we tolerate them silently, though. Where is the test case that covers that?

If the first argument is not an HTMLSelectElement, this entire function silently does nothing. Is that the correct behavior or are we supposed to raise an exception in that case? Where is the test case that covers that?

[Darin Adler]

(In reply to comment #33)
>       1) Adding question marks to indicate that the arguments are nullable
> seems to work fine, also added new add() api below the old add() api. But no
> need to make argument1 as nullable

Are you sure the first argument is not optional? We need to be sure that we have test cases that cover that, one way or another.

>      2) With above changes all tests worked fine, but
> add(element1,Undefined), adds element1 to begining of list, but it should
> add element1 to end of list.

Yes, sounds like a real problem.

>      3) Yes, right HTMLOptionsCollection also needs to change, but here they
> are using custom code instead of generated code. so should we remove custom
> code and make add() api to use generated code ? i.e same as
> HTMLSelectElement.
> Or modify generated code ?. Will try both methods and tests to update patch.

Longer term we would like both of these methods to use generated code, but only if we can make the generated code work properly. Correct behavior is more important than generating the code, but it’s not good for us to have a lot of custom functions. Each custom function is an opportunity to have subtle mistakes and code that’s harder to refactor later as we improve how DOM bindings work. Whereas we can make changes to all the generated functions much more efficiently and consistently.

[Shivakumar J M]

Created attachment 243820 [details]
Patch-Updated-Review5

Updated test patch to check for build/tests for other ports.

[Darin Adler]

Comment on attachment 243820 [details]
Patch-Updated-Review5

View in context: https://bugs.webkit.org/attachment.cgi?id=243820&action=review

Nice work. I think we are almost there. review- because the Mac build is still failing

At this point it’s a little unclear to me the total set of changes in behavior we are introducing. For changes that are incompatible with older versions of WebKit, I am concerned that there may be web content or perhaps apps on iOS or Mac that depend on the old behavior, even if it’s not consistent with other browsers. So I want to be really clear about what we are changing.

The change that is explicit here is handling numeric indices as the second argument to both add functions.

But I think another thing this changes is that the first argument to HTMLSelectElement's add is now type checked and we throw an exception if it's not the right type. But we have left it so you can omit this first argument and it will silently do nothing. Is that really what other web browsers do and what the standard calls for? Or are we just retaining a WebKit quirk by allowing the argument to be omitted?

What about extra arguments? Where’s the test that covers extra arguments to see if we raise an exception for the extra ones or just ignore them silently? This is another thing that might have changed when we changed how the bindings worked.

I can’t tell if we got all the expectations right or not because it’s hard for me to read through the test to see what is expected, and this worries me slightly about a change that affects web-visible API. What I want to know is exactly what we are changing, how this relates to what WebKit used to do, what other browsers do, and what the standard says.

> Source/WebCore/ChangeLog:10
> +        Also This matches the behavior of Chrome and FireFox.

Stray capital letter "This".

> Source/WebCore/html/HTMLOptionsCollection.idl:32
> +    [ObjCLegacyUnnamedParameters, RaisesException] void add([Default=Undefined] optional HTMLElement element, 
> +                            [Default=Undefined] optional HTMLElement? before);

ObjCLegacyUnnamedParameters is incorrect here and should be omitted. That’s why the Mac build is failing.

Indentation of the second line is wrong. Correct style for WebKit would be to put this all on one line, or to indent the second line by 4 spaces. Indenting by 24 spaces is strange.

> Source/WebCore/html/HTMLSelectElement.cpp:235
> +    add(element, downcast<HTMLElement>(options()->item(beforeIndex)), ec);

I would have written:

    add(element, item(beforeIndex), ec);

I like my version better because there’s no need for an additional type cast.

> Source/WebCore/html/HTMLSelectElement.idl:50
> +    [RaisesException] void add(HTMLElement element, long before);

I think it’s a little strange to put this after the "remove" function just to share the #if. It puts "remove" between the two overloads of "add", and it puts "add" between the two alternative versions of "remove". Seems like we should move this at least above the "// In JavaScript ..." comment for clarity.

> LayoutTests/ChangeLog:10
> +        * fast/dom/HTMLSelectElement/select-add-optgroup-options.html: Added.

The name of this test is not so great. I would call it "options-collection-add.html" since it’s in the HTMLSelectElement directory. Or put it in a new HTMLOptionsCollection directory and name it "add.html".

> LayoutTests/ChangeLog:11
> +        * fast/dom/HTMLSelectElement/select-add-optgroup.html:

The name of this test is no longer all that good. The test covers many different aspects of the add function, not just the handling of optgroup. Also, since it’s in the HTMLSelectElement directory I don’t think the test needs "select" in its name. I would just name it "add.html".

> LayoutTests/fast/dom/incompatible-operations-expected.txt:15
> +PASS aSelect.add(aDOMImplementation, aDOMImplementation) threw exception TypeError: Type error.

To me it seems strange that this test that seems to be intended to to cover DOM bindings in general is actually only testing HTMLSelectElement’s add function. I think we should be adding coverage of some other DOM function if the intent of this test is not specific to the add function. Worth researching when this test was added.

> LayoutTests/fast/dom/HTMLSelectElement/select-add-optgroup-options.html:74
> +mySelect.options.add(new Option("X", "Y", false, false), null);
> +shouldBeEqualToString('deepCopy()', '0,1,2,Y');

When creating a test like this one it's useful to have a substantive part of the test in the "should be" expression so we can see what is being tested. Typically we make helper functions so that we can write a small exception and it can be clear what we are testing. For example, parts of the test would look like this:

    shouldBeEqualToString('testAdd(0)', 'Y,0,1,2');
    shouldBeEqualToString('testAdd(1)', '0,Y,1,2');

    shouldBeEqualToString('testAdd("0")', 'Y,0,1,2');
    shouldBeEqualToString('testAdd("1")', 'Y,0,1,2');

    shouldBeEqualToString('testAdd(null)', '0,1,2,Y');

And so on. Note how this makes both the test code itself, and the test output, easier to read. Just a tip for future tests. Not insisting that you improve this one, although you are welcome to.

If you had written the test like this it would have been easier for me to read it and see what the expectations are. I’m having a bit of a hard time seeing what's being tested. It would be especially useful to use a helper function to create the option elements because the repetitive "new Option('X', 'X', false, false)" is distracting.

[Shivakumar J M]

Created attachment 243836 [details]
Patch-Updated-Review6

Test patch updated with comments, to check build. Will re-write tests as per comments to make more readable.

[Shivakumar J M]

Created attachment 243837 [details]
Patch-Updated-Review7

Updated patch with HTMLOptionsCollection.idl file

[Shivakumar J M]

Created attachment 243838 [details]
Patch-Updated-Review6

Updated patch for build fail.

[Shivakumar J M]

Created attachment 243839 [details]
Patch-Updated-Review6

updated for build fail.

[Build Bot]

Comment on attachment 243839 [details]
Patch-Updated-Review6

Attachment 243839 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5023612616572928

New failing tests:
fast/dom/HTMLSelectElement/select-add-optgroup.html
fast/dom/HTMLSelectElement/options-collection-add.html
js/dom/select-options-add.html

[Build Bot]

Created attachment 243840 [details]
Archive of layout-test-results from ews103 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 243839 [details]
Patch-Updated-Review6

Attachment 243839 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5596550919094272

New failing tests:
fast/dom/HTMLSelectElement/select-add-optgroup.html
fast/dom/HTMLSelectElement/options-collection-add.html
js/dom/select-options-add.html

[Build Bot]

Created attachment 243841 [details]
Archive of layout-test-results from ews104 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Shivakumar J M]

(In reply to comment #37)
> Comment on attachment 243820 [details]
> Patch-Updated-Review5
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243820&action=review
> 
> Nice work. I think we are almost there. review- because the Mac build is
> still failing
> 
> At this point it’s a little unclear to me the total set of changes in
> behavior we are introducing. For changes that are incompatible with older
> versions of WebKit, I am concerned that there may be web content or perhaps
> apps on iOS or Mac that depend on the old behavior, even if it’s not
> consistent with other browsers. So I want to be really clear about what we
> are changing.
> 
> The change that is explicit here is handling numeric indices as the second
> argument to both add functions.
> 
> But I think another thing this changes is that the first argument to
> HTMLSelectElement's add is now type checked and we throw an exception if
> it's not the right type. But we have left it so you can omit this first
> argument and it will silently do nothing. Is that really what other web
> browsers do and what the standard calls for? Or are we just retaining a
> WebKit quirk by allowing the argument to be omitted?
> 
> What about extra arguments? Where’s the test that covers extra arguments to
> see if we raise an exception for the extra ones or just ignore them
> silently? This is another thing that might have changed when we changed how
> the bindings worked.
> 
> I can’t tell if we got all the expectations right or not because it’s hard
> for me to read through the test to see what is expected, and this worries me
> slightly about a change that affects web-visible API. What I want to know is
> exactly what we are changing, how this relates to what WebKit used to do,
> what other browsers do, and what the standard says.
> 
> > Source/WebCore/ChangeLog:10
> > +        Also This matches the behavior of Chrome and FireFox.
> 
> Stray capital letter "This".
> 
> > Source/WebCore/html/HTMLOptionsCollection.idl:32
> > +    [ObjCLegacyUnnamedParameters, RaisesException] void add([Default=Undefined] optional HTMLElement element, 
> > +                            [Default=Undefined] optional HTMLElement? before);
> 
> ObjCLegacyUnnamedParameters is incorrect here and should be omitted. That’s
> why the Mac build is failing.
> 
> Indentation of the second line is wrong. Correct style for WebKit would be
> to put this all on one line, or to indent the second line by 4 spaces.
> Indenting by 24 spaces is strange.
> 
> > Source/WebCore/html/HTMLSelectElement.cpp:235
> > +    add(element, downcast<HTMLElement>(options()->item(beforeIndex)), ec);
> 
> I would have written:
> 
>     add(element, item(beforeIndex), ec);
> 
> I like my version better because there’s no need for an additional type cast.
> 
> > Source/WebCore/html/HTMLSelectElement.idl:50
> > +    [RaisesException] void add(HTMLElement element, long before);
> 
> I think it’s a little strange to put this after the "remove" function just
> to share the #if. It puts "remove" between the two overloads of "add", and
> it puts "add" between the two alternative versions of "remove". Seems like
> we should move this at least above the "// In JavaScript ..." comment for
> clarity.
> 
> > LayoutTests/ChangeLog:10
> > +        * fast/dom/HTMLSelectElement/select-add-optgroup-options.html: Added.
> 
> The name of this test is not so great. I would call it
> "options-collection-add.html" since it’s in the HTMLSelectElement directory.
> Or put it in a new HTMLOptionsCollection directory and name it "add.html".
> 
> > LayoutTests/ChangeLog:11
> > +        * fast/dom/HTMLSelectElement/select-add-optgroup.html:
> 
> The name of this test is no longer all that good. The test covers many
> different aspects of the add function, not just the handling of optgroup.
> Also, since it’s in the HTMLSelectElement directory I don’t think the test
> needs "select" in its name. I would just name it "add.html".
> 
> > LayoutTests/fast/dom/incompatible-operations-expected.txt:15
> > +PASS aSelect.add(aDOMImplementation, aDOMImplementation) threw exception TypeError: Type error.
> 
> To me it seems strange that this test that seems to be intended to to cover
> DOM bindings in general is actually only testing HTMLSelectElement’s add
> function. I think we should be adding coverage of some other DOM function if
> the intent of this test is not specific to the add function. Worth
> researching when this test was added.
> 
> > LayoutTests/fast/dom/HTMLSelectElement/select-add-optgroup-options.html:74
> > +mySelect.options.add(new Option("X", "Y", false, false), null);
> > +shouldBeEqualToString('deepCopy()', '0,1,2,Y');
> 
> When creating a test like this one it's useful to have a substantive part of
> the test in the "should be" expression so we can see what is being tested.
> Typically we make helper functions so that we can write a small exception
> and it can be clear what we are testing. For example, parts of the test
> would look like this:
> 
>     shouldBeEqualToString('testAdd(0)', 'Y,0,1,2');
>     shouldBeEqualToString('testAdd(1)', '0,Y,1,2');
> 
>     shouldBeEqualToString('testAdd("0")', 'Y,0,1,2');
>     shouldBeEqualToString('testAdd("1")', 'Y,0,1,2');
> 
>     shouldBeEqualToString('testAdd(null)', '0,1,2,Y');
> 
> And so on. Note how this makes both the test code itself, and the test
> output, easier to read. Just a tip for future tests. Not insisting that you
> improve this one, although you are welcome to.
> 
> If you had written the test like this it would have been easier for me to
> read it and see what the expectations are. I’m having a bit of a hard time
> seeing what's being tested. It would be especially useful to use a helper
> function to create the option elements because the repetitive "new
> Option('X', 'X', false, false)" is distracting.


Dear  Darin,

      Thanks for the inputs, below are some observations:
	  
      Now patch is passing the build, will modify all the tests to use "testAdd(null)" kind of functions and helper functions to create the different option/group elements to make more readable. will upload new patch for modified tests to cover extra arguments also.
	  
      As per the Spec both HTMLSelectElement and HTMLOptionsCollection should support adding option element using index as well as using other element. But HTMLSelectElement was not supporting adding option element using index and 
HTMLOptionsCollection was not supporting adding option element using other element ( i.e add( newelement, oldelement)).
     
     So currently apps has to use HTMLSelectElement add(), if we want to add element using other element and to add option element using index, need to use HTMLOptionsCollection add().
	  
     Also now First argument cannot be Optional and second argument is optional, nullable also default is null. But setting [Default=Null] for second argument, did not compile, we need to find way to set default as null.
[TreatUndefinedAs=Null] also did not work. so with new change both HTMLSelectElement and HTMLOptionsCollection add() now supports adding option element using index as well as using other element.
	  
    1) The failed tests js/dom/select-options-add.html and fast/dom/HTMLSelectElement/select-add-optgroup.html needs to be updated as per new behavior. since now select2.options.add() with index as -2, -1/0, 0/0, 1/0 adds element to list so length needs to be updated.
		 
    2) In HTMLOptionsCollection.idl, now add() with index is defined to take HTMLOptionElement, since it gives Public API missing error in build.
"[RaisesException] void add([Default=Undefined] optional HTMLOptionElement option, unsigned long index);". So mySelect.options.add(group, 1), to add group failed, so may need to add one more add() function which takes HTMLOptGroupElement as a parameter as below, need to try these in build and test. "[RaisesException] void add([Default=Undefined] optional HTMLOptGroupElement option, unsigned long index);"
	  
   3) Also select2.options.add(option2, -2) is giving error, need to check these test for error case as done earlier in HTMLOptionsCollection add() ("if (index == -1 || unsigned(index) >= length())"), need to try these in build and test.

   4) Need to explore if add is called with no inputs add(), should be undefined Or throw type error.
	   
Thanks

[Darin Adler]

Comment on attachment 243839 [details]
Patch-Updated-Review6

View in context: https://bugs.webkit.org/attachment.cgi?id=243839&action=review

> Source/WebCore/html/HTMLOptionsCollection.cpp:39
> +void HTMLOptionsCollection::add(HTMLElement* element, HTMLElement* before, ExceptionCode& ec)

Would be nice to call the argument beforeElement like we do in the header file.

> Source/WebCore/html/HTMLOptionsCollection.idl:33
> +    [RaisesException] void add([Default=Undefined] optional HTMLOptionElement option, unsigned long index);

This should be long, not unsigned long, to match HTMLSelectElement.idl and the implementation. Unless that’s all some kind of mistake?

> Source/WebCore/html/HTMLSelectElement.idl:45
>      [ObjCLegacyUnnamedParameters, RaisesException] void add([Default=Undefined] optional HTMLElement element,
> -                            [Default=Undefined] optional HTMLElement before);
> +                            [Default=Undefined] optional HTMLElement? before);

It would be nice to fix the indentation here, or put this all on one long line.

[Shivakumar J M]

Created attachment 243885 [details]
Patch-Updated-Review9

Updated patch with modified test and code.

[Shivakumar J M]

Created attachment 243886 [details]
Patch-Updated-Review10

Updated HTMLOptionsCollection.idl to retain old API, since it give build error:

ENABLE_WEBVTT_REGIONS ENABLE_XHR_TIMEOUT ENABLE_XSLT ENABLE_FTL_JIT  ENABLE_SATURATED_LAYOUT_ARITHMETIC   LANGUAGE_OBJECTIVE_C" --generator ObjC --supplementalDependencyFile ./SupplementalDependencies.txt WebCore/html/HTMLTableElement.idl
Public API change. There are missing public properties and/or methods from the "DOMHTMLOptionsCollection" class.
- (void)add:(DOMHTMLOptionElement *)option index:(unsigned)index;
Died at WebCore/bindings/scripts//CodeGeneratorObjC.pm line 386.
make: *** [DOMHTMLOptionsCollection.h] Error 255

[Build Bot]

Comment on attachment 243886 [details]
Patch-Updated-Review10

Attachment 243886 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6190007622565888

New failing tests:
imported/w3c/canvas/2d.shadow.alpha.2.html
imported/w3c/canvas/2d.shadow.enable.y.html
imported/w3c/canvas/2d.shadow.alpha.4.html
imported/w3c/canvas/2d.shadow.clip.3.html
imported/w3c/canvas/2d.shadow.alpha.5.html
imported/w3c/canvas/2d.shadow.enable.x.html
imported/w3c/canvas/2d.shadow.composite.2.html
imported/w3c/canvas/2d.shadow.offset.negativeY.html
imported/w3c/canvas/2d.shadow.offset.negativeX.html
imported/w3c/canvas/2d.shadow.transform.2.html
imported/w3c/canvas/2d.shadow.alpha.3.html
imported/w3c/canvas/2d.shadow.enable.blur.html
imported/w3c/canvas/2d.shadow.offset.positiveY.html
imported/w3c/canvas/2d.shadow.offset.positiveX.html
imported/w3c/canvas/2d.shadow.clip.1.html
imported/w3c/canvas/2d.shadow.outside.html
imported/w3c/canvas/2d.shadow.transform.1.html
imported/w3c/canvas/2d.fillRect.shadow.html
imported/w3c/canvas/2d.shadow.composite.1.html

[Build Bot]

Created attachment 243887 [details]
Archive of layout-test-results from ews105 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Shivakumar J M]

Created attachment 243888 [details]
Patch-Updated-Review10

Uploading same patch once again, since the test failures are in some canvas modules.

[Shivakumar J M]

patch has passed build and tests:

1) as per spec now arg1 default value is undefined (Default=Undefined) and arg1 is not optional and is not nullable. arg2 default value is undefined (Default=Undefined) and arg2 is optional and is nullable in HTMLSelectElement.idl

2) as per spec now arg1 default value is undefined (Default=Undefined) and arg1 is not optional and is not nullable. arg2 default value is undefined (Default=Undefined) and arg2 is optional and is nullable in HTMLOptionsCollection.idl

3) But agr2 in HTMLOptionsCollection.idl is using "unsigned long" for index parameter, if we use just "long" it will give build error as : ENABLE_WEBVTT_REGIONS ENABLE_XHR_TIMEOUT ENABLE_XSLT ENABLE_FTL_JIT  ENABLE_SATURATED_LAYOUT_ARITHMETIC   LANGUAGE_OBJECTIVE_C" --generator ObjC --supplementalDependencyFile ./SupplementalDependencies.txt WebCore/html/HTMLTableElement.idl
Public API change. There are missing public properties and/or methods from the "DOMHTMLOptionsCollection" class.
- (void)add:(DOMHTMLOptionElement *)option index:(unsigned)index;
Died at WebCore/bindings/scripts//CodeGeneratorObjC.pm line 386.
make: *** [DOMHTMLOptionsCollection.h] Error 255

4) We have 2 add functions with "unsigned long index" as arg2 in HTMLOptionsCollection.idl, one to suppourt HTMLOptionElement and other to
suppourt HTMlOptGroupElement. But tried with "[RaisesException] void add([Default=Undefined] HTMlOptGroupElement element, [Default=Undefined]  
optional unsigned long index);", it give build error, so using HTMLElement instead of HTMlOptGroupElement.

5) if add is called with no inputs add(), it should throw not enough arguments, same as other browsers.

[Darin Adler]

When it comes to preserving the old Objective-C API, this should be done with conditionals specific to Objective-C and should have as little effect as possible on anything outside the IDL file. For example, for Objective-C we can just have the add function take an HTMLOptionElement, but we can probably get away without having anything in the actual DOM source files, just the IDL file.

Because the tests no longer reset anything, now if any one test fails, all the subsequent ones will fail too. We want each test to be as independent as possible unless there is a good reason to make them depend on each other.

[Shivakumar J M]

(In reply to comment #54)
> When it comes to preserving the old Objective-C API, this should be done
> with conditionals specific to Objective-C and should have as little effect
> as possible on anything outside the IDL file. For example, for Objective-C
> we can just have the add function take an HTMLOptionElement, but we can
> probably get away without having anything in the actual DOM source files,
> just the IDL file.
> 
> Because the tests no longer reset anything, now if any one test fails, all
> the subsequent ones will fail too. We want each test to be as independent as
> possible unless there is a good reason to make them depend on each other.


So, if i understand correctly, we can just retain old add() api with HTMLOptionElement in .idl file and do not have any definition/declaration in .h  and source files. We can retain 2 new add() apis added in .idl files under #if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT block and have definition/declaration for only these two new add() apis in .h and source files.

So current changes in both .idl files are ok, we just need to remove old add() api definition/declaration in .h and source files, is it right ?.


Ok, will make each test independent, thought if we do not reset, we can just read expected.txt to know tests and results. 

Do we need to print list contents on each reset (i.e for each tests ?) Or printing list contents at beginning only once is fine ?.

[Shivakumar J M]

Created attachment 243968 [details]
Patch-Updated-Review11

Patch updated to change tests and remove the add() from source files.

[Darin Adler]

Comment on attachment 243968 [details]
Patch-Updated-Review11

View in context: https://bugs.webkit.org/attachment.cgi?id=243968&action=review

We are almost there. There is still a bit of unnecessary overloading and unneeded syntax in the IDL files.

> Source/WebCore/html/HTMLOptionsCollection.idl:35
> +#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> +    [RaisesException] void add([Default=Undefined] HTMLElement element, [Default=Undefined] optional HTMLElement? before);
> +    [RaisesException] void add([Default=Undefined] HTMLElement element, [Default=Undefined] optional long index);
> +#endif
> +    [RaisesException] void add([Default=Undefined] HTMLOptionElement option, [Default=Undefined] optional unsigned long index);

Here’s what this really should say:

    #if !defined(LANGUAGE_OBJECTIVE_C) || !LANGUAGE_OBJECTIVE_C
    [RaisesException] void add([Default=Undefined] HTMLElement element, [Default=Undefined] optional HTMLElement? before);
    [RaisesException] void add([Default=Undefined] HTMLElement element, optional long index);
    #else
    [RaisesException] void add(HTMLOptionElement option, optional unsigned long index);
    #endif

I removed some of the [Default=Undefined] that are not needed, but I’m also not sure which if any of those remaining [Default=Undefined] are needed. Maybe we can remove them all? Tests should make that clear.

> Source/WebCore/html/HTMLSelectElement.idl:45
> +    [ObjCLegacyUnnamedParameters, RaisesException] void add([Default=Undefined] HTMLElement element,
> +    [Default=Undefined] optional HTMLElement? before);

This is indented wrong. It should all be on one line, or the second line should be indented by 4 spaces. I’m not certain we still need the [Default=Undefined] here.

> Source/WebCore/html/HTMLSelectElement.idl:47
> +    [RaisesException] void add([Default=Undefined] HTMLElement element, [Default=Undefined] optional long index);

We don’t need [Default=Undefined] for index and I’m also not sure we need it for element.

> LayoutTests/fast/dom/HTMLSelectElement/add.html:49
> +var option1 = createOption("X1", "Y1");
> +shouldBeEqualToString('testAdd3(option1,null)', '0,1,2,Y1');

This test would be even better without all these local variables:

    shouldBeEqualToString('testAdd3(createOption("X1", "Y1"))', '0,1,2,Y1');

Also not sure that having separate arguments for "text" and "value" for createOption is helpful to the test. Could just generate the text by adding something to the value, since the text has no effect on the outcome of the test. And I would put the resetSelection call into the test function so it doesn’t have to be repeated over and over again.

[Shivakumar J M]

Created attachment 244041 [details]
Updatedpatch-review12

Updated the patch to remove some unneeded syntax in the IDL files. also updated tests. Due to Proxy/firewall issue updating same patch, which had r+ failed, so now manually updating the patch again.

[Shivakumar J M]

Working to fix these gtk build fail issue, looks we may need to retain some of the [Default=Undefined] in old add() api signature.

[Shivakumar J M]

Created attachment 244148 [details]
Patch-Updated-Review13

Fixed the GTK build error and removed unwanted syntax from idl files, also updated the tests as well.

[Shivakumar J M]

Created attachment 244149 [details]
Patch-Updated-Review14

Uploaded same patch once again, since build failed mac is due to fail in css module. gtk build has passed now.

[Shivakumar J M]

Patch passed build and tests, except in mac due to some existing failures.

[Shivakumar J M]

Patch has passed build and tests now. for gtk used macro:
#if (!defined(LANGUAGE_OBJECTIVE_C) || !LANGUAGE_OBJECTIVE_C) && (!defined(LANGUAGE_GOBJECT) || !LANGUAGE_GOBJECT) instead of just #if (!defined(LANGUAGE_OBJECTIVE_C) || !LANGUAGE_OBJECTIVE_C) to resolve gtk build error.

[WebKit Commit Bot]

Comment on attachment 244149 [details]
Patch-Updated-Review14

Clearing flags on attachment: 244149

Committed r178097: <http://trac.webkit.org/changeset/178097>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

(In reply to comment #65)
> All reviewed patches have been landed.  Closing bug.

This change regressed Yahoo Mail. See bug #146515.