Add support for fullscreen #10102 by farodin91 · Pull Request #13489 · servo/servo

farodin91 · 2016-09-28T21:15:48Z

I'm start working on fullscreen support.
@jdm Should be the entry_point in ScriptReflow a Option if fullscreen is enabled or point on the entry_node? For example the RootNode.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Implement the Fullscreen API #10102 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-09-28T21:15:52Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/webidls/Document.webidl, components/script/dom/document.rs, components/script/dom/webidls/Element.webidl, components/script_layout_interface/message.rs, components/script/dom/element.rs

highfive · 2016-09-28T21:15:53Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

jdm · 2016-09-28T21:18:25Z

I suspect an Option makes the most sense; I expect we still want to reflow the rest of the page even if an element is fullscreen.

emilio · 2016-09-29T10:46:27Z

Do we plan to land this without implementing the per-platform bits? (Not a huge deal, just curious).

paulrouget · 2016-09-29T11:52:05Z

Is the plan to make the DOM element full window, and than send a message to the glutin window to go fullscreen?

jdm · 2016-09-29T11:53:31Z

Yep.

farodin91 · 2016-10-02T16:18:36Z

@jdm For which purpose exists target_element ?
https://github.com/servo/servo/blob/master/components/script/dom/document.rs#L1966

jdm · 2016-10-02T16:20:42Z

@farodin91 https://html.spec.whatwg.org/multipage/browsers.html#target-element

farodin91 · 2016-10-02T16:21:39Z

Thank you

farodin91 · 2016-10-02T17:04:31Z

@jdm How to set a Pseudo-Class (:fullscreen)?

jdm · 2016-10-02T17:38:42Z

I recommend looking at how the :target pseudoclass is implemented (look at target_element).

jdm

Good start!

jdm · 2016-10-04T20:32:59Z

components/script/dom/window.rs

        let stylesheets_changed = document.get_and_reset_stylesheets_changed_since_reflow();

+        // fullscreen actions
+        let entry_node = match self.Document().GetFullscreenElement() {


let entry_node = self.Document() .GetFullScreenElement() .map(|e| e.upcast::<Node>().to_trusted_node_address());

jdm · 2016-10-04T20:33:10Z

components/script/dom/window.rs

        let document = self.Document();
        let stylesheets_changed = document.get_and_reset_stylesheets_changed_since_reflow();

+        // fullscreen actions


I don't think this comment adds anything to the code.

jdm · 2016-10-04T20:33:31Z

components/script_layout_interface/message.rs

    pub reflow_info: Reflow,
    /// The document node.
    pub document: TrustedNodeAddress,
+    /// The render entry point need for fullscreen.


s/need/needed/

jdm · 2016-10-04T20:34:16Z

components/script/dom/element.rs

    }
+
+    #[allow(unrooted_must_root)]
+    fn RequestFullscreen(&self) -> Rc<Promise> {


There's a missing specification link comment.

jdm · 2016-10-04T20:36:48Z

components/script/dom/element.rs

+    fn RequestFullscreen(&self) -> Rc<Promise> {
+        let doc = document_from_node(self);
+        doc.set_fullscreen_element(Some(self));
+        Promise::new(self.global().r())


This promise needs to be resolved when the fullscreen status has changed. We can probably use this instead:

let global = self.global().r(); Promise::Resolve(global, global.get_cx(), HandleValue::undefined())

jdm · 2016-10-04T20:37:09Z

components/script/dom/document.rs

    // https://html.spec.whatwg.org/multipage/#documentandelementeventhandlers
    document_and_element_event_handlers!();
+
+    event_handler!(fullscreenerror, GetOnfullscreenerror, SetOnfullscreenerror);


Specification links are missing for these changes.

jdm · 2016-10-04T20:37:40Z

components/script/dom/document.rs

+    #[allow(unrooted_must_root)]
+    fn ExitFullscreen(&self) -> Rc<Promise> {
+        self.set_fullscreen_element(None);
+        Promise::new(self.global().r())


Same promise comment. set_fullscreen_element should probably return the Promise value so we don't duplicate the code.

bors-servo · 2016-10-07T14:10:17Z

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

farodin91 · 2016-10-12T18:16:11Z

@jdm i read the spec a bit more, due to spec i shouldn't set a new entry in reflow.
https://fullscreen.spec.whatwg.org/#user-agent-level-style-sheet-defaults. I think this would be better.
Any preference?

jdm · 2016-10-12T18:38:34Z

Oh, that's interesting indeed. If we can do it entirely with CSS that would definitely be preferable.

bors-servo · 2016-11-15T20:40:46Z

📌 Commit e9050b6 has been approved by jdm

jdm · 2016-11-15T20:41:16Z

@bors-servo: r-
Sorry, the manifest is still not up to date.

farodin91 · 2016-11-15T21:10:28Z

@jdm Updated

KiChjang · 2016-11-15T21:13:34Z

@bors-servo r=jdm

bors-servo · 2016-11-15T21:13:34Z

📌 Commit a9e5227 has been approved by jdm

bors-servo · 2016-11-16T11:10:10Z

⌛ Testing commit a9e5227 with merge b07f270...

bors-servo · 2016-11-16T11:50:32Z

💔 Test failed - linux-rel-wpt

farodin91 · 2016-11-16T12:52:34Z

@jdm ?

FAIL [expected PASS] /_mozilla/css/incremental_trailing_whitespace_a.html
└ → /_mozilla/css/incremental_trailing_whitespace_a.html da54041086f2975f8e3c776d2283ad6609e6862a
/_mozilla/css/incremental_trailing_whitespace_ref.html f5d0147c8bddbdc772a7e5e86c7c9e433fcd486b
Testing da54041086f2975f8e3c776d2283ad6609e6862a == f5d0147c8bddbdc772a7e5e86c7c9e433fcd486b

emilio · 2016-11-16T13:11:05Z

@bors-servo retry #10473

@farodin91 that's a known intermittent, no worries :)

farodin91 · 2016-11-16T13:30:41Z

Stop bors servo that's not the only issue

emilio · 2016-11-16T13:41:22Z

@bors-servo r-

Right, you need to update a lot more expectations.

bors-servo · 2016-11-16T23:59:32Z

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

farodin91 · 2016-11-17T16:29:43Z

@jdm Any tipp to update cargo.lock after this commit #14172? The following command doesn't work correct: ./mach update-cargo -p html5ever-atoms

farodin91 · 2016-11-22T22:41:50Z

@jdm This should run.

jdm · 2016-11-23T00:03:59Z

tests/wpt/harness/wptrunner/update/.wpt-update.lock

@@ -0,0 +1,308 @@
+ccopy_reg


This file should be removed from this commit.

farodin91 · 2016-11-28T09:28:42Z

Resolved

bors-servo · 2016-11-28T12:06:13Z

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

jdm · 2016-11-28T15:14:07Z

components/script/dom/element.rs

        if let Some(ref value) = *self.id_attribute.borrow() {
            let doc = document_from_node(self);
            doc.unregister_named_element(self, value.clone());
+            let fullscreen = doc.GetFullscreenElement();


Oh, I just noticed that this comment wasn't addressed.

jdm

Sorry, I noticed one last problem :( I've filed #14500 about making this impossible in the future.

jdm · 2016-12-08T20:12:26Z

components/script/dom/bindings/refcounted.rs

 /// in asynchronous operations. The underlying DOM object is guaranteed to live at least
 /// as long as the last outstanding `TrustedPromise` instance. These values cannot be cloned,
 /// only created from existing Rc<Promise> values.
+#[derive(Clone)]


This is not safe. We need to make a new TrustedPromise value from the Rc<Promise> value instead.

jdm · 2016-12-08T20:17:08Z

components/script/dom/element.rs

+        let document = document_from_node(element.r());
+        // JSAutoCompartment needs to be manually made.
+        // Otherwise, Servo will crash.
+        let promise = self.promise.clone().root();


Why we can't use self.promise.root() here?

I don't like my new solution.

bors-servo · 2016-12-08T21:30:40Z

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

jdm · 2016-12-09T17:52:31Z

@bors-servo: r+

bors-servo · 2016-12-09T17:52:32Z

📌 Commit 55f0e56 has been approved by jdm

bors-servo · 2016-12-09T17:52:35Z

⌛ Testing commit 55f0e56 with merge 8b69e73...

bors-servo · 2016-12-09T18:40:45Z

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

foolip · 2017-02-09T19:07:40Z

Could @farodin91 or someone else take a look at web-platform-tests/wpt#4394 and my comment in 8b69e73?

highfive assigned jdm Sep 28, 2016

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

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 Sep 28, 2016

farodin91 force-pushed the fullscreen branch from a40a7bd to 9db3134 Compare October 2, 2016 17:09

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 Oct 2, 2016

jdm requested changes Oct 4, 2016

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-fails-tidy `./mach test-tidy` reported errors. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 4, 2016

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

farodin91 force-pushed the fullscreen branch from 9db3134 to c4a1849 Compare October 8, 2016 13:03

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 Oct 8, 2016

farodin91 force-pushed the fullscreen branch from c4a1849 to 7aa9a93 Compare October 8, 2016 23:32

jdm requested changes Nov 23, 2016

View reviewed changes

jdm requested changes Nov 28, 2016

View reviewed changes

jdm requested changes Dec 8, 2016

View reviewed changes

Add support for fullscreen servo#10102

55f0e56

This was referenced Dec 9, 2016

Bump rust-openssl, add support for OpenSSL 1.1.0 [WIP] #14484

Closed

Expose Quirks Mode information in the layout data and code #14430

Merged

Implement discarding Document objects to reclaim space. #14312

Merged

jdm mentioned this pull request Feb 9, 2017

Revert changes to document-onfullscreenerror.html web-platform-tests/wpt#4778

Merged

Uh oh!

Conversation

farodin91 commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Sep 28, 2016

Uh oh!

highfive commented Sep 28, 2016

Uh oh!

jdm commented Sep 28, 2016

Uh oh!

emilio commented Sep 29, 2016

Uh oh!

paulrouget commented Sep 29, 2016

Uh oh!

jdm commented Sep 29, 2016

Uh oh!

farodin91 commented Oct 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdm commented Oct 2, 2016

Uh oh!

farodin91 commented Oct 2, 2016

Uh oh!

farodin91 commented Oct 2, 2016

Uh oh!

jdm commented Oct 2, 2016

Uh oh!

jdm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors-servo commented Oct 7, 2016

Uh oh!

farodin91 commented Oct 12, 2016

Uh oh!

jdm commented Oct 12, 2016

Uh oh!

bors-servo commented Nov 15, 2016

Uh oh!

jdm commented Nov 15, 2016

Uh oh!

farodin91 commented Nov 15, 2016

Uh oh!

KiChjang commented Nov 15, 2016

Uh oh!

bors-servo commented Nov 15, 2016

Uh oh!

bors-servo commented Nov 16, 2016

Uh oh!

bors-servo commented Nov 16, 2016

Uh oh!

farodin91 commented Nov 16, 2016

Uh oh!

emilio commented Nov 16, 2016

Uh oh!

farodin91 commented Nov 16, 2016

Uh oh!

emilio commented Nov 16, 2016

Uh oh!

bors-servo commented Nov 16, 2016

Uh oh!

farodin91 commented Nov 17, 2016

Uh oh!

farodin91 commented Nov 22, 2016

Uh oh!

Choose a reason for hiding this comment

farodin91 commented Sep 28, 2016 •

edited

Loading

farodin91 commented Oct 2, 2016 •

edited

Loading