Closed
Bug 1201394
Opened 10 years ago
Closed 10 years ago
ProcessPriorityManager should use Prefs cache
Categories
(Core :: DOM: Content Processes, defect)
Core
DOM: Content Processes
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: kanru, Assigned: kanru)
Details
Attachments
(2 files)
13.57 KB,
patch
|
gsvelto
:
review+
|
Details | Diff | Splinter Review |
1.87 KB,
patch
|
gsvelto
:
review+
|
Details | Diff | Splinter Review |
To avoid having to call the static Preferences API and automatically get updated value when pref changes. I found this when I wrote a test that uses ProcessPriorityManager test mode but didn't set any LRUPoolLevels, which broke the test_BackgroundLRU.html test because prefs were only read once.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8656981 -
Flags: review?(gsvelto)
Comment 3•10 years ago
|
||
Comment on attachment 8656981 [details] [diff] [review]
Remove unused mLRUPoolSize member variable.
LGTM, I'll have a look at the other patch ASAP.
Attachment #8656981 -
Flags: review?(gsvelto) → review+
Comment 4•10 years ago
|
||
Comment on attachment 8656979 [details] [diff] [review]
Use cached preferences value in ProcessPriorityManager
Review of attachment 8656979 [details] [diff] [review]:
-----------------------------------------------------------------
I like these changes quite a bit, thanks for doing this!
::: dom/ipc/ProcessPriorityManager.cpp
@@ +204,5 @@
> */
> void Unfreeze();
>
> + /**
> + * Call ShutDown before destroy ProcessPriorityManager.
nit: "Call ShutDown before destroying the ProcessPriorityManager", it would also be nice to describe why this is needed.
@@ +392,5 @@
> void* aClosure)
> {
> StaticInit();
> + if (!PrefsEnabled() && sSingleton) {
> + sSingleton->ShutDown();
Do we really need to call ShutDown() explicitly? Doesn't setting the StaticRefPtr<> to nullptr do that for us already?
Attachment #8656979 -
Flags: review?(gsvelto) → review+
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #4)
> Comment on attachment 8656979 [details] [diff] [review]
> Use cached preferences value in ProcessPriorityManager
>
> Review of attachment 8656979 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I like these changes quite a bit, thanks for doing this!
>
> ::: dom/ipc/ProcessPriorityManager.cpp
> @@ +204,5 @@
> > */
> > void Unfreeze();
> >
> > + /**
> > + * Call ShutDown before destroy ProcessPriorityManager.
>
> nit: "Call ShutDown before destroying the ProcessPriorityManager", it would
> also be nice to describe why this is needed.
>
> @@ +392,5 @@
> > void* aClosure)
> > {
> > StaticInit();
> > + if (!PrefsEnabled() && sSingleton) {
> > + sSingleton->ShutDown();
>
> Do we really need to call ShutDown() explicitly? Doesn't setting the
> StaticRefPtr<> to nullptr do that for us already?
It's needed because WakeLockObserver holds a strong reference to us.
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f5047a4f41b4
https://hg.mozilla.org/mozilla-central/rev/16a9a6385720
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•