Bug 89546 – [BlackBerry] Wrong scale after leaving fullscreen <video>

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Jacky Jiang]

PR 164948.
When leaving fullscreen <video> of a web page, the scale became larger than the scale before entering fullscreen.

[Jacky Jiang]

Created attachment 149560 [details]
Patch

[Yong Li]

Comment on attachment 149560 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149560&action=review

> Source/WebKit/blackberry/Api/WebPage.cpp:1715
> +    m_transformationMatrix->setM11(scale);
> +    m_transformationMatrix->setM22(scale);

I know we assume m11=m12=scale. But m_transformationMatrix.scale(scale) looks better :)

[Rob Buis]

Comment on attachment 149560 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149560&action=review

Looks fine, will eave it for Antonio though, maybe we have to rename m_xScrollOffsetPriorGoingFullScreen too?

> Source/WebKit/blackberry/Api/WebPage.cpp:375
> +    , m_webPageScalePriorGoingFullScreen(-1.0)

It is actually "prior to". But nicer here would be m_scaleBeforeFullScreen.

[Jacky Jiang]

Comment on attachment 149560 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149560&action=review

>> Source/WebKit/blackberry/Api/WebPage.cpp:1715
>> +    m_transformationMatrix->setM22(scale);
> 
> I know we assume m11=m12=scale. But m_transformationMatrix.scale(scale) looks better :)

Well, actually m_transformationMatrix.scale(scale) is relative to the current scale, e.g: the original scale = 1.5, we scale to scale =2, then we got 3. But we need to set the scale to 2 not 3.

[Jacky Jiang]

Comment on attachment 149560 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149560&action=review

>> Source/WebKit/blackberry/Api/WebPage.cpp:375
>> +    , m_webPageScalePriorGoingFullScreen(-1.0)
> 
> It is actually "prior to". But nicer here would be m_scaleBeforeFullScreen.

What do you think Antonio?  If it is like this, I need to rename your m_xScrollOffsetPriorGoingFullScreen as well.

>>> Source/WebKit/blackberry/Api/WebPage.cpp:1715
>>> +    m_transformationMatrix->setM22(scale);
>> 
>> Well, actually m_transformationMatrix.scale(scale) is relative to the current scale, e.g: the original scale = 1.5, we scale to scale =2, then we got 3. But we need to set the scale to 2 not 3.
> 
> I know we assume m11=m12=scale. But m_transformationMatrix.scale(scale) looks better :)

So m_transformationMatrix.scale(scale) is wrong, that's why I need to setM11 and setM22 directly.

[Yong Li]

(In reply to comment #4)
> (From update of attachment 149560 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149560&action=review
> 
> >> Source/WebKit/blackberry/Api/WebPage.cpp:1715
> >> +    m_transformationMatrix->setM22(scale);
> > 
> > I know we assume m11=m12=scale. But m_transformationMatrix.scale(scale) looks better :)
> 
> Well, actually m_transformationMatrix.scale(scale) is relative to the current scale, e.g: the original scale = 1.5, we scale to scale =2, then we got 3. But we need to set the scale to 2 not 3.

OK. But what if m_transformationMatrix contains a translate (non-zero e/m13, f/m23)?

[Antonio Gomes]

Comment on attachment 149560 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149560&action=review

what if we rotate the device? does it invalidate the previous scale?

> Source/WebKit/blackberry/ChangeLog:7
> +        Patch by Jacky Jiang  <[e-mail]>

unneeded :)

>>> Source/WebKit/blackberry/Api/WebPage.cpp:375
>>> +    , m_webPageScalePriorGoingFullScreen(-1.0)
>> 
>> It is actually "prior to". But nicer here would be m_scaleBeforeFullScreen.
> 
> What do you think Antonio?  If it is like this, I need to rename your m_xScrollOffsetPriorGoingFullScreen as well.

Ok

>>>>> Source/WebKit/blackberry/Api/WebPage.cpp:1715
>>>>> +    m_transformationMatrix->setM22(scale);
>>>> 
>>>> I know we assume m11=m12=scale. But m_transformationMatrix.scale(scale) looks better :)
>>> 
>>> Well, actually m_transformationMatrix.scale(scale) is relative to the current scale, e.g: the original scale = 1.5, we scale to scale =2, then we got 3. But we need to set the scale to 2 not 3.
>> 
>> So m_transformationMatrix.scale(scale) is wrong, that's why I need to setM11 and setM22 directly.
> 
> OK. But what if m_transformationMatrix contains a translate (non-zero e/m13, f/m23)?

at least add a comment

[Jacky Jiang]

Comment on attachment 149560 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149560&action=review

>>>>> Source/WebKit/blackberry/Api/WebPage.cpp:1715
>>>>> +    m_transformationMatrix->setM22(scale);
>>>> 
>>>> OK. But what if m_transformationMatrix contains a translate (non-zero e/m13, f/m23)?
>>> 
>>> So m_transformationMatrix.scale(scale) is wrong, that's why I need to setM11 and setM22 directly.
>> 
>> Well, actually m_transformationMatrix.scale(scale) is relative to the current scale, e.g: the original scale = 1.5, we scale to scale =2, then we got 3. But we need to set the scale to 2 not 3.
> 
> I know we assume m11=m12=scale. But m_transformationMatrix.scale(scale) looks better :)

As far as I have tested, it only affects the zoom scale a and d which works well. The scroll position might be adjusted in setViewportSize right after leaving fullscreen.  As I can see the web page doesn't respect the y scroll position after leaving fullscreen is an old issue though, I didn't introduce new regression so far.

[Jacky Jiang]

Comment on attachment 149560 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149560&action=review

>>>> Source/WebKit/blackberry/Api/WebPage.cpp:375
>>>> +    , m_webPageScalePriorGoingFullScreen(-1.0)
>>> 
>>> Ok
>> 
>> What do you think Antonio?  If it is like this, I need to rename your m_xScrollOffsetPriorGoingFullScreen as well.
> 
> It is actually "prior to". But nicer here would be m_scaleBeforeFullScreen.

Will update the patch then.

[Jacky Jiang]

(In reply to comment #7)
> (From update of attachment 149560 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149560&action=review
> 
> what if we rotate the device? does it invalidate the previous scale?
> 

Missed this comment...   This scale is restored in exitFullScreenForElement(), setViewportSize still have the chance to clamp it by the minimum and maximum scale. As I can imagine that when we rotate device, we will try to restore this scale first, if this scale is even smaller than the minimum scale of the new rotation,  it will use the minimum scale instead.

[Yong Li]

(In reply to comment #8)

> As I can see the web page doesn't respect the y scroll position after leaving fullscreen is an old issue though, I didn't introduce new regression so far.

It would be nice if you can fix it together ;)

At least, can you add a FIXME there?

[Jacky Jiang]

(In reply to comment #11)
> (In reply to comment #8)
> 
> > As I can see the web page doesn't respect the y scroll position after leaving fullscreen is an old issue though, I didn't introduce new regression so far.
> 
> It would be nice if you can fix it together ;)
> 
> At least, can you add a FIXME there?
Is this expected Antonio? As I can see you only added m_xScrollOffsetPriorGoingFullScreen to respected the x position of the ScrollView in your m_xScrollOffsetPriorGoingFullScreen patch. 
If this is not expected, I would like to add a FIXME and get back to this later.

[Jacky Jiang]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #8)
> > 
> > > As I can see the web page doesn't respect the y scroll position after leaving fullscreen is an old issue though, I didn't introduce new regression so far.
> > 
> > It would be nice if you can fix it together ;)
> > 
> > At least, can you add a FIXME there?
> Is this expected Antonio? As I can see you only added m_xScrollOffsetPriorGoingFullScreen to respected the x position of the ScrollView in your m_xScrollOffsetPriorGoingFullScreen patch. 
> If this is not expected, I would like to add a FIXME and get back to this later.
Just confirmed with Antonio in person that always scroll to the top of the web page after leaving screen is not expected, we may need save y position as well as x position or create a struct to save those need to be restored data in the future. Will do it in a follow up.

[Jacky Jiang]

Created attachment 149617 [details]
Patch

Renamed m_xScrollOffsetPriorGoingFullScreen and m_webPageScalePriorGoingFullScreen, removed author in the changelog, added a FIXME.

[Antonio Gomes]

Comment on attachment 149617 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149617&action=review

> Source/WebKit/blackberry/Api/WebPage.cpp:1715
> +    m_transformationMatrix->setM11(scale);
> +    m_transformationMatrix->setM22(scale);

please add a comment about why you do not use TransformationMatrix::scale()

> Source/WebKit/blackberry/Api/WebPage.cpp:6225
> +        m_scaleBeforeFullScreen = currentScale();

I would also add a comment about when the scale changes, and why we cache and restore it.

> Source/WebKit/blackberry/Api/WebPage.cpp:6260
> +        if (m_scaleBeforeFullScreen > 0.0) {

s/0.0/0

> Source/WebKit/blackberry/Api/WebPage_p.h:215
> +    void setCurrentScale(double);

instead of setCurrentScale, maybe we should rename it to something more specific, so others would not try to use it. Also should it be wrapped by #if enabled(video) && enabled(video_fullscreen_api)?

[Jacky Jiang]

Comment on attachment 149617 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149617&action=review

>> Source/WebKit/blackberry/Api/WebPage.cpp:1715
>> +    m_transformationMatrix->setM22(scale);
> 
> please add a comment about why you do not use TransformationMatrix::scale()

OK. Will do.

>> Source/WebKit/blackberry/Api/WebPage.cpp:6225
>> +        m_scaleBeforeFullScreen = currentScale();
> 
> I would also add a comment about when the scale changes, and why we cache and restore it.

OK.

>> Source/WebKit/blackberry/Api/WebPage.cpp:6260
>> +        if (m_scaleBeforeFullScreen > 0.0) {
> 
> s/0.0/0

OK.

>> Source/WebKit/blackberry/Api/WebPage_p.h:215
>> +    void setCurrentScale(double);
> 
> instead of setCurrentScale, maybe we should rename it to something more specific, so others would not try to use it. Also should it be wrapped by #if enabled(video) && enabled(video_fullscreen_api)?

I will just get rid of this method and inline this in exitFullScreenForElement if the method might be misused by somebody in the future.

[Jacky Jiang]

Created attachment 149764 [details]
Patch

Updated the patch based on the comments.

[Jacky Jiang]

Comment on attachment 149764 [details]
Patch

Committed manually r121362: http://trac.webkit.org/changeset/121362.