817134 - Delete the low-res screenshot code

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Now that the low-res screenshot has been obsoleted by the progressive and low-res tiles stuff, we can delete the code. This includes all of the code in ScreenshotHandler as well as the AfterPaintListener goop in nsAppShell.cpp. We'll need to keep the AndroidBridge::TakeScreenshot and related code for the thumbnail codepaths, although it will need some renaming and rebinding of JNI functions.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

This should also save a bunch of memory because we're still allocating a 2-meg buffer in ScreenshotLayer which we don't need to be doing.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Delete some code

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Delete moar code and stop wasting memory

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 692373 [details] [diff] [review]
Delete some code

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

Can we get rid of the ScreenshotLayer entirely?

::: widget/android/AndroidBridge.cpp
@@ +2459,5 @@
>  jobject JNICALL
>  Java_org_mozilla_gecko_GeckoAppShell_allocateDirectBuffer(JNIEnv *env, jclass, jlong size);
>  
>  
> +nsresult AndroidBridge::TakeThumbnail(nsIDOMWindow *window, int32_t bufW, int32_t bufH, int32_t tabId, jobject buffer)

naming nit, I'd go with CaptureThumnail()

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 692374 [details] [diff] [review]
Delete moar code and stop wasting memory

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

guess I should have looked at this patch before asking about removing ScreenshotLayer

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Fixed the naming and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/35a2b968dc98
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f45583754ca

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35a2b968dc98
https://hg.mozilla.org/mozilla-central/rev/0f45583754ca

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Note: the removal of the 2-meg direct-allocation buffer reduces Fennec's startup vsize by the corresponding 2 megs, but doesn't reduce RSS that much because most of the 2 megs was never touched and so remained unmapped anyway.

Comment 9

[Robert Kaiser]

Can/should we land this on 19 as well? It looks like this "fixes" topcrash bug 817514.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

I think we can uplift it, yeah. I'll request approval once I have a chance to apply the patch on aurora and make sure none of the conflicts are dealbreakers (it's a large patch and will definitely not apply cleanly).

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 692373 [details] [diff] [review]
Delete some code

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: some crashes in screenshotting-related code, like bug 805353 and bug 817514.
Testing completed (on m-c, etc.): on m-c, nightlies (FF20) since mid-december
Risk to taking this patch (and alternatives if risky): touches android code only. the patches are large-ish but i was able to rebase them successfully to 19 branch, so I don't think there's a lot of risk. Note that the second patch here rebases much more easily if patch #2 from bug 820556 is also uplifted, so i will request uplift for that as well.
String or UUID changes made by this patch: none

Try build with the rebased patches: https://tbpl.mozilla.org/?tree=Try&rev=1248c582658d

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 692374 [details] [diff] [review]
Delete moar code and stop wasting memory

[Approval Request Comment]
Same as above

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 692373 [details] [diff] [review]
Delete some code

Approving in support of fixing top crasher bug 817514 - we'll have a few weeks of bake time with this change.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Landed on mozilla-beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/d0af06536aad
https://hg.mozilla.org/releases/mozilla-beta/rev/dd0bc34010e8