Closed Bug 1249606 Opened 4 years ago Closed 9 months ago

Would be nice to have operator== for WebIDL dictionaries

Categories

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

47 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox47 --- affected
firefox65 --- fixed

People

(Reporter: baku, Assigned: dpino, Mentored)

References

Details

(Whiteboard: dom-triaged)

Attachments

(1 file, 3 obsolete files)

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.
Flags: needinfo?(bzbarsky)
Depends on: 1245184
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...
Flags: needinfo?(bzbarsky)
Whiteboard: dom-triaged btpp-backlog
> 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.
Happy to mentor someone who wants to add this for dictionaries that only contain primitive types (in the IDL sense) and strings.
Mentor: bzbarsky
Priority: -- → P3
Whiteboard: dom-triaged btpp-backlog → dom-triaged
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.
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.
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;
}
```
Attachment #9026982 - Attachment is obsolete: true
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
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;
}
```
Attachment #9028260 - Attachment is obsolete: true
Diego, are you at a point where you feel like the patch is ready for review?
Flags: needinfo?(dpino)
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?
Flags: needinfo?(dpino)
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!
Attachment #9028625 - Flags: review+
Updated patch addressing suggested changes. I think everything is in place now. Thanks for the multiple reviews and tips.
Attachment #9028625 - Attachment is obsolete: true
Comment on attachment 9028784 [details] [diff] [review]
Bug-1249606-Automatically-generate-operator-for-WebI.patch

r=me
Attachment #9028784 - Flags: review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2a7b9a99226
Automatically generate operator== for WebIDL dictionaries. r=bzbarsky
Thank you for the fix!
Assignee: nobody → dpino
https://hg.mozilla.org/mozilla-central/rev/e2a7b9a99226
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.