mozilla :: #servo

11 Jul 2017
01:32dbaronManishearth: canaltinova: btw, I joined the stylo meeting from SFO-7B
01:39luke_nukem[m]-MAnyone have an idea why I would be getting this error on trying to build mozjs_sys? mozbuild.preprocessor.Error: ('', 0, 'FILE_NOT_FOUND', '/home/luke/src/gsoc/mozjs/.cargo/config.in')
01:39luke_nukem[m]-MI can see that it's complaining of missing file.. But why?
01:40luke_nukem[m]-MTrying to trace the cause of that through the build system is doing my head in
01:42luke_nukem[m]-Msighs https://raw.githubusercontent.com/fitzgen/mozjs/smup/mozjs/.cargo/config.in
02:13canaltinovadbaron: Thanks but I left the office :( I'm gonna join tomorrow's meeting
02:28paulCan someone delegate the review of #17652 to me (paulrouget)?
02:28crowbot1PR #17652: Move Ctrl-F12 keybindings outside of libservo - https://github.com/servo/servo/pull/17652
02:45emiliopaul: done :)
02:54paulemilio: thank you.
03:10stshineDo we have incremental display list building for now?
03:17emiliostshine: nope
03:18stshineemilio: so, when animation happens and CSS properties change, we simply resend the entire display list?
03:18emiliostshine: pretty much, yeah
03:19stshineemilio: hmm, okay. If this is feasible for HTML I guess it might also be feasible for SVG...
03:19stshineemilio: thanks
03:20emiliostshine: np :)
03:29heycamxidorn: oh, can you r- that style scoped warning patch
03:29heycamI realise I uploaded the wrong version ;_;
03:29heycam(aOldDocument can be null somtimes)
03:29xidornheycam: ah, but I have pushed the button to land...
03:29xidornheycam: not sure how we can stop that
03:29heycamxidorn: the tree is closed right now
03:30heycamdon't know if r-ing will cancel that request..
03:30xidornheycam: doesn't seem to
03:30heycamok :)
03:30xidornit lands :/
03:31heycamwell I'll just push a fixup once it lands
03:31xidornheycam: it has landed
03:31* heycam types quickly
03:48emilioheycam: ping?
03:48heycamemilio: hi!
03:48emilioheycam: hi! Can I ask you a favor? Could you sanity-check me real quick on the XBL-on-the-root patch on bug 1379505?
03:48firebothttps://bugzil.la/1379505 NEW, emilio+bugs@crisal.io stylo: Isolate style resolution to avoid bugs like bug 1379041
03:48* heycam looks
03:48emilioheycam: it has driven me crazy during the whole evening :)
03:49heycamemilio: which patch is it?
03:49emilioheycam: "Less fishyness when resolving the style of the document element."
03:49emilioheycam: basically, the way it worked before was super-fishy... The LazyRestyleBehavior::Allow made us store the style on the element if it was the initial style
03:50emilioheycam: which means that we'd effectively never hit the "initial style with XBL binding case", which just happened to work
03:51emilioheycam: also, that second style resolution (http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/layout/base/nsCSSFrameConstructor.cpp#2562)...
03:51emilioheycam: looks to me that in servo that doesn't do the right thing
03:52emilioheycam: it really expects to get the new style, but instead we'd return the previous one (since we'd store it)
03:52heycamemilio: interesting, yeah I buy that
03:53heycamthe "we delay styling because doc style sheets haven't applied yet" isn't really a valid thing is it
03:53emilioheycam: right, I think so
03:53heycamI mean who knows at what point our <style> elements are going to come in
03:54emilioheycam: that makes no sense to me, I&#39;d buy the &quot;XBL binding&#39;s stylesheets&quot; could start applying
03:55emilioheycam: but that&#39;s also not the case, we do something extremely weird
03:55heycamemilio: yeah indeed. anyway I think what you&#39;re doing in that patch makes sense.
03:55heycamand nice to see more Asserts instead of Allows :)
03:55emilioheycam: cool! I hope try thinks the same ;)
03:55* heycam will continue reviewing that bug after lunch
03:56emilioheycam: cool, I&#39;ll go to sleep for a bit, have a good day!
03:56heycambfn!
11:47noxcynicaldevil: Ping?
12:17cynicaldevilnox: pong
12:17noxcynicaldevil: Can&#39;t you use tendril 0.3.1 for what&#39;s needed in your pR?
12:17noxPR*
12:19cynicaldevilnox: From what I&#39;ve seen, tendril has been used is Servo by including it like this: &#39;use html5ever::tendril::SendTendril&#39;, for example
12:20cynicaldevilnox: So this means that tendril version in h5e has to be updated first?
12:20noxSimonSapin: What&#39;s the difference between 0.3.1 and 0.4.0?
12:20SimonSapinabout 0.0.9
12:21* SimonSapin reads backlog
12:21SimonSapinoh yea mean tendril
12:21noxSimonSapin: Ah ah ah ah.
12:21SimonSapinnox: removing unsound Sync impl
12:25noxSimonSapin: Ok.
12:25noxSimonSapin: And the new API in 0.3.1 is what?
12:26SimonSapinnox: look at the commit log like I do. &quot;Port NonZero optimization to stable Rust&quot;
12:26noxTrue, got lazy. :D
12:26noxcynicaldevil: What it is that you need again from tendril?
12:26SimonSapin0.4.0 also removed the &#39;unstable&#39; feature
12:26noxI&#39;m so confused right now.
12:26nox&quot;Yes, it requires the new tendril version, which implements clone for Sendtendril. It works, I have compiled this locally using that.&quot;
12:27noxSimonSapin: You added that too, drive-by style, right?
12:27SimonSapin0.3.1 also includes cynicaldevil:derive-clone-for-SendTendril
12:27noxPerfect.
12:27noxcynicaldevil: Just bump tendril in your PR and we can try it.
12:27cynicaldevilok
12:27SimonSapincynicaldevil: ./mach cargo update -p tendril
12:30cynicaldevilnox: Should I add an additional commit to change the preference to use the async tokenizer before testing?
12:30noxcynicaldevil: Yes.
12:30noxSimonSapin: That works without the dash now?
12:30noxcargo-update vs invoking cargo and using its update command.
12:31SimonSapinoh yeah, theyve been approximately the same since we switched to a workspace with a single lock file
12:31noxOh right.
12:31SimonSapin&quot;cargo update&quot; without mach works too
13:26noxcanaltinova: Ping.
13:40noxjdm: Thanks.
13:43noxSimonSapin: Around for a #[derive(ToCss)] patch review?
13:43SimonSapinnox: sure
13:43noxOk, doing the PR pretty soon.
13:58noxSimonSapin: ^
14:53noxSimonSapin: Seems like I broke some stuff with my PR. :(
14:54noxOh found the issue.
14:54noxWe use String for some identifiers.
15:01noxI guess I need to do #[css(identifier)] at some point.
15:02noxemilio: You sure about custom-ident there?
15:03emilionox: they&#39;re parsed as input.expect_ident()?.into_owned()
15:03emilionox: and serialized with serialize_identifier(..)
15:03emilionox: so your code is breaking that
15:03emilionox: which makes tests fail
15:03emilionox: So, relatively sure, yeah :P
15:04noxemilio: Custom idents allow all identifiers?
15:04emilionox: oh, ok, you had debugged it before... good point
15:05emilionox: seems like the counter style parsing function is buggy?
15:05noxWhat?
15:05emilionox: I was surprised by it not returning a result, and using try.unwrap_or(decimal()), but I guess it&#39;s fine because of the parse_entirely calls
15:05emilionox: nevermind about that, was just random
15:06noxCustom idents are https://drafts.csswg.org/css-values-4/#custom-idents
15:06SimonSapinjack: Did anything come out of SF discussions about a policy for incrementing the minimum Rust version required by Gecko?
15:06emilionox: yeah, I&#39;m going through the counter-style spec
15:06jackSimonSapin: i believe they are planning to be no more than one version behind.
15:07jackand they will update tooltool for new releases, so people can build on $current but mininum require will be $current-1
15:08SimonSapinjack: So requiring 1.N when 1.(N+1) is stable should be expected without making a case for it every time? Sounds good. Can we get someone from their team to write it down?
15:08jackbut iw asn&#39;t at the meeting, so i&#39;m paraphrasing from what i heard
15:08jacki may have it written down. i&#39;ll check
15:09SimonSapinI mean to dev-platform, or somewhere we can point to when someone inevitably wants to argue
15:11emilionox: yeah, so this is is about whether to accept the inherit and such keywords... Seems like counter-style already uses CustomIdent(Atom::from(foo)) directly in a few places
15:12emilionox: so maybe you should do the same?
15:13noxemilio: https://drafts.csswg.org/css-lists-3/#counter-functions
15:13noxemilio: This is about the first <ident>,
15:13noxnot <counter-style>.
15:13noxemilio: I&#39;ll remove that change for my commit for now.
15:14emilionox: right, I&#39;m just saying that there&#39;s a precedent for using CustomIdent with a random ident atom without the filtering
15:14noxemilio: Ah.
15:14noxemilio: Not sure that&#39;s a good idea though.
15:14noxemilio: Wait what do you mean? What&#39;s foo?
15:16noxemilio: We do that with either Atoms coming from Gecko, or with some codegen that filters &#39;foo&#39;, AFAIK.
15:17emilionox: yeah, I see, nvm me then
15:17jackSimonSapin: please send email to jonathan. he was at the meeting and probably took notes. I think it&#39;s totally reasonable to make sure the decisions get published somewhere obvious.
15:24cynicaldevilnox: Any idea why this is happening on the travis build for my PR? https://travis-ci.org/servo/servo/jobs/252421643#L2464
15:25emiliocynicaldevil: travis is out of disk space, it&#39;s happening on every PR
15:25emiliolarsberg: jack: ^
15:25cynicaldevilah
15:25emiliowell, I guess it&#39;s disk space, but who knows
15:29larsbergedunham: ^ travis b0rk3n :-(
15:29larsberg(do we just need to clear caches, I assume?)
15:29larsberghahaahaha https://travis-ci.org/servo/servo/caches 1.2gb yeah probably too big
15:30larsbergBLAMMO
15:45canaltinovanox: pong
15:58fensterservo build still exiting with return value -4
16:01mbrubeckfenster: Any other error messages?
16:01bholleycpeterson: is vidyo down or is it just me?
16:02cpetersonbholley: works for me.
16:04fenstermbrubeck: https://github.com/servo/servo/issues/17642 this link
16:04crowbot1Issue #17642: Servo fails with stacktrace for every website - https://github.com/servo/servo/issues/17642
16:04fensterexactly
16:05mbrubeckfenster: ah, got it
16:11fenstermbrubeck:any suggestions?
16:12noxcanaltinova: Apart from quirkiness,
16:12noxcanaltinova: what&#39;s the difference between LegacyPosition::parse_quirky and TransformOrigin::parse?
16:13noxcanaltinova: Never mind I&#39;m dumb.
16:13emiliofenster: I believe waiting for a nightly after https://github.com/rust-lang/rust/pull/43116, or use a local nightly with a .servobuild file
16:13crowbot1PR #43116: Update compiler_builtins submodule for probestack fix - https://github.com/rust-lang/rust/pull/43116
16:14canaltinovanox: :)
16:15canaltinovatransform one has a z-offset additionally
16:26jryansjdm: meta bug 1357715 has various things that block devtools tests, and meta bug 1322657 more generally collects anything devtools related with stylo
16:26firebothttps://bugzil.la/1357715 NEW [meta] Stylo: Pass DevTools tests
16:26firebothttps://bugzil.la/1322657 NEW [meta] stylo: support devtools
16:29_____vinsci_____looked at the servo source a bit a while ago. perhaps I&#39;m worrying without reason, being new to both rust and servo, but I did not expect to see thousands of &#39;unsafe&#39; declarations and calls. how many of these are part of the core servo engine as opposed to part of glue code?
16:30mbrubeck_____vinsci_____: The vast majority are in generated glue code
16:30_____vinsci_____yes, about 8000 or so
16:31dbaronbtw, the point I made in yesterday&#39;s meeting about stylo crashes is that a bunch of them are being filed in layout as new nightly topcrashes without any indication that they&#39;re stylo related. But I think that&#39;s getting dealt with at this point...
16:31nox_____vinsci_____: A lot of them in the script crate are due to us not having the current bandwidth in terms of time to make safe wrappers for the JS engine.
16:32nox_____vinsci_____: There are lots of unsafe code, but at least we can see it is unsafe.
16:36cpetersonjdm: bug 1373878 has more info about running the bindgen struct layout tests that might help with your one-off test run on Linux32.
16:36firebothttps://bugzil.la/1373878 NEW, nobody@mozilla.org Run stylo struct layout tests on automation.
16:43_____vinsci_____nox, yes. it is not as clear as it could be, though, as not only the declaration of an unsafe part of the code as well as calls are marked unsafe in current Rust. Perhaps Rust code would be easier to analyze if calls to unsafe code was marked &#39;trusting&#39; instead of &#39;unsafe&#39;.
16:43nox_____vinsci_____: What?
16:44_____vinsci_____erm, sry
16:44noxOf course calls need to be marked unsafe,
16:44noxotherwise what would be the point of it?
16:46nox_____vinsci_____: Oh you mean two different keywords? What about an unsafe function that calls other unsafe functions?
16:46_____vinsci_____nox, &#39;trusting&#39; would have the same semantics, but would make it clear that otherwise safe code is trusting that unsafe code.
16:46noxWhat would that bring to the table?
16:46noxSafe code that calls unsafe code doesn&#39;t trust the unsafe code,
16:47noxit&#39;s the unsafe code that trusts callers to do the right thing.
16:49_____vinsci_____ok, I&#39;m using &#39;trust&#39; in the sense that something you &#39;trust&#39; is something that could hurt you, such as unsafe code.
16:49noxIt can hurt you only if you call it with the wrong inputs.
16:49_____vinsci_____or if it is opaque and evil
16:50mbrubeck_____vinsci_____: There were a number of discussions in pre-1.0 days about alternate keywords for one or both uses of `unsafe`
16:50noxEvil?
16:50mbrubeckbut that ship has sailed by now...
16:50noxUnsafe code has no relation with good or evil.
16:50_____vinsci_____mbrubeck, I guess I&#39;m looking for that discussion to figure out the background.
16:50mbrubeck_____vinsci_____: Let me see if I can figure out the background...
16:51mbrubeck_____vinsci_____: By the way, I read `unsafe { ... }` as `checked_manually_instead_of_automatically { ... }`
16:51nox_____vinsci_____: Safe code should never call unsafe code if it can&#39;t know for sure that it will not do wrong things.
16:51noxIf some code needs to call unsafe code without knowing if it will do the right thing, that code needs to be unsafe itself.
16:52_____vinsci_____nox, for an explanation of where I got this sense of &#39;trust&#39; and &#39;trusted&#39;, see the Trusted Computing FAQ, www.cl.cam.ac.uk/~rja14/tcpa-faq.html
16:53noxThat&#39;s not what safety as meant in Rust is about, AFAIK.
16:53mbrubeck_____vinsci_____: heh, https://github.com/rust-lang/rfcs/pull/117
16:53crowbot1PR #117: rename &#39;unsafe&#39; to &#39;trusted.&#39; - https://github.com/rust-lang/rfcs/pull/117
16:54cpetersonjryans: I filed bug 1380053 about running Stylo tests on Mac, Windows, and Linux32.
16:54firebothttps://bugzil.la/1380053 NEW, nobody@mozilla.org stylo: Enable Stylo tests on all desktop platforms: Linux32, Linux64, Win32, Win64, Mac
16:54jryanscpeterson: great, thanks!
16:57noxmbrubeck: Let&#39;s just do like svn blame,
16:58noxmbrubeck: provide a myriad of synonyms. :P
16:58noxmbrubeck: inscrutable { ... }
16:58noxprobably_fine { ... }
16:58mbrubeckThere is also the infamous https://github.com/rust-lang/rust/issues/666
16:58crowbot1Issue #666: &quot;unsafe&quot; doesn&#39;t sound unsafe enough - https://github.com/rust-lang/rust/issues/666
16:58_____vinsci_____summer_intern { ... }
16:59mbrubeckooh, issue 666 celebrates its 6th birthday today!
16:59crowbot1PR #666: Simplify HTMLCollection predicates - https://github.com/servo/servo/pull/666
16:59noxemilio was an intern and I trust him more around unsafe code than I trust myself so that doesn&#39;t work. :P
16:59mbrubecknot that one, crowbot
17:00noxgh666
17:00noxAh that doesn&#39;t work here?
17:00noxOh right that&#39;s not rustbot.
17:01mbrubeck!gh666
17:09cynicaldevilI demand an A-amusing label for Servo too.
17:12jdmthere is nothing amusing about servo
17:12bradwerthmacOS support in nightly is great; I&#39;m enjoying my tasty dogfood nom nom
17:14catleejryans: hey, I have some try pushes trying to enable stylo tests with the new env var on the regular linux builds
17:14catleehttps://treeherder.mozilla.org/#/jobs?repo=try&revision=3aabc1d2ee6e06087720cbae6d016f3cea7ca45d
17:14catleegetting a lot of orange
17:16jryanscatlee: hmm, let&#39;s see...
17:17emiliocatlee: given most of them are unexpected passes, I suspect stylo isn&#39;t really enabled there
17:17catleeI wonder if I rebased on top of the envvar support...
17:19jryanscatlee: here&#39;s the patch, you can check nsLayoutUtils.cpp https://hg.mozilla.org/mozilla-central/rev/05d1610a4d79
17:20jryanscatlee: looks like it&#39;s in your branch...
17:21catleeyeah
17:21emiliojryans: does the environment get propagated to the content process?
17:22emiliojryans: well, I guess otherwise there&#39;d be no orange in the non-e10s tests, so something else goes on there...
17:22jryansemilio: hmm, well, i would expect it to by default...
17:23catleeI think I see the variable getting set
17:23SimonSapinbholley: so, obj-x86_64-pc-linux-gnu/toolkit/library/libxul.so is 1.5 GB with --enable-optimize and --disable-debug. Am I missing something?
17:24jryanscatlee: hmm, so the mozinfo from the build task says stylo: false. is stylo getting disabled at build time?
17:25jryanscatlee: ah, nevermind, i guess the mozinfo value is based on the MOZ_STYLO_ENABLE, not MOZ_STYLO
17:25catleeso stylo should be compiled in, but off by default
17:26jryanscatlee: ah, but won&#39;t the mozinfo affect which tests are run?
17:26catleeI don&#39;t know
17:26catleeI don&#39;t think so...
17:26jryanscatlee: yes, it will. the failing tests in your run are marked &quot;skip-if = stylo&quot;
17:27catleeah, hm...
17:27catleeok
17:27jryanscatlee: so we need to correct the mozinfo somehow
17:27jryanscatlee: http://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/mozinfo.py#85
17:29jryanscatlee: i guess the trick is that the mozinfo is produced at build time...
17:29jryanscatlee: i wonder if we have previously adjusted skip-if decisions at runtime...?
17:32bholleySimonSapin: --disable-profiling? Not sure if it&#39;s the default
17:34ajeffreystandups: Debugging test failures in PR #16508.
17:34crowbot1PR #16508: Properly set origin of fetch requests - https://github.com/servo/servo/pull/16508
17:34standupsOk, submitted #48088 for https://www.standu.ps/user/ajeffrey/
17:35catleejryans: yeah, I don&#39;t know
17:36bholleyemilio: yt?
17:37emiliobholley: yup, though not for long
17:37bholleyemilio: just need a null-check review
17:37bholleyemilio: bug 1379893
17:37firebothttps://bugzil.la/1379893 NEW, bobbyholley@gmail.com stylo: Crash in Gecko_GetStyleContext
17:38SimonSapinbholley: Trying. Or is there another step thats part of &quot;mach dist&quot; but not &quot;mach build&quot;?
17:39bholleySimonSapin: there might be, I&#39;m really not sure. You probably want glandium or gps
17:39emiliobholley: so, how can that happen, exactly?
17:39emiliobholley: in particular, can a pres-context exist without a presshell?
17:39emiliobholley: if not, something extremely wrong is going on there
17:39bholleyemilio: I don&#39;t follow
17:40gpseh?
17:40jryanscatlee: okay, i guess i&#39;ll ask a test automation peer about the right why to pass that info down
17:40jryans*way
17:40emiliobholley: so, we&#39;re supposed to call there just from restyling, right? That needs a styleset, which needs a pres-context
17:40mbrubeckSimonSapin: There&#39;s &quot;mach package&quot;
17:40bholleygps: SimonSapin is trying to measure codesize but gets enormous libxul, and is wondering how to produce the binaries that get measured
17:41emiliobholley: so _hopefully_ the pres-context exists... can there be a &quot;detached&quot; prescontext without a shell?
17:41jdmajeffrey: good sleuthing. I&#39;ve added pointers to the culprits, as far as I can tell.
17:41bholleyemilio: the shell might exist too, it just may be disconnected from the document
17:41mbrubeckSimonSapin: `mach package` puts the files to package in $OBJDIR/dist
17:41gpsi would look at the build logs in automation and copy configure options as closely as possible
17:41bholleyemilio: in general this entire function will go away with Manishearth&#39;s patches
17:41emiliobholley: oh, right, we&#39;re going through the OwnerDoc...
17:41bholleyemilio: I just wanted to patch the null check drive-by
17:42bholleyemilio: http://searchfox.org/mozilla-central/source/dom/base/nsIDocument.h#860
17:42gpsalso, pgo builds on windows can be a bit wonky
17:42emiliobholley: ok, sounds fine, I guess... r=me
17:42bholleyemilio: thanks
17:43emiliobholley: thanks for the explanation
17:45Manishearthbholley: which function?
17:45ajeffreyjdm: Thanks!
17:45bholleyManishearth: Gecko_GetStyleContext
17:45emiliobholley: btw, this is the underlying bug I was talking you about yesterday, upon reflection I was pretty unclear :)
17:45emiliobholley: https://reviewboard.mozilla.org/r/155914/diff/#index_header
17:45ajeffreyAnd we&#39;re now at 123 comments on that PR!
17:45bholleyemilio: looking
17:46Manishearthbholley: ah, good point, i should kill that too
17:46catleejryans: there&#39;s a thing called pushPrefEnv, which the tests can use to set prefs for individual tests
17:46emilioManishearth: yes, please, kill it with fire
17:47Manishearthemilio: ... what is it even doing
17:47bholleyemilio: ah I see
17:47Manishearthemilio: I assume this should just be element.data.primary().blah ?
17:47emilioManishearth: getting the old style context for the element/pseudo
17:47emilioManishearth: yeah, pretty much
17:47jryanscatlee: yes, but we&#39;ve already got stylo running, so we don&#39;t need to modify the pref... it&#39;s just about deciding which tests should even be run at all
17:47Manishearthemilio: but ... why call the frame constructor
17:47emilioManishearth: but we need a nsStyleContext there
17:48emilioManishearth: display: contents stores their style in the undisplayed map
17:48Manishearthemilio: .primary() is an nsstylecontext now
17:48emilioManishearth: well, not yet
17:48emilioManishearth: that&#39;s why I want you to kill it with fire :)
17:48Manishearthemilio: in my tree it is :)
17:48Manishearth(I&#39;m talking in the context of my tree, sorry)
17:48catleejryans: what if you leave the env var disabled, and then use that to flip the pref for individual tests?
17:49jryanscatlee: i don&#39;t think it&#39;s a good match, because we don&#39;t want to _always_ run the tests with the pref in a single value.
17:49SimonSapinbholley: --disable-profiling forced a rebuild of C++ code so maybe wasnt the default, but didnt affect the size of libxul
17:49emiliobholley: let me know if that patch doesn&#39;t make sense to you. But the previous behavior was super-fishy (and indeed that&#39;s the patch that fixes a few of the reftests I think)
17:50SimonSapinmbrubeck: obj-x86_64-pc-linux-gnu/dist/firefox/libxul.so is 110 MB, much better, thanks!
17:50jryanscatlee: it&#39;s also just much nicer to see what conditions fail for test from the short annotations in the manifest files
17:51bholleyemilio: I only skimmed the patch but it seems sensible. Doing the traversal eagerly and then relying on the XBL-detection to cull it seems like a nice approach, assuming that the XBL detection actually works :-)
17:51bholleyemilio: if not we style the document twice :-)
17:52SimonSapinah but of course without debug info trying to show size by symbols is useless
17:52emiliobholley: Note that that&#39;s what happened before afaict... The problem was LazyComputeBehavior::Allow ended up sticking the data, so the XBL detection didn&#39;t work before
17:53emiliobholley: with my patch we wouldn&#39;t have data there, and when styling the binding it would fail due to the parent being unstyled
17:56Manishearthok so transitions stopped working completely :|
17:57SimonSapinitsafeature :]
17:58nehajdm : Hi
17:58jdmhi neha
17:59nehajdm : I have so many doubts so I am shooting it now
17:59jdmok!
18:00wafflesheh
18:01nehahttps://html.spec.whatwg.org/multipage/images.html#adaptive-images as I am following this...
18:02nehawaffles : hi
18:02nehajdm, waffles : in 3rd point can you tell me how to define source set here
18:03emilionox: btw, killing 850 lines of code in https://bugzilla.mozilla.org/show_bug.cgi?id=1379505 :-)
18:03firebotBug 1379505 NEW, emilio+bugs@crisal.io stylo: Isolate style resolution to avoid bugs like bug 1379041
18:03Manishearthbirtles: there?
18:03emilioManishearth: I can probably help with transitions too
18:04emilioManishearth: did you forget a call to process_animations somewhere somewher?
18:04Manishearthemilio: yeah, basically, transitions are broken but animations are not -- any idea where the bug would be?
18:04Manishearthemilio: uuhhhhhhhhhhhhh idk? :)
18:04Manishearthi don&#39;t recall removing any functionality related to animations
18:04emilioManishearth: that sounds like you broke get_after_change_style
18:04* emilio needs to run for a bit
18:06jdmneha: sorry, which point is the 3rd point?
18:07nehajdm : 3. Let candidates be an initially empty source set.
18:08jdmneha: oh, that&#39;s https://html.spec.whatwg.org/multipage/images.html#parse-a-srcset-attribute, not the link you gave me :)
18:08nehajdm : sorry for that I didn&#39;t notice
18:08jdmneha: step 3 is an empty list. what is the question, exactly?
18:08jdmoh, &quot;source set&quot;
18:09jdmneha: the source set will be a vector of image sources
18:09jdmwhere an image source is a structure that contains this information: https://html.spec.whatwg.org/multipage/images.html#image-source
18:14nehajdm : Is it true -> select-image-sources (https://html.spec.whatwg.org/multipage/images.html#selecting-an-image-source) and image sources are same
18:14jdmneha: yes
18:18nehajdm : next ques is--> descriptors, descriptor or In Descriptor. They all are different or not?
18:22canaltinovajyc: hey you want to land this right?
18:22jyccanaltinova: yep! was reviewed by manish on mozreview!
18:22canaltinovajyc: oh ok, it was wip before so I wanted to ask
18:22jyccanaltinova: oh oops, thanks!
18:22Manishearthlol yeah bors won&#39;t merge something with WIP in the title
18:22jychaha
18:23jdm-Neha: In Descriptor is a label for a step of the algorithm
18:24jdm-Neha: descriptors is a list, and descriptor is one of the elements of the list
18:30nehajdm : inside 13th point you will find something like this &quot;If width and density are not both absent, then let error be yes.&quot;
18:31nehathis simply means that &quot;if width and density is present, then let error be yes&quot;
18:50jdm-Ack, this is frustrating to look up on a phone
18:50jdm-Rakhisharma_: at a high-level, MediaQuery::parse lives in comonents/style/media_queries.rs
18:52jdm-Rakhisharma_: depending on how the case specification defines a media query, we may want to either refactor that code to depend on a new MediaCondition::parse, or copy the code instead
18:52jdm-Bah, the css specification
18:53jdm-Since the specification says a mefia-query is a media-condition or some other input that starts with `not`, we probably want to tefactor the existing implementation
18:58poesiajdm: Do you know where in the test harness I can find the event that is likely not being fired and causing the timeout in #16091? I&#39;m looking at tests/wpt/harness/wptrunner/executors/reftest-wait*.js, but adding console.log or simply log() isn&#39;t showing anything in stdout/stderr (or is it printed somewhere else?)
18:58crowbot1PR #16091: Fix iframe.onload being fired twice on about:blank. - https://github.com/servo/servo/pull/16091
18:58poesiajust setting iframe onload=&quot;...&quot; or body onload=&quot;...&quot; for printing a message works. It seems to be something more subtle
19:00jdm-poesia: I liked a little bit; the test harness does not control the screenshot. There is code in the compositor that takes a screenshot and then exits, and code in the constellation that keeps track of when a page is ready for a screenshot
19:00jdm-The test harness just waits until the process exits and retrieves the screenshot from the designated file
19:00jdm-S/liked/lied
19:01rakhisharma_jdm-: So, As I don&#39;t see anything related to media-condition(I mean doc/code). do you mean, I have to replace the whole media-query to media-condition(as I can see how board media-query is)?
19:01poesiahmm, I see. I guess the constellation event should be easier to find then
19:01poesiajdm: thanks :)
19:01jdm-Poesia: you can replicate the test behaviour with ./mach run test.html -o foo.png
19:03poesiajdm: nice, it hangs!
19:03jdm-Rakhisharma_: so I&#39;m not sure about whether Expression is the same thing as the media-condition that the spec defines
19:03jdm-If it is, we can use that instead
19:06nehahttps://drafts.csswg.org/mediaqueries/#typedef-media-condition
19:07nehajdm : In the abouve link we can see that media-query = media-condition. so we don&#39;t need to refactor that code
19:07jdm-Rakhisharma_: ok, so the media-condition definition is more recent than the specification that was used to implement MefiaQuery::parse
19:07jdm-That explains my confusion
19:07nehajdm : https://drafts.csswg.org/mediaqueries/#typedef-media-query sorry this one
19:08jdm-Neha: the problem is that than expressions also has an |, which matches syntax that we don:&#39;struth want to support
19:08jdm-Sigh, don&#39;t want to
19:09nehajdm : yeah you are right
19:10rakhisharma_jdm-: So, what is the conclusion then? I am confuse :/
19:11jdm-Rakhisharma_: I think we can do something that&#39;s not too difficult, and use MediaQuery::parse as a model
19:12rakhisharma_jdm-: we are using that in code already, right?
19:13jdm-Rakhisharma_: I mean that we will need to write a new parser and use that instead, but we can use MediaQuery::parse as inspiration
19:15rakhisharma_jdm-: ok, so media-condition is our new parser here?
19:15jdm-Rakhisharma_: just keep using MQ::parse for right now and I will write down my plan separately
19:18rakhisharma_jdm-: ok. thanks :)
19:19nehajdm : &quot;Collect a sequence of code points&quot; is also available somewhere?
19:20jdm-Neha: that means take a sequence of characters
19:20jdm-Neha: so one option is to use string.chars() to get an iterator over the characters
19:21jdm-Then you could use things like take_while to control how man, for example
19:22jdm-S/man/many
19:23nehajdm : ok :)
19:44Gankroget_inheritedtext is defined where? stylo?
19:45Gankro(I need to know the types it holds, grep isn&#39;t helping)
19:47jdmGankro: it&#39;s generated from the properties.mako.rs
19:47jdmGankro: http://doc.servo.org/servo/index.html?search=get_inheritedtext
19:48Gankrojdm: &quot;pub text_shadow: T,&quot;
19:48jdmGankro: they&#39;re clickable Ts!
19:49Gankrojdm: what is this hell dimension
19:49Gankro(found what I needed, thanks)
19:50jdmGankro: we like our documentation to feel like a twine game
19:52Gankrojdm: so who do I have to wrastle to make changes to the set of layout display items
19:52jdmGankro: gw and mrobinson, probably
19:53Gankrosweet, that should be easy
19:53Gankroin that they&#39;ve already bought into the webrender version of this change, mostly
19:57Gankrois BaseDisplayItem a base-class sorta thing? So every item needs to have a Box even if there&#39;s literally no state for it?
19:59Gankromrobinson: ^
20:05Gankrooh right, spain
20:07emilioGankro: It is a base class sorta thing. Display items used to not be boxed, but turns out that we memmoved them a lot and boxing them gave a 10% speedup off-hand
20:07emilio*for free
20:07Gankroemilio: so something will blow up if I make an item with no associated data?
20:07emilioGankro: I guess so... what are you trying to do?
20:08Manishearthemilio: so the bug was introduced in part 9 it seems
20:09Manishearthwhich is weird, part 9 shouldn&#39;t change behavior at all?
20:09Gankroemilio: I&#39;m adding these two display items to webrender, and they need mirrors in the servo display lists https://github.com/servo/webrender/pull/1454/files#diff-937670fa1a68b9bb7dab70622773f50d
20:09GankroPopTextShadow, notably, has absolutely no data
20:12emilioGankro: DisplayItem&#39;s have a fn base() that is supposed to return the base display item... I guess if you&#39;re pretty sure that won&#39;t be called for your DI you can unreachable! it, but otherwise...
20:13Gankroemilio: gotcha -- not gonna take the risk on that, someone else can optimize it out if they want
20:14Gankroemilio: other question: can I reuse bases? Should I? (pushing a series of items in a loop)
20:15emilioGankro: I suppose that you can, given the info they contain shouldn&#39;t be affected by it, should it? (thinking of text shadows)
20:16Gankroemilio: it seems fine to me, but what do I know
20:17emilioGankro: do that, mrobinson, gw, pcwalton and I can review it once it&#39;s up on GH :)
20:17emilios/and/or
20:17Gankrosure thing
20:20Gankroemilio: last(?) question: would you care if I just make PushTextShadow store a SimpleShadow directly? (does that some sort of &quot;style shall not leak this far&quot; rule?)
20:22Gankro(BoxShadowDisplayItem redeclares the fields)
20:24emilioGankro: that sounds fine to me, I don&#39;t know why wouldn&#39;t we want to have style types on layout... Seems like duplicating code is a worse alternative
20:24Gankroemilio: the only thing I see is that the offset is two ints rather than a Vector
20:26emilioGankro: shrug, I&#39;d kill that with fire, or make style have a Vector
20:27Gankroemilio: kill what with fire?
20:27Manishearthemilio: ping
20:30GankroAh I do need to resolve the color, probably need to copy the fields then
20:36emilioManishearth: pong
20:37Manishearthemilio: yeah, so you were saying something about get_after_change_style?
20:37Manishearthemilio: it&#39;s part 9 that causes the breakage, part 9 isn&#39;t doing much though
20:38emilioManishearth: well, get_after_change_style is the first thing that comes to mind if it _only_ breaks transitions
20:38Manishearthyeah
20:38Manishearthi haven&#39;t touched that func in part 9
20:39emilioManishearth: where&#39;s part 8 btw?
20:39Manishearthemilio: it died
20:39Manishearthemilio: (it&#39;s unnecessary now)
20:40* emilio trying to load part 9 in mozreview, failing
20:41Manishearthemilio: wat
20:41Manishearthemilio: pushing latest up just in case
20:41emilioManishearth: can&#39;t have enough shitty internet :)
20:42Manishearthemilio: home internet, or new office internet?
20:42emilioManishearth: temporary housing internet
20:42emilioManishearth: its 11pm here :)
20:42Manishearthah
20:42Manishearthyou&#39;re not in the office at 11pm? shame.
20:44emilioManishearth: I haven&#39;t been on the office yet, actually (I start the 17th)
20:44emilioManishearth: I don&#39;t see anything obvious in that patch off-hand
20:44ManishearthI know, I&#39;m poking fun :)
20:49Manishearthemilio: any idea what i should look into then?
21:01Manishearthemilio: seems like has_css_transitions() returns false?
21:09canaltinovawaffles: why gecko, why :( there is one last bug to fix and it&#39;s very annoying. It&#39;s not even a bug, we just need to do what gecko does in an unspecced serialization
21:09wafflescanaltinova: which one?
21:09waffles(I presume it&#39;s grid) :)
21:10canaltinovawaffles: yep, we needed to convert `repeat(2, [a] 20px)` into `[a] 20px [a] 20px` before to follow gecko. now it&#39;s being a problem in serialization because there can&#39;t be consecutive line names
21:11canaltinovawaffles: for example `[x] repeat(2, [a] 20px)` should be `[x a] 20px [a] 20px` instead of `[x] [a] 20px [a] 20px` latter is invalid
21:11canaltinovawaffles: and it has many edge cases
21:11wafflescanaltinova: annoying indeed
21:11wafflescanaltinova: did you notice the repeat(2, 0 0 []) ?
21:12waffles(which is invalid too)
21:12wafflesehm, repeat(auto-foo, 0 0 [])
21:13canaltinovawaffles: wait it&#39;s invalid right?
21:13wafflesaccording to new spec?
21:13canaltinovait&#39;s in gecko
21:14canaltinovaoh, right. It&#39;s not according to new spec but it was in the old spec
21:14canaltinovaso gecko implemented the old one
21:14wafflesah ok
21:15wafflescanaltinova: once the spec changes, it looks like a lot of stuff from subgrid will be stripped and rewritten
21:16canaltinovawaffles: I have added a fixme for that behavior: https://dxr.mozilla.org/servo/rev/ee95bda3bff01d4ca0e1987a0355d01238d60cf6/components/style/values/specified/grid.rs#147
21:16canaltinovawaffles: I guess so
21:17wafflescanaltinova: cool, how many failures left?
21:18canaltinovawaffles: 21. other tests in the stylo-failures are disabled for now. I need to try them to enable at some point
21:18wafflescanaltinova: nice!
21:19canaltinovawaffles: all 21 failures are related to that repeat - line name behavior
21:20wafflescanaltinova: yeah, heh, that happened a lot of times in grid - I fix one tiny bug, and I could chop at least 50 failures
21:21canaltinovawaffles: heh, yeah
21:21wafflesit was exponential though - the final hundred or so failures needed a lot of fixes (or worse yet, hacks)
21:21canaltinovaI hope that will end soon :)
21:21wafflescanaltinova: made my escape safely, didn&#39;t I? :p
21:22canaltinovawaffles: heh :)
21:25emilioManishearth: huh, weird... that&#39;s because we&#39;re not creating the transitions?
21:25emilioManishearth: it literally just checks that the animation collection is not empty
21:26Manishearthemilio: yeah seems like it
21:26Manishearthemilio: i made it return true and it still didn&#39;t work
21:26emilioManishearth: just check whether we&#39;re calling process_animations correctly
21:26Manishearthif it returns true then the without_transition_rules stuff fails
21:26Manishearthemilio: &quot;correctly&quot;?
21:26emilioManishearth: perhaps you just passed the wrong style there?
21:26emilioManishearth: it needs the old and new primary style
21:26Manishearthoh good point
21:27Manishearthemilio: