Closed Bug 942240 Opened 6 years ago Closed 6 years ago

Improve the skippability of nsGlobalWindow

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: smaug, Assigned: smaug)

References

Details

(Whiteboard: [qa-])

Attachments

(3 files, 1 obsolete file)

(I ended up doing some bigger changes so the patch isn't quite as trivial as I was hoping,
but it will be more correct.)
Comment on attachment 8337236 [details] [diff] [review]
focused_windows_are_black_simple.diff

This sanitizes nsGlobalWindow blackness check to be closer to normal
CCable objects, so that once we have fully webidl-fied window object, things
should just work right.
Attachment #8337236 - Flags: review?(continuation)
Comment on attachment 8337236 [details] [diff] [review]
focused_windows_are_black_simple.diff

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

::: content/base/src/nsCCUncollectableMarker.cpp
@@ +211,5 @@
>      }
>    }
> +  if (doc) {
> +    nsPIDOMWindow* inner = doc->GetInnerWindow();
> +    nsPIDOMWindow* outer = doc->GetWindow();

maybe move outer down to where it is used

::: dom/base/nsFocusManager.h
@@ +120,5 @@
>    static InputContextAction::Cause GetFocusMoveActionCause(uint32_t aFlags);
>  
>    static bool sMouseFocusesFormControl;
>  
> +  static bool IsKeepingWindowAlive(nsPIDOMWindow* aWindow)

Rather than calling this for every window, it would make a little more sense to me to have this be some kind of mark uncollectable method that sets the CC generation on the windows, and is called once from nsCCUncollectableMarker.  That would also make it easier to have the focus manager optimize out other things it owns later.  Do you not want to do that because the focusmanager usually doesn't hold things for long or something?

::: dom/base/nsGlobalWindow.cpp
@@ +1793,5 @@
> +  if (!nsCCUncollectableMarker::sGeneration) {
> +    return false;
> +  }
> +
> +  return (nsCCUncollectableMarker::InGeneration(GetMarkedCCGeneration()) ||

It would be a little clearer if you split out the first part of this into a predicate like isDefinitelyAlive.

::: dom/base/nsWrapperCacheInlines.h
@@ +49,5 @@
>    return false;
>  }
>  
> +inline bool
> +nsWrapperCache::HasNothingToTrace(nsISupports* aThis)

Please define IsBlackAndDoesNotNeedTracing in terms of this function rather than duplicating the code.
Attachment #8337236 - Flags: review?(continuation) → review+
Hurray for making windows just a little bit less weird! :)
Attached patch v2 (obsolete) — Splinter Review
Indeed, marking focusmanager's windows (and other interesting member variables) to be in cc generation is better.
Attached patch v2Splinter Review
Attachment #8337375 - Attachment is obsolete: true
Comment on attachment 8337376 [details] [diff] [review]
v2

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

::: dom/base/nsFocusManager.cpp
@@ +3438,5 @@
>  
> +void
> +nsFocusManager::MarkUncollectableForCCGeneration(uint32_t aGeneration)
> +{
> +  if (sInstance) {

Maybe change this to early return?

::: dom/base/nsWrapperCacheInlines.h
@@ +36,5 @@
>    }
>  }
>  
>  inline bool
> +nsWrapperCache::HasNothingToTrace(nsISupports* aThis)

Now that I think about it, it might be nicer if this was NeedsTracing() and you could just return hasGrayObjects.  Just to avoid a negation in there.

@@ +50,4 @@
>  nsWrapperCache::IsBlackAndDoesNotNeedTracing(nsISupports* aThis)
>  {
>    if (IsBlack()) {
> +    return HasNothingToTrace(aThis);

The method body could just be |return IsBlack() && HasNothingToTrace(aThis);|
Attached patch v3Splinter Review
HasNothingToTrace is what the callers need, so nicer than NeedsTracing
Yeah, I guess you have to reason about a negation either way.
https://hg.mozilla.org/mozilla-central/rev/41937f231795
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.