RuntimeArray does not behave like a regular JSArray regarding the listing of its properties. The indices of the array are not enumerated.
Created attachment 39138 [details] patch suggestion I am not sure how I could do the test for this since I need a RuntimeArray in the runtime.
Created attachment 39144 [details] patch suggestion Change the for() loop to follow the coding convention.
Comment on attachment 39144 [details] patch suggestion Code looks good. You can test RuntimeArray by passing an NSArray * from Objective-C to JavaScript. The best place to do that is probably inside DumpRenderTree's ObjcController. r- because there's no regression test.
Created attachment 39641 [details] patch suggestion The same patch with a test for Objective-C.
Style is wrong here: 46 virtual void getPropertyNames(ExecState* exec, PropertyNameArray &propertyNames); (yes, it's clearly wrong in other parts of the file too.) No need for either argument name (they don't add clarity) and the & goes next to the type. Doesn't Obj-C transparently convert JS arrays to Obj-C arrays? Would it make more sense to have a more generic testing method which converted whatever was passed in to Obj-C and then back to a string on output?
+ for (unsigned i = 0; i < length; ++i) { + propertyNames.add(Identifier::from(exec, i)); + } This part should not have braces. 46 virtual void getPropertyNames(ExecState* exec, PropertyNameArray &propertyNames); This is similar to what is already used in that file, so maybe it should be changed in a follow-up, clean-up patch?
Created attachment 39685 [details] proposed patch Same patch, same test. Fixed the code to follow the coding convention (thanks Eric and Kenneth).
(In reply to comment #5) > Doesn't Obj-C transparently convert JS arrays to Obj-C arrays? Would it make > more sense to have a more generic testing method which converted whatever was > passed in to Obj-C and then back to a string on output? The NSArray is "converted" to RuntimeArray in convertObjcValueToValue(). The purpose of the test is to use RuntimeArray in a for() loop of Javascript.
Comment on attachment 39685 [details] proposed patch I still think we could write a smarter test. But the code change looks OK to me. Did you mean to set commit-queue? or do you have other plans for how this should be committed?
Comment on attachment 39685 [details] proposed patch Schedule for cq, as correctly suggested by Eric :)
Comment on attachment 39685 [details] proposed patch Clearing flags on attachment: 39685 Committed r48712: <http://trac.webkit.org/changeset/48712>
All reviewed patches have been landed. Closing bug.
Comment on attachment 39685 [details] proposed patch It is incorrect to override getPropertyNames in almost any case. You should be overriding getOwnPropertyNames.
I rolled this patch out in r48712 due to it being incorrect
Thank you oliver.
Created attachment 44394 [details] Patch Exactly the same patch as before, but use getOwnPropertyNames() instead of qetPropertyNames(). Oliver, could you please describe in which cases qetPropertyNames() is appropriate?
style-queue ran check-webkit-style on attachment 44394 [details] without any errors.
(In reply to comment #16) > Oliver, could you please describe in which cases getPropertyNames() is > appropriate? I'm not sure this is an entirely pertinent question, but I'll try to answer. The getPropertyNames function is a loop that calls getOwnPropertyNames on all the objects in the prototype chain. To add properties, it would never be good to override getPropertyNames, because it would create properties that you can see when enumerating the entire chain, but not when looking at an individual object with the getOwnPropertyNames code path. The overrides of getPropertyNames inside the bridge directory in classes like RuntimeObjectImp, CInstance, and QTInstance are almost certainly all indicative of bugs that should be fixed. The code in RuntimeObjectImp::getPropertyNames should probably be moved to RuntimeObjectImp::getOwnPropertyNames, and in fact the code in the latter looks like it will just cause an infinite loop and stack exhaustion. The trick here is finding a way to test. Similar with CInstance::getPropertyNames and QtInstance::getPropertyNames. JSDOMWindow overrides getPropertyNames because it wants to control access to the properties of its prototypes. That’s probably OK. It's a workaround for the fact that the call to get the prototype inside the getPropertyNames function doesn't have a convenient hook to allow the object to control access to the prototype. Long term we might decide that getPropertyNames shouldn't even be a virtual function.
Comment on attachment 44394 [details] Patch > + const unsigned int length = getLength(); On the next line of code you use "unsigned" but here you used "unsigned int". Further, you mark this local variable "const" and we don't do that in WebKit. There are so many that could be marked const, and I don't think we want to start heading that direction now. r=me despite this slight strangeness
Comment on attachment 44394 [details] Patch Darin has suggested revisions. I think we should see an update patch if we're going to cq this.
Created attachment 44665 [details] Patch Sorry for the delay. I have updated the patch to change the "unsigned int" to "unsigned" and to remove the "const" keyword. I have also put getOwnPropertyNames() with the other getOwnPropertyXXX() in the header to group the related functions.
Attachment 44665 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/bridge/runtime_array.cpp:68: Tab found; better to use spaces [whitespace/tab] [1] Total errors found: 1
Thanks Darin for the explanation. I had totally misunderstood the purpose of getPropertyNames(). I will open a bug report for the problem of QtInstance.
Created attachment 44667 [details] Patch Sorry for the delay. I have updated the patch to change the "unsigned int" to "unsigned" and to remove the "const" keyword. I have also put getOwnPropertyNames() with the other getOwnPropertyXXX() in the header to group the related functions. + removed a \t for my misconfigured XCode...
style-queue ran check-webkit-style on attachment 44667 [details] without any errors.
Comment on attachment 44667 [details] Patch Clearing flags on attachment: 44667 Committed r51985: <http://trac.webkit.org/changeset/51985>