Closed Bug 1201394 Opened 4 years ago Closed 4 years ago

ProcessPriorityManager should use Prefs cache

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: kanru, Assigned: kanru)

Details

Attachments

(2 files)

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: nobody → kchen
Status: NEW → ASSIGNED
Attachment #8656979 - Flags: review?(gsvelto)
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 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+
(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.
https://hg.mozilla.org/mozilla-central/rev/f5047a4f41b4
https://hg.mozilla.org/mozilla-central/rev/16a9a6385720
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.