Bug 9683 - Implement select.options.remove() method
: Implement select.options.remove() method
Status: RESOLVED FIXED
: WebKit
Forms
: 420+
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To:
: http://msdn.microsoft.com/workshop/au...
:
:
:
  Show dependency treegraph
 
Reported: 2006-07-01 22:01 PST by
Modified: 2007-12-10 09:51 PST (History)


Attachments
Added tests for select.options.remove(boolean). (7.20 KB, patch)
2007-11-24 16:03 PST, David Kilzer (:ddkilzer)
no flags Review Patch | Details | Formatted Diff | Diff
Implement select.options.remove() method (21.45 KB, patch)
2007-11-24 16:03 PST, David Kilzer (:ddkilzer)
mjs: review-
Review Patch | Details | Formatted Diff | Diff
Patch v2 (21.32 KB, patch)
2007-11-25 11:19 PST, David Kilzer (:ddkilzer)
no flags Review Patch | Details | Formatted Diff | Diff
Patch v3 (23.02 KB, patch)
2007-12-10 00:09 PST, David Kilzer (:ddkilzer)
mjs: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2006-07-01 22:01:05 PST
Since Bug 9179 is implementing select.options.add(), I should probably implement the remove() method next for completeness.  It should be trivially easy once Bug 9179 lands.
------- Comment #1 From 2007-11-19 15:18:15 PST -------
Hello David,

Are you still working on this issue? Would you please share with me about the status?
Thanks
------- Comment #2 From 2007-11-19 16:43:34 PST -------
I believe I have an old patch lying around somewhere, along with an incomplete suite of tests.

As I recall, the thing that was holding me up was getting access to MSIE 6/7 to test its behavior so that it could be matched.  (I have access to MSIE 7 now.)

Is this something you'd like to work on in WebKit, or are you simply interested in it being available as an end-user?  :)
------- Comment #3 From 2007-11-20 11:27:38 PST -------
Hi David,

Thanks for your rapid response. 
For this problem, in Gecko/WebKit,they follows W3C standards and add one more method: void add(in HTMLOptionElement option, in [Optional] unsigned long index)
(in Gecko: nsIDOMHTMLOptionsCollection.idl, nsIDOMNSHTMLOptionCollectn.idl, nsDomClassInfo.cpp
 in WebKit: WebCore/html/HTMLOptionsCollection.h,HTMLOptionsCollection.idl)

In IE, it add more methods to Interface HTMLOptionsCollection     
relative Gecko and WebKit, such as method: void remove( unsigned long index).
So Gecko/Webkit based browsers, like FireFox/Safari, will be failure on some webpages' functionality which rely the IE specific method "remove" because some web developer use it on their webpages.

For example, one of Chinese most popular social network community and dating websites, www.51.com, which is also in top 100 Chinese web sites(based on Alexa data), use the IE specific method "remove".(see http://my.51.com/inc/js/global/GlobalProvinces.js, line 22), so Firefox, Safari all can not use the function about searching by location. Unfortunately, lots of Chinese websites use the no standard "options.remove", so I think probably WebKit community should support this function. That's why I am looking this bug. 

So far both WebKit source code and latest Safari binary do not support options.remove, so when will you merge your patch to code tree for supporting this ?

And yes, I want to do something in WebKit. I was a web browser developer in China on embedded system. also I had written a desktop browser on windows platform when I worked on developing mobile browser, which was just a prototype and compatible with some IE special quirk mode and special DHTML object model. If you are interesting with it, I can send a demo to you:-) . 

I think the compatibility problem is one of big problems for Chinese user to use WebKit based browser to surf Chinese websites. After roughly using safari for test,  more than 50% sites of top 100 Chinese websites (based on Alexa data) have major or minor layout and functionality problems. Be honest, after reading WebKit code, I think WebKit is one of best W3 standard support browser engine, but since IE has very big marketing share in China(maybe also in Asia) and millions of websites in China mainly support IE, should WebKit do something for dealing with compatibility problems?
------- Comment #4 From 2007-11-20 12:00:22 PST -------
IIRC, in IE, select === select.options. This is an open issue in WF2.
------- Comment #5 From 2007-11-20 12:40:51 PST -------
(In reply to comment #3)
> So far both WebKit source code and latest Safari binary do not support
> options.remove, so when will you merge your patch to code tree for supporting
> this ?

I'll try to clean it up, add some tests and post a patch this week.

> And yes, I want to do something in WebKit. I was a web browser developer in
> China on embedded system. also I had written a desktop browser on windows
> platform when I worked on developing mobile browser, which was just a prototype
> and compatible with some IE special quirk mode and special DHTML object model.
> If you are interesting with it, I can send a demo to you:-) . 

I appreciate the offer, but I think it may be a conflict of interest for me to view it.

> I think the compatibility problem is one of big problems for Chinese user to
> use WebKit based browser to surf Chinese websites. After roughly using safari
> for test,  more than 50% sites of top 100 Chinese websites (based on Alexa
> data) have major or minor layout and functionality problems. Be honest, after
> reading WebKit code, I think WebKit is one of best W3 standard support browser
> engine, but since IE has very big marketing share in China(maybe also in Asia)
> and millions of websites in China mainly support IE, should WebKit do something
> for dealing with compatibility problems?

The WebKit project generally tries to provide IE compatibility as long as it doesn't violate standards.  More information here:

http://webkit.org/projects/compat/index.html

If you find specific issues (that affect popular web sites), please file bugs.  If you'd like to start hacking on WebKit, please see this page for more details:

http://webkit.org/coding/contributing.html

Thanks for your interest!
------- Comment #6 From 2007-11-20 14:26:20 PST -------
(In reply to comment #4)
> IIRC, in IE, select === select.options. This is an open issue in WF2.
> 

In technically speaking, yes in IE, select === select.options. According to MSDN http://msdn2.microsoft.com/en-us/library/ms537472.aspx, it's a options collection object.
------- Comment #7 From 2007-11-20 14:27:13 PST -------
(In reply to comment #5)
Great, thanks David!
> (In reply to comment #3)
> > So far both WebKit source code and latest Safari binary do not support
> > options.remove, so when will you merge your patch to code tree for supporting
> > this ?
> 
> I'll try to clean it up, add some tests and post a patch this week.
> 
> > And yes, I want to do something in WebKit. I was a web browser developer in
> > China on embedded system. also I had written a desktop browser on windows
> > platform when I worked on developing mobile browser, which was just a prototype
> > and compatible with some IE special quirk mode and special DHTML object model.
> > If you are interesting with it, I can send a demo to you:-) . 
> 
> I appreciate the offer, but I think it may be a conflict of interest for me to
> view it.
> 
> > I think the compatibility problem is one of big problems for Chinese user to
> > use WebKit based browser to surf Chinese websites. After roughly using safari
> > for test,  more than 50% sites of top 100 Chinese websites (based on Alexa
> > data) have major or minor layout and functionality problems. Be honest, after
> > reading WebKit code, I think WebKit is one of best W3 standard support browser
> > engine, but since IE has very big marketing share in China(maybe also in Asia)
> > and millions of websites in China mainly support IE, should WebKit do something
> > for dealing with compatibility problems?
> 
> The WebKit project generally tries to provide IE compatibility as long as it
> doesn't violate standards.  More information here:
> 
> http://webkit.org/projects/compat/index.html
> 
> If you find specific issues (that affect popular web sites), please file bugs. 
> If you'd like to start hacking on WebKit, please see this page for more
> details:
> 
> http://webkit.org/coding/contributing.html
> 
> Thanks for your interest!
> 
------- Comment #8 From 2007-11-23 10:45:53 PST -------
The MSDN documentation effectively points to here as the definition of remove():

http://www.w3.org/TR/2000/WD-DOM-Level-1-20000929/level-one-html.html#ID-33404570
------- Comment #9 From 2007-11-24 16:03:08 PST -------
Created an attachment (id=17489) [details]
Added tests for select.options.remove(boolean).

This is unrelated to fixing the bug, but I noticed booleans weren't tested with select.options.add() when writing the tests for select.options.remove().
------- Comment #10 From 2007-11-24 16:03:53 PST -------
Created an attachment (id=17490) [details]
Implement select.options.remove() method

Patch with test and ChangeLog.
------- Comment #11 From 2007-11-24 19:14:47 PST -------
(From update of attachment 17489 [details])
r=me
------- Comment #12 From 2007-11-24 19:22:50 PST -------
(From update of attachment 17490 [details])
I think this has a potential bug. If you retrieve select.options on a select, then remove it from the document and let a GC happen, this line will crash because the wrapper will already have been GC'd:

+    JSHTMLSelectElement* base = static_cast<JSHTMLSelectElement*>(toJS(exec, imp->base()));

Please add a test case along those lines and if it indeed causes a crash where I expect please fix patch appropriately (you may have to duplicate the custom logic from HTMLSelectElement.
------- Comment #13 From 2007-11-24 23:16:03 PST -------
(From update of attachment 17489 [details])
Committed r28012.

Clearing review flag.
------- Comment #14 From 2007-11-25 11:19:44 PST -------
Created an attachment (id=17510) [details]
Patch v2

(In reply to comment #12)
> (From update of attachment 17490 [details] [edit])
> I think this has a potential bug. If you retrieve select.options on a select,
> then remove it from the document and let a GC happen, this line will crash
> because the wrapper will already have been GC'd:
> 
> +    JSHTMLSelectElement* base = static_cast<JSHTMLSelectElement*>(toJS(exec,
> imp->base()));
> 
> Please add a test case along those lines and if it indeed causes a crash where
> I expect please fix patch appropriately (you may have to duplicate the custom
> logic from HTMLSelectElement.

I've added a test (LayoutTests/fast/js/select-options-remove-gc.html) for the condition described above.  However, I couldn't get WebKit to crash either in the browser (using the "Caches Window" between removing select and using options.remove()) or in a layout test.

Otherwise, there are no changes in this patch compared to Attachment #17490 [details].
------- Comment #15 From 2007-11-27 02:19:48 PST -------
I think you need to not only GC but also force JS allocations which will overwrite the select's JS wrapper object. For example:

var garbage = document.createElement("p");
for (var i = 0; i < 5000; i++) {
    var new = document.createElement("p");
    new.appendChild(garbage);
    garbage = new;
}
garbage = null;
------- Comment #16 From 2007-11-28 14:11:15 PST -------
(From update of attachment 17510 [details])
r=me
------- Comment #17 From 2007-11-28 14:11:48 PST -------
(From update of attachment 17510 [details])
Oops, just saw Maciej's comment. Not sure why he didn't review- because of wanting a change to the test.
------- Comment #18 From 2007-11-28 14:18:34 PST -------
(From update of attachment 17510 [details])
Clearing the review flag.  I need to write a different test per Comment #15.

Sorry Darin.
------- Comment #19 From 2007-12-10 00:09:15 PST -------
Created an attachment (id=17816) [details]
Patch v3

Patch v3

Added code per Comment #15 to LayoutTests/fast/js/select-options-remove-gc.html (code taken from other garbage collection tests).  Still doesn't crash either in DumpRenderTree or in Safari.
------- Comment #20 From 2007-12-10 05:59:06 PST -------
I was talking crazy talk because toJS() will make the wrapper if needed, it does not matter if the cached one has been GC'd. Sorry about that.
------- Comment #21 From 2007-12-10 07:52:11 PST -------
(From update of attachment 17816 [details])
r=me
------- Comment #22 From 2007-12-10 09:51:40 PST -------
Committed r28584