Closed Bug 1332322 Opened 3 years ago Closed 3 years ago

Rename nsWrapperCache::IsBlack to HasKnownLiveWrapper

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(3 files)

It's more idiomatic.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8830006 - Flags: review?(continuation) → review+
Comment on attachment 8830006 [details] [diff] [review]
part 1.  Add an nsWrapperCache function to mark the wrapper as 'live' for GC purposes

Review of attachment 8830006 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsWrapperCache.h
@@ +174,5 @@
>  
>    bool HasNothingToTrace(nsISupports* aThis);
>  
> +  /**
> +   * Mark our wrapper, if any, as live as far as the GC is concerned.

I think this should be CC instead of GC, to be consistent with later patches.
Comment on attachment 8830007 [details] [diff] [review]
part 2.  Rename nsWrapperCache::IsBlack to nsWrapperCache::HasKnownLiveWrapper

Review of attachment 8830007 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/FragmentOrElement.cpp
@@ +1732,5 @@
>      }
>    }
>  
>    // Traverse the subtree and check if we could know without CC
> +  // that it is known-live.

There are some more references in comments to "black" earlier in the function that would be good to fix if you want to.

::: dom/base/nsWrapperCacheInlines.h
@@ +26,3 @@
>  {
> +  // If we have a wrapper and it's not gray in the GC-marking sense, that means
> +  // that we can't cycle-collect it.

Maybe spell out a little more why this is? ie because the wrapper holds the wrapper cache alive.
Attachment #8830007 - Flags: review?(continuation) → review+
Comment on attachment 8830008 [details] [diff] [review]
part 3.  Rename nsWrapperCache::IsBlackAndDoesNotNeedTracing to nsWrapperCache::HasKnownLiveWrapperAndDoesNotNeedTracing

Review of attachment 8830008 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsWrapperCache.h
@@ +169,5 @@
>  
>    /**
> +   * Returns true if the object has a known-live wrapper (from the CC point of
> +   * view) and all the GC things it is keeping alive are already known-live from
> +   * CC's point of view as well.

the "as well" feels redundant
Attachment #8830008 - Flags: review?(continuation) → review+
> I think this should be CC instead of GC, to be consistent with later patches.

That's fair.  I guess it doesn't really affect GC, because it will just go from gray to black, and both are live for GC purposes...  Fixed.

> There are some more references in comments to "black" earlier in the function that would be good to fix

Yep, done.

> Maybe spell out a little more why this is?

Done.

> the "as well" feels redundant

Removed.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b60e2ecd3e8
part 1.  Add an nsWrapperCache function to mark the wrapper as 'live' for GC purposes.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/77087c94f931
part 2.  Rename nsWrapperCache::IsBlack to nsWrapperCache::HasKnownLiveWrapper.  r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/b87d9d7b10b5
part 3.  Rename nsWrapperCache::IsBlackAndDoesNotNeedTracing to nsWrapperCache::HasKnownLiveWrapperAndDoesNotNeedTracing.  r=mccr8
https://hg.mozilla.org/mozilla-central/rev/6b60e2ecd3e8
https://hg.mozilla.org/mozilla-central/rev/77087c94f931
https://hg.mozilla.org/mozilla-central/rev/b87d9d7b10b5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.