1201394 - ProcessPriorityManager should use Prefs cache

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+8)]

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.

Comment 1

[Kan-Ru Chen [:kanru] (UTC+8)]

Attachment: Use cached preferences value in ProcessPriorityManager

Comment 2

[Kan-Ru Chen [:kanru] (UTC+8)]

Attachment: Remove unused mLRUPoolSize member variable.

Comment 3

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8656981 [details] [diff] [review]
Remove unused mLRUPoolSize member variable.

LGTM, I'll have a look at the other patch ASAP.

Comment 4

[Gabriele Svelto [:gsvelto]]

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?

Comment 5

[Kan-Ru Chen [:kanru] (UTC+8)]

(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 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5047a4f41b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/16a9a6385720

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f5047a4f41b4
https://hg.mozilla.org/mozilla-central/rev/16a9a6385720