Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support WindowProxy return values in bindings #11214

Merged
merged 1 commit into from Jun 10, 2016

Conversation

@farodin91
Copy link
Contributor

farodin91 commented May 16, 2016

Thank you for contributing to Servo! Please replace each [ ] by [X] when the step is complete, and replace __ with appropriate data:

  • ./mach build -d does not report any errors
  • ./mach test-tidy --faster does not report any errors
  • These changes fix #10965 (github issue number if applicable).

Either:

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

Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.


This change is Reviewable

@highfive
Copy link

highfive commented May 16, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/bindings/codegen/parser/WebIDL.py, components/script/dom/mod.rs, components/script/dom/webidls/WindowProxy.webidl, components/script/dom/windowproxy.rs
@highfive
Copy link

highfive commented May 16, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@farodin91
Copy link
Contributor Author

farodin91 commented May 16, 2016

@jdm I have no idea to fix this, any tipp for me? Thanks!

windowproxy.rs:12:1: 1:1 error: cannot borrow immutable borrowed content as mutable
windowproxy.rs:12:1: 1:1 note: in this expansion of #[_generate_reflector] (windowproxy.rs)

(Complete output)

@jdm
Copy link
Member

jdm commented May 16, 2016

Rather than answer the question that was asked, I'm going to suggest that this isn't the correct way to solve this. The BrowsingContext type that already exists is the type in Servo that corresponds to WindowProxy; we shouldn't have to create a separate interface and type for this work.

@farodin91
Copy link
Contributor Author

farodin91 commented May 17, 2016

I'll start over. I miss understood something.

@farodin91
Copy link
Contributor Author

farodin91 commented May 17, 2016

@jdm
My idea:

  1. use this function https://github.com/servo/servo/blob/master/components/script/dom/browsingcontext.rs#L119.
  2. uncomment this lines https://github.com/servo/servo/blob/master/components/script/dom/webidls/Window.webidl#L9-L10 & comment the following
  3. adding support of this WindowProxy in WebIDL like ArrayBufferView in this file https://github.com/servo/servo/blob/master/components/script/dom/bindings/codegen/parser/WebIDL.py
    • Possible as IDLWrapperType or IDLBuiltinType
  4. adapt the CodeGen.py

Any tips or additions?

@jdm
Copy link
Member

jdm commented May 17, 2016

Instead of teaching the WebIDL parser about WindowProxy, I would prefer to try to teach our specific bindings about the WindowProxy -> BrowsingContext translation. The window_proxy method on BrowsingContext delegates to the object's reflector (self.reflector().get_jsobject()), so our code generation should be able to use to BrowsingContext values already since it understands objects with reflectors. We should try modifying Bindings.conf to teach it that a WindowProxy WebIDL type should correspond to a BrowsingContext Rust type.

@farodin91 farodin91 force-pushed the farodin91:windowproxy branch from 7bc9534 to f20bb5c May 17, 2016
@highfive
Copy link

highfive commented May 17, 2016

New code was committed to pull request.

@farodin91
Copy link
Contributor Author

farodin91 commented May 17, 2016

Currently, i'm looking to prevent generating Binding for the WebIDL type, like ChildNode.
After find a solution. I'm will teach that a WindowProxy WebIDL type should correspond to a BrowsingContext Rust type.

@highfive
Copy link

highfive commented May 17, 2016

New code was committed to pull request.

1 similar comment
@highfive
Copy link

highfive commented May 19, 2016

New code was committed to pull request.

@bors-servo
Copy link
Contributor

bors-servo commented May 27, 2016

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

@asajeffrey
Copy link
Member

asajeffrey commented Jun 1, 2016

Is this still active?

@farodin91
Copy link
Contributor Author

farodin91 commented Jun 1, 2016

Yes. During the last weeks i completed some tasks for my education.

@asajeffrey
Copy link
Member

asajeffrey commented Jun 1, 2016

Cool!

@farodin91
Copy link
Contributor Author

farodin91 commented Jun 1, 2016

@jdm r?

@jdm jdm assigned jdm and unassigned asajeffrey Jun 2, 2016
@jdm
Copy link
Member

jdm commented Jun 2, 2016

I reviewed your changes and made some of my own. I'm going to make a pull request to your branch when I finish fixing the merge conflicts, since the HTMLIFrameElement changes took me by surprise.

@jdm
Copy link
Member

jdm commented Jun 2, 2016

Opened farodin91#1 with my changes.

@highfive
Copy link

highfive commented Jun 9, 2016

New code was committed to pull request.

unscopable
@jdm jdm changed the title [WIP] Support WindowProxy return values in bindings #10965 Support WindowProxy return values in bindings Jun 10, 2016
@jdm
Copy link
Member

jdm commented Jun 10, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 10, 2016

📌 Commit fda0119 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jun 10, 2016

Testing commit fda0119 with merge 919df00...

bors-servo added a commit that referenced this pull request Jun 10, 2016
Support WindowProxy return values in bindings

Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data:
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy --faster` does not report any errors
- [x] These changes fix #10965 (github issue number if applicable).

Either:
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.

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

bors-servo commented Jun 10, 2016

💔 Test failed - linux-rel

@highfive
Copy link

highfive commented Jun 10, 2016

  ▶ CRASH [expected OK] /html/semantics/embedded-content/the-iframe-element/move_iframe_in_dom_03.html
  │ 
  │ Xlib:  extension &#34;XFree86-VidModeExtension&#34; missing on display &#34;:0&#34;.
  │ ERROR:constellation::constellation: Panic: resize sent to nonexistent pipeline
  │ ERROR:constellation::constellation: Backtrace:
  │ frame #0  - 0x00007f08202bf66d - backtrace::backtrace::trace::hccde8df28b4db2a2
  │ frame #1  - 0x00007f08202bf5f5 - backtrace::capture::Backtrace::new::h42f95930bb8c5ee8
  │ frame #2  - 0x00007f081f315dc9 - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::h89adae1a802be550
  │ frame #3  - 0x00007f08202b0ea8 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::he2b22674ad1748f3
  │ frame #4  - 0x00007f082057b7cc - std::panicking::rust_panic_with_hook::h983af77c1a2e581b
  │ frame #5  - 0x00007f081ecef78f - std::panicking::begin_panic::h0bf39f6d43ab9349
  │ frame #6  - 0x00007f081f36b2d0 - script::script_thread::ScriptThread::handle_msgs::_$u7b$$u7b$closure$u7d$$u7d$::he41aa177b24dc4f9
  │ frame #7  - 0x00007f081f34e6c7 - script::script_thread::ScriptThread::handle_msgs::h1e1abab71191c950
  │ frame #8  - 0x00007f081f314447 - std::panicking::try::call::h78dcd5319adf08fa
  │ frame #9  - 0x00007f082059fdcb - __rust_try
  │ frame #10 - 0x00007f082059fd6e - __rust_maybe_catch_panic
  │ frame #11 - 0x00007f081f31572d - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::hce043844655ea6a9
  │ frame #12 - 0x00007f0820593d94 - std::sys::thread::Thread::new::thread_start::h9c883b6d445ece46
  │ frame #13 - 0x00007f081c3fd181 - start_thread
  │ frame #14 - 0x00007f081bf1447c - __clone
  │ frame #15 - 0x0000000000000000 - &lt;unknown&gt;
  │ 
  └ ERROR:constellation::constellation: Pipeline failed in hard-fail mode.  Crashing!

  ▶ CRASH [expected OK] /html/semantics/embedded-content/the-iframe-element/change_parentage.html
  │ 
  │ Xlib:  extension &#34;XFree86-VidModeExtension&#34; missing on display &#34;:0&#34;.
  │ ERROR:constellation::constellation: Panic: called `Option::unwrap()` on a `None` value
  │ ERROR:constellation::constellation: Backtrace:
  │ frame #0  - 0x00007f71d010466d - backtrace::backtrace::trace::hccde8df28b4db2a2
  │ frame #1  - 0x00007f71d01045f5 - backtrace::capture::Backtrace::new::h42f95930bb8c5ee8
  │ frame #2  - 0x00007f71cf15adc9 - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::h89adae1a802be550
  │ frame #3  - 0x00007f71d00f5ea8 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::he2b22674ad1748f3
  │ frame #4  - 0x00007f71d03c07cc - std::panicking::rust_panic_with_hook::h983af77c1a2e581b
  │ frame #5  - 0x00007f71d03dabd1 - std::panicking::begin_panic::he426e15a3766089a
  │ frame #6  - 0x00007f71d03c203a - std::panicking::begin_panic_fmt::hdddb415186c241e7
  │ frame #7  - 0x00007f71d03dab6e - rust_begin_unwind
  │ frame #8  - 0x00007f71d041116f - core::panicking::panic_fmt::hf4e16cb7f0d41a25
  │ frame #9  - 0x00007f71d0411448 - core::panicking::panic::h907815f47e914305
  │ frame #10 - 0x00007f71cef7cc81 - script::dom::event::Event::new_uninitialized::h05ae188a870e92b9
  │ frame #11 - 0x00007f71cef84b3b - script::dom::event::Event::new::h7a12c706eaf845bc
  │ frame #12 - 0x00007f71cefcb95c - script::dom::eventtarget::EventTarget::fire_event::h0d5b30039f5820d2
  │ frame #13 - 0x00007f71cf1cd7fa - script::task_source::dom_manipulation::DOMManipulationTask::handle_task::h0ea31f97e475cb7a
  │ frame #14 - 0x00007f71cf1652c5 - script::script_thread::ScriptThread::handle_msg_from_script::h47e979ae1e7fb676
  │ frame #15 - 0x00007f71cf1b1d9f - script::script_thread::ScriptThread::handle_msgs::_$u7b$$u7b$closure$u7d$$u7d$::hb50cf7fd97943b65
  │ frame #16 - 0x00007f71cf19a0e9 - script::script_thread::ScriptThread::handle_msgs::h1e1abab71191c950
  │ frame #17 - 0x00007f71cf159447 - std::panicking::try::call::h78dcd5319adf08fa
  │ frame #18 - 0x00007f71d03e4dcb - __rust_try
  │ frame #19 - 0x00007f71d03e4d6e - __rust_maybe_catch_panic
  │ frame #20 - 0x00007f71cf15a72d - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::hce043844655ea6a9
  │ frame #21 - 0x00007f71d03d8d94 - std::sys::thread::Thread::new::thread_start::h9c883b6d445ece46
  │ frame #22 - 0x00007f71cc242181 - start_thread
  │ frame #23 - 0x00007f71cbd5947c - __clone
  │ frame #24 - 0x0000000000000000 - &lt;unknown&gt;
  │ 
  └ ERROR:constellation::constellation: Pipeline failed in hard-fail mode.  Crashing!
@jdm
Copy link
Member

jdm commented Jun 10, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 10, 2016

Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows are reusable. Rebuilding only linux-rel...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 10, 2016

💔 Test failed - linux-rel

@highfive
Copy link

highfive commented Jun 10, 2016

  ▶ CRASH [expected OK] /html/semantics/embedded-content/the-iframe-element/change_parentage.html
  │ 
  │ Xlib:  extension &#34;XFree86-VidModeExtension&#34; missing on display &#34;:0&#34;.
  │ ERROR:constellation::constellation: Panic: ScriptThread: received an event message for a layout channel that is not associated with this script thread.This is a bug.
  │ ERROR:constellation::constellation: Backtrace:
  │ frame #0  - 0x00007fe4af6564fd - backtrace::backtrace::trace::hccde8df28b4db2a2
  │ frame #1  - 0x00007fe4af656485 - backtrace::capture::Backtrace::new::h42f95930bb8c5ee8
  │ frame #2  - 0x00007fe4ae6acc39 - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::h89adae1a802be550
  │ frame #3  - 0x00007fe4af647d38 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::he2b22674ad1748f3
  │ frame #4  - 0x00007fe4af91265c - std::panicking::rust_panic_with_hook::h983af77c1a2e581b
  │ frame #5  - 0x00007fe4af92ca61 - std::panicking::begin_panic::he426e15a3766089a
  │ frame #6  - 0x00007fe4af913eca - std::panicking::begin_panic_fmt::hdddb415186c241e7
  │ frame #7  - 0x00007fe4af92c9fe - rust_begin_unwind
  │ frame #8  - 0x00007fe4af962fff - core::panicking::panic_fmt::hf4e16cb7f0d41a25
  │ frame #9  - 0x00007fe4af96a6b4 - core::option::expect_failed::hdb92832549f56a85
  │ frame #10 - 0x00007fe4ae6b9c71 - script::script_thread::ScriptThread::handle_msg_from_script::h47e979ae1e7fb676
  │ frame #11 - 0x00007fe4ae703c0f - script::script_thread::ScriptThread::handle_msgs::_$u7b$$u7b$closure$u7d$$u7d$::hb50cf7fd97943b65
  │ frame #12 - 0x00007fe4ae6ebf59 - script::script_thread::ScriptThread::handle_msgs::h1e1abab71191c950
  │ frame #13 - 0x00007fe4ae6ab2b7 - std::panicking::try::call::h78dcd5319adf08fa
  │ frame #14 - 0x00007fe4af936c5b - __rust_try
  │ frame #15 - 0x00007fe4af936bfe - __rust_maybe_catch_panic
  │ frame #16 - 0x00007fe4ae6ac59d - _&lt;F as alloc..boxed..FnBox&lt;A&gt;&gt;::call_box::hce043844655ea6a9
  │ frame #17 - 0x00007fe4af92ac24 - std::sys::thread::Thread::new::thread_start::h9c883b6d445ece46
  │ frame #18 - 0x00007fe4ab794183 - start_thread
  │ frame #19 - 0x00007fe4ab2ab37c - clone
  │ frame #20 - 0x0000000000000000 - &lt;unknown&gt;
  │ 
  └ ERROR:constellation::constellation: Pipeline failed in hard-fail mode.  Crashing!
@farodin91
Copy link
Contributor Author

farodin91 commented Jun 10, 2016

@jdm
Copy link
Member

jdm commented Jun 10, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 10, 2016

Testing commit fda0119 with merge 08a55e2...

bors-servo added a commit that referenced this pull request Jun 10, 2016
Support WindowProxy return values in bindings

Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data:
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy --faster` does not report any errors
- [x] These changes fix #10965 (github issue number if applicable).

Either:
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.

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

bors-servo commented Jun 10, 2016

@bors-servo bors-servo merged commit fda0119 into servo:master Jun 10, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Jun 10, 2016
4 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.