Closed
Bug 942240
Opened 11 years ago
Closed 11 years ago
Improve the skippability of nsGlobalWindow
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: smaug, Assigned: smaug)
References
Details
(Whiteboard: [qa-])
Attachments
(3 files, 1 obsolete file)
11.40 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
14.22 KB,
patch
|
Details | Diff | Splinter Review | |
14.19 KB,
patch
|
Details | Diff | Splinter Review |
(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.)
Assignee | ||
Comment 1•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=7e13c02083f7
Assignee | ||
Comment 2•11 years ago
|
||
Er, this one https://tbpl.mozilla.org/?tree=Try&rev=9c553eb0ffae
Assignee | ||
Comment 3•11 years ago
|
||
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 4•11 years ago
|
||
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+
Comment 5•11 years ago
|
||
Hurray for making windows just a little bit less weird! :)
Assignee | ||
Comment 6•11 years ago
|
||
Indeed, marking focusmanager's windows (and other interesting member variables) to be in cc generation is better.
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #8337375 -
Attachment is obsolete: true
Comment 8•11 years ago
|
||
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);|
Assignee | ||
Comment 9•11 years ago
|
||
HasNothingToTrace is what the callers need, so nicer than NeedsTracing
Comment 10•11 years ago
|
||
Yeah, I guess you have to reason about a negation either way.
Assignee | ||
Comment 11•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/41937f231795
Comment 12•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/41937f231795
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•11 years ago
|
Whiteboard: [qa-]
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•