LibJS: Let Object::get_own_properties() return both strings and symbols

The new default return_type argument is GetOwnPropertyReturnType::All,
which returns properties with both string and symbol keys (which is also
the default for [[OwnPropertyKeys]]). This means that in some cases we
need to iterate the ordered property table twice, as we don't store
string and symbol properties separately but symbols must - there's
certainly room for (performance) improvements here. On the other hand
this makes Reflect.ownKeys() return symbol properties now :^)

Userland/Libraries/LibJS/Runtime/Object.cpp

@@ -235,29 +235,44 @@ Value Object::get_own_properties(PropertyKind kind, bool only_enumerable_propert
         ++property_index;
     }
 
-    for (auto& it : shape().property_table_ordered()) {
+    auto add_property_to_results = [&](auto& property) {
-        if (only_enumerable_properties && !it.value.attributes.is_enumerable())
-            continue;
-
-        if (return_type == GetOwnPropertyReturnType::StringOnly && it.key.is_symbol())
-            continue;
-        if (return_type == GetOwnPropertyReturnType::SymbolOnly && it.key.is_string())
-            continue;
-
         if (kind == PropertyKind::Key) {
-            properties_array->define_property(property_index, it.key.to_value(vm()));
+            properties_array->define_property(property_index, property.key.to_value(vm()));
         } else if (kind == PropertyKind::Value) {
-            properties_array->define_property(property_index, get(it.key));
+            properties_array->define_property(property_index, get(property.key));
         } else {
             auto* entry_array = Array::create(global_object());
-            entry_array->define_property(0, it.key.to_value(vm()));
+            entry_array->define_property(0, property.key.to_value(vm()));
-            entry_array->define_property(1, get(it.key));
+            entry_array->define_property(1, get(property.key));
             properties_array->define_property(property_index, entry_array);
         }
-        if (vm().exception())
+    };
-            return {};
 
-        ++property_index;
+    // NOTE: Most things including for..in/of and Object.{keys,values,entries}() use StringOnly, and in those
+    // cases we won't be iterating the ordered property table twice. We can certainly improve this though.
+    if (return_type == GetOwnPropertyReturnType::All || return_type == GetOwnPropertyReturnType::StringOnly) {
+        for (auto& it : shape().property_table_ordered()) {
+            if (only_enumerable_properties && !it.value.attributes.is_enumerable())
+                continue;
+            if (it.key.is_symbol())
+                continue;
+            add_property_to_results(it);
+            if (vm().exception())
+                return {};
+            ++property_index;
+        }
+    }
+    if (return_type == GetOwnPropertyReturnType::All || return_type == GetOwnPropertyReturnType::SymbolOnly) {
+        for (auto& it : shape().property_table_ordered()) {
+            if (only_enumerable_properties && !it.value.attributes.is_enumerable())
+                continue;
+            if (it.key.is_string())
+                continue;
+            add_property_to_results(it);
+            if (vm().exception())
+                return {};
+            ++property_index;
+        }
     }
 
     return properties_array;

Userland/Libraries/LibJS/Runtime/Object.h

@@ -74,6 +74,7 @@ public:
     };
 
     enum class GetOwnPropertyReturnType {
+        All,
         StringOnly,
         SymbolOnly,
     };
@@ -96,7 +97,7 @@ public:
     virtual bool put(const PropertyName&, Value, Value receiver = {});
 
     Value get_own_property(const PropertyName&, Value receiver) const;
-    Value get_own_properties(PropertyKind, bool only_enumerable_properties = false, GetOwnPropertyReturnType = GetOwnPropertyReturnType::StringOnly) const;
+    Value get_own_properties(PropertyKind, bool only_enumerable_properties = false, GetOwnPropertyReturnType = GetOwnPropertyReturnType::All) const;
     Value get_enumerable_own_property_names(PropertyKind) const;
     virtual Optional<PropertyDescriptor> get_own_property_descriptor(const PropertyName&) const;
     Value get_own_property_descriptor_object(const PropertyName&) const;

Userland/Libraries/LibJS/Tests/builtins/Reflect/Reflect.ownKeys.js

@@ -23,12 +23,14 @@ describe("normal behavior", () => {
     });
 
     test("regular object with some properties has own keys", () => {
-        var objectOwnKeys = Reflect.ownKeys({ foo: "bar", bar: "baz", 0: 42 });
+        var symbol = Symbol("symbol");
+        var objectOwnKeys = Reflect.ownKeys({ foo: "bar", [symbol]: "qux", bar: "baz", 0: 42 });
         expect(objectOwnKeys instanceof Array).toBeTrue();
-        expect(objectOwnKeys).toHaveLength(3);
+        expect(objectOwnKeys).toHaveLength(4);
         expect(objectOwnKeys[0]).toBe("0");
         expect(objectOwnKeys[1]).toBe("foo");
         expect(objectOwnKeys[2]).toBe("bar");
+        expect(objectOwnKeys[3]).toBe(symbol);
     });
 
     test("empty array has only 'length' own key", () => {
@@ -38,7 +40,7 @@ describe("normal behavior", () => {
         expect(arrayOwnKeys[0]).toBe("length");
     });
 
-    test("array with some values has 'lenght' and indices own keys", () => {
+    test("array with some values has 'length' and indices own keys", () => {
         var arrayOwnKeys = Reflect.ownKeys(["foo", [], 123, undefined]);
         expect(arrayOwnKeys instanceof Array).toBeTrue();
         expect(arrayOwnKeys).toHaveLength(5);