Add support for gecko-only array values; use for stylo #11851

Manishearth · 2016-06-24T11:06:11Z

Doesn't yet work, because I can't grow nsTArrays from Rust. If anyone knows how to add bindings for that, let me know!

Thoughts on the design so far? Once this PR lands, all of the array-accepting background- properties can just use gecko_autoarray_longhand and some iterators in the geckolib implementation without changing much other code to work with arrays.

cc @emilio @bholley

This change is

highfive · 2016-06-24T11:06:15Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/longhand/background.mako.rs, components/style/properties/helpers.mako.rs

highfive · 2016-06-24T11:06:16Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify style code, but no tests are modified. Please consider adding a test!

Manishearth · 2016-06-24T11:10:03Z

ports/geckolib/properties.mako.rs

+        self.gecko.mImage.mImageCount = cmp::max(self.gecko.mImage.mLayers.len() as u32,
+                                                 self.gecko.mImage.mImageCount);
+
+        // TODO: pre-grow the nsTArray to the right capacity


This is where I need the bindings

Manishearth · 2016-06-24T11:10:51Z

ports/geckolib/properties.mako.rs

+
+        // TODO: pre-grow the nsTArray to the right capacity
+        // otherwise the below code won't work
+        for (image, geckoimage) in images.0.into_iter().zip(self.gecko.mImage.mLayers.iter_mut()) {


Github doesn't show this diff well, but the below code is simply indented one level with a change to the Gecko_SetGradientImageValue line

Manishearth · 2016-06-24T11:47:40Z

components/style/properties/helpers.mako.rs

+    "<thing> [ , <thing> ]*", but only support a single value
+    in servo
+</%doc>
+<%def name="gecko_autoarray_longhand(name, **kwargs)">


once servo starts supporting <thing> [ , <thing> ]* properties we can refactor this to work in general and have a thin conditional-compile wrapper around it.

bholley · 2016-06-24T15:18:39Z

Nice! In terms of reallocating, I think we should just do an FFI call, otherwise there's lots of stuff we need to worry about.

Didn't look too closely here, @emilio should probably review since he did the initial array stuff. But it's great to see a clean and easy abstraction on this stuff! :-)

emilio · 2016-06-24T17:18:05Z

Yeah, reallocating is basically why I didn't made a more vec-like interface for nsTArray.

I can't probably take a deeper look today (I'm packing things), but yeah, I'd go with the FFI call to reallocate.

Probably we could share allocators (I already had a patch for it, though I need to double-check it does the correct thing evrrywhere), but for now I think we should try to avoid duplicating all the logic there.

If a template function with a pointer to the nsTArray doesn't work (I don't think it'd work, since the functions won't ever be instantiated), probably passing the size of the element would be sufficient, though it'd require to do a bit more math.

Probably receiving a size and a pointer to nsTArray<uint8_t>, and reallocate to n * size elements could work.

I can prioritize the allocator patch though, if you'd prefer to try to write that logic in rust.

emilio · 2016-06-24T17:19:03Z

nsTArray<uint8_t>, I meant

emilio · 2016-06-24T17:19:42Z

Ugh, github mobile strips my less than signs, I meant an nsTArray of uint8_t

emilio · 2016-06-24T17:21:24Z

components/style/properties/helpers.mako.rs

+
+            impl ToCss for computed_value::T {
+                fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+                    if self.0.len() >= 1 {


This is probably a bit more tricky. In some props we might need to write none in the empty case, eg, animation-name.

Oh, also: !self.0.is_empty()

emilio · 2016-06-24T17:23:33Z

components/style/properties/helpers.mako.rs

+                    single_value::parse(context, parser)
+                }).map(|ok| SpecifiedValue(ok))
+            }
+            impl ToComputedValue for SpecifiedValue {


Some (most?) of these properties are computed-value as specified, which would just involve a generic clone which would be cheaper.

I'm not opposed to land this as is, but I'd want a note about that here.

bholley · 2016-06-24T17:27:36Z

I think you can force-instantiate a generic function in C++. Something like http://stackoverflow.com/a/2155790/3670407 in the cpp file.

bholley · 2016-06-24T17:28:12Z

Oh I guess the issue here is that we don't necessarily know the type we want it for in C++? I guess in that case just passing a size might be better.

Manishearth · 2016-06-24T17:32:42Z

Is the allocator patch necessary? I was hoping we could call whatever grow function gecko uses through FFI; I just don't yet know which function that is (havent really looked), and what button to press to get bindings for it.

That way we can use whatever reallcoation logic that gecko uses. And we are not dependent on shared allocators.

Manishearth · 2016-06-24T17:33:53Z

Oh, monomorphization. Ugh.

emilio · 2016-06-24T17:42:58Z

Oh, sure you can force-instantiate it, and we can probably know most if not all of the types we'd need. I don't want to know what MSVC would think about that though :P

So yea, I'd go for an ffi fn like:

void Gecko_ResizeArray(nsTArray<uint8_t>* arr, uint32_t n, uint32_t element_size);

bholley · 2016-06-24T17:48:58Z

If there are only a couple of types, it might be nicer to just have explicit separate FFI calls for each T to avoid too much scary munging. Is it more like 3 or more like 15?

emilio · 2016-06-24T17:52:35Z

I think there were about 6 different types in the layers structure, and a few more at least.

Manishearth · 2016-06-27T12:06:12Z

So, https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#380 exists and is really all we need here. It is not parametrized on the type, just on the ActualAlloc (not sure what actualalloc we use, or what exactly that is). So it will always be monomorphized.

How do I get bindings to this?

emilio · 2016-06-28T02:25:12Z

@Manishearth So our nsTArray bindings are bindings to an actual simpler replacement of nsTArray (search for nsTArray_Simple.

That doesn't change the way we can generate bindings to it though, and it'd be creating a gecko function with the signature as above, and just calling that other function from gecko instead of doing it manually.

Manishearth · 2016-06-28T10:05:10Z

I'm having tons of trouble building bindgen.

First, I tried using llvm38, getting the following error:

"_clang_Cursor_isFunctionInlined", referenced from:
bindgen::clang::Cursor::is_inlined_function::hbf3ecb6d1f288ea8 in libbindgen.rlib(bindgen.0.o)

I have a local clone of LLVM that is close to trunk (cloned a month ago), which I installed. This made the bindgen build work. (I later ran brew install llvm --head which also worked)

Then when running bindgen I get obj-x86_64-apple-darwin15.3.0//dist/include/mozilla/mozalloc.h:16:12: fatal error: 'new' file not found. If I include m-c/build/stlport/stlport/, that goes away and I get a similar error for cstddef. Fixing that by explicitly including /usr/include/c++/4.2.1/ seems to get rid of all the include errors, but now I have errors like:

unsupported type `?` (obj-x86_64-apple-darwin15.3.0//dist/include/mozilla/ServoBindings.h:37:31)
unsupported type `?` (obj-x86_64-apple-darwin15.3.0//dist/include/mozilla/ServoBindings.h:174:36)

The bindings files are just emptied out.

I'm pretty sure I've done something wrong at this point 😄

Could someone with a working bindgen setup explain how theirs was set up? Preferably on a mac.

bholley · 2016-06-28T16:03:58Z

Yeah, you need to brew install llvm38. We don't actually need the tip stuff for stylo, so @emilio is going to make that dep optional.

For the headers, you need to make sure your standard library is installed to /usr/local. On mac, I think xcode-select --install should do it.

Manishearth · 2016-06-28T16:13:09Z

I did run xcode-select --install, it seemed to already exist. Mine are in /usr/include though. They work when used on an external C++ file; just seems like bindgen can't find them.

So currently we need to use llvm tip to get things working, right? llvm38 is just a future goal?

bors-servo · 2016-07-14T04:23:05Z

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


        Add framework for properties which are single elements in servo and a…

…rrays in gecko


        Add iterators for nsStyleAutoArray


        Make background-image an array for stylo


        Add bindings for growing nsTArray


        Pre-set length to desired value in image setter


        Address review comments


        Resync bindings

emilio · 2016-07-14T09:00:02Z

@bors-servo: r+

Awesome! :)

bors-servo · 2016-07-14T09:00:04Z

📌 Commit 6a378a8 has been approved by emilio

emilio · 2016-07-14T09:00:36Z

@bors-servo: p=1

It makes the repo in sync for stylo development.

bors-servo · 2016-07-14T09:32:49Z

⌛ Testing commit 6a378a8 with merge ed985f7...


        Auto merge of #11851 - Manishearth:stylo-autoarray, r=emilio

@emilio

Add support for gecko-only array values; use for stylo Doesn't yet work, because I can't grow nsTArrays from Rust. If anyone knows how to add bindings for that, let me know! Thoughts on the design so far? Once this PR lands, all of the array-accepting background- properties can just use gecko_autoarray_longhand and some iterators in the geckolib implementation without changing much other code to work with arrays. cc @emilio @bholley  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11851)

bors-servo · 2016-07-14T10:30:28Z

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

Manishearth · 2016-07-15T08:47:22Z

First, we probably want a fast path for the "computed value as specified case", so an arg here like computed_value_as_specified=False, and then skipping all the re-declaration of the computed values and just do impl ComputedValueAsSpecified for computed_value::T {}.

This doesn't actually work in this case, because the values aren't the same. One is computed::Image, the other is specified::Image (they differ because the linear gradients contain lengths which are represented differently once computed)


        Auto merge of #12458 - Manishearth:style-gecko-only, r=<try>

@emilio

Cleanups in autoarray helper Addresses @emilio's comments from #11851 - Replace gecko_autoarray_longhand with vector_longhand, make it configurable - Allow for empty vectors, use empty vector longhand in box-shadow  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12458)


        Auto merge of #12458 - Manishearth:style-gecko-only, r=<try>

@emilio

Cleanups in autoarray helper Addresses @emilio's comments from #11851 - Replace gecko_autoarray_longhand with vector_longhand, make it configurable - Allow for empty vectors, use empty vector longhand in box-shadow  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12458)


        Auto merge of #12458 - Manishearth:style-gecko-only, r=emilio

@emilio

Cleanups in autoarray helper Addresses @emilio's comments from #11851 - Replace gecko_autoarray_longhand with vector_longhand, make it configurable - Allow for empty vectors, use empty vector longhand in box-shadow  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12458)

highfive assigned mbrubeck Jun 24, 2016

highfive added the S-awaiting-review label Jun 24, 2016

Manishearth assigned emilio and unassigned mbrubeck Jun 24, 2016

Manishearth force-pushed the Manishearth:stylo-autoarray branch from 6e9eaeb to 871bb57 Jun 24, 2016

Manishearth reviewed Jun 24, 2016
View changes

emilio reviewed Jun 24, 2016
View changes

emilio added S-needs-code-changes and removed S-awaiting-review labels Jun 27, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 13, 2016

Manishearth force-pushed the Manishearth:stylo-autoarray branch from f0dd478 to c0c4b34 Jul 13, 2016

highfive added the S-needs-rebase label Jul 14, 2016

Manishearth added 6 commits Jun 24, 2016

Add framework for properties which are single elements in servo and a…

b1a3831

…rrays in gecko

Add iterators for nsStyleAutoArray

3a5a568

Make background-image an array for stylo

38b57c4

Add bindings for growing nsTArray

154bfb0

Pre-set length to desired value in image setter

5be15e5

Address review comments

Loading status checks…

209255b

Manishearth force-pushed the Manishearth:stylo-autoarray branch from c0c4b34 to 209255b Jul 14, 2016

Resync bindings

Loading status checks…

6a378a8

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Jul 14, 2016

bors-servo merged commit 6a378a8 into servo:master Jul 14, 2016
2 checks passed

2 checks passed

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 14, 2016

Manishearth deleted the Manishearth:stylo-autoarray branch Jul 15, 2016

Manishearth mentioned this pull request Jul 15, 2016

Cleanups in autoarray helper #12458

Merged

servo / servo

Add support for gecko-only array values; use for stylo #11851

Add support for gecko-only array values; use for stylo #11851

Manishearth commented Jun 24, 2016 •

edited by larsbergstrom

highfive commented Jun 24, 2016

highfive commented Jun 24, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bholley commented Jun 24, 2016

emilio commented Jun 24, 2016 •

edited by jdm

emilio commented Jun 24, 2016

emilio commented Jun 24, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bholley commented Jun 24, 2016

bholley commented Jun 24, 2016

Manishearth commented Jun 24, 2016

Manishearth commented Jun 24, 2016

emilio commented Jun 24, 2016

bholley commented Jun 24, 2016

emilio commented Jun 24, 2016

Manishearth commented Jun 27, 2016

emilio commented Jun 28, 2016

Manishearth commented Jun 28, 2016 •

edited

bholley commented Jun 28, 2016

Manishearth commented Jun 28, 2016

bors-servo commented Jul 14, 2016

emilio commented Jul 14, 2016

bors-servo commented Jul 14, 2016

emilio commented Jul 14, 2016

bors-servo commented Jul 14, 2016

bors-servo commented Jul 14, 2016

Manishearth commented Jul 15, 2016

servo / servo

Join GitHub today

Add support for gecko-only array values; use for stylo #11851

Add support for gecko-only array values; use for stylo #11851

Conversation

Manishearth commented Jun 24, 2016 • edited by larsbergstrom

highfive commented Jun 24, 2016

highfive commented Jun 24, 2016

This comment has been minimized.

Manishearth Jun 24, 2016

This comment has been minimized.

Manishearth Jun 24, 2016

This comment has been minimized.

Manishearth Jun 24, 2016

bholley commented Jun 24, 2016

emilio commented Jun 24, 2016 • edited by jdm

emilio commented Jun 24, 2016

emilio commented Jun 24, 2016

This comment has been minimized.

emilio Jun 24, 2016

This comment has been minimized.

emilio Jul 5, 2016

This comment has been minimized.

emilio Jun 24, 2016

bholley commented Jun 24, 2016

bholley commented Jun 24, 2016

Manishearth commented Jun 24, 2016

Manishearth commented Jun 24, 2016

emilio commented Jun 24, 2016

bholley commented Jun 24, 2016

emilio commented Jun 24, 2016

Manishearth commented Jun 27, 2016

emilio commented Jun 28, 2016

Manishearth commented Jun 28, 2016 • edited

bholley commented Jun 28, 2016

Manishearth commented Jun 28, 2016

bors-servo commented Jul 14, 2016

emilio commented Jul 14, 2016

bors-servo commented Jul 14, 2016

emilio commented Jul 14, 2016

bors-servo commented Jul 14, 2016

bors-servo commented Jul 14, 2016

Manishearth commented Jul 15, 2016

Manishearth commented Jun 24, 2016 •

edited by larsbergstrom

emilio commented Jun 24, 2016 •

edited by jdm

Manishearth commented Jun 28, 2016 •

edited