1249606 - Would be nice to have operator== for WebIDL dictionaries

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Andrea Marchesini [:baku]]

I'm implementing this method:

ChromeUtils::IsOriginAttributesEqual(dom::GlobalObject& aGlobal,
                                     const dom::OriginAttributesDictionary& aA,
                                     const dom::OriginAttributesDictionary& aB)

and would be nice just do: return aA == aB instead comparing each member.

Comment 1

[Boris Zbarsky [:bzbarsky]]

What would the semantics of this operator be for various IDL types?  I guess for object types you can try to do pointer compares, for primitives you just compare them.  Arrays and dictionaries would just work.  What about more interesting things like callbacks or typed arrays?  Should the former be pointer compares or compares of the underlying JS object?  Should the latter be compares of the underlying JS object or of the actual data?

I suppose we could restrict this to only dictionaries consisting of primitives for now...

Comment 2

[Andrea Marchesini [:baku]]

> I suppose we could restrict this to only dictionaries consisting of
> primitives for now...

Right. This would be good for what I have to do. Currently I have to compare each member of the dictionary and that seems fragile in case we modify that dictionary.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Happy to mentor someone who wants to add this for dictionaries that only contain primitive types (in the IDL sense) and strings.

Comment 4

[Diego Pino]

Attachment: Bug-1249606-Would-be-nice-to-have-operator-for-WebID.patch

I made an attempt to fix this bug.

Basically I added an `equalsOperator` method (similar to `assignmentOperator`) to `Codegen.py`. The method is only applied if all member variable types are primitive. The patch is not working yet, I got several compilation errors.  I'd  appreciate some guidance.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Sorry for the lag; the holiday weekend in the US happened in there....

In the future, it would be helpful to put the compile errors you get into an attachment as well, so it's easier for others to see what you're running into.

At first glance the issue you're hitting is that mozilla::dom::Optional_base does not define an operator!=.  Defining that should help.

Comment 6

[Diego Pino]

Attachment: Bug-1249606-Automatically-generate-operator-for-WebI.patch

Thanks, I added an != operator to Operational_base.

I also modified  `Codegen.py` a bit. Now I'm including string types, which before I was skipping them as only Primitive types were considered. Since most of the errors I got were related with Optional arguments, I'm skipping them for now (but likely is not the right thing to do). The code is compiling now.

Here's how the automatically generated == for OriginAttributesDictionary looks like:

``
bool
OriginAttributesDictionary::operator==(const OriginAttributesDictionary& aOther) const
{
  if (mAppId != aOther.mAppId) return false;
  if (!mFirstPartyDomain.Equals(aOther.mFirstPartyDomain)) return false;
  if (mInIsolatedMozBrowser != aOther.mInIsolatedMozBrowser) return false;
  if (mPrivateBrowsingId != aOther.mPrivateBrowsingId) return false;
  if (mUserContextId != aOther.mUserContextId) return false;
  return true;
}
`
```


And this is the original handwritten comparison:

```
 ChromeUtils::IsOriginAttributesEqual(const dom::OriginAttributesDictionary& aA,
                                      const dom::OriginAttributesDictionary& aB)
 {
  return aA.mAppId == aB.mAppId &&
         aA.mInIsolatedMozBrowser == aB.mInIsolatedMozBrowser &&
         aA.mUserContextId == aB.mUserContextId &&
         aA.mPrivateBrowsingId == aB.mPrivateBrowsingId;
 }
``` 

Regarding handling Optional arguments, I believe I'd need to do something like (or !Equals if optional string):

```
if (aOther.${name}.WasPassed()) {
    if (${name} != aOther.${name}) return false;
}
```

Comment 7

[Diego Pino]

Running try to check if tests pass https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cc1c8127ab0ffbeb18b4458a371c95bed70a3be

Comment 8

[Boris Zbarsky [:bzbarsky]]

You shouldn't need to skip optional things now that you added the operator!= to Optional_base.

> I believe I'd need to do something like

No, just using != on the member should work.  That will call https://searchfox.org/mozilla-central/rev/f2028b4c38bff2a50ed6aa1763f6dc5ee62b0cc4/mfbt/Maybe.h#642-645 which will do the isNothing() checks for you, via the operator== on Maybe.

And strings have an operator== and operator!= too, so no need for !Equals() there.  See https://searchfox.org/mozilla-central/rev/f2028b4c38bff2a50ed6aa1763f6dc5ee62b0cc4/xpcom/string/nsTStringRepr.h#351-357 and https://searchfox.org/mozilla-central/rev/f2028b4c38bff2a50ed6aa1763f6dc5ee62b0cc4/xpcom/string/nsTStringRepr.h#383-389

Comment 9

[Diego Pino]

Attachment: Bug-1249606-Automatically-generate-operator-for-WebI.patch

I removed the extra code I added that was not necessary. The code is still building. Thank you for the pointers.

This is how the automatically generated `OriginAttributesDictionary::operator==` looks like (other would look like similar):

```
bool
OriginAttributesDictionary::operator==(const OriginAttributesDictionary& aOther) const
{
  if (mAppId != aOther.mAppId) return false;
  if (mFirstPartyDomain != aOther.mFirstPartyDomain) return false;
  if (mInIsolatedMozBrowser != aOther.mInIsolatedMozBrowser) return false;
  if (mPrivateBrowsingId != aOther.mPrivateBrowsingId) return false;
  if (mUserContextId != aOther.mUserContextId) return false;
  return true;
}
```

Comment 10

[Boris Zbarsky [:bzbarsky]]

Diego, are you at a point where you feel like the patch is ready for review?

Comment 11

[Diego Pino]

Yes, I think the patch is now ready for review. I tried to add you as a reviewer, but when I tried to do so it said you no longer accept r?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9028625 [details] [diff] [review]
Bug-1249606-Automatically-generate-operator-for-WebI.patch

Yeah, I have my review queue closed right now for various reasons... I can do this one, though.  ;)

>Bug 1249606 - Automatically generate operator== for WebIDL dictionaries

You should probably add "r=bzbarsky" to the end of the commmit message.

>+++ b/dom/bindings/Codegen.py
>+    def canEqualsOperator(self):

Maybe name this canHaveEqualsOperator?

>+        for m, _ in self.memberInfo:
>+            if m.type.isString():
>+                continue
>+            if not m.type.isPrimitive():
>+                return False

Why the asymmetry between the isString() and isPrimitive() checks?

In any case, I feel this would be more idiomatic as:

  return all(m.type.isString() or m.type.isPrimitive() for
             (m, _) in self.memberInfo);

>+            memberAssign = CGGeneric(
>+                "if (%s != aOther.%s) return false;\n" % (memberName, memberName))

This is more of a memberTest than memberAssign.

Also, the common style for Gecko code is to brace one-line ifs, so this should probably be:

    memberTest = CGGeneric(fill(
      """
      if (${memberName} != aOther.${memberName}) {
        return false;
      }
      """, 
      memberName=memberName))

or so.

>@@ -13068,16 +13091,18 @@ class CGDictionary(CGThing):
>+            if self.canEqualsOperator():
>+                methods.append(self.equalsOperator())

This is being added in the "is copy constructible" branch.  I think we should add it no matter what the copy-constructibility looks like.

Right now, the restrictions in canEqualsOperator() are stronger than those in isTypeCopyConstructible(), but that might not remain the case, and if that changes it would be a bit confusing to not get operator== when expecting one.

r=me with the above issues fixed.  Thank you!

Comment 13

[Diego Pino]

Attachment: Bug-1249606-Automatically-generate-operator-for-WebI.patch

Updated patch addressing suggested changes. I think everything is in place now. Thanks for the multiple reviews and tips.

Comment 14

[Diego Pino]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb0078c3743e991d4034a75be56410ca4b0bb403

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9028784 [details] [diff] [review]
Bug-1249606-Automatically-generate-operator-for-WebI.patch

r=me

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2a7b9a99226
Automatically generate operator== for WebIDL dictionaries. r=bzbarsky

Comment 17

[Boris Zbarsky [:bzbarsky]]

Thank you for the fix!

Comment 18

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/e2a7b9a99226