PR 164948. When leaving fullscreen <video> of a web page, the scale became larger than the scale before entering fullscreen.
Created attachment 149560 [details] Patch
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 :)
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.
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.
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.
(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)?
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 <zhajiang@rim.com> 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
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.
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.
(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.
(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?
(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.
(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.
Created attachment 149617 [details] Patch Renamed m_xScrollOffsetPriorGoingFullScreen and m_webPageScalePriorGoingFullScreen, removed author in the changelog, added a FIXME.
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)?
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.
Created attachment 149764 [details] Patch Updated the patch based on the comments.
Comment on attachment 149764 [details] Patch Committed manually r121362: http://trac.webkit.org/changeset/121362.