Last Comment Bug 448816 - Implement Map interface in ScriptableObject
: Implement Map interface in ScriptableObject
Status: RESOLVED FIXED
:
Product: Rhino
Classification: Components
Component: Core (show other bugs)
: other
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-01 13:10 PDT by Bill Wallace
Modified: 2010-10-05 13:53 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch and test case (10.12 KB, patch)
2008-11-13 04:45 PST, Hannes Wallnoefer
no flags Details | Diff | Review
improved patch and testcase (10.07 KB, patch)
2008-11-13 05:15 PST, Hannes Wallnoefer
no flags Details | Diff | Review
additional patch (5.17 KB, patch)
2008-11-21 10:16 PST, Hannes Wallnoefer
no flags Details | Diff | Review

Description Bill Wallace 2008-08-01 13:10:53 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: 

It would be quite useful if ScriptableObject implemented at least a minimal version of the Map interface - containsKey, get, put, remove at a minimum, but also perhaps a read-only version of the various set views of the object.  This would allow accessing a NativeObject much more cleanly than having to know about the Scriptable Interface etc, and it could be accessed by the many classes that already can deal with java Maps.  In terms of general behaviour, this is much more useful than having ScriptableObject be implemented in terms of a HashMap, and should not cause any performance degredation.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Comment 1 Hannes Wallnoefer 2008-11-13 04:45:25 PST
Created attachment 347951 [details] [diff] [review]
patch and test case

Patch and testcase for implementing java.util.Map in ScriptableObject. Currently, put() is not supported (mainly because of object wrapping concerns, which probably isn't hard to implement), while remove() is, both directly and through the key/value/entry collections.
Comment 2 Hannes Wallnoefer 2008-11-13 05:15:15 PST
Created attachment 347953 [details] [diff] [review]
improved patch and testcase

Simplified patch, improved test case.
Comment 3 Hannes Wallnoefer 2008-11-14 07:23:04 PST
Committed to HEAD:

Checking in src/org/mozilla/javascript/ScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptableObject.java,v  <--  ScriptableObject.java
new revision: 1.140; previous revision: 1.139
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/Bug448816Test.java,v
done
Checking in testsrc/org/mozilla/javascript/tests/Bug448816Test.java;
/cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/Bug448816Test.java,v  <--  Bug448816Test.java
initial revision: 1.1
done
Comment 4 Hannes Wallnoefer 2008-11-21 10:16:14 PST
Created attachment 349447 [details] [diff] [review]
additional patch

additional patch to make the code compile and work with Java 1.5.
Comment 5 Hannes Wallnoefer 2008-11-21 12:46:39 PST
Some thoughts and observations: 

First, maybe the implementation of Map should happen in NativeObject, not ScriptableObject. ScriptableObject is the parent class of virtually every scriptable object in Rhino, so implementing Map there may be a bit too 'broad', and I think the idea is to have this feature explicitly for plain JS Objects.

Second, I don't think it would be difficult to implement write access over the Map interface. I think all we'd have to make sure is that values are properly wrapped using the context's WrapFactory. 

I was trying to reopen this bug until these issues are sorted out but I can't - don't know if this is due to a bugzilla bug or some missing permissions.
Comment 6 Hannes Wallnoefer 2010-10-05 13:53:10 PDT
java.util.Map implementation has moved from ScriptableObject to NativeObject in the process of fixing bug 466207 (implementing java.util.List interface in NativeArray). The primary reason was that it is impossible to implement both Map ans List in the same class as they have remove(Object) methods with incompatible return types. Also, implementing Map makes most sense in NativeObject. It's not clear any JS object should be treated as Map when passed to Java code.

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