Closed Bug 1258594 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attachment #8733168 - Attachment is obsolete: true
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
Attachment #8733168 - Attachment is obsolete: false
Attachment #8733168 - Flags: review+
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. =)
Attachment #8733168 - Attachment is obsolete: true
Summary: Add a FrameProperties::IsSet() method for use in assertions → Add a FrameProperties::Has() method for use in assertions
https://hg.mozilla.org/mozilla-central/rev/eeff98d49948
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.