Closed
Bug 993930
Opened 10 years ago
Closed 10 years ago
The Some App has very smaller scale after playing full-screen on browser(m.youtube.com)
Categories
(Core :: Graphics, defect)
Tracking
()
People
(Reporter: hansu9866, Assigned: botond)
References
Details
(Keywords: regression)
Attachments
(4 files)
STR: 1. run setting app 2. go to home screen with home key, and then setting app is going to background. 3. run browser app -> go to m.youtube.com -> watch full screen video. 4. switch to setting app in during playing full screen video. problems. setting app lost original scale(1X). it looks changed small scale expected doesn't change scale in other app in additionally, nexus-4(v1.5) can reproduced this problems. attachment reproduced app's screenshot attached
Who can handle this issue? Please inform some suitable person, if you know. thanks
Comment 3•10 years ago
|
||
What device was this reproduced this? What build ID?
Flags: needinfo?(jsmith)
Comment 4•10 years ago
|
||
I just tried this on a nexus 4 and can't reproduce from master. Does this happen everytime?
Flags: needinfo?(mchang)
(In reply to Jason Smith [:jsmith] from comment #3) > What device was this reproduced this? What build ID? Nexus4 on master(v1.5) was reproduced it. gecko commit : 63e9fff3bca167b1ab0fce67f3ae122fe82b84ba gaia commit : 1958454595b1fa0e061f0652ae965629993f5708
(In reply to Mason Chang [:mchang] from comment #4) > I just tried this on a nexus 4 and can't reproduce from master. Does this > happen everytime? This is reproduce step. example)run settings app -> press home button (settings app is background)->run browser app and m.youtube.com-> play any video and then fullscreen mode running -> press home button(browser app is background) -> now homescreen -> and then touch settings app(background->foreground). example application is settings app, other application result is same for that.
Comment 7•10 years ago
|
||
Reproduced this on a 1.4 over a nexus 4 device in Taipei: 1. launch settings 2. hit home key 3. launch browser, go to m.youtube.com 4. play any video, tap the video while its playing and select full screen. Allow share fullscreen if prompted. 5. with video play, hit home key 6. hit settings icon to bring settings app to foreground. Nom'ing blocker.
blocking-b2g: --- → 1.4?
Comment 8•10 years ago
|
||
Adding qawanted to see if we can reproduce this on a production device.
Keywords: qawanted
Comment 9•10 years ago
|
||
Following repro steps at comment 7, issue does NOT reproduce on today's 1.5 master: Device: Buri v1.5 Master MOZ BuildID: 20140414040203 Gaia: f3abbd2d0a60f1a1618db93f8b1957cae6de379c Gecko: 215080b813a7 Version: 31.0a1 Firmware Version: v1.2-device.cfg Issue DOES reproduce on today's 1.4 Aurora: Device: Buri v1.4 Aurora MOZ BuildID: 20140414000201 Gaia: 8dff633372022723e2ebad17fe3c826436b3b258 Gecko: bc14179fc49c Version: 30.0a2 Firmware Version: v1.2-device.cfg Attaching a logcat reproducing the issue.
Comment 11•10 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #10) > Can we check if this is reproducing on 1.3? Following repro steps from comment 7, issue does NOT reproduce on Buri 1.3. Settings app didn't lose original scale when user returned to that app. Environmental Variables: Device: Buri 1.3 MOZ BuildID: 20140414164003 Gaia: 03340f9b00343512066ab0dceca1641ef9a5374c Gecko: 0b0822ffdbf8 Version: 28.0 Firmware version: v1.2-device.cfg
Keywords: qawanted
Updated•10 years ago
|
Keywords: regression,
regressionwindow-wanted
Reporter | ||
Comment 12•10 years ago
|
||
In my version trace, this issue seems to be related with Bug 959694. after apply the patch, it have normal operation. Thanks.
Updated•10 years ago
|
QA Contact: ddixon
Comment 13•10 years ago
|
||
analysis of our member. This is due to issues in app bounds calculation. This issue can be fixed by adding width: 100%; height: 100%; to system/style/window.css .appwindow, .activitywindow css selector. The patch of Bug 959694 is having this modification. That’s the reason why the issue is not happening.
Comment 14•10 years ago
|
||
I don't think this happens on a buri, can QA test with any device with a denser screen?
Comment 17•10 years ago
|
||
I was able to reproduce this issue on a Buri device and found a regression window. The earliest builds we have for Mozilla Inbound start at 3/27/14 so we could not provide a deeper regression window than this one. Tinderbox Central 1.4 Regression Window Last Working Build: 1.4 Environmental Variables: Device: buri 1.4 MOZ BuildID: 20140222031748 Gaia: 7c320aa3291761196ccd6ed3131faffb255f3fb6 Gecko: 2d5d5fd03050 Version: 30.0a1 Firmware Version: v1.2-device.cfg First Broken Build: 1.4 Environmental Variables: Device: buri 1.4 MOZ BuildID: 20140223084714 Gaia: 7c320aa3291761196ccd6ed3131faffb255f3fb6 Gecko: 2621a1cc8c6c Version: 30.0a1 Firmware Version: v1.2-device.cfg Last Working Gecko First Broken Gaia: Issue DOES NOT reproduce here. Gaia: 7c320aa3291761196ccd6ed3131faffb255f3fb6 Gecko: 2d5d5fd03050 Last Working Gaia First Broken Gecko: Issue DOES reproduce here. Gaia: 7c320aa3291761196ccd6ed3131faffb255f3fb6 Gecko: 2621a1cc8c6c Gecko Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2d5d5fd03050&tochange=2621a1cc8c6c
Updated•10 years ago
|
Keywords: qawanted,
regressionwindow-wanted
Comment 18•10 years ago
|
||
The window here is inaccurate. If you don't have inbound builds, then you should produce a window using either 1.4 or m-c.
Keywords: regressionwindow-wanted
Comment 19•10 years ago
|
||
I have rechecked this regression window using Buri M-C Tinderbox builds and have found the following results. The pushlog seems to indicate a merge from mozilla-inbound to m-c, we unfortunately do not have mozilla-inbound builds available in this date range to provide a deeper window. --- Tinderbox Regression Window: Last Working Environmental Variables: Device: Buri BuildID: 20140222031748 Gaia: 7c320aa3291761196ccd6ed3131faffb255f3fb6 Gecko: 2d5d5fd03050 Version: 30.0a1 Base Image: V1.2-device.cfg First Broken Environmental Variables: Device: Buri BuildID: 20140223084714 Gaia: 7c320aa3291761196ccd6ed3131faffb255f3fb6 Gecko: 2621a1cc8c6c Version: 30.0a1 Base Image: V1.2-device.cfg The Gaia ID is the same in both builds, indicating that this is a Gecko issue. Gecko Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2d5d5fd03050&tochange=2621a1cc8c6c --- This issue DOES still reproduce in the latest Buri Aurora build, and does NOT reproduce in the latest Buri Master build. It appears that this was introduced in Master in February, carried on to Aurora, and then fixed in Master some time after the branch (Comment 13 suggests Bug 959694). We can find and provide the reverse window where the issue stopped reproducing in Master if that is something that would be helpful.
Keywords: regressionwindow-wanted
Comment 20•10 years ago
|
||
Maybe graphics?
Component: Gaia::System → Graphics
Product: Firefox OS → Core
Version: unspecified → 30 Branch
Comment 21•10 years ago
|
||
Could bug 968991 be the cause of this bug?
Comment 22•10 years ago
|
||
(In reply to J Zimbrick from comment #19) > I have rechecked this regression window using Buri M-C Tinderbox builds and > have found the following results. > > The pushlog seems to indicate a merge from mozilla-inbound to m-c, we > unfortunately do not have mozilla-inbound builds available in this date > range to provide a deeper window. > > --- > > Tinderbox Regression Window: > > Last Working Environmental Variables: > Device: Buri > BuildID: 20140222031748 > Gaia: 7c320aa3291761196ccd6ed3131faffb255f3fb6 > Gecko: 2d5d5fd03050 > Version: 30.0a1 > Base Image: V1.2-device.cfg > > First Broken Environmental Variables: > Device: Buri > BuildID: 20140223084714 > Gaia: 7c320aa3291761196ccd6ed3131faffb255f3fb6 > Gecko: 2621a1cc8c6c > Version: 30.0a1 > Base Image: V1.2-device.cfg > > The Gaia ID is the same in both builds, indicating that this is a Gecko > issue. > > Gecko Pushlog: > http://hg.mozilla.org/mozilla-central/ > pushloghtml?fromchange=2d5d5fd03050&tochange=2621a1cc8c6c > > --- > > This issue DOES still reproduce in the latest Buri Aurora build, and does > NOT reproduce in the latest Buri Master build. > > It appears that this was introduced in Master in February, carried on to > Aurora, and then fixed in Master some time after the branch (Comment 13 > suggests Bug 959694). > > We can find and provide the reverse window where the issue stopped > reproducing in Master if that is something that would be helpful. So this is reproducing inconsistently on devices. Please provide the window to help us narrow down the cause.
Comment 23•10 years ago
|
||
(In reply to Preeti Raghunath(:Preeti) from comment #22) > So this is reproducing inconsistently on devices. Please provide the window > to help us narrow down the cause. Clarified in person, but that isn't what is seen here. All the above window says is that this is already fixed on master, but broken on 1.4.
Comment 26•10 years ago
|
||
Botond, could you take a quick look, see if you can reproduce the issue, see if comment 13 solves it and maybe shed some light as to where this bug belongs?
Assignee: nobody → botond
Flags: needinfo?(botond)
Comment 27•10 years ago
|
||
Bug 992318 may also contain some clues.
Assignee | ||
Comment 28•10 years ago
|
||
I can reproduce this issue with an old-ish build of master (Apr 16 Gecko, Mar 25 Gaia). However, applying the fix described in comment 13 does not fix it for me. I will investigate further.
Comment 29•10 years ago
|
||
can not reproduce on master, but can reproduce on 1.4 pvt build.
Assignee | ||
Comment 30•10 years ago
|
||
It(In reply to Jason Smith [:jsmith] from comment #21) > Could bug 968991 be the cause of this bug? It looks that way. Here's what's happening: - When the settings app is initially loaded, in the before-first- paint handler, TabChild::InitializeRootMetrics() and TabChild::HandlePossibleViewportChange() are both called. - InitializeRootMetrics() calculates an initial zoom based on aviewport width of 980. - HandlePossibleViewportChange() calculates a correct viewport width of 768, and calls ZoomBy() [1] to adjust the zoom by what is essentially the ratio of the old and new viewport widths. To figure out the old viewport width, it looks at mOldViewportWidth, sees that it's 0, and uses 980 instead, which matches what InitializeRootMetrics() assumed. - The result of this adjustment is that the zoom is as if it had been initially calculated based on a viewport width of 768. This is the correct zoom for the app. - Since we set the viewport of 768 that we calculate here, mOldViewportWidth is updated to 768. - When the video in the browser app goes full screen, the size of the settings app's iframe becomes (0, 0), and TabChild:: RecvUpdateDimensions() is called with size = (0, 0). - When the settings app is brought to the foreground again, TabChild::RecvUpdateDimensions() is called again with a nonzero size. Since the size before was zero and is now nonzero, TabChild treates this as an "initial sizing" and calls InitializeRootMetrics() again in addition to HandlePossibleViewportChange(). - InitializeRootMetrics() again calculates an initial zoom based on a viewport width of 980. Note however that mOldViewportWidth is still 768. - HandlePossibleViewportChange() again calculates the correct viewport of 768, and performs the zoom adjustment again, but this time the adjustment factor is 1, since mOldViewportWidth and the calculated viewport width are both 768. As a result, the zoom remains the one calculated for the 980-width viewport, which is incorrect and causes the reported rendering problem. Some additional notes: - The reason this regression was _introduced_ by bug 968991 is that that's what introduced the call to InitializeRootMetrics() in RecvUpdateDimensions(). - The reason this regression _went away_ in master, is that the video going fullscreen no longer causes the settings app's iframe to have size zero - instead it remains nonzero. I don't know why that is. If we want to know, a reverse regression window that points out when the regression went away would be very helpful. I can think of three possible approaches: - Get TabChild to handle changes of size to zero and then back to nonzero gracefully. I can think of two ways to do this: 1) Instead of using mInnerSize being zero as a proxy for having a valid size, have a flag that is set the first time mInnerSize is set. This would avoid us calling InitializeRootMetrics() the second time. --> I have tried this approach, and verified that it fixes the problem. Patch forthcoming. 2) Set mOldViewportWidth to 980 in InititalizeRootMetrics(). This would mean InitializeRootMetrics() would still be called a second time and set the viewport width to 980 again, but HandlePossibleViewportChange() would then correct this the same way it corrects it the first time. 3) Consider changing the size to zero to be an invalid operation. Find out what change between 1.4 and master got rid of this invalid operation being performed (preferrably using a reverse regression window), and uplift it to 1.4. Kats, any thoughts on which of these solutions is the most appropriate? Timothy also pointed out that sending size change events at all for apps in the background indicates that we are doing unnecessary work. I will file a follow-up to investigate this (but not in the 1.4 timeframe, as that is not what regressed.) [1] http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?from=TabChild.cpp#275
Flags: needinfo?(botond) → needinfo?(bugmail.mozilla)
Assignee | ||
Comment 31•10 years ago
|
||
Attached is a patch that implements approach (1) from the previous comment. I have verified that this fixes the problem.
Assignee | ||
Comment 32•10 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #30) > Timothy also pointed out that sending size change events at all for > apps in the background indicates that we are doing unnecessary work. > I will file a follow-up to investigate this (but not in the 1.4 > timeframe, as that is not what regressed.) Filed bug 1001214.
Comment 33•10 years ago
|
||
Comment on attachment 8412251 [details] [diff] [review] Patch that implements approach (1), against 1.4 Review of attachment 8412251 [details] [diff] [review]: ----------------------------------------------------------------- I think we can land this on master and uplift it to 1.4. It does make the code more robust anyway. Since the size isn't getting reset back to zero on master I'm not sure it's worth the effort to track that down.
Attachment #8412251 -
Flags: review+
Updated•10 years ago
|
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 34•10 years ago
|
||
green try |
Rebased patch to apply to master. Re-requesting review because I moved HasValidInnerSize() from TabChildBase to TabChild. T-push with this patch: https://tbpl.mozilla.org/?tree=Try&rev=615afc5cac71
Attachment #8412667 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8412251 -
Attachment description: Patch that implements approach (1) → Patch that implements approach (1), against 1.4
Updated•10 years ago
|
QA Contact: ddixon
Comment 35•10 years ago
|
||
Comment on attachment 8412667 [details] [diff] [review] Patch that implements approach (1), against master Review of attachment 8412667 [details] [diff] [review]: ----------------------------------------------------------------- I suspect HasValidInnerSize (and mHasValidInnerSize) need to be on TabChildBase. f? to tmeshkova.
Attachment #8412667 -
Flags: feedback?(tanya.meshkova)
Assignee | ||
Comment 36•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #35) > I suspect HasValidInnerSize (and mHasValidInnerSize) need to be on > TabChildBase. f? to tmeshkova. If that's the case, then whatever code sets mInnerSize in the other derived class(es) of TabChildBase would need to be updated to set mHasValidInnerSize as well.
Comment 37•10 years ago
|
||
I think it might just be that other derived classes call HasValidInnerSize() but don't update anything. But yes, it's worth clarifying.
Assignee | ||
Comment 38•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #37) > I think it might just be that other derived classes call HasValidInnerSize() > but don't update anything. But yes, it's worth clarifying. If they don't update mInnerSize, it will always be zero, and HasValidInnerSize() will always return false, in which case why call it?
Comment 39•10 years ago
|
||
Comment on attachment 8412667 [details] [diff] [review] Patch that implements approach (1), against master Review of attachment 8412667 [details] [diff] [review]: ----------------------------------------------------------------- I'll r+ this for now and we can change it as needed if :tmeshkova responds.
Attachment #8412667 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 40•10 years ago
|
||
landing |
https://hg.mozilla.org/integration/mozilla-inbound/rev/d14d29f8fee6
https://hg.mozilla.org/mozilla-central/rev/d14d29f8fee6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•10 years ago
|
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → fixed
Comment 42•10 years ago
|
||
Comment on attachment 8412667 [details] [diff] [review] Patch that implements approach (1), against master Oh, sorry for late response, what was the reason of not keeping this as part of TabChildBase?
Comment 43•10 years ago
|
||
Comment on attachment 8412667 [details] [diff] [review] Patch that implements approach (1), against master ok, I see it is implementation specific now.
Attachment #8412667 -
Flags: feedback?(tanya.meshkova) → feedback+
Comment 44•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/fa1c4fa393be
You need to log in
before you can comment on or make changes to this bug.
Description
•