1533112 - Exposing capturePixels method through GeckoSession

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P1)

Description

[Arturo Mejia [:amejia]]

To integrate the functionality of taking screenshots on Android Components. It will be ideal to have access to capturePixels throught the GeckoSession. As we have the GeckoSession completely decoupled from the GeckoView instance, it's pretty hard for us to integrate it as it is.

Comment 1

[Emily Toop (:fluffyemily)]

This is kinda gnarly as the GeckoSession must be connected to a GeckoView in order to be able to render the screenshot. If we place a method on GeckoSession and you have no access to the GeckoView you will have no way of knowing if the session is attached to a view when you take the screenshot. This means the chances that the returned screenshot is empty, or the function will return an error are much higher than we would like for our API.

One solution could be to expose the GeckoDisplay held onto by the GeckoSession via a getter and allow consumers to call session.getDisplay().capturePixels(). getDisplay will return a null value if the session has been disconnected from a view and therefore will prevent the requesting of screenshots in that case. However we would then be exposing the sessions GeckoDisplay which is something that we do not really want - the desire being to ensure that GeckoDisplay is only accessible through a GeckoView and therefore only "owned" by one instance.

The other solution would be to add a capturePixels to GeckoSession, and doing the check for a non-null GeckoDisplay here and resolving the GeckoResult with either a null value or an IllegalStateException if there is no GeckoDisplay present.

Another possibility would be to expose the Display of the GeckoView via a getter and require GeckoEngineView to add this to the GeckoEngineSession when a session is added, and removing it again when the session is removed. This would allow GeckoEngineSession to be made aware of the attachment state of the GeckoSession and capturePixels can be called on the Display directly. Once again, this then leads us to having more than one "owner" of the Display, which is less than desirable.

Whatever the solution, it pushes the onus onto the consumer to ensure that the session is connected to a view before requesting a screenshot, something we wanted to avoid when we decided to place capturePixels on the GeckoView.

Comment 2

[Emily Toop (:fluffyemily)]

This is probably out there but, would this work as a solution? Adding an extra observer for onProgress when you set the session on GeckoEngineView? You'd want to unregister when updating a session and in onDetachedFromWindow too.

    override fun render(session: EngineSession) {
        val internalSession = session as GeckoEngineSession

        if (currentGeckoView.session != internalSession.geckoSession) {
            currentGeckoView.session?.let {
                // Release a previously assigned session. Otherwise GeckoView will close it
                // automatically.
                currentGeckoView.releaseSession()
            }

            currentGeckoView.session = internalSession.geckoSession

            session.register(object : EngineSession.Observer {
                override fun onProgress(progress: Int) {
                    if (progress == GeckoEngineSession.PROGRESS_STOP) {
                        currentGeckoView.capturePixels().then { ... }
                    }
                }
            })
        }

    }

Then, if the session is not connected to a view, the call to capturePixels will never occur....

Comment 3

[Arturo Mejia [:amejia]]

The other solution would be to add a capturePixels to GeckoSession, and doing the check for a non-null GeckoDisplay here and resolving the GeckoResult with either a null value or an IllegalStateException if there is no GeckoDisplay present.

I think the approach above serve us better because we want to be able to request to take a screenshot on demand. And we are OK with GeckoResult being null able

Comment 4

[Emily Toop (:fluffyemily)]

Why can the screenshot on demand not be added to the GeckoEngineView rather than the session? Surely the consumers of the component will have access to both view and session?

Comment 5

[Emily Toop (:fluffyemily)]

Attachment: Bug 1533112 - Add a GeckoSession method for capturing screenshots r=snorp

We don't really want to be doing this, the method belongs on the GeckoView, but Android Components have no access to the
GeckoView at the point where they wish to take screenshots and so here we are.

This patch may end up being abandoned if we can come up with another way of doing it.

Comment 6

[Arturo Mejia [:amejia]]

Thanks for all your help Emily,

We are going to close this one because we are going to use another approach using the existing API on GeckoView

We are going to change our API, and provide a new way, were apps that want to take screenshots will have to provide us with a GeckoView instance.