1258594 - Add a FrameProperties::Has() method for use in assertions

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Add a FrameProperties::IsSet() method for use in assertions.

Here's the patch.

Comment 2

[Daniel Holbert [:dholbert]]

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)

Comment 3

[Seth Fowler [:seth] [:s2h]]

Thanks for the quick review! Will upload a revised patch shortly.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: Add a FrameProperties::IsSet() method for use in assertions.

Addressed review comments.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 6

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eeff98d4994875599327edf73cfaa282f287c941
Bug 1258594 - Add a FrameProperties::IsSet() method for use in assertions. r=dholbert

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: (Followup) - Rename FrameProperties::IsSet() to FrameProperties::Has().

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. =)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb8b0d8e3ddfc3fb61c6ca4aa3a1dfb1b5711cee
Bug 1258594 (Followup) - Rename FrameProperties::IsSet() to FrameProperties::Has(). r=dbaron

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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