Implement non-visible pipeline and iframe visibility methods by jmr0 · Pull Request #10225 · servo/servo

jmr0 · 2016-03-27T19:19:59Z

This addresses #9566 and a good part of #9751, specifically:

Pipeline has a notion of visibility
IFrame setVisible/getVisible interface with IFrame's pipeline visibility
IFrame mozbrowservisibilitychange responds to changes in visibility
Pipeline visibility is used to limit animations (requestAnimationFrame does not tick animations when hidden) and to increase timer intervals (currently set to a minimum of 1 second while hidden)

Absent for now are any changes to the Document API and general implementation of the Page Visibility API, since the more interesting parts require knowledge of whether the user agent is minimized, OS screen locked, etc.

cc @paulrouget @jdm

This change is

highfive · 2016-03-27T19:20:04Z

Heads up! This PR modifies the following files:

@jgraham: tests/wpt/mozilla/tests/mozilla/mozbrowser/iframe_visibility.html, tests/wpt/mozilla/meta/MANIFEST.json
@KiChjang: components/script/dom/htmliframeelement.rs, components/script/timers.rs, components/script/dom/webidls/BrowserElement.webidl, components/script/dom/window.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/script_thread.rs, components/script_traits/lib.rs, components/script_traits/lib.rs

larsbergstrom · 2016-03-27T19:22:14Z

r? @jdm

emilio · 2016-03-27T21:34:42Z

Just left a few nits and questions, @jdm should still review it thoroughly.

Review status: 0 of 14 files reviewed at latest revision, 7 unresolved discussions.

components/compositing/pipeline.rs, line 363 [r1] (raw file):
nit: } else {

components/compositing/pipeline.rs, line 368 [r1] (raw file):
nit: Maybe a missing unwrap()?

components/compositing/pipeline.rs, line 369 [r1] (raw file):
nit: This return is not needed

components/script/script_thread.rs, line 1384 [r1] (raw file):
nit: } else {

components/script/script_thread.rs, line 1397 [r1] (raw file):
I'm not sure if we should panic here, there is ongoing work preventing constellation panics. Maybe a warn!() or error!() is enough?

components/script/dom/htmliframeelement.rs, line 228 [r1] (raw file):
Maybe only dispatch VisibilityChange when visibility != self.visibility.get()?

tests/wpt/mozilla/tests/mozilla/mozbrowser/iframe_visibility.html, line 34 [r1] (raw file):
nit: whitespace

Comments from the review on Reviewable.io

emilio · 2016-03-27T21:37:41Z

Review status: 0 of 14 files reviewed at latest revision, 7 unresolved discussions.

components/script/script_thread.rs, line 1397 [r1] (raw file):
Well, disregard the constellation part (this is script), but I still think we might not want to panic here. @jdm rules here :P

Comments from the review on Reviewable.io

jmr0 · 2016-03-27T21:59:20Z

components/script/dom/htmliframeelement.rs, line 228 [r1] (raw file):
This turns out to be the case in practice because of a previous check in pipeline.rs, but probably best to have that check here explicitly either way

Comments from the review on Reviewable.io

emilio · 2016-03-27T22:10:31Z

Review status: 0 of 14 files reviewed at latest revision, 7 unresolved discussions.

components/script/dom/htmliframeelement.rs, line 228 [r1] (raw file):
Sounds good, either that or a comment + debug_assert!() to make it explicit while reading this code could work :)

Comments from the review on Reviewable.io

paulrouget · 2016-03-28T07:32:23Z

I haven't tested, but can you make sure:

if requestAnimationFrame is called while not visible, it's properly called once visibility is set back to true?
visibility is recursively set in the frame tree (iframes inside iframes)

jmr0 · 2016-03-28T21:48:09Z

This is the case now -- setting it back to visible results in compositing / running callbacks
Will do

bors-servo · 2016-03-29T08:05:24Z

☔ The latest upstream changes (presumably #8641) made this pull request unmergeable. Please resolve the merge conflicts.

paulrouget · 2016-04-04T03:33:54Z

@jmr0 not sure you saw what I was saying on IRC:

jmr0: visibility is not a mozbrowser-only thing. A mozbrowser iframe happens to have methods to change the visibility, but the 'visibilitychange' event and the fact the page is slowed down si not specific the mozbrowsers.
jmr0: if mozBrowserIframe.setVisible(true) is called, the mozbrowser iframe node element will fire the BrowserAPI-event mozbrowservisbilitychange, and all the documents within this iframe will fire a w3c-event 'visibilitychange'.

jmr0 · 2016-04-04T04:05:19Z

Had a chat with @paulrouget on IRC and decided it might be better to work on the W3C Page Visibility API changes as part of a separate PR, noting that the visibility changes triggered by the Mozbrowser API should be reflected by documents within the Mozbrowser iframe (i.e. point #2 discussed above)

jmr0 · 2016-04-06T15:29:26Z

Addressed @emilio's and @paulrouget's comments

jdm · 2016-04-06T15:40:15Z

I'm really sorry that I haven't reviewed this yet; I've had a million other items vying for my attention. If I don't think I'm going to get to it this week I'll hand it off to somebody else.

paulrouget · 2016-04-06T15:58:28Z

components/script/script_thread.rs

+                }
+                let document = inner_page.document();
+                for iframe in &document.collect_descendant_iframes() {
+                    iframe.set_visible(visible);


What about doing that at the pipeline level instead, where the frame tree is available.

I mean in the constellation.

Also - a frame can have multiple pipelines (history). And you want to change the visibility of these pipelines as well (in case of navigation). So this definitively needs to happen at the constellation level.

paulrouget · 2016-04-06T16:21:25Z

I looked quickly at the PR and added some comments. Please note that I'm not a reviewer, so take my comments with a grain of salt :D

jdm · 2016-04-11T20:00:11Z

Note: I'm about halfway through reviewing these changes.

jdm · 2016-04-13T15:34:33Z

Thank you so much for tackling this, @jmr0! This is really great work, and I apologize for the delay in reviewing it. I think the general design here is correct, but there are some subtleties about what we can skip and where we should be sending messages that need to be sorted out. I also agree with the majority of the comments that @paulrouget left. Let us know if there's anything we can do to help you!

-S-awaiting-review +S-needs-code-changes

Reviewed 2 of 14 files at r1, 6 of 10 files at r2, 6 of 7 files at r3.
Review status: all files reviewed at latest revision, 26 unresolved discussions.

components/compositing/compositor.rs, line 255 [r3] (raw file):
s/current/this, since an instance of PipelineDetails never refers to more than one pipeline.

components/compositing/compositor.rs, line 728 [r3] (raw file):
I'm worried about what happens if an animation starts while a pipeline's invisible, and then the pipeline is made visible. I suspect the animation doesn't run.

components/compositing/compositor.rs, line 736 [r3] (raw file):
Same concern about above with respect to requestAnimationFrame not getting called.

components/compositing/compositor.rs, line 1632 [r3] (raw file):
nit: indent this line by one space.

components/compositing/constellation.rs, line 988 [r3] (raw file):
This should probably be part of new_pipeline instead, to ensure that we never forget to propagate visibility to new child pipelines.

components/compositing/constellation.rs, line 993 [r3] (raw file):
Let's use if let instead of map.

components/compositing/constellation.rs, line 1353 [r3] (raw file):
Let's use if let instead of map.

components/compositing/pipeline.rs, line 368 [r1] (raw file):
Error check instead per #10423.

components/compositing/pipeline.rs, line 62 [r3] (raw file):
Let's go with /// Whether this pipeline should be treated as visible for the purposes of scheduling and resource management.

components/compositing/pipeline.rs, line 361 [r3] (raw file):
We don't appear to use this return value anywhere.

components/compositing/pipeline.rs, line 366 [r3] (raw file):
We should be marking all the child pipelines of this one similarly, since it's an inherited property.

components/compositing/pipeline.rs, line 368 [r3] (raw file):
We'll need to perform error checking here instead of calling unwrap, per #10423.

components/compositing/pipeline.rs, line 372 [r3] (raw file):
The pipeline corresponding to containing_id could live in a separate script thread than the pipeline for id, so we'll need a separate message that is sent to that thread that is in charge of dispatching the mozbrowser event.

components/script/script_thread.rs, line 1397 [r1] (raw file):
Let's make this a warn!

components/script/script_thread.rs, line 1420 [r3] (raw file):
I think the current code organization makes sense, since the other caller of change_loaded_page_visibility will never deal with the incomplete loads.

components/script/dom/document.rs, line 1495 [r3] (raw file):
We can remove this if we move all the child pipeline-related code to the constellation.

components/script/dom/htmliframeelement.rs, line 247 [r3] (raw file):
Let's use window.pipeline() instead.

components/script/dom/webidls/BrowserElement.webidl, line 87 [r3] (raw file):
This should be dom.mozbrowser.enabled for future compatibility with #10081.

components/script/dom/webidls/BrowserElement.webidl, line 92 [r3] (raw file):
Same as above.

components/script_traits/lib.rs, line 127 [r3] (raw file):
I think merging these into one ChangeFrameVisibilityStatus(/* parent */ PipelineId, /* frame */ PipelineId, bool) message would make sense.

tests/wpt/mozilla/tests/mozilla/mozbrowser/iframe_visibility.html, line 19 [r3] (raw file):
This can use t.step_func.

tests/wpt/mozilla/tests/mozilla/mozbrowser/iframe_visibility.html, line 24 [r3] (raw file):
This should also use t.step_func.

Comments from Reviewable

jmr0 · 2016-06-15T01:30:38Z

Review status: all files reviewed at latest revision, 11 unresolved discussions.

tests/wpt/mozilla/tests/mozilla/mozbrowser/iframe_visibility.html, line 19 [r3] (raw file):

Previously, jdm (Josh Matthews) wrote…

This still needs to be addressed.

Ah right -- I keep thinking these are only meant to wrap functions that interact with `t`. Could you remind me why we wrap all of these?

Comments from Reviewable

jdm · 2016-06-15T23:47:47Z

Thanks for sticking with this! Please squash these into one descriptive commit :)
-S-awaiting-review +S-needs-squash

Reviewed 2 of 2 files at r11.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

tests/wpt/mozilla/tests/mozilla/mozbrowser/iframe_visibility.html, line 19 [r3] (raw file):

Previously, jmr0 (Jose Rosello) wrote…

Ah right -- I keep thinking these are only meant to wrap functions that interact with t. Could you remind me why we wrap all of these?

At minimum, it allows the test harness to notice any exceptions that might get thrown, so it's easier to debug if things go wrong. It's also required to asserts to function correctly inside of async_test.

Comments from Reviewable

jmr0 · 2016-06-16T11:08:30Z

@jdm Thanks for all your help -- definitely learned quite a bit more about Servo's architecture with this one :)

jdm · 2016-06-16T13:53:51Z

@bors-servo: r+

bors-servo · 2016-06-16T13:53:53Z

📌 Commit 2bff131 has been approved by jdm

bors-servo · 2016-06-16T13:53:57Z

⌛ Testing commit 2bff131 with merge d620ab7...

@paulrouget

Implement non-visible pipeline and iframe visibility methods This addresses #9566 and a good part of #9751, specifically: * Pipeline has a notion of visibility * IFrame setVisible/getVisible interface with IFrame's pipeline visibility * IFrame mozbrowservisibilitychange responds to changes in visibility * Pipeline visibility is used to limit animations (requestAnimationFrame does not tick animations when hidden) and to increase timer intervals (currently set to a minimum of 1 second while hidden) Absent for now are any changes to the Document API and general implementation of the Page Visibility API, since the more interesting parts require knowledge of whether the user agent is minimized, OS screen locked, etc. cc @paulrouget @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10225)

bors-servo · 2016-06-16T14:41:32Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

paulrouget · 2016-06-20T08:43:01Z

Thank you @jmr0 ! This is excellent work.

jmr0 · 2016-06-20T19:39:15Z

Thanks for all the help (and patience) @paulrouget! :)

highfive assigned larsbergstrom Mar 27, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 27, 2016

highfive assigned jdm and unassigned larsbergstrom Mar 27, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Mar 29, 2016

jdm removed the S-needs-rebase There are merge conflict errors. label Apr 6, 2016

jdm mentioned this pull request Apr 6, 2016

S-needs-rebase still doesn't get removed servo/highfive#82

Closed

paulrouget reviewed Apr 6, 2016
View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 13, 2016

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-needs-rebase There are merge conflict errors. S-awaiting-review There is new code that needs to be reviewed. labels Jun 15, 2016

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jun 15, 2016

jdm added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 15, 2016

Implement non-visible pipeline and iframe visibility methods

2bff131

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 16, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Jun 16, 2016

bors-servo merged commit 2bff131 into servo:master Jun 16, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jun 16, 2016

bors-servo mentioned this pull request Jun 16, 2016

Use unguessable pipeline ids #10808

Closed

jmr0 deleted the visibility_api branch June 16, 2016 14:47

timvandermeij mentioned this pull request Jun 16, 2016

Browser API: implement setVisible #9566

Closed

jmr0 mentioned this pull request Jun 16, 2016

Implement visibility API and the notion of non-visible pipeline #9751

Closed

paulrouget mentioned this pull request Sep 7, 2016

The pipeline visibility logic is wrong #13191

Closed

paulrouget mentioned this pull request Apr 17, 2017

[multi window] Constellation should be able to handle multiple root frame #13994

Closed

Uh oh!

Conversation

jmr0 commented Mar 27, 2016

Uh oh!

highfive commented Mar 27, 2016

Uh oh!

larsbergstrom commented Mar 27, 2016

Uh oh!

emilio commented Mar 27, 2016

Uh oh!

emilio commented Mar 27, 2016

Uh oh!

jmr0 commented Mar 27, 2016

Uh oh!

emilio commented Mar 27, 2016

Uh oh!

paulrouget commented Mar 28, 2016

Uh oh!

jmr0 commented Mar 28, 2016

Uh oh!

bors-servo commented Mar 29, 2016

Uh oh!

paulrouget commented Apr 4, 2016

Uh oh!

jmr0 commented Apr 4, 2016

Uh oh!

jmr0 commented Apr 6, 2016

Uh oh!

jdm commented Apr 6, 2016

Uh oh!

paulrouget Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

paulrouget Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

paulrouget Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

paulrouget commented Apr 6, 2016

Uh oh!

jdm commented Apr 11, 2016

Uh oh!

jdm commented Apr 13, 2016

Uh oh!

jmr0 commented Jun 15, 2016

Uh oh!

jdm commented Jun 15, 2016

Uh oh!

jmr0 commented Jun 16, 2016

Uh oh!

jdm commented Jun 16, 2016

Uh oh!

bors-servo commented Jun 16, 2016

Uh oh!

bors-servo commented Jun 16, 2016

Uh oh!

bors-servo commented Jun 16, 2016

Uh oh!

paulrouget commented Jun 20, 2016

Uh oh!

jmr0 commented Jun 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants