Bug 9683 - Implement select.options.remove() method
: Implement select.options.remove() method
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Forms
: 420+
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To: David Kilzer (:ddkilzer)
http://msdn.microsoft.com/workshop/au...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-07-01 22:01 PDT by David Kilzer (:ddkilzer)
Modified: 2007-12-10 09:51 PST (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2006-07-01 22:01:05 PDT
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 johnnyding 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 David Kilzer (:ddkilzer) 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 johnnyding 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 Ian 'Hixie' Hickson 2007-11-20 12:00:22 PST
IIRC, in IE, select === select.options. This is an open issue in WF2.
Comment 5 David Kilzer (:ddkilzer) 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 johnnyding 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 johnnyding 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 David Kilzer (:ddkilzer) 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 David Kilzer (:ddkilzer) 2007-11-24 16:03:08 PST
Created attachment 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 David Kilzer (:ddkilzer) 2007-11-24 16:03:53 PST
Created attachment 17490 [details]
Implement select.options.remove() method

Patch with test and ChangeLog.
Comment 11 Maciej Stachowiak 2007-11-24 19:14:47 PST
Comment on attachment 17489 [details]
Added tests for select.options.remove(boolean).

r=me
Comment 12 Maciej Stachowiak 2007-11-24 19:22:50 PST
Comment on attachment 17490 [details]
Implement select.options.remove() method

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 David Kilzer (:ddkilzer) 2007-11-24 23:16:03 PST
Comment on attachment 17489 [details]
Added tests for select.options.remove(boolean).

Committed r28012.

Clearing review flag.
Comment 14 David Kilzer (:ddkilzer) 2007-11-25 11:19:44 PST
Created attachment 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 Maciej Stachowiak 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 Darin Adler 2007-11-28 14:11:15 PST
Comment on attachment 17510 [details]
Patch v2

r=me
Comment 17 Darin Adler 2007-11-28 14:11:48 PST
Comment on attachment 17510 [details]
Patch v2

Oops, just saw Maciej's comment. Not sure why he didn't review- because of wanting a change to the test.
Comment 18 David Kilzer (:ddkilzer) 2007-11-28 14:18:34 PST
Comment on attachment 17510 [details]
Patch v2

Clearing the review flag.  I need to write a different test per Comment #15.

Sorry Darin.
Comment 19 David Kilzer (:ddkilzer) 2007-12-10 00:09:15 PST
Created attachment 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 Maciej Stachowiak 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 Maciej Stachowiak 2007-12-10 07:52:11 PST
Comment on attachment 17816 [details]
Patch v3

r=me
Comment 22 David Kilzer (:ddkilzer) 2007-12-10 09:51:40 PST
Committed r28584