DOMMatrix and DOMMatrixReadOnly #12202

peterjoel · 2016-07-03T20:29:04Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #8509.

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

This change is

highfive · 2016-07-03T20:29:08Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/dommatrixreadonly.rs, components/script/dom/webidls/DOMMatrix.webidl, components/script/dom/dommatrix.rs, components/script/dom/webidls/DOMMatrixReadOnly.webidl, components/script/dom/mod.rs, components/script/dom/bindings/trace.rs

highfive · 2016-07-03T20:29:09Z

Warning

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

peterjoel · 2016-07-03T20:33:06Z

components/script/dom/dommatrix.rs

+        }
+
+        // https://drafts.fxtf.org/geometry-1/#dom-dommatrix-multiplyself
+        fn MultiplySelf(&self, other:&DOMMatrixInit) -> Root<DOMMatrix> {


@nox The various methods on DOMMatrixReadOnly, that are actually really there to be used by DOMMatrix, all now have a return value. However, this is a DOMMatrixReadOnly because they can only return self. I can't see a clean way to make this value be used by the DOMMatrix methods. Currently the implementation is technically correct because these DOM methods return the current object, but the implementations (e.g. multiply_self) are a bit misleading because they return a value that is never used.

I have tried a few things, but advice would be welcome in how to make that better.

Oh right damn it. Please remove the returning values from the DOMMatrixReadOnly helpers and keep only Root::from_ref(&*self) in their DOMMatrix call sites.

Ok. But if I'm going to add matrix and is_2d accessors to DOMMatrix anyway, then I can just move the contents of the *Self methods to DOMMatrixReadOnly and get rid of the *_self methods from DOMMatrix.

It will be overall less code, and will keep each of the sets of steps together nicely.

No you can't, because the matrix method would just return a Matrix4D<f32>, not a Ref<Matrix4D<_>> or a RefMut<Matrix4D<_>>.

It is no worry to have mutating methods on DOMMatrixReadOnly, the only thing that matters is what gets exposed to JS.

Ok, fair enough. I can document the steps like this:

// https://drafts.fxtf.org/geometry-1/#dom-dommatrix-multiplyself fn MultiplySelf(&self, other:&DOMMatrixInit) -> Root<DOMMatrix> { // Steps 1-3. self.upcast::<DOMMatrixReadOnly>().multiply_self(other); // Step 4. Root::from_ref(&self) }

With the remaining steps listed inside multiply_self.

peterjoel · 2016-07-03T20:54:01Z

components/script/dom/dommatrixreadonly.rs

+            })
+    }
+
+    fn to_DOMMatrix(&self) -> Root<DOMMatrix> {


@nox In your previous feedback, you suggested that I move this to DOMMatrix and rename it from_readonly. However, I have struggled to find a way to make that work. Here are my comments from the previous discussion:

Are you suggesting an implementation like one of these?

pub fn from_readonly(ro: &DOMMatrixReadOnly) -> Root<DOMMatrix> { Self::new(ro.global().r(), ro.is2D.get(), ro.matrix.borrow().clone()) }

I'm unsure about the global ref. I would also have to make the DOMMatrixReadOnly fields public, which isn't permitted without adding a pragma.

I tried explicitly downcasting, but I'd still have to move the value, which can't be done since it's borrowed already and DOMMatrixReadOnly cannot be cloned.

pub fn from_readonly(ro: &DOMMatrixReadOnly) -> Root<DOMMatrix> { reflect_dom_object(box ro.downcast().unwrap(), ro.global().r(), Wrap) }

I also tried skipping the root and letting the consumer deal with that, but it doesn't work because parent can't be a reference, so it's the same problem.

pub fn from_readonly(ro: &DOMMatrixReadOnly) -> DOMMatrix { DOMMatrix { parent: ro } }

Another way would be to add public accessors to DOMMatrixReadOnly for matrix and is2D, so that a new one can be constructed from those parts inside from_readonly.

Another way would be to add public accessors to DOMMatrixReadOnly for matrix and is2D, so that a new one can be constructed from those parts inside from_readonly.

That is what I had in mind. Just add two new methods on DOMMatrixReadOnly, matrix and is_2d.

Ms2ger · 2016-07-04T07:33:19Z

@bors-servo try

bors-servo · 2016-07-04T07:33:22Z

⌛ Trying commit 9ee175d with merge 17a316c...


        Auto merge of #12202 - peterjoel:issue_8509, r=<try>

DOMMatrix and DOMMatrixReadOnly  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #8509.  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12202)

bors-servo · 2016-07-04T08:16:40Z

💔 Test failed - mac-rel-css

nox · 2016-07-04T08:19:36Z

▶ CRASH [expected OK] /geometry-1_dev/html/DOMMatrix-003.htm
  │ 
  │ ERROR:constellation::constellation: Panic: DOMRefCell<T> already borrowed
  │ ERROR:constellation::constellation: Backtrace:
  │ frame #0  - 0x0000000108f7752e - backtrace::backtrace::trace::hccde8df28b4db2a2
  │ frame #1  - 0x0000000108f774b1 - backtrace::capture::Backtrace::new::h42f95930bb8c5ee8
  │ frame #2  - 0x00000001081cb396 - _<F as alloc..boxed..FnBox<A>>::call_box::h34a5dd0a6120609b
  │ frame #3  - 0x0000000108eeb937 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::hea7a9e6e9237fdc4
  │ frame #4  - 0x000000010915f6a4 - std::panicking::rust_panic_with_hook::h983af77c1a2e581b
  │ frame #5  - 0x00000001091785e6 - std::panicking::begin_panic::he426e15a3766089a
  │ frame #6  - 0x0000000109160ce8 - std::panicking::begin_panic_fmt::hdddb415186c241e7
  │ frame #7  - 0x000000010917823f - rust_begin_unwind
  │ frame #8  - 0x000000010919f0c0 - core::panicking::panic_fmt::hf4e16cb7f0d41a25
  │ frame #9  - 0x00000001091a6b65 - core::option::expect_failed::hdb92832549f56a85
  │ frame #10 - 0x0000000107b217bc - _<script..dom..dommatrixreadonly..DOMMatrixReadOnly as script..dom..bindings..codegen..Bindings..DOMMatrixReadOnlyBinding..DOMMatrixReadOnlyMethods>::Scale::hf5ec86a37c617539
  │ frame #11 - 0x0000000107b21738 - std::panicking::try::call::h4b2bccbcd091369e
  │ frame #12 - 0x000000010917b2cb - __rust_try
  │ frame #13 - 0x000000010917b265 - __rust_maybe_catch_panic
  │ frame #14 - 0x0000000107b213b5 - script::dom::bindings::codegen::Bindings::DOMMatrixReadOnlyBinding::scale::h93c950aecb9ba011
  │ frame #15 - 0x000000010887bee0 - CallJitMethodOp
  │ frame #16 - 0x0000000107a0a4fa - script::dom::bindings::utils::generic_call::h5239024fbe275c4d
  │ frame #17 - 0x0000000108c819e3 - 2js6InvokeEP9JSContextRKN2JS8CallArgsENS_14MaybeConstruct
  │ frame #18 - 0x0000000108c94883 - _ZL9InterpretP9JSContextRN2js8RunStateE
  │ frame #19 - 0x0000000108c89c5c - 2js9RunScriptEP9JSContextRNS_8RunState
  │ frame #20 - 0x0000000108c81d2a - 2js6InvokeEP9JSContextRKN2JS8CallArgsENS_14MaybeConstruct
  │ frame #21 - 0x0000000108bb862e - 2js9fun_applyEP9JSContextjPN2JS5Value
  │ frame #22 - 0x0000000108c819e3 - 2js6InvokeEP9JSContextRKN2JS8CallArgsENS_14MaybeConstruct
  │ frame #23 - 0x0000000108c94883 - _ZL9InterpretP9JSContextRN2js8RunStateE
  │ frame #24 - 0x0000000108c89c5c - 2js9RunScriptEP9JSContextRNS_8RunState
  │ frame #25 - 0x0000000108c9bbd6 - _ZN2js13ExecuteKernelEP9JSContextN2JS6HandleIP8JSScriptEER8JSObjectRKNS2_5ValueENS_16AbstractFramePtrEPS9_
  │ frame #26 - 0x0000000108c9bd1e - 2js7ExecuteEP9JSContextN2JS6HandleIP8JSScriptEER8JSObjectPNS2_5Value
  │ frame #27 - 0x0000000108b8ed9e - _ZL8EvaluateP9JSContextN2JS6HandleIP8JSObjectEENS2_IPN2js11StaticScopeEEERKNS1_22ReadOnlyCompileOptionsERNS1_18SourceBufferHolderENS1_13MutableHandleINS1_5ValueEEE
  │ frame #28 - 0x0000000108b8ef78 - 2JS8EvaluateEP9JSContextRKNS_22ReadOnlyCompileOptionsEPKDsmNS_13MutableHandleINS_5ValueEE
  │ frame #29 - 0x0000000107a04327 - _<&'a T as script..dom..window..ScriptHelpers>::evaluate_script_on_global_with_result::h1f741fc12ebd09b4
  │ frame #30 - 0x0000000107fb2abd - script::dom::htmlscriptelement::HTMLScriptElement::execute::h77b516dd4b5e9348
  │ frame #31 - 0x00000001080974e2 - script::dom::htmlscriptelement::HTMLScriptElement::prepare::h1cd205f8ba43e3e4
  │ frame #32 - 0x00000001081080a1 - _<html5ever..tree_builder..TreeBuilder<Handle, Sink> as html5ever..tree_builder..rules..TreeBuilderStep>::step::h6ca7a54a6ed327a2
  │ frame #33 - 0x00000001080f7570 - _<html5ever..tree_builder..TreeBuilder<Handle, Sink> as html5ever..tokenizer..interface..TokenSink>::process_token::h5a2d3a9d0da1acfd
  │ frame #34 - 0x00000001080f6931 - _<html5ever..tokenizer..Tokenizer<Sink>>::process_token::h40804d6518116554
  │ frame #35 - 0x0000000108137b15 - _<html5ever..tokenizer..Tokenizer<Sink>>::emit_current_tag::h52f9cf7b088ac5d5
  │ frame #36 - 0x00000001081333de - _<html5ever..tokenizer..Tokenizer<Sink>>::step::h258ea0f0d098b456
  │ frame #37 - 0x0000000108124ed7 - _<html5ever..tokenizer..Tokenizer<Sink>>::run::h978f0dac31568f04
  │ frame #38 - 0x00000001080f25dc - script::dom::servohtmlparser::ServoHTMLParser::parse_sync::h844433bc21b959f5
  │ frame #39 - 0x00000001079e5d1f - script::dom::document::Document::finish_load::h3cf039802ed14b93
  │ frame #40 - 0x0000000108094d38 - _<script..dom..htmlscriptelement..ScriptContext as net_traits..AsyncResponseListener>::response_complete::hacefddd888d90a92
  │ frame #41 - 0x000000010809a945 - _<script..network_listener..ListenerRunnable<A, Listener> as script..script_thread..Runnable>::handler::h7f424141ccc60eea
  │ frame #42 - 0x00000001081d6c6c - script::script_thread::ScriptThread::handle_msg_from_script::ha3bf1b1020b0029c
  │ frame #43 - 0x0000000108232d81 - script::script_thread::ScriptThread::handle_msgs::_$u7b$$u7b$closure$u7d$$u7d$::hb0cd8ac2e7000de4
  │ frame #44 - 0x0000000108218a4f - script::script_thread::ScriptThread::handle_msgs::h54933e7251d179ba
  │ frame #45 - 0x00000001081c9c17 - std::panicking::try::call::h7089ada77042d108
  │ frame #46 - 0x000000010917b2cb - __rust_try
  │ frame #47 - 0x000000010917b265 - __rust_maybe_catch_panic
  │ frame #48 - 0x00000001081cadc4 - _<F as alloc..boxed..FnBox<A>>::call_box::hca26f1567eb6269c
  │ frame #49 - 0x0000000109176d48 - std::sys::thread::Thread::new::thread_start::h9c883b6d445ece46
  │ frame #50 - 0x00007fff8e158059 - _pthread_body
  │ frame #51 - 0x00007fff8e157fd6 - _pthread_start

This is relevant.

peterjoel · 2016-07-04T22:45:33Z

@nox DOMRefCell doesn't get static protection against being borrowed multiple times?

peterjoel · 2016-07-04T23:49:19Z

I've reorganised the code so that the borrows don't overlap. Is there a better way, or a way that can make this a static check?

jdm · 2016-07-05T00:35:08Z

@peterjoel Yes, DOMRefCell is no different than RefCell in that respect.

peterjoel · 2016-07-05T00:37:46Z

@jdm I've re-read the docs on this now. Is there a best practice for avoiding panics here? Using borrow_state would be a bit pointless in this situation - there wouldn't be a sensible alternative path to take anyway.

jdm · 2016-07-05T01:18:04Z

There's no better practice than making mutable borrows as small as possible, unfortunately.

peterjoel · 2016-07-05T01:28:48Z

Thanks @jdm. I've made an improvement to move all the RefCell borrows as close as possible to where they are used, and to limit their scopes. I think it's more readable too.

nox · 2016-07-08T09:40:38Z

@peterjoel You could even do:

*self.matrix.borrow_mut() = ...;

That way you don't even bind the borrow to a local variable.

peterjoel · 2016-07-08T19:30:58Z

@nox I find it a bit weird to look at (first glance it looks like it's assigning to a function call), but I think you are right, it's better.

I'll need to use the borrowed value again on the right hand side, so it will need two borrows and it won't work in one line.

  let rotated = self.matrix.borrow().mul(&rotation);
 *self.matrix.borrow_mut() = rotated;

Unless you can see a better way?

peterjoel · 2016-07-23T12:33:52Z

@nox Can you suggest how I should deal with the differences between Euclid matrices and the geom spec? This is what is blocking me, and I'm not sure of the right approach.

KiChjang · 2016-09-14T16:40:08Z

@bors-servo r=nox

bors-servo · 2016-09-14T16:40:08Z

📌 Commit ec088c8 has been approved by nox

bors-servo · 2016-09-14T16:40:11Z

⌛ Testing commit ec088c8 with merge f0d1747...


        Auto merge of #12202 - peterjoel:issue_8509, r=nox

DOMMatrix and DOMMatrixReadOnly  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #8509.  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12202)

bors-servo · 2016-09-14T17:29:25Z

💔 Test failed - mac-rel-css

KiChjang · 2016-09-14T18:10:55Z

Needs update on test expectations. Good work!


        DOMMatrix and DOMMatrixReadOnly

Updated test expectations

peterjoel · 2016-09-16T15:28:20Z

@nox can this be merged now?

KiChjang · 2016-09-16T17:41:35Z

@bors-servo r=nox

bors-servo · 2016-09-16T17:41:36Z

📌 Commit 8ff8ee9 has been approved by nox

bors-servo · 2016-09-16T18:02:37Z

⌛ Testing commit 8ff8ee9 with merge 1fee88e...


        Auto merge of #12202 - peterjoel:issue_8509, r=nox

DOMMatrix and DOMMatrixReadOnly  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #8509.  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12202)

bors-servo · 2016-09-16T18:54:05Z

💔 Test failed - linux-rel

highfive · 2016-09-16T18:54:08Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-table-007.htm
  └   → /css-transforms-1_dev/html/transform-table-007.htm a5c014b20ef1363bea6f24eda28c7efb7c45698a
/css-transforms-1_dev/html/reference/transform-blank-ref.htm fa6407b1acbbfea27e27061e7d1bdeca98e4a728
Testing a5c014b20ef1363bea6f24eda28c7efb7c45698a == fa6407b1acbbfea27e27061e7d1bdeca98e4a728

KiChjang · 2016-09-16T18:55:34Z

@bors-servo retry

#11574

bors-servo · 2016-09-16T18:55:36Z

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

bors-servo · 2016-09-16T19:45:10Z

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

highfive assigned Manishearth Jul 3, 2016

highfive added the S-awaiting-review label Jul 3, 2016

peterjoel reviewed Jul 3, 2016
View changes

highfive added the S-tests-failed label Jul 4, 2016

nox assigned nox and unassigned Manishearth Jul 4, 2016

highfive removed the S-tests-failed label Jul 4, 2016

peterjoel force-pushed the peterjoel:issue_8509 branch from 71e04b6 to d8dc2c6 Jul 5, 2016

peterjoel force-pushed the peterjoel:issue_8509 branch from d8dc2c6 to df71a17 Jul 5, 2016

peterjoel force-pushed the peterjoel:issue_8509 branch 3 times, most recently from fdeb039 to 14cf6cb Jul 6, 2016

peterjoel force-pushed the peterjoel:issue_8509 branch from 14cf6cb to bc99378 Jul 10, 2016

highfive added the S-awaiting-review label Sep 14, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 14, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 14, 2016

DOMMatrix and DOMMatrixReadOnly

Loading status checks…

8ff8ee9

Updated test expectations

peterjoel force-pushed the peterjoel:issue_8509 branch from ec088c8 to 8ff8ee9 Sep 14, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Sep 14, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 16, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 16, 2016

bors-servo merged commit 8ff8ee9 into servo:master Sep 16, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

servo / servo

DOMMatrix and DOMMatrixReadOnly #12202

DOMMatrix and DOMMatrixReadOnly #12202

peterjoel commented Jul 3, 2016 •

edited

highfive commented Jul 3, 2016

highfive commented Jul 3, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Ms2ger commented Jul 4, 2016

bors-servo commented Jul 4, 2016

bors-servo commented Jul 4, 2016

nox commented Jul 4, 2016

peterjoel commented Jul 4, 2016

peterjoel commented Jul 4, 2016

jdm commented Jul 5, 2016

peterjoel commented Jul 5, 2016 •

edited

jdm commented Jul 5, 2016

peterjoel commented Jul 5, 2016

nox commented Jul 8, 2016

peterjoel commented Jul 8, 2016

peterjoel commented Jul 23, 2016

KiChjang commented Sep 14, 2016

bors-servo commented Sep 14, 2016

bors-servo commented Sep 14, 2016

bors-servo commented Sep 14, 2016

KiChjang commented Sep 14, 2016

peterjoel commented Sep 16, 2016

KiChjang commented Sep 16, 2016

bors-servo commented Sep 16, 2016

bors-servo commented Sep 16, 2016

bors-servo commented Sep 16, 2016

highfive commented Sep 16, 2016

KiChjang commented Sep 16, 2016

bors-servo commented Sep 16, 2016

bors-servo commented Sep 16, 2016

servo / servo

Join GitHub today

DOMMatrix and DOMMatrixReadOnly #12202

DOMMatrix and DOMMatrixReadOnly #12202

Conversation

peterjoel commented Jul 3, 2016 • edited

highfive commented Jul 3, 2016

highfive commented Jul 3, 2016

This comment has been minimized.

peterjoel Jul 3, 2016 • edited

This comment has been minimized.

nox Jul 4, 2016

This comment has been minimized.

peterjoel Jul 4, 2016 • edited

This comment has been minimized.

nox Jul 4, 2016

This comment has been minimized.

peterjoel Jul 4, 2016 • edited

This comment has been minimized.

peterjoel Jul 3, 2016 • edited

This comment has been minimized.

nox Jul 4, 2016

Ms2ger commented Jul 4, 2016

bors-servo commented Jul 4, 2016

bors-servo commented Jul 4, 2016

nox commented Jul 4, 2016

peterjoel commented Jul 4, 2016

peterjoel commented Jul 4, 2016

jdm commented Jul 5, 2016

peterjoel commented Jul 5, 2016 • edited

jdm commented Jul 5, 2016

peterjoel commented Jul 5, 2016

nox commented Jul 8, 2016

peterjoel commented Jul 8, 2016

peterjoel commented Jul 23, 2016

KiChjang commented Sep 14, 2016

bors-servo commented Sep 14, 2016

bors-servo commented Sep 14, 2016

bors-servo commented Sep 14, 2016

KiChjang commented Sep 14, 2016

peterjoel commented Sep 16, 2016

KiChjang commented Sep 16, 2016

bors-servo commented Sep 16, 2016

bors-servo commented Sep 16, 2016

bors-servo commented Sep 16, 2016

highfive commented Sep 16, 2016

KiChjang commented Sep 16, 2016

bors-servo commented Sep 16, 2016

bors-servo commented Sep 16, 2016

peterjoel commented Jul 3, 2016 •

edited

peterjoel Jul 3, 2016 •

edited

peterjoel Jul 4, 2016 •

edited

peterjoel Jul 4, 2016 •

edited

peterjoel Jul 3, 2016 •

edited

peterjoel commented Jul 5, 2016 •

edited