Support -moz-binding in geckolib #11287

heycam · 2016-05-20T08:41:43Z

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 #__ (github issue number if applicable).

Either:

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

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

highfive · 2016-05-20T08:41:46Z

Heads up! This PR modifies the following files:

@bholley: components/style/selector_matching.rs, components/style/properties/properties.mako.rs, components/style/Cargo.toml, components/style/parser.rs, components/style/lib.rs, components/style/properties/longhand/box.mako.rs, components/style/stylesheets.rs, components/style/properties/helpers.mako.rs
@KiChjang: components/script/dom/htmllinkelement.rs, components/script/dom/cssstyledeclaration.rs, components/script/dom/element.rs, components/script/dom/htmlstyleelement.rs

highfive · 2016-05-20T08:41:47Z

Warning

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

heycam · 2016-05-20T08:42:14Z

r? @bholley
CC @emilio

highfive · 2016-05-20T09:13:58Z

New code was committed to pull request.

bors-servo · 2016-05-20T10:04:30Z

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

bholley · 2016-05-23T23:13:40Z

components/style/parser.rs

+#[cfg(feature = "gecko")]
+pub fn dummy_extra_data() -> ParserContextExtraData {
+    ParserContextExtraData { }
+}


Instead of exposing this everywhere, I think we should have a ParserContext::new and ParserContext::new_with_extra_data, where the former delegates to the latter and passes in a sensible default. Then we can make ::default() a static factory method on ParserContextExtraData.

I cant add a static method to ParserContextExtraData when it's a typedef for () in the non-geckolib config. I could make it an enum with no constructors. (And all the non-geckolib places where I pass in () for the extra_data parameter I'll replace with ParserContextExtraData::default()).

bholley · 2016-05-24T00:06:17Z

ports/geckolib/gecko_bindings/ptr.rs

+use std::fmt::{self, Debug};
+
+macro_rules! define_holder_gecko_arc {
+    ($x:ident, $y:ident, $z:ident) => (


More descriptive names please. :-)

bholley · 2016-05-24T00:06:40Z

ports/geckolib/gecko_bindings/ptr.rs

+use heapsize::HeapSizeOf;
+use std::fmt::{self, Debug};
+
+macro_rules! define_holder_gecko_arc {


This could use some comments.

I'd also call this define_gecko_holder_arc or just define_holder_arc since we're in geckolib.

bholley · 2016-05-24T00:07:22Z

ports/geckolib/gecko_bindings/ptr.rs

+
+        impl $x {
+            pub fn new(data: *mut $z) -> $x {
+                assert!(!data.is_null());


debug_assert

bholley · 2016-05-24T00:14:23Z

ports/geckolib/gecko_bindings/ptr.rs

+            }
+
+            fn clone_from(&mut self, source: &$x) {
+                unsafe {


I think we can just take the default impl and not override this here - we don't save any work unless I'm missing something.

bholley · 2016-05-24T00:18:01Z

ports/geckolib/gecko_bindings/tools/regen_style_structs.sh

@@ -102,8 +102,10 @@ export RUST_BACKTRACE=1
  -match "pair"                                                     \
  -match "SheetParsingMode.h"                                       \
  -match "StaticPtr.h"                                              \
+  -match "nsProxyRelease.h"                                         \


Do we really need to do this? Why not just use opaque types?

Opaque types would work, but we'd need those opaque types to look like:

pub enum nsMainThreadHolder<T> {} pub enum nsMainThreadHandle<T> {}

but currently using -opaque-type I get:

pub enum nsMainThreadHolder {} pub enum nsMainThreadHandle {}

which doesn't work with the types in bindings.rs we generate:

pub type ThreadSafePrincipalHolder = nsMainThreadPtrHolder<nsIPrincipal>; pub type ThreadSafeURIHolder = nsMainThreadPtrHolder<nsIURI>;

bholley · 2016-05-24T00:25:47Z

components/style/properties/longhand/box.mako.rs

+                SpecifiedValue::Url(ref url, _) => {
+                    try!(dest.write_str("url(\""));
+                    try!(write!(&mut CssStringWriter::new(dest), "{}", url));
+                    try!(dest.write_str("\")"));


Can we implement ToCss for Url somewhere, and then just delegate to that here (and in the other places where we need to do this)? Followup patch is fine.

bholley · 2016-05-24T00:40:10Z

components/style/properties/longhand/box.mako.rs

+                        referrer: referrer.clone(),
+                        principal: principal.clone(),
+                    };
+                    Ok(SpecifiedValue::Url(context.parse_url(&*try!(input.expect_url())), extra_data))


It seems a bit clearer to me to hoist the parse_url piece to an assignment to a temporary right above the |match| expression. That also avoids doing all the refcounting in error cases.

bholley · 2016-05-24T00:41:16Z

ports/geckolib/properties.mako.rs

+    fn set__moz_binding(&mut self, v: longhands::_moz_binding::computed_value::T) {
+        use style::properties::longhands::_moz_binding::SpecifiedValue as BindingValue;
+        match v {
+            BindingValue::None => assert!(self.gecko.mBinding.mRawPtr.is_null()),


debug_assert - please remember to do this, since I might not catch them all.

emilio · 2016-05-24T01:11:00Z

components/style/properties/longhand/box.mako.rs

+
+    pub fn parse(context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue, ()> {
+        if input.try(|input| input.expect_ident_matching("none")).is_ok() {
+            Ok(SpecifiedValue::None)


nit: Also, this can use early return to dedent the rest of the function a bit.


        Add an extra data field to ParserContext.

This will be used for passing Gecko-specific information through the CSS parser in stylo.


        Regenerate bindings with Holder types.


        Add smart pointer types for Gecko refcounted objects.


        Pass Gecko sheet base/referrer/principal from Servo_StylesheetFromUTF…

…8Bytes through ParserContext.


        Support -moz-binding in geckolib.


        Re-use Url serialization in values::specified::Image.

highfive · 2016-05-24T02:12:42Z

New code was committed to pull request.

highfive · 2016-05-24T04:03:28Z

New code was committed to pull request.


        fixup! Add an extra data field to ParserContext.

bholley · 2016-05-24T17:44:12Z

I was hoping that we could avoid all the DOM changes by the new_with_extra_data thing, but I guess there are other callers.

I think now that the ideal refactoring would be to rename ParserContextExtraData to ParserContextOptions, and hoist the error reporter and the base URL into that, along with any additional optional Gecko fields.

We can clean that up later though, and I'd rather get this landed.

@bors-servo r+

bors-servo · 2016-05-24T17:44:13Z

📌 Commit f2b1ef4 has been approved by bholley

bors-servo · 2016-05-24T18:26:57Z

⌛ Testing commit f2b1ef4 with merge 6bd05f8...


        Auto merge of #11287 - heycam:moz-binding, r=bholley

Support -moz-binding in geckolib 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 - [ ] These changes fix #__ (github issue number if applicable). Either: - [ ] There are tests for these changes OR - [x] These changes do not require tests because changes target stylo 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 [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11287)

bors-servo · 2016-05-24T19:20:56Z

💔 Test failed - linux-rel

highfive · 2016-05-24T19:20:59Z

  ▶ Unexpected subtest result in /css-transitions-1_dev/html/detached-container-001.htm:
  │ FAIL [expected PASS] transition within detached container / values
  │   → assert_equals: no intermediate values expected 2 but got 1
  │ 
  │ .cases.values.done@http://web-platform.test:8000/css-transitions-1_dev/html/detached-container-001.htm:69:29
  │ runLoop/&lt;/&lt;/&lt;/&lt;@http://web-platform.test:8000/css-transitions-1_dev/html/support/runParallelAsyncHarness.js:121:26
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1226:20
  │ runLoop/&lt;/&lt;/&lt;@http://web-platform.test:8000/css-transitions-1_dev/html/support/runParallelAsyncHarness.js:120:21
  │ runLoop/&lt;/&lt;@http://web-platform.test:8000/css-transitions-1_dev/html/support/runParallelAsyncHarness.js:119:17
  └ runLoop/&lt;@http://web-platform.test:8000/css-transitions-1_dev/html/support/runParallelAsyncHarness.js:117:13

mbrubeck · 2016-05-24T19:40:12Z

@bors-servo retry

#10549

bors-servo · 2016-05-24T21:41:05Z

⌛ Testing commit f2b1ef4 with merge 2a2b88f...


        Auto merge of #11287 - heycam:moz-binding, r=bholley

Support -moz-binding in geckolib 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 - [ ] These changes fix #__ (github issue number if applicable). Either: - [ ] There are tests for these changes OR - [x] These changes do not require tests because changes target stylo 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 [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11287)

bors-servo · 2016-05-24T22:59:29Z

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

highfive assigned jdm May 20, 2016

highfive added the S-awaiting-review label May 20, 2016

highfive assigned bholley and unassigned jdm May 20, 2016

heycam changed the title ~~Moz binding~~ Support -moz-binding in geckolib May 20, 2016

heycam force-pushed the heycam:moz-binding branch from eb3d10d to e07dffd May 20, 2016

highfive added the S-needs-rebase label May 20, 2016

heycam force-pushed the heycam:moz-binding branch from e07dffd to 7af125a May 20, 2016

bholley reviewed May 23, 2016
View changes

bholley reviewed May 24, 2016
View changes

ports/geckolib/gecko_bindings/ptr.rs Outdated

use std::fmt::{self, Debug};

macro_rules! define_holder_gecko_arc {

($x:ident, $y:ident, $z:ident) => (

This comment has been minimized.

Sign in to view

bholley May 24, 2016
Contributor

More descriptive names please. :-)

bholley reviewed May 24, 2016
View changes

ports/geckolib/gecko_bindings/ptr.rs Outdated

impl $x {

pub fn new(data: *mut $z) -> $x {

assert!(!data.is_null());

This comment has been minimized.

Sign in to view

bholley May 24, 2016
Contributor

debug_assert

bholley reviewed May 24, 2016
View changes

emilio reviewed May 24, 2016
View changes

heycam added 6 commits May 19, 2016

Add an extra data field to ParserContext.

bb85f5f

This will be used for passing Gecko-specific information through the CSS parser in stylo.

Regenerate bindings with Holder types.

9b63deb

Add smart pointer types for Gecko refcounted objects.

6dfb51f

Pass Gecko sheet base/referrer/principal from Servo_StylesheetFromUTF…

6d67aa8

…8Bytes through ParserContext.

Support -moz-binding in geckolib.

44bdbed

Re-use Url serialization in values::specified::Image.

Loading status checks…

e75b1c3

heycam force-pushed the heycam:moz-binding branch from 7af125a to e75b1c3 May 24, 2016

fixup! Add an extra data field to ParserContext.

Loading status checks…

f2b1ef4

bors-servo merged commit f2b1ef4 into servo:master May 24, 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

This was referenced May 24, 2016

Update style attributes on CSSStyleDeclaration changes #11377

Merged

geckolib: Add bindings and minimal sugar for already_AddRefed #11350

Closed

heycam deleted the heycam:moz-binding branch May 25, 2016

servo / servo

Support -moz-binding in geckolib #11287

Support -moz-binding in geckolib #11287

heycam commented May 20, 2016 •

edited by larsbergstrom

highfive commented May 20, 2016

highfive commented May 20, 2016

heycam commented May 20, 2016

highfive commented May 20, 2016

bors-servo commented May 20, 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.

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.

highfive commented May 24, 2016

highfive commented May 24, 2016

bholley commented May 24, 2016

bors-servo commented May 24, 2016

bors-servo commented May 24, 2016

bors-servo commented May 24, 2016

highfive commented May 24, 2016

mbrubeck commented May 24, 2016

bors-servo commented May 24, 2016

bors-servo commented May 24, 2016

servo / servo

Join GitHub today

Support -moz-binding in geckolib #11287

Support -moz-binding in geckolib #11287

Conversation

heycam commented May 20, 2016 • edited by larsbergstrom

highfive commented May 20, 2016

highfive commented May 20, 2016

heycam commented May 20, 2016

highfive commented May 20, 2016

bors-servo commented May 20, 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.

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.

highfive commented May 24, 2016

highfive commented May 24, 2016

bholley commented May 24, 2016

bors-servo commented May 24, 2016

bors-servo commented May 24, 2016

bors-servo commented May 24, 2016

highfive commented May 24, 2016

mbrubeck commented May 24, 2016

bors-servo commented May 24, 2016

bors-servo commented May 24, 2016

heycam commented May 20, 2016 •

edited by larsbergstrom