Closed Bug 762939 Opened 8 years ago Closed 7 years ago

[BrowserAPI] Correctly propagate setVisibile changes across <iframe mozbrowser>

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mikeh, Assigned: justin.lebar+bug)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Observed on b2g, pulled from github.com/mozilla-b2g/mozilla-central.git:1a7df179243a612b6836c0b6169338d70e677cd5.

Steps to reproduce:
1. patch gecko with patch from bug 740997
2. drop camera.js from bug 740997 in $B2G/gaia/apps/camera/js
3. ./build.sh gecko && ./flash.sh gecko && ./flash.sh gaia
4. adb logcat
5. unlock DUT and open the camera app
6. allow the screen to timeout
7. press the power button to wake up the DUT and get the lock screen

Expected behaviour:
On step 5, the camera hardware should be opened and the preview should be visible and active.  One step 6, the camera hardware should be closed and should remain closed.

Observed behaviour:
On step 7, the camera hardware is re-opened, even though the camera app is not visible.

adb logcat output (pruned to show GeckoConsole and power only):
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:196 in anonymous: Hello, hidden = 'mozHidden', visibilityChange = 'mozvisibilitychange'
 ...camera is opened and preview is started...
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:77 in camera_setSource: Camera 0 focus modes: auto,infinity,macro
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:79 in camera_setSource: auto focus mode supported
 ...picture is taken...
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:114 in takePictureSuccess: got image: 1294123 bytes, image/jpeg
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:123 in anonymous: image saved as 'img_20120608-105818.jpg'
 ...preview is resumed...
 ...display times out and turns off, camera is successfully closed/destroyed...
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:188 in anonymous: camera app is now hidden
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:93 in pause: pausing viewfinder
I/power   (   78): *** set_screen_state 0
 ...pressed power button...
I/power   (   78): *** set_screen_state 1
 ...lock-screen is now visible on the screen, but the camera believes it is too and restarts the preview...
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:191 in anonymous: camera app is now visible
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:77 in camera_setSource: Camera 0 focus modes: auto,infinity,macro
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:79 in camera_setSource: auto focus mode supported
E/GeckoConsole(   78): No chrome package registered for chrome:///content/images/throbber.png
 ...display times out...
I/power   (   78): *** set_screen_state 0
 ...camera is successfully closed/destroyed...
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:188 in anonymous: camera app is now hidden
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:93 in pause: pausing viewfinder
 ...pressed power button...
I/power   (   78): *** set_screen_state 1
 ...again, lock-screen is now visible on the screen, but the camera believes it is too...
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:191 in anonymous: camera app is now visible
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:77 in camera_setSource: Camera 0 focus modes: auto,infinity,macro
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:79 in camera_setSource: auto focus mode supported
 ...display times out...
I/power   (   78): *** set_screen_state 0
 ...camera is successfully closed/destroyed...
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:188 in anonymous: camera app is now hidden
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:93 in pause: pausing viewfinder
The value document.hidden is completely controlled by the embedding (that is, the browser UI).  The DOM just reports the last thing it was told.

Sounds like some part of the b2g UI is not flagging things correctly....
Component: DOM → General
Product: Core → Boot2Gecko
QA Contact: general → general
Version: Trunk → unspecified
Do you mean that the UI literally has to set the value of |document.hidden|, or that it has to use docShell.isActive to indirectly set it?

We (should be) already doing the latter.
The latter.  The docshell then notifies all the documents inside itself.
Guys, we have to ensure the gaia window manager does setVisible(false) when we turn off the screen.

I think screen-off may be controlled by the b2g chrome code, which would make fixing this really hard.  We need gaia to control that.  And for that, I guess we need the idle timeout API.
Depends on: 715041
(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> Guys, we have to ensure the gaia window manager does setVisible(false) when
> we turn off the screen.
> 
> I think screen-off may be controlled by the b2g chrome code, which would
> make fixing this really hard.  We need gaia to control that.  And for that,
> I guess we need the idle timeout API.

That's exactly what happen.

Also it seems like there is an other bug when the top docshell is turned on (e.g the main system app), then all the sud-docshells (e.g apps contained as iframes) are turned on even if they have been turned off by setVisible(false) before.
The behavior of isActive on docshells is that setting it recursively sets in on all descendant docshells.  This is needed to make sure it propagates to subframes, for example, and makes total sense in the original use case (where the idea was that it would be set on a per-tab basis).

We could add some sort of "lock the visibility" API, or alternately the app needs to keep track of active state itself....
The gaia window manager keeps track of active state for apps.  If we could selectively disable recursive (de)activation that would do the trick.  Perhaps disabling it automatically at mozapp boundaries makes sense?

We still want the recursive behavior for tabs (iframes) within the browser app, e.g.
Actually, the browser app itself wants to manually manage active state for its tabs.  So probably disabling at mozbrowser boundaries is the right way to go (gecko treats mozapp as a superset of mozbrowser).
> Perhaps disabling it automatically at mozapp boundaries makes sense?

I think what logically needs to happen is that we have a hierarchy of visibility states

 "system level" -- screen is on or off
   "app level" -- app is foreground or background
     "browser tab level" -- browser tab is foreground or background

(Note that apps can nest.)

We save a visible state at each of these levels, but you can't be |on| unless everyone above you is |on|.  So you are Actually Visible (tm) if your Saved Visible State is |on| and your parent is Actually Visible.

I don't really care how we implement this...
My own tests shows that System app does receive mozvisibilitychange event from platform, and app frames is being set to false when switch away (by Gaia windows manager), and set to false when screen is off (by platform)

The problem seem to be that when the screen is turn back on, the app frames will be set to visibility *regardless they are on foreground (for Gaia) or not*.

I think the proper solution here would be

1) keep the logic Justin described (you can be visible only if everyone above you is on), and
2) if the parent frame (Gaia window manager) has ever set the visibility of the child frame to off, don't put it back to on when the screen is being turn back.

For condition (2), we would need a separate on/off state Boolean for the each frame, and do some AND/OR for between two.
> For condition (2), we would need a separate on/off state Boolean for the each frame, and do some 
> AND/OR for between two.

Yes, this is what I was trying to propose in comment 9.

> 2) if the parent frame (Gaia window manager) has ever set the visibility of the child frame to 
> off, don't put it back to on when the screen is being turn back.

This isn't quite right.  Suppose Gaia sets the child frame to not-visible, then later sets it to visible.  We turn the screen off, then on.  That child frame is now visible, right?
Marking depends-on browser-api mostly because I don't want to forget about this.  But also because the browser api lets us explicitly set the visibility state of frames.
Blocks: browser-api
Blocks: 702880
(In reply to Justin Lebar [:jlebar] from comment #11)
> > For condition (2), we would need a separate on/off state Boolean for the each frame, and do some 
> > AND/OR for between two.
> 
> Yes, this is what I was trying to propose in comment 9.
> 
> > 2) if the parent frame (Gaia window manager) has ever set the visibility of the child frame to 
> > off, don't put it back to on when the screen is being turn back.
> 
> This isn't quite right.  Suppose Gaia sets the child frame to not-visible,
> then later sets it to visible.  We turn the screen off, then on.  That child
> frame is now visible, right?

Yes. (I don't see any difference between your comment and mine)
Assignee: nobody → justin.lebar+bug
Depends on: 772275
bz, can you please review the docshell change?  

Dale, could you look at the other changes?  I think a peer should still review, but would you mind doing a first pass?
Attachment #640466 - Flags: review?(bzbarsky)
Attachment #640466 - Flags: feedback?(dale)
Comment on attachment 640466 [details] [diff] [review]
Patch + tests, v1

The docshell parts look fine as long as the browser app marks all its browserframes not active when it gets backgrounded and so forth.

I hate XPCOM signatures.  :(
Attachment #640466 - Flags: review?(bzbarsky) → review+
> The docshell parts look fine as long as the browser app marks all its browserframes not active when 
> it gets backgrounded and so forth.

If you meant actually meant "browser app" and not browserelement{parent,child}: The browser app doesn't have to do anything, under this patch.  BrowserElementParent listens to mozvisibilitychange on the window containing its iframe, and it propagates those visibility changes down to the BrowserElementChild.
Summary: document.visibilitychange event handler is reporting document.hidden == 0 on device wake-up to lock screen → [BrowserAPI] Correctly propagate setVisibile changes across <iframe mozbrowser>
Attachment #640466 - Flags: review?(mounir)
Comment on attachment 640466 [details] [diff] [review]
Patch + tests, v1

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

::: docshell/base/nsDocShell.cpp
@@ +5052,4 @@
>    PRInt32 n = mChildList.Count();
>    for (PRInt32 i = 0; i < n; ++i) {
>        nsCOMPtr<nsIDocShell> docshell = do_QueryInterface(ChildAt(i));
> +      if (docshell) {

nit: for code readability, I think doing:
if (!docshell) {
  continue;
}
is better, but do whatever you want ;)

::: dom/browser-element/mochitest/browserElement_SetVisibleFrames2.js
@@ +34,5 @@
> +        SimpleTest.executeSoon(function() {
> +          SimpleTest.executeSoon(function() {
> +            SimpleTest.executeSoon(function() {
> +              SimpleTest.executeSoon(function() {
> +                SimpleTest.finish();

This is quite sad to see that but given that this will create random-green, I guess we can live with it.

::: dom/browser-element/mochitest/file_browserElement_SetVisibleFrames_Outer.html
@@ +36,5 @@
> +
> +document.body.appendChild(iframe1);
> +document.body.appendChild(iframe2);
> +iframe1.src = 'file_browserElement_SetVisibleFrames_Inner.html?child1';
> +iframe2.src = 'file_browserElement_SetVisibleFrames_Inner.html?child2';

nit: setting the src before adding a frame to the document is a good practice.
Attachment #640466 - Flags: review?(mounir) → review+
Attachment #640466 - Flags: feedback?(dale)
https://hg.mozilla.org/mozilla-central/rev/2b68a159759f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.