Closed Bug 1258594 Opened 4 years ago Closed 4 years ago
Add a Frame
Properties::Has() method for use in assertions
Sometimes we want to assert that a frame property already exists when we set or get the property. If we're getting the property, the |aFoundResult| outparam can be used to determine whether or not the property exists, so this can usually be used for assertions. However, there is no equivalent for setting a property, and it's not easy to implement efficiently because the underlying nsTHashtable doesn't support it. Since an efficient implementation is not possible for now anyway, let's just add a FrameProperty::IsSet() method to make assertions easier when setting a property. This of course requires an additional lookup of the frame property, but using this only for debug-only assertions related to setting a property won't impose a significant burden. Presumably this could be used in situations other than assertions, although I think in most cases when we access a frame property we get or set it.
Here's the patch.
Attachment #8733168 - Flags: review?(dbaron)
Comment on attachment 8733168 [details] [diff] [review] Add a FrameProperties::IsSet() method for use in assertions. Review of attachment 8733168 [details] [diff] [review]: ----------------------------------------------------------------- Stealing review, at seth's suggestion on IRC. r=me, one nit below (and remember to update the "r=dbaron" to "r=dholbert" in the commit message before landing) ::: layout/base/FramePropertyTable.h @@ +180,5 @@ > + * > + * In most cases, this shouldn't be used outside of assertions, because if > + * you're doing a lookup anyway it would be far more efficient to call Get() > + * and check the aFoundResult outparam to find out whether the property is > + * set. As discussed on IRC, please hint at the circumstances under which you expect this function to be legitimately (efficiently) callable *outside* of assertions. (e.g. sounds like this could reasonably be called before Set(), in cases where we don't want to overwrite an existing property-table entry)
Attachment #8733168 - Flags: review?(dbaron) → review+
Thanks for the quick review! Will upload a revised patch shortly.
Addressed review comments.
Comment on attachment 8733168 [details] [diff] [review] Add a FrameProperties::IsSet() method for use in assertions. I reviewed this on the plane today since it was in my queue when I left, and I'm now pasting the review comments I wrote offline: I think I'd slightly prefer the name Has over IsSet. Curious if you have any other ideas or pointers to other things we should base the name on. r=dbaron either way, though
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeff98d4994875599327edf73cfaa282f287c941 Bug 1258594 - Add a FrameProperties::IsSet() method for use in assertions. r=dholbert
Attachment #8734547 - Attachment is obsolete: true
Here's a quick followup to rename IsSet() to Has(), per dbaron's review. Marking this r+ dbaron since it's literally just an s/IsSet/Has as requested. =)
Summary: Add a FrameProperties::IsSet() method for use in assertions → Add a FrameProperties::Has() method for use in assertions
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb8b0d8e3ddfc3fb61c6ca4aa3a1dfb1b5711cee Bug 1258594 (Followup) - Rename FrameProperties::IsSet() to FrameProperties::Has(). r=dbaron
You need to log in before you can comment on or make changes to this bug.