805907 - Handle memory pressure notification in the TileStore

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch

This is almost done. It just doesn't find the right layer. Have to catch a bus but it just needs 2 minutes of work + testing:

adb shell am broadcast -a org.mozilla.gecko.FORCE_MEMORY_PRESSURE

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch

This will recreate the tile store on a memory pressure.

Comment 3

[Chris Lord [:cwiiis]]

Comment on attachment 676228 [details] [diff] [review]
patch

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

I'm not sure I'm the best person to review this, but I don't see anything obviously wrong. If you're confident about this, take my r+, but if you want a more thorough review I'd recommend seeking out someone that knows this kind of code a bit better.

Interested to hear the answer for my two nits though, especially the second.

::: gfx/layers/ipc/CompositorChild.cpp
@@ +16,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> +// Observer for the memory-pressure notification, to trigger
> +// flushing of the shaped-word caches

The shaped-word caches? I don't know what this means, which indicates to me that it might be worth getting someone that knows this kind of code better to review this section. Unless this is a copy-paste error, in which case my review may be fine.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +236,5 @@
> +{
> +  if (!mLayerManager)
> +    return true;
> +
> +  Layer* layer = mLayerManager->GetPrimaryScrollableLayer();

Could we not just send it to the root layer in case here?

Comment 4

[Benoit Girard (:BenWa)]

The first is in fact a copy and paste mistake. Are you just not comfortable with the observer code? I can ask Ehsan to look at that.

Comment 5

[Benoit Girard (:BenWa)]

Attachment: patch

Can you take a look at the event observer Ehsan?

Comment 6

[(no longer active)]

Comment on attachment 676250 [details] [diff] [review]
patch

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

r+ on the observer stuff with the below nits fixed.

::: gfx/layers/ipc/CompositorChild.cpp
@@ +24,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIOBSERVER
> +
> +  MemoryPressureObserver(CompositorChild* cc)

Nit: explicit, please.

::: gfx/layers/ipc/CompositorChild.h
@@ +53,5 @@
>    { return false; }
>  
>  private:
>    nsRefPtr<LayerManager> mLayerManager;
> +  nsRefPtr<nsIObserver> mMemoryPressureObserver;

nsCOMPtr please.

Comment 7

[Benoit Girard (:BenWa)]

Attachment: patch to land

Comment 8

[Benoit Girard (:BenWa)]

Attachment: patch to land

Comment 9

[Benoit Girard (:BenWa)]

Comment on attachment 676349 [details] [diff] [review]
patch to land

https://hg.mozilla.org/integration/mozilla-inbound/rev/537120d7827f

Comment 10

[Benoit Girard (:BenWa)]

This got merged. Looks like my comment was bad.