serialize font: to empty on non-default subprops #15604

zploskey · 2017-02-16T22:57:58Z

Fixes font shorthand serialization so that it serializes to "" when non-default subproperties are defined. These subproperties are those defined in #15033.

Adds tests:

font_should_serialize_to_empty_if_there_are_nondefault_subproperties
font_should_serialize_all_available_properties

The second test was previously commented out and underwent some cleanup to make it run.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix font shorthand should be serialized to empty when some of its subproperties have non-default value #15036

There are tests for these changes

This change is

highfive · 2017-02-16T22:58:04Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon.

highfive · 2017-02-16T22:58:05Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/shorthand/font.mako.rs
@emilio: components/style/properties/shorthand/font.mako.rs

metajack · 2017-02-17T00:17:16Z

r? @SimonSapin

zploskey · 2017-02-18T16:50:37Z

components/style/properties/shorthand/font.mako.rs

-                try!(stretch.to_css(dest));
-                try!(write!(dest, " "));
+    % for sub_property in "font_style font_variant font_weight font_stretch".split():
+            match extract_value(self.${sub_property}) {


Would something like this be preferable?

if let Some(val) = extract_value(self.${sub_property}) { try!(val.to_css(dest)); try!(write!(dest, " ")); }

zploskey · 2017-02-19T21:43:52Z

These latest changes fix some problems with my first attempt, and considerably simplify the changes needed to to_css_declared(). I believe the handling of non-default subproperties now meets the requirements. Unless the mako variable product is somehow set to "gecko" or "none", it is not really possible for the should_serialize_to_empty_if_there_are_nondefault_subproperties() test to work properly. It passes in its current form, but I believe that is only a coincidence since when I change the test to only serialize a property that is part of the font shorthand it also passes. So clearly the test doesn't work correctly right now.

What I can do is fix the test so that it will fail with the current test configuration, but we may want to either disable it or somehow mark it as a known failure. I see some documentation about annotating with "ignore" but nothing about known failures... maybe should_panic fills this purpose but doesn't seem to really describe my meaning. That being that the test will fail until the build system in test can set product to "gecko" or "none".

If there are no objections, I'll go ahead and fix the test by switching it to have at least one normal property of the font shorthand, call shorthand_properties_to_string(properties) and assert that the result == "". The test fails but I can annotate with #[ignore] for now. In the event that we eventually can build with product set to those values we would want to remove the ignore. Does this sound reasonable?

zploskey · 2017-02-19T22:27:39Z

Went ahead and adjusted the test as I described and ignored it. When enabled it fails for me with:

thread 'properties::serialization::shorthand_serialization::font::should_serialize_to_empty_if_there_are_nondefault_subproperties' panicked at 'assertion failed: (left == right) (left: "font-style: italic; font-language-override: normal; font-kerning: none;", right: "")', /home/zach/src/servo/tests/unit/style/properties/serialization.rs:660

And now of course I just noticed that it is not producing a font: property at all, just the individual CSS properties. I guess that's expected. I was mistakenly thinking I could produce a font shorthand without adding all or most of its subproperties.

zploskey · 2017-02-19T22:40:46Z

Ok I think the test is now fixed to the extent that I think it can be. When enabled, it now fails with:

thread 'properties::serialization::shorthand_serialization::font::should_serialize_to_empty_if_there_are_nondefault_subproperties' panicked at 'assertion failed: (left == right) (left: "font: italic normal bolder expanded 4px/3 serif; font-language-override: normal; font-kerning: none;", right: "font-language-override: normal; font-kerning: none;")', /home/zach/src/servo/tests/unit/style/properties/serialization.rs:678

font: ... doesn't get removed for reasons mentioned previously. I've left the test ignored with a TODO about enabling it when have the capability to run those code paths in test.

I think that's all I can do for now. I'll wait for feedback.

zploskey · 2017-02-22T19:05:00Z

Changes have been rebased on master, intermediate commits squashed, and test changes split into a separate commit. Moved comment on one of the test functions above its annotations. Made some improvements to the commit messages.

jdm · 2017-02-22T22:08:17Z

@SimonSapin Review ping.

SimonSapin · 2017-02-24T15:18:12Z

Sorry I only realized now, but I think there is a misunderstanding. When @upsuper in #15036 writes "The font shorthand should be serialized to empty string", it means that the empty string should be returned when requesting the value of the font property by itself hrough the CSSOM method CSSStyleDeclaration.getPropertyValue. In our implementation this is the Rust method PropertyDeclarationBlock::single_value_to_css.

The tests in this PR are based on the Rust method PropertyDeclarationBlock::to_css_string, which implements the getter for the CSSOM attribute CSSStyleDeclaration.cssText.

Although the two algorithms have some parts in common, they are different. They should be tested separately.

When asking for the value of a specific shorthand, there may be no syntax that would expand to the current values of all the corresponding longhands. In that case we return an empty string. When serializing an entire block, however, there is no such limitations since (in the general case) we’re serializing multiple declarations anyway. When a shorthand doesn’t have appropriate syntax, we can fall back to multiple longhand declarations instead. In no case should a (set of) declarations be entirely skipped from the serialization.

The links above lead to the parts of the CSSOM specification that specify these algorithms, including the fallback case:

Part of https://drafts.csswg.org/cssom/#serialize-a-css-declaration-block

Let value be the result of invoking serialize a CSS value of current longhands.

If value is the empty string, continue with the steps labeled shorthand loop.

zploskey · 2017-02-24T19:07:51Z

@SimonSapin Thanks for the clarification. My apologies for misunderstanding, this is my first PR on servo.

If I change the test to the following, attempting to use single_value_to_css() to serialize, it fails each time on the first assert because x is Err(Error). It's unclear to me why that is the case.

        fn font_should_serialize_to_empty_if_there_are_nondefault_subproperties() {
            let declarations = vec![
                (PropertyDeclaration::FontStyle(DeclaredValue::Value(FontStyle::italic)),
                    Importance::Normal),
                (PropertyDeclaration::FontKerning(DeclaredValue::Value(FontKerning::none)),
                    Importance::Normal),
            ];

            let block = PropertyDeclarationBlock {
                declarations: declarations,
                important_count: 0
            };

            let mut s = String::new();

            let id = PropertyId::parse("font".into()).unwrap();
            let x = block.single_value_to_css(&id, &mut s);

            assert_eq!(x.is_ok(), true);
            assert_eq!(s, "");
        }

The approach is similar to scroll_snap_type::should_serialize_to_empty_string_if_sub_types_not_equal(). I actually tried this approach before but was never able to get it to work. I may be missing something basic about how single_value_to_css() is supposed to work. It fails even if I set only the first PropertyDeclaration.

zploskey · 2017-02-25T01:39:28Z

Sorry I only realized now, but I think there is a misunderstanding. When @upsuper in #15036 writes "The font shorthand should be serialized to empty string", it means that the empty string should be returned when requesting the value of the font property by itself hrough the CSSOM method CSSStyleDeclaration.getPropertyValue. In our implementation this is the Rust method PropertyDeclarationBlock::single_value_to_css.

Do you mean PropertyDeclarationBlock::property_value_to_css? It has a that same link in its doc comment and it seems to work in this case while single_value_to_css does not because I need to pass more than a single PropertyDeclaration.

zploskey · 2017-02-25T02:26:42Z

Ignored test font_should_serialize_to_empty_if_there_are_nondefault_subproperties now fails with

panicked at 'assertion failed: (left == right) (left: "italic normal bolder expanded 4px/3 serif", right: "")', /home/zach/src/servo/tests/unit/style/properties/serialization.rs:687

because the products that would let this test pass are not enabled in the unit tests.

bors-servo · 2017-03-02T01:00:40Z

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

zploskey · 2017-03-02T19:29:50Z

PR was rebased on master. Changes in #15779 made the fn to_css rather than to_css_declared. Because LonghandsToSerialize now contains fields of type DeclaredValue, I have eliminated fn extract_value and its use. Because to_css returns a fmt::Result, I've adjusted it to return Ok(()) when we encounter subproperties that are not part of the font shorthand.

@SimonSapin This is ready for review again.

nox

A few nits.

nox · 2017-03-03T10:38:59Z

components/style/properties/shorthand/font.mako.rs

+
+    % if product == "gecko":
+        % for sub_property in "font_size_adjust font_kerning font_variant_caps font_variant_position".split():
+            if let Some(_) = self.${sub_property} {


if self.${sub_property}.is_some() { ... }

nox · 2017-03-03T10:39:12Z

components/style/properties/shorthand/font.mako.rs

+    % endif
+
+    % if product == "none":
+            if let Some(_) = self.font_language_override {


if self.font_language_override.is_some() { ... }

nox · 2017-03-03T10:40:53Z

tests/unit/style/properties/serialization.rs

            let font_family = DeclaredValue::Value(
-                FamilyContainer(vec![FontFamily::Generic(atom!("serif"))])
+                FamilyContainer(vec![FontFamily::Generic(Atom::from("serif"))])


Why change that?

@nox It looks like you removed this comment, but in case it's not clear to others now that I've addressed the other nits: This did not compile using atom!, because the string_cache crate is not available, but this variation works. This test was commented out before, so I just uncommented and got it to compile by using Atom::from based on a suggestion from someone in IRC.

I removed it because I thought you had reverted the change, but I'm ok with your explanation and we don't need atom! performance benefits here anyway.

nox · 2017-03-04T11:24:01Z

@SimonSapin This is ready to be reviewed.

bors-servo · 2017-03-19T08:15:48Z

⌛ Testing commit 65e2ed7 with merge 0fe2a0e...

…=SimonSapin serialize font: to empty on non-default subprops Fixes font shorthand serialization so that it serializes to "" when non-default subproperties are defined. These subproperties are those defined in #15033. Adds tests: - font_should_serialize_to_empty_if_there_are_nondefault_subproperties - font_should_serialize_all_available_properties The second test was previously commented out and underwent some cleanup to make it run. ---  - [x] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15036  - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15604)

bors-servo · 2017-03-19T08:41:47Z

💔 Test failed - linux-rel-wpt

SimonSapin · 2017-03-19T09:30:06Z

The two not-already-marked-intermittent failures match error messages and stack traces in #13509 and #14323. (@jdm, is this the right way to tag things, with the list of filenames removed from #14323’s title?)

@bors-servo retry

  ▶ TIMEOUT [expected PASS] /html/dom/elements/global-attributes/dir_auto-textarea-N-EN.html
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 12.0.1
  │ Shutting down the Constellation after generating an output file or exit flag specified
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 12.0.1
  │ failed to receive response to font request: IoError(Error { repr: Os { code: 104, message: "Connection reset by peer" } }) (thread LayoutThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(1) }, at /checkout/src/libcore/result.rs:860)
  │ stack backtrace:
  │    0:     0x7fc9a0662c2d - backtrace::backtrace::trace::hce6a3819d897a887
  │    1:     0x7fc9a0663112 - backtrace::capture::Backtrace::new::hb5a725a088a2a2fc
  │    2:     0x7fc99f586523 - servo::main::{{closure}}::h9d96b5c32a8afffa
  │    3:     0x7fc9a147c6bd - std::panicking::rust_panic_with_hook
  │                         at /checkout/src/libstd/panicking.rs:550
  │    4:     0x7fc9a147c554 - std::panicking::begin_panic
  │                         at /checkout/src/libstd/panicking.rs:511
  │    5:     0x7fc9a147c489 - std::panicking::begin_panic_fmt
  │                         at /checkout/src/libstd/panicking.rs:495
  │    6:     0x7fc9a147c417 - std::panicking::rust_begin_panic
  │                         at /checkout/src/libstd/panicking.rs:471
  │    7:     0x7fc9a14a947d - core::panicking::panic_fmt
  │                         at /checkout/src/libcore/panicking.rs:69
  │    8:     0x7fc9a038f51b - core::result::unwrap_failed::hd6c954846ed51283
  │    9:     0x7fc9a03a4a28 - gfx::font_cache_thread::FontCacheThread::last_resort_font_template::h1802e896535d574d
  │   10:     0x7fc9a03a5d87 - gfx::font_context::FontContext::layout_font_group_for_style::he3ec5275d9cc8fd2
  │   11:     0x7fc9a02ebf75 - layout::text::TextRunScanner::scan_for_runs::h9e7b4d5cef563b9e
  │   12:     0x7fc99f846c18 - >::build_flow_for_block_starting_with_fragments::heca9cf2293a81bc4
  │   13:     0x7fc99f842f7c - >::build_flow_for_block_like::h3f6021bd678bdf31
  │   14:     0x7fc99f8425e0 - >::build_flow_for_block::h99cf3ea7a8a001fe
  │   15:     0x7fc99f7f2e20 -  as layout::traversal::PostorderNodeMutTraversal>::process::h7163180639338351
  │   16:     0x7fc99f7e36be - >::process_postorder::hb395296ca2e298d3
  │   17:     0x7fc99f81fbb7 - style::sequential::traverse_dom::doit::h473529d272cb7d1d
  │   18:     0x7fc99f81fb86 - style::sequential::traverse_dom::doit::h473529d272cb7d1d
  │   19:     0x7fc99f81fb86 - style::sequential::traverse_dom::doit::h473529d272cb7d1d
  │   20:     0x7fc99f81fb86 - style::sequential::traverse_dom::doit::h473529d272cb7d1d
  │   21:     0x7fc99f85dbb8 - layout_thread::LayoutThread::handle_reflow::hbab16fcce01ae6f3
  │   22:     0x7fc99f856495 - layout_thread::LayoutThread::handle_request_helper::h6861378e293d9e98
  │   23:     0x7fc99f852218 - ::create::{{closure}}::he70c89f1fd32aa44
  │   24:     0x7fc99f804487 - std::panicking::try::do_call::h761cbb26c4556062
  │   25:     0x7fc9a14836da - panic_unwind::__rust_maybe_catch_panic
  │                         at /checkout/src/libpanic_unwind/lib.rs:98
  │   26:     0x7fc99f81a3d7 - >::call_box::h1a58b1b3d97e5f50
  │   27:     0x7fc9a147b2a4 - alloc::boxed::{{impl}}::call_once<(),()>
  │                         at /checkout/src/liballoc/boxed.rs:650
  │                          - std::sys_common::thread::start_thread
  │                         at /checkout/src/libstd/sys_common/thread.rs:21
  │                          - std::sys::imp::thread::{{impl}}::new::thread_start
  │                         at /checkout/src/libstd/sys/unix/thread.rs:84
  │   28:     0x7fc99d450183 - start_thread
  │   29:     0x7fc99cf6737c - clone
  │   30:                0x0 - 
  │ ERROR:servo: failed to receive response to font request: IoError(Error { repr: Os { code: 104, message: "Connection reset by peer" } })
  │ called `Result::unwrap()` on an `Err` value: "PoisonError { inner: .. }" (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(1) }, at /checkout/src/libcore/result.rs:860)
  │ stack backtrace:
  │    0:     0x7fc9a0662c2d - backtrace::backtrace::trace::hce6a3819d897a887
  │    1:     0x7fc9a0663112 - backtrace::capture::Backtrace::new::hb5a725a088a2a2fc
  │    2:     0x7fc99f586523 - servo::main::{{closure}}::h9d96b5c32a8afffa
  │    3:     0x7fc9a147c6bd - std::panicking::rust_panic_with_hook
  │                         at /checkout/src/libstd/panicking.rs:550
  │    4:     0x7fc9a147c554 - std::panicking::begin_panic
  │                         at /checkout/src/libstd/panicking.rs:511
  │    5:     0x7fc9a147c489 - std::panicking::begin_panic_fmt
  │                         at /checkout/src/libstd/panicking.rs:495
  │    6:     0x7fc9a147c417 - std::panicking::rust_begin_panic
  │                         at /checkout/src/libstd/panicking.rs:471
  │    7:     0x7fc9a14a947d - core::panicking::panic_fmt
  │                         at /checkout/src/libcore/panicking.rs:69
  │    8:     0x7fc9a0275a39 - core::result::unwrap_failed::h8f88b98f0a536369
  │    9:     0x7fc9a02d756d - ::pending_images::h91063c5b1c3f0432
  │   10:     0x7fc99fe37a59 - script::dom::window::Window::force_reflow::hc4df658a79834119
  │   11:     0x7fc99fe3accb - script::dom::window::Window::reflow::h394a9997bedf7be7
  │   12:     0x7fc99fcacddf - script::dom::document::Document::finish_load::h1d8d2d557ecf4cc8
  │   13:     0x7fc99fdd86b3 - script::dom::servoparser::ServoParser::do_parse_sync::h2676270b9bff96b9
  │   14:     0x7fc99fdd806c - script::dom::servoparser::ServoParser::parse_sync::h4a7419b789141155
  │   15:     0x7fc99fde096a - ::process_response_eof::h0e6d7b63846ee146
  │   16:     0x7fc99fe6447a -  as script::script_thread::Runnable>::handler::h9c76f08103f95c48
  │   17:     0x7fc99fe872b5 - script::script_thread::ScriptThread::handle_msg_from_script::h4fb921a02f36cead
  │   18:     0x7fc99fe7ea8c - script::script_thread::ScriptThread::handle_msgs::{{closure}}::h95a96e772221ddd7
  │   19:     0x7fc99fe79d5a - script::script_thread::ScriptThread::handle_msgs::hb7c261538c9b5201
  │   20:     0x7fc99fe746c7 - script::script_thread::ScriptThread::start::hb46c4910de904afb
  │   21:     0x7fc99f9e0914 - std::panicking::try::do_call::ha18c2fc1e2f674e1
  │   22:     0x7fc9a14836da - panic_unwind::__rust_maybe_catch_panic
  │                         at /checkout/src/libpanic_unwind/lib.rs:98
  │   23:     0x7fc99fae5597 - >::call_box::h667d881caee9bfc3
  │   24:     0x7fc9a147b2a4 - alloc::boxed::{{impl}}::call_once<(),()>
  │                         at /checkout/src/liballoc/boxed.rs:650
  │                          - std::sys_common::thread::start_thread
  │                         at /checkout/src/libstd/sys_common/thread.rs:21
  │                          - std::sys::imp::thread::{{impl}}::new::thread_start
  │                         at /checkout/src/libstd/sys/unix/thread.rs:84
  │   25:     0x7fc99d450183 - start_thread
  │   26:     0x7fc99cf6737c - clone
  │   27:                0x0 - 
  └ ERROR:servo: called `Result::unwrap()` on an `Err` value: "PoisonError { inner: .. }"
  ▶ CRASH [expected OK] /navigation-timing/nav2_test_open_data_uri.html
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 12.0.1
  │ ERROR:script::dom::bindings::error: Error at data:text/html;charset=utf-8,%3C%21DOCTYPE%20html%3E%0D%0A%3Ctitle%3Edata%20URL%20source%20for%20navigation-timing%2Fnav2_test_open_data_uri.html%3C%2Ftitle%3E%0D%0A%3C%21--%20NB%3A%20this%20file%20isn%27t%20actually%20used%20any%20where%21%20--%3E%0D%0A%3Clink%20rel%3D%22author%22%20title%3D%22Google%22%20href%3D%22http%3A%2F%2Fwww.google.com%2F%22%20%2F%3E%0D%0A%3Cscript%3E%0D%0Avar%20observer%20%3D%20new%20PerformanceObserver%28%0D%0A%20%20%20%20function%20%28entryList%29%20%7B%0D%0A%20%20%20%20%20%20%20%20parent.postMessage%28%22observed%22%2C%20%22%2A%22%29%3B%0D%0A%20%20%20%20%20%20%20%20observer.disconnect%28%29%3B%0D%0A%20%20%20%20%7D%29%3B%0D%0Aobserver.observe%28%7BentryTypes%3A%20%5B%22navigation%22%5D%7D%29%3B%0D%0A%3C%2Fscript%3E%0D%0A:6:5 PerformanceObserver is not defined
  │ called `Result::unwrap()` on an `Err` value: Error { repr: Custom(Custom { kind: UnexpectedEof, error: StringError("failed to fill whole buffer") }) } (thread RenderBackend, at /checkout/src/libcore/result.rs:860)
  │ stack backtrace:
  │    0:     0x7f93640e7c2d - backtrace::backtrace::trace::hce6a3819d897a887
  │    1:     0x7f93640e8112 - backtrace::capture::Backtrace::new::hb5a725a088a2a2fc
  │    2:     0x7f936300b523 - servo::main::{{closure}}::h9d96b5c32a8afffa
  │    3:     0x7f9364f016bd - std::panicking::rust_panic_with_hook
  │                         at /checkout/src/libstd/panicking.rs:550
  │    4:     0x7f9364f01554 - std::panicking::begin_panic
  │                         at /checkout/src/libstd/panicking.rs:511
  │    5:     0x7f9364f01489 - std::panicking::begin_panic_fmt
  │                         at /checkout/src/libstd/panicking.rs:495
  │    6:     0x7f9364f01417 - std::panicking::rust_begin_panic
  │                         at /checkout/src/libstd/panicking.rs:471
  │    7:     0x7f9364f2e47d - core::panicking::panic_fmt
  │                         at /checkout/src/libcore/panicking.rs:69
  │    8:     0x7f936414b10d - core::result::unwrap_failed::h482e2085da99dab3
  │    9:     0x7f936419c017 - webrender::render_backend::RenderBackend::run::h586004cebbd294f6
  │   10:     0x7f936413e957 - std::panicking::try::do_call::h17d08941604712ad
  │   11:     0x7f9364f086da - panic_unwind::__rust_maybe_catch_panic
  │                         at /checkout/src/libpanic_unwind/lib.rs:98
  │   12:     0x7f9364155837 - >::call_box::h4111409bfcdae236
  │   13:     0x7f9364f002a4 - alloc::boxed::{{impl}}::call_once<(),()>
  │                         at /checkout/src/liballoc/boxed.rs:650
  │                          - std::sys_common::thread::start_thread
  │                         at /checkout/src/libstd/sys_common/thread.rs:21
  │                          - std::sys::imp::thread::{{impl}}::new::thread_start
  │                         at /checkout/src/libstd/sys/unix/thread.rs:84
  │   14:     0x7f9360ed5183 - start_thread
  │   15:     0x7f93609ec37c - clone
  │   16:                0x0 - 
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: Error { repr: Custom(Custom { kind: UnexpectedEof, error: StringError("failed to fill whole buffer") }) }
  └ Pipeline failed in hard-fail mode.  Crashing!

bors-servo · 2017-03-19T09:30:08Z

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

bors-servo · 2017-03-19T09:30:48Z

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

bors-servo · 2017-03-19T09:56:09Z

💔 Test failed - linux-rel-wpt

SimonSapin · 2017-03-19T12:32:17Z

#14323 again ¯\_(ツ)_/¯

jdm · 2017-03-19T15:26:40Z

@bors-servo: retry

Intermittent crash ("failed to fill whole buffer") in /navigation-timing/nav2_test_open_data_uri.html, /html/browsers/browsing-the-web/navigating-across-documents/javascript-url-query-fragment-components.html, and /html/semantics/document-metadata/the-meta-element/pragma-directives/attr-meta-http-equiv-refresh/allow-scripts-flag-changing-2.html, /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-2.html #14323

bors-servo · 2017-03-19T15:26:44Z

⌛ Testing commit 65e2ed7 with merge fbd561b...

…=SimonSapin serialize font: to empty on non-default subprops Fixes font shorthand serialization so that it serializes to "" when non-default subproperties are defined. These subproperties are those defined in #15033. Adds tests: - font_should_serialize_to_empty_if_there_are_nondefault_subproperties - font_should_serialize_all_available_properties The second test was previously commented out and underwent some cleanup to make it run. ---  - [x] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15036  - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15604)

bors-servo · 2017-03-19T16:06:37Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: SimonSapin
Pushing fbd561b to master...

…o#16028. r=me MozReview-Commit-ID: 9c8jmRqvVEz

…o#16028: Fix reftest expectation format. r=reftest-fix on a CLOSED TREE --HG-- extra : amend_source : 673cc7490524789c15a9dca831c70b6cfe1fe8c4

…o#16028. r=me MozReview-Commit-ID: 9c8jmRqvVEz

…o#16028: Fix reftest expectation format. r=reftest-fix on a CLOSED TREE

…o#16028. r=me MozReview-Commit-ID: 9c8jmRqvVEz UltraBlame original commit: c6e919aa3aa9d1e73d3e649a1bae7c8488f05554

…o#16028: Fix reftest expectation format. r=reftest-fix on a CLOSED TREE UltraBlame original commit: d81996338334449f47ad72be802f564c3d39a43d

…o#16028. r=me MozReview-Commit-ID: 9c8jmRqvVEz UltraBlame original commit: c6e919aa3aa9d1e73d3e649a1bae7c8488f05554

…o#16028: Fix reftest expectation format. r=reftest-fix on a CLOSED TREE UltraBlame original commit: d81996338334449f47ad72be802f564c3d39a43d

…o#16028. r=me MozReview-Commit-ID: 9c8jmRqvVEz UltraBlame original commit: c6e919aa3aa9d1e73d3e649a1bae7c8488f05554

…o#16028: Fix reftest expectation format. r=reftest-fix on a CLOSED TREE UltraBlame original commit: d81996338334449f47ad72be802f564c3d39a43d

highfive assigned metajack Feb 16, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 16, 2017

highfive assigned SimonSapin and unassigned metajack Feb 17, 2017

zploskey commented Feb 18, 2017

View reviewed changes

zploskey force-pushed the reset_font_shorthand_serialization branch from fb0f4a7 to 14b2065 Compare February 22, 2017 18:26

zploskey force-pushed the reset_font_shorthand_serialization branch from 14b2065 to ed3a40a Compare February 22, 2017 22:23

SimonSapin added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-needs-tests New tests have been requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 24, 2017

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Feb 25, 2017

highfive added the S-needs-rebase There are merge conflict errors. label Mar 2, 2017

zploskey force-pushed the reset_font_shorthand_serialization branch from 32b0e1a to 2f8eb9d Compare March 2, 2017 19:10

nox suggested changes Mar 3, 2017

View reviewed changes

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 Mar 19, 2017

highfive added the S-tests-failed The changes caused existing tests to fail. label Mar 19, 2017

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 Mar 19, 2017

bors-servo merged commit 65e2ed7 into servo:master Mar 19, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 19, 2017

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 20, 2017

Bug 1341102: Adjust expectations for servo/servo#15604 and servo/serv…

3fad952

…o#16028. r=me MozReview-Commit-ID: 9c8jmRqvVEz

Manishearth pushed a commit to Manishearth/gecko-dev that referenced this pull request Mar 28, 2017

Bug 1341102: Adjust expectations for servo/servo#15604 and servo/serv…

c4d86d7

…o#16028. r=me MozReview-Commit-ID: 9c8jmRqvVEz

Manishearth pushed a commit to Manishearth/gecko-dev that referenced this pull request Mar 28, 2017

Bug 1341102: Adjust expectations for servo/servo#15604 and servo/serv…

7b8ea67

…o#16028: Fix reftest expectation format. r=reftest-fix on a CLOSED TREE

zploskey deleted the reset_font_shorthand_serialization branch November 1, 2017 04:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

serialize font: to empty on non-default subprops #15604

serialize font: to empty on non-default subprops #15604

zploskey commented Feb 16, 2017 •

edited

Loading

highfive commented Feb 16, 2017

highfive commented Feb 16, 2017

metajack commented Feb 17, 2017

zploskey Feb 18, 2017 •

edited by nox

Loading

nox Feb 19, 2017

zploskey commented Feb 19, 2017

zploskey commented Feb 19, 2017

zploskey commented Feb 19, 2017 •

edited

Loading

zploskey commented Feb 22, 2017

jdm commented Feb 22, 2017

SimonSapin commented Feb 24, 2017

zploskey commented Feb 24, 2017

zploskey commented Feb 25, 2017

zploskey commented Feb 25, 2017

bors-servo commented Mar 2, 2017

zploskey commented Mar 2, 2017

nox left a comment

nox Mar 3, 2017

nox Mar 3, 2017

nox Mar 3, 2017

zploskey Mar 3, 2017 •

edited

Loading

nox Mar 3, 2017

nox commented Mar 4, 2017

bors-servo commented Mar 19, 2017

bors-servo commented Mar 19, 2017

SimonSapin commented Mar 19, 2017 •

edited

Loading

bors-servo commented Mar 19, 2017

bors-servo commented Mar 19, 2017

bors-servo commented Mar 19, 2017

SimonSapin commented Mar 19, 2017 •

edited

Loading

jdm commented Mar 19, 2017

bors-servo commented Mar 19, 2017

bors-servo commented Mar 19, 2017

serialize font: to empty on non-default subprops #15604

serialize font: to empty on non-default subprops #15604

Conversation

zploskey commented Feb 16, 2017 • edited Loading

highfive commented Feb 16, 2017

highfive commented Feb 16, 2017

metajack commented Feb 17, 2017

zploskey Feb 18, 2017 • edited by nox Loading

Choose a reason for hiding this comment

nox Feb 19, 2017

Choose a reason for hiding this comment

zploskey commented Feb 19, 2017

zploskey commented Feb 19, 2017

zploskey commented Feb 19, 2017 • edited Loading

zploskey commented Feb 22, 2017

jdm commented Feb 22, 2017

SimonSapin commented Feb 24, 2017

zploskey commented Feb 24, 2017

zploskey commented Feb 25, 2017

zploskey commented Feb 25, 2017

bors-servo commented Mar 2, 2017

zploskey commented Mar 2, 2017

nox left a comment

Choose a reason for hiding this comment

nox Mar 3, 2017

Choose a reason for hiding this comment

nox Mar 3, 2017

Choose a reason for hiding this comment

nox Mar 3, 2017

Choose a reason for hiding this comment

zploskey Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

nox Mar 3, 2017

Choose a reason for hiding this comment

nox commented Mar 4, 2017

bors-servo commented Mar 19, 2017

bors-servo commented Mar 19, 2017

SimonSapin commented Mar 19, 2017 • edited Loading

bors-servo commented Mar 19, 2017

bors-servo commented Mar 19, 2017

bors-servo commented Mar 19, 2017

SimonSapin commented Mar 19, 2017 • edited Loading

jdm commented Mar 19, 2017

bors-servo commented Mar 19, 2017

bors-servo commented Mar 19, 2017

zploskey commented Feb 16, 2017 •

edited

Loading

zploskey Feb 18, 2017 •

edited by nox

Loading

zploskey commented Feb 19, 2017 •

edited

Loading

zploskey Mar 3, 2017 •

edited

Loading

SimonSapin commented Mar 19, 2017 •

edited

Loading

SimonSapin commented Mar 19, 2017 •

edited

Loading