Skip to content

Kill browserhtml#19975

Merged
bors-servo merged 5 commits intoservo:masterfrom
paulrouget:killbhtml
Feb 13, 2018
Merged

Kill browserhtml#19975
bors-servo merged 5 commits intoservo:masterfrom
paulrouget:killbhtml

Conversation

@paulrouget
Copy link
Copy Markdown
Contributor

@paulrouget paulrouget commented Feb 7, 2018 •
edited by SimonSapin
Loading

Fixes #19971

This change is Reviewable

@highfive
Copy link
Copy Markdown

highfive commented Feb 7, 2018

Heads up! This PR modifies the following files:

  • @wafflespeanut: python/servo/command_base.py, python/servo/package_commands.py, python/servo/post_build_commands.py
  • @fitzgen: components/script/dom/mod.rs, components/script/dom/webidls/BrowserElement.webidl, components/script/dom/window.rs, components/script/dom/webidls/Window.webidl, components/script_traits/lib.rs and 13 more
  • @KiChjang: components/script/dom/mod.rs, components/script/dom/webidls/BrowserElement.webidl, components/script/dom/window.rs, components/script/dom/webidls/Window.webidl, components/script_traits/lib.rs and 13 more
  • @asajeffrey: components/script/dom/mod.rs, components/script/dom/webidls/BrowserElement.webidl, components/script/dom/window.rs, components/script/dom/webidls/Window.webidl, components/script/dom/document.rs and 13 more
  • @cbrewster: components/constellation/pipeline.rs, components/constellation/constellation.rs
  • @edunham: servo-tidy.toml

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 7, 2018
@highfive
Copy link
Copy Markdown

highfive commented Feb 7, 2018

warning Warning warning

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

@paulrouget
Copy link
Copy Markdown
Contributor Author

paulrouget commented Feb 7, 2018

Taskclusters says: "It looks like rustup is not installed."

@paulrouget paulrouget force-pushed the killbhtml branch 3 times, most recently from aa01383 to 981e8bd Compare February 7, 2018 17:40
@mbrubeck
Copy link
Copy Markdown
Contributor

mbrubeck commented Feb 7, 2018

Looks good to me, can land with r=mbrubeck if no further changes are needed.

@mbrubeck mbrubeck added S-work-in-progress and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 7, 2018
@cbrewster
Copy link
Copy Markdown
Contributor

cbrewster commented Feb 7, 2018

What is the plan for nightly builds? Will there be no browser UI for now?

@paulrouget
Copy link
Copy Markdown
Contributor Author

paulrouget commented Feb 8, 2018

What is the plan for nightly builds? Will there be no browser UI for now?

See https://groups.google.com/forum/#!topic/mozilla.dev.servo/U5zhsBLXovM

No "real" UI, but full keyboard control. I'm porting ServoShell's minimal UI: https://github.com/paulrouget/servoshell#mini-ui

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 8, 2018
@paulrouget
Copy link
Copy Markdown
Contributor Author

paulrouget commented Feb 8, 2018

@mbrubeck thanks - I added a few more comments in an early commit, and added another extra commit to bring crash reports up to the embedder.

@paulrouget paulrouget changed the title [wip] Kill browserhtml Kill browserhtml Feb 8, 2018
@paulrouget
Copy link
Copy Markdown
Contributor Author

paulrouget commented Feb 9, 2018

Fixed a typo.

@paulrouget
Copy link
Copy Markdown
Contributor Author

paulrouget commented Feb 9, 2018

@mbrubeck r?

@paulrouget paulrouget mentioned this pull request Feb 9, 2018
Merged
emilio
emilio approved these changes Feb 12, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me on that last commit with that nit fixed.

components/compositing/compositor_thread.rs
/// The load of a page has completed
LoadComplete(TopLevelBrowsingContextId),
/// A pipeline panicked.
Panic(TopLevelBrowsingContextId, String, Option<String>),
Copy link
Copy Markdown
Member

@emilio emilio Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please mention what the string args are, or use a named enum like:

Panic { id: TopLevelBCId, reason: String, backtrace: Option<String> }

@emilio
Copy link
Copy Markdown
Member

emilio commented Feb 12, 2018

@bors-servo r=mbrubeck,emilio

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Feb 12, 2018

📌 Commit 7e99ab5 has been approved by mbrubeck,emilio

@highfive 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. labels Feb 12, 2018
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Feb 12, 2018
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Feb 12, 2018

💔 Test failed - linux-rel-wpt

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Feb 12, 2018
@jdm
Copy link
Copy Markdown
Member

jdm commented Feb 12, 2018 

  ▶ OK [expected TIMEOUT] /fullscreen/api/element-request-fullscreen-not-allowed.html

  ▶ Unexpected subtest result in /fullscreen/api/element-request-fullscreen-not-allowed.html:
  │ FAIL [expected TIMEOUT] Element#requestFullscreen() when not allowed to request fullscreen
  │   → assert_equals: event.target expected Element node <div id="log"></div> but got Document node with 2 children
  │ 
  │ @http://web-platform.test:8000/fullscreen/api/element-request-fullscreen-not-allowed.html:11:5
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1494:20
  └ Test.prototype.step_func_done/<@http://web-platform.test:8000/resources/testharness.js:1534:17

  ▶ Unexpected subtest result in /html/semantics/embedded-content/the-iframe-element/iframe-allowfullscreen.html:
  │ FAIL [expected PASS] iframe-allowfullscreen
  │   → assert_true: Top level document has fullscreen enabled flag set expected true got false
  │ 
  │ @http://web-platform.test:8000/html/semantics/embedded-content/the-iframe-element/iframe-allowfullscreen.html:22:5
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1494:20
  │ async_test@http://web-platform.test:8000/resources/testharness.js:528:13
  └ @http://web-platform.test:8000/html/semantics/embedded-content/the-iframe-element/iframe-allowfullscreen.html:13:3

  ▶ Unexpected subtest result in /html/semantics/embedded-content/the-iframe-element/iframe-allowfullscreen.html:
  │ FAIL [expected PASS] iframe-noload-noallowfullscreen
  │   → assert_false: Fullscreen should not be enabled without allowfullscreen attribute expected false got true
  │ 
  │ @http://web-platform.test:8000/html/semantics/embedded-content/the-iframe-element/iframe-allowfullscreen.html:44:7
  │ test_allowfullscreen_noload@http://web-platform.test:8000/html/semantics/embedded-content/the-iframe-element/iframe-allowfullscreen.html:38:5
  │ @http://web-platform.test:8000/html/semantics/embedded-content/the-iframe-element/iframe-allowfullscreen.html:43:5
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1494:20
  │ test@http://web-platform.test:8000/resources/testharness.js:511:9
  └ @http://web-platform.test:8000/html/semantics/embedded-content/the-iframe-element/iframe-allowfullscreen.html:42:3

@paulrouget
Copy link
Copy Markdown
Contributor Author

paulrouget commented Feb 13, 2018

The problem is in the window.is_top_level function. Returning parent_info.is_some() instead of parent_info.is_none().

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Feb 13, 2018
@paulrouget
Copy link
Copy Markdown
Contributor Author

paulrouget commented Feb 13, 2018

@bors-servo r=mbrubeck,emilio

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Feb 13, 2018

📌 Commit ee25413 has been approved by mbrubeck,emilio

@highfive 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. labels Feb 13, 2018
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Feb 13, 2018

⌛ Testing commit ee25413 with merge b1d3d6f...

bors-servo pushed a commit that referenced this pull request Feb 13, 2018
Auto merge of #19975 - paulrouget:killbhtml, r=mbrubeck,emilio
b1d3d6f 
Kill browserhtml

Fixes #19971

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19975)
<!-- Reviewable:end -->
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Feb 13, 2018

💔 Test failed - linux-rel-css

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Feb 13, 2018
@paulrouget
Copy link
Copy Markdown
Contributor Author

paulrouget commented Feb 13, 2018

@bors-servo retry

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Feb 13, 2018

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only linux-rel-css...

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Feb 13, 2018

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: mbrubeck,emilio
Pushing b1d3d6f to master...

@bors-servo bors-servo merged commit ee25413 into servo:master Feb 13, 2018
This was referenced Feb 13, 2018
Closed
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emilio emilio emilio approved these changes

Assignees

@mbrubeck mbrubeck

Labels

S-tests-failed The changes caused existing tests to fail. S-work-in-progress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@paulrouget @highfive @mbrubeck @cbrewster @emilio @bors-servo @jdm