mozilla :: #servo

15 May 2017
02:12birtlesWhat do I need to do to qualify for try privileges?
02:20stshinebirtles: you need to make a PR to add your github account here: https://github.com/servo/saltfs/blob/master/homu/files/cfg.toml#L224
02:21birtlesstshine: thanks!
02:21stshinenp :)
02:21birtlesstshine: are there any criteria for the PR being accepted?
02:21birtlese.g. number of accepted PRs etc.?
02:21stshinebirtles: I belive it is not a problem for you :)
02:21birtlesstshine: ok great, thanks!
02:23stshinetry privileges are given out quit permissively
02:23stshinequite*
02:46gwis homu busted or just slow? https://github.com/servo/servo/pull/16860
02:46crowbotPR #16860: Update WR (preserve-3d, AA improvements) - https://github.com/servo/servo/pull/16860
03:07Manishearthgw: https://www.youtube.com/watch?v=soiw4Bh6lig
03:08Manishearthkicked it
03:08Manishearthlets see
03:13gwManishearth: thanks
03:16paulgw: are programs ever removed from the Device::programs hashmap?
03:17paulgw: https://github.com/servo/webrender/blob/eb4e33e61ad4fa0640123310992fdc729f2d339c/webrender/src/device.rs#L1472
03:18paulgw: trying to make sense of the assert that I see when using 2 WR instances: https://github.com/servo/webrender/issues/1233#issue-227904881
03:18crowbotIssue #1233: 2nd window stays blank - https://github.com/servo/webrender/issues/1233
03:19paulI have the same issue with textures
03:32paulgw: Ignore this. I think I know what's going on.
03:55gwpaul: They are probably never removed, right now. They should eventually be removed when destroying the renderer on shutdown.
03:57paulgw: when there are 2 devices, things are getting weird
03:59paulgw: can you please tell me if that looks weird to you: I print this: println!("device: {:?}, gl {:?}", &self as *const _, &self.gl as *const _);
03:59paulgw: and get this:
03:59pauldevice: 0x7fff5c945bc0, gl 0x7fff5c94d5b0
03:59pauldevice: 0x7fff5c945bc0, gl 0x7fff5c94e9c0
04:00paulgw: see how gl has 2 different addresses? Is that normal?
04:02gwpaul: I think you&#39;re printing the address of the Rc<> containing the GL functions
04:04paulgw: and is it supposed to change? And actually, the 2nd address is the address of self.gl of the other device
04:05paulsorry, probably hard to understand without any context :)
04:07gwpaul: yea, missing a bit of context - it wouldn&#39;t surprise me if they&#39;re different, since the fn ptrs for GL are loaded dynamically
04:08paulSo, in device.rs, when inserting a program or a texture in the hashmaps, it crashes because the program/texture id already exist. These ids are created by device.gl. But for some reasons, when there&#39;s multiple device instances, device1.gl points to device2.gl, and vice-versa.
04:08paulgw: that explains the collision
04:09paulgw: anyway, I&#39;ll file an issue.
04:09gwpaul: hmmm, ok
04:23birtlescan someone please delegate+ https://github.com/servo/servo/pull/16863 for me? (Or just merge https://github.com/servo/saltfs/pull/668 so I can do it myself)
04:23crowbotPR #668: Allow birtles to use try and land PRs - https://github.com/servo/saltfs/pull/668
04:23crowbotPR #16863: Additive animation - https://github.com/servo/servo/pull/16863
04:24birtleshiro: Oh, thank you!
04:24hirobirtles: np
05:01paulgw: if you have some time this week to take a look at this: https://github.com/servo/webrender/issues/1233#issuecomment-301376427 - I attached a patch to show how this fails.
05:01crowbotIssue #1233: 2nd window stays blank - https://github.com/servo/webrender/issues/1233
05:20gwpaul: sure - I&#39;ll take a look at it in between compiles of servo sometime this week :)
06:38xidornheycam: a frame can still hold a pointer to a stale style struct after style flush in stylo?
06:39heycamxidorn: are you finding that, for your counter-style work?
06:39xidornheycam: yes because a test crashes
06:40heycamxidorn: FlushCounterStyles calls PostRebuildAllStyleDataEvent, which isn&#39;t implemented for ServoRestyleManager yet...
06:40heycamthat might be the problem
06:40xidornah
06:40xidornthat explains...
06:41xidornwait... let me think a bit...
06:41xidornheycam: oh, hmmm, yep, that is probably the problem. any idea how to fix it?
06:43* heycam looking
06:43xidornheycam: but if you rebuild the stylist, shouldn&#39;t all data already be dirty?
06:43heycamxidorn: does it throw away the rule tree? I&#39;m not sure it does
06:44xidornmaybe it doesn&#39;t
06:44xidornthere could be some optimization there to avoid rebuilding everything I guess
06:44heycamyeah I think so
06:45heycamxidorn: maybe Servo_StyleSet_RebuildData does that?
06:45heycamit&#39;s called from the synchronous ServoStyleSet::RebuildAllStyleData
06:46heycamno, maybe it doesn&#39;t either
06:46heycam(and maybe it needs to?)
06:52xidornheycam: it doesn&#39;t seem to me we have such a mechanism to throw away the whole rule tree at all?
06:54heycamxidorn: maybe we don&#39;t. oh, that&#39;s going to be tricky, because of the way its gc works...
06:54heycamwe might need to keep a hold on to the old RuleTree, so that we can call gc() on it later
06:58xidorncan we require it to recompute everything without throwing away the rule tree?
06:58xidornI guess probably RebuildAllStyleData works
07:00heycamxidorn: oh I suppose we don&#39;t have the problem of old data being cached in rule nodes
07:00heycamwhich is what I guess requires us to throw away the rule tree in Gecko
07:05xidornheycam: PostRebuildAllStyleDataEvent seems to be just the async version of RebuildAllStyleData?
07:08heycamxidorn: yes I think so
07:10heycamxidorn: although I notice that ServoRestyleManager::RebuildAllStyleData is not doing is synchronously
07:10xidornheycam: yep... that sounds like I can actually use RebuildAllStyleData... but that makes code much more complicated...
07:11xidornprobably I should just copy some code from ServoRestyleManager::RebuildAllStyleData to PostRebuildAllStyleDataEvent?
07:12heycamxidorn: might be fine. oh, the actual StyleSet()->RebuildData() thing is done synchronously, in RebuildAllStyleData, so I&#39;m not sure what the implications are of not doing that work until later
07:27xidornheycam: oh, you dupe the old one into the new one...
07:27heycamxidorn: yeah
07:27xidornI thought the reverse way so add a dependency edge...
07:27heycamxidorn: since you&#39;d already set the needinfo on the new one
07:28xidornheycam: I guess there is a way to workaround this issue that we can avoid cleaning up the retired list after restyle...
07:29xidornthen we are leaking counter style rules within the lifetime of presshell, which is probably fine for now
07:29heycamxidorn: yeah, not a super big deal. but probably should be fixed before shipping
07:29xidornheycam: yep
07:32emilioxidorn: just replied in that bug. If deferring the restyle is causing problems we can make it synchronous
07:33xidornemilio: the problem is the restyle doesn&#39;t rebuild style structs
07:33xidornI&#39;m fine with doing it async...
07:33emilioxidorn: how not? It&#39;s a subtree restyle on the root, we should pretty much recompute everything
07:34xidornthe problem is that the code currently calls PostRebuildAllStyleDataEvent, which is a no-op for stylo
07:34xidornso things are not rebuilt :/
07:35emilioxidorn: Why can&#39;t PostRebuildAllStyleData just call RebuildAllStyleData?
07:35xidornemilio: that&#39;s a good question... can we?
07:36xidornif we can... then great, just do it, and all things relying on PostRebuildALlStyleData would just start to work
07:36emilioxidorn: Why not? the difference between those functions in Gecko is that RebuildAllStyleData is sync IIUC. In Servo that&#39;s not the case, so I&#39;d expect that to be possible
07:37emilioheycam: Is ^ right? Do you know off-hand of any other more subtle difference between PostRebuildAllStyleData and RebuildAllStyleData?
07:37heycamemilio: xidorn: I don&#39;t actually know in what situations we might need the async vs sync calls
07:37* heycam would have to investigate
07:38emilioheycam: the only sync call is RebuildAllStyleData, that I made async after talking with bz when implementing it
07:38emilioheycam: so if that&#39;s a problem, that&#39;s a problem we already have :)
07:39heycamemilio: yeah :)
07:40emilioheycam: well, I guess the restyle is async, but the styleset flush is sync... We could make that async too, I think, but I&#39;ll leave a TODO for now
07:41heycamemilio: I wouldn&#39;t be surprised if the call sites didn&#39;t care about that. but would be good to check of course :)
07:56emilioheycam: err, the signed-off line is because of git-format-patch, sigh, will remove
08:38heycamemilio: did you say you were busy the next few days? got some cascade-related patches was going to send your way, probably tomorrow, but happy to direct them elsewhere too
08:39emilioheycam: I think I can take time to review, I&#39;ll redirect if that happens not to be the case :)
08:39heycamemilio: ok, great :)
09:20xidornemilio: fail to import your patch because your name includes some non-ascii character :D
09:32emilioxidorn: oh, great, wow
09:32emilioxidorn: should I change my name? :D
09:41bholleyemilio: hm
09:41bholleyemilio: is there a reason we can&#39;t just store the pseudo-element inline in the selector like everything else?
09:42emiliobholley: What do you mean? Not, particularly, afaict, but why would that be an advantage? Space?
09:42emiliobholley: note that the SelectorInner is a member of Selector, so the patches don&#39;t introduce more pointer-chasing
09:42bholleyemilio: well, the current patch both makes Selector bigger, _and_ causes us to copy it more often by passing that to the Rule rather than the SelectorInner
09:43bholleyemilio: yeah, not pointer-chasing, just a lot of unconditional memmoving to copy what is 99% a None
09:43emiliobholley: re. the size of selector, that&#39;s right, but that&#39;s why the &quot;convert to an enum&quot; bit is for
09:44bholleyemilio: I haven&#39;t looked at that patch yet, what does it do?
09:45emiliobholley: basically changing PseudoElement from (Atom, bool), to just an enum
09:45bholleyemilio: ok, that wins us back the word we lost by adding the state I guess - but we&#39;re still copying more than we need
09:45emiliobholley: which i was going to need to reject states at parse time without making it take more space
09:46bholleyemilio: maybe it&#39;s ok
09:46emiliobholley: I guess I could hack that up, but it feels less clean, and we&#39;d need to traverse rtl to find the pseudo selector, which isn&#39;t great
09:46emiliobholley: well, or storing the state in the pseudo selector I guess, but still not quite as clean
09:47bholleyemilio: alright, would you mind measuring if this impacts stylist update time in 100x myspace
09:47emiliobholley: sure, I can do that. Also, note that the states we store are only three, so we could compact that even more if needed
09:48bholleyemilio: oh, but I guess there&#39;s also the cache performance hit of storing more stuff in the rule array
09:48bholleyemilio: since we&#39;ll have cache mosses more often
09:48bholleyemilio: (storing Selector instead of SelectorInner in Rule)
09:48emiliobholley: which rule array? You mean in the SelectorMap?
09:49bholleyemilio: yes
09:49bholleyemilio: the entries in the SelectorMap
09:49bholleyso this might also impact bloom-basic
09:50bholleyemilio: where does Chrome store the PE?
09:50emiliobholley: I doubt chrome has this &quot;state in pseudo-element&quot; stuff, so they might not need to do that, but I can look it up
09:51bholleyemilio: they&#39;re currently beating stylo sequential on bloom-basic performance, I&#39;m still digging into why - but I want to match their setup as closely as possible
09:52emiliobholley: with or without style sharing?
09:52bholleyemilio: without style sharing - bloom-basic disables style sharing
09:52bholleyemilio: (each left and right child is a div/span respectively)
09:52heycambholley: does chrome support style sharing across cousins?
09:52bholleyheycam: yes
09:52heycambholley: do your changes to build_dom account for that?
09:53bholleyheycam: but I&#39;m assuming only if the parent styles match
09:53bholleyheycam: which they wouldn&#39;t in this case
09:53emiliobholley: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/CSSSelector.h?l=359
09:53heycamok
09:53bholleyemilio: what would be less clean about treating :: as just another kind of combinator?
09:54bholleyemilio: and storing the PE as just the rightmost sequence of simple selectors?
09:54emiliobholley: well, the fact that it isn&#39;t a combinator
09:54bholleyemilio: I know it isn&#39;t in the spec, but how would it make the code less clean
09:54bholleyemilio: IIRC we talked about treating it as a combinator for restyle hints anyway
09:54emiliobholley: and that you need to look into the sequence to check the selector&#39;s combinator, and tweak the restyle hint machinery to special-case for that to remove the rightmost bit
09:56bholleyemilio: I don&#39;t understand the first bit - we already need to traverse the sequence to find the combinator
09:56bholleyemilio: as for the second - are you talking about when computing dependencies, or something else?
09:56emiliobholley: err, the pseudo, I mean
09:56heycamfwiw in the future we might need to support combinators to the right of pseudos: https://drafts.csswg.org/selectors-4/#pseudo-element-structure
09:56emiliobholley: and the second, I mean that we can&#39;t blindly do if sequence_start == 0 { dep_selector = selector.inner.clone() }
09:57emiliobholley: I guess I can make the pseudo selector be under 32 bit and then the size wouldn&#39;t change at all if it really matters
09:57bholleyemilio: the size still changes because we&#39;re putting more stuff in the array, right?
09:57emiliobholley: which more stuff?
09:58bholleyemilio: we&#39;re putting Selector instead of SelectorInner, right?
09:58emiliobholley: the selector is inner, pseudo, specifity
09:58emiliobholley: the rule had specifity already, which I remove
09:59bholleyemilio: fair - I&#39;d still like to dig into why my proposal isn&#39;t a good idea
09:59bholleyemilio: note_selector already traverses the entire sequence right to left, and already has special pseudo handling - so I&#39;m skeptical that this is very difficult to support there
09:59emiliobholley: it&#39;s not really a bad idea, it&#39;s just a rework which I think it&#39;s not necessary
10:00bholleyemilio: in terms of &quot;checking whether we have a pseudo-element&quot;, we can just just look at the rightmost thing, right?
10:00emiliobholley: and in any case, could I do the rework in a followup bug?
10:00emiliobholley: yes
10:01bholleyemilio: I know it&#39;s a rework, but keep in mind that I&#39;ve also been digging into exactly this machinery and spending hours trying to speed it up, and really want to be deliberate about the performance implications of all these design decisions
10:01bholleyemilio: yes, a followup bug is ok
10:02bholleyemilio: and if you commit to doing it within the next few days, you don&#39;t need to do the measurements before landing this, which should save you a little bit of time
10:02emiliobholley: ok, makes sense, I&#39;ll file, I think those patches are already big enough :)
10:03emiliobholley: yeah, my plan is addressing that as soon as it lands, I just don&#39;t want to bloat a change that is already quite big.
10:03bholleyemilio: sure
10:04bholleyemilio: in terms of quickly detecting the presence of a pseudo - we can just make pseudo state a different enum variant, and then we can just look at the rightmost component and check if its a Pseudo or PseudoState
10:04emiliobholley: I think Pseudo(Impl::PseudoSelector) should be fine, shouldn&#39;t it? Not sure the extra variant makes a lot of sense, and it&#39;d be difficult to not hardcode the state in selectors
10:05emiliobholley: and anyway, those patches should allow making Selector just inner, specificity
10:05emiliobholley: which means that we can store Selectors in rules without any loss :)
10:05bholleyemilio: oh I see, you&#39;re talking about keeping Pseudo bundled with state
10:05bholleysure that&#39;s fine
10:06xidornemilio: it seems your patch doesn&#39;t quite work... it crashes at startup :/
10:07emilioxidorn: which one, the RebuildAllStyleData?
10:07xidornyep
10:07emilioxidorn: the second one was missing a force_dirty call I shouldn&#39;t have removed
10:07emilioxidorn: but only the first should work
10:07xidornk
10:10emiliobholley: filed https://bugzilla.mozilla.org/show_bug.cgi?id=1364850
10:10firebotBug 1364850 NEW, emilio+bugs@crisal.io stylo: Store pseudo-element selectors as another Component.
10:22bholleyemilio: hm, I&#39;m also really worried about that extra branch in get_matching_rules
10:23bholleyemilio: will it go away with your followup patch?
10:23bholleyemilio: basically, doing _any_ work before bloom filter rejection just kills us
10:23bholleyemilio: but if the pseudo becomes a Component then it can just be handled in matches_complex_selector, right?
10:33emiliobholley: I guess so, yeah, though we&#39;d need to propagate the pseudo state down matches_complex_selector
10:33bholleyemilio: if it&#39;s in the Component, it would already be there, no?
10:33emiliobholley: we could also expose may_match and use it there
10:33emiliobholley: the pseudo element&#39;s state, not the state in the pseudo selector
10:34emiliobholley: though that branch is fairly well-predictable
10:35emiliobholley: (we could also store in the type parameter whether the map has any pseudo, or what not, and skip the branch for normal maps)
10:35bholleyemilio: I think we&#39;re going to want a sort of SelectorMatchContext thing anyway
10:35bholleyemilio: to handle quirks mode and whatnot
10:36bholleyemilio: rather than passing ever-more arguments
10:36bholleyemilio: so we can put it in that
10:36bholleyemilio: and hoist the set_selector_flags callback to that object as well
10:37bholleyemilio: I think it definitely makes sense to do that kind of matching inside of rust-selectors
10:38emiliobholley: Yeah, jryans&#39; visited patches already add that
10:39emiliobholley: But anyway, that&#39;s unfeasible right now in rust-selectors because matching only acts in SelectorInner
10:40bholleyemilio: right, but with the followup it will work fine, right?
10:40emiliobholley: we can move that into selectors once we have pseudo-as-a-component, yeah
10:40bholleyemilio: I&#39;m not saying fix it in this patch, just in the followup
10:40emiliobholley: yeah, that&#39;s fine for me
10:40bholleyk great
10:41bholleyemilio: hm, when do we call HasAuthorSpecifiedRules on something unstyled?
10:41emiliobholley: I guess nowhere? I was just preserving the same checks.
10:42emiliobholley: Seems easy enough to return false, but I could also debug_assert! or expect
10:43bholleyemilio: well, the old code definitely asserted that the element had ElementData
10:44bholleyemilio: (we have the HasServoData() early-return in the caller)
10:44emiliobholley: right, but I removed that, didn&#39;t I?
10:44bholleyemilio: right, I guess I think we should keep it
10:44emiliobholley: (sorry, on a bug, so with shitty internet)
10:45* bholley comments
10:45emilio*bus
10:49bholleyok, one more patch to go...
10:49bholleyoh man that&#39;s big
10:50bholleyoh wait, that was the enum patch
10:50bholleythis one is actually manageable
10:59emilioxidorn: ping?
10:59xidornemilio: pong
10:59emilioxidorn: does really the PostRebuildAllStyleDataEvent not fix the counter-style problems?
10:59emilioxidorn: do we have counter-style on pseudos or something like that?
11:00xidornemilio: that test doesn&#39;t
11:00emilioxidorn: which test is it?
11:00xidornlayout/style/crashtests/1066089-1.html
11:01xidornemilio: hmmm, it makes me wonder...
11:01emilioxidorn: and what are we leaking?
11:01xidornemilio: how do we handle the initial style struct?
11:01xidornemilio: nothing is leaking, but it crashes because the old counter styles are destroyed
11:01emilioxidorn: when we rebuild all the style data, we also recompute them
11:02xidornif we leak the counter styles, they we would not crash
11:02emilioxidorn: oh, I see
11:02xidornemilio: if we recompute the initial style struct, then it should be fine...
11:03emilioxidorn: hmm, but we may recompute them before the actual flush
11:03emilioxidorn: would that explain it?
11:03xidornemilio: when do we recompute them?
11:03emilioxidorn: Device::reset() in servo
11:04xidornemilio: and Post*Event should call Device::reset(), right?
11:04xidornPostRebuildAllStyleDataEvent
11:05xidornshould call Device::reset() and thus recompute shouldn&#39;t happen before calling PostRebuildAllStyleDataEvent I suppose?
11:05xidornin that case things should be fine as well, because at that time stale counter styles should have been unlinked from the manager
11:06emilioxidorn: hmmm, yeah, it should, but it should do so once stylesheets have been flushed
11:07emilioxidorn: I wonder what kind of reference does the initial style hold alive to a counter style? That sounds not really intuitive to me
11:07xidornemilio: that&#39;s kind of raw pointer
11:09xidornemilio: so in my patch, stylo would set an atom to nsStyleList::mListStyleType, and it would be resolved to a CounterStyle object at FinishStyle() in the main thread
11:10xidornthe CounterStyle object is referred by a &quot;raw pointer&quot; on nsStyleList::mCounterStyle
11:11xidornthe CounterStyle object is managed by CounterStyleManager which collects stale objects when rule changes, and destroy stale objects after each flush
11:13emilioxidorn: I see
11:14xidornemilio: so it relies on that all frames get new style data
11:14xidornat the next restyle
11:15emilioxidorn: we may be missing a few frames (bug 1364361)
11:15firebothttps://bugzil.la/1364361 ASSIGNED, cam@mcc.id.au stylo: AllChildrenIterator doesn&#39;t find NAC created by non-primary frames of elements
11:15emilioxidorn: can you see which kind of frame is it?
11:15xidornemilio: nsBlockFrame and nsBulletFrame
11:15emilioxidorn: do they belong to any pseudo or anything like that?
11:15xidornI don&#39;t think there is any complicated case in the crashtest actually
11:15xidornno
11:16xidornthat is just a &quot;<ul><li>xxx&quot;
11:16emilioxidorn: Hmm... We may need to make RebuildAllStyleData sync?
11:16emilioxidorn: i.e., process the restyles synchronously?
11:17emilioxidorn: do you know if there are pending restyles when the crash happens?
11:17xidornthat&#39;s not necessary in theory...
11:17xidornemilio: hmm
11:17emilioxidorn: Gecko definitely does it
11:17xidornbut the cleanup happens at a PostRefreshObserver
11:17xidornof refresh driver
11:18emilioxidorn: try to add ProcessPendingRestyles at the bottom of RebuildStyleData? (just in case)
11:18* emilio needs to run now
11:18xidornemilio: ok I can try
11:20bholleyemilio: (gotta run to lunch, didn&#39;t quite finish reviewing the last patch - will do it later today)
11:32noxmystor: Around?
11:32noxmystor: synstructure can only help for &quot;imperative&quot; traits, right?
11:32noxWait no, wrong question.
11:33noxmystor: If I need to return a particular value from an enum variant with no fields, I can&#39;t do that with synstructure::each_fields, right?
11:40xidornemilio: I think I know why... you&#39;re right that the frame is a pseudo-element
11:40xidornemilio: nsBulletFrame is frame of pseudo-element ::-moz-list-bullet or ::-moz-list-number
11:40xidornemilio: so how should we update the style data of pseudo-element?
11:57emilioxidorn: yeah, I need to implement restyle for those frame-baked pseudos
11:57emilioxidorn: perhaps not worth to block counter-style on that though
11:57xidornemilio: I can just leak them then :)
11:58emilioxidorn: file a bug for that and in? me please
11:58emilio*ni
11:58xidornemilio: ok
11:59xidornemilio: about restyling the specific pseudos or a general bug?
11:59xidornemilio: if each specific pseudos can be implemented independently, maybe I can help?
12:08SimonSapinemilio: whats impl_trait? in gecko.mako.rs
12:08SimonSapinIm asked to review the stylo bits of https://reviewboard.mozilla.org/r/138434/diff/4 but I dont really know what this does
12:08emilioxidorn: I&#39;m not sure we have a bug on file for the frame backed pseudos... I guess both. And sure, I guess it&#39;s going to be something like UpdateStyleOfOwnedAnonBoxes, so it should be parallelizable
12:09emilioSimonSapin: I think it&#39;s a misname from when structs were traits?
12:09emilioSimonSapin: it basically implementing methods on a style struct iirc
12:10SimonSapinmaybe Manish knows more about XUL properties
12:35froydnjugh, bindgen can&#39;t find clang.dll with our clang on infra
12:41xidornfroydnj: have you setup --with-libclang-path?
12:41xidornfroydnj: or you&#39;re trying to make it config-less?
12:42xidornfroydnj: btw I tried using mach bootstrap to download libclang... but it doesn&#39;t seem to work for me
12:43froydnjxidorn: I confirmed that LIBCLANG_PATH is being set, yes
12:45* froydnj wonders why cssparser fails to build so often
12:46froydnjalso why there&#39;s so little information printed about the actual failure
12:46noxfroydnj: Fail how?
12:47froydnjnox: the only information that seems to be there is that rustc hit a trace/breakpoint trap
12:47noxI don&#39;t understand. Where does it fail?
12:48froydnjnox: https://pastebin.mozilla.org/9021729
12:48froydnjthat&#39;s it, that&#39;s all it says
12:49noxAh ok in Gecko.
12:53SimonSapinfroydnj: can I somehow run &quot;cargo test -p selectors&quot; in m-c and use the same target dir as &quot;mach build&quot;?
12:56mystornox: sorry, missed you
12:57froydnjSimonSapin: yes, something like CARGO_TARGET_DIR=$OBJDIR/toolkit/library cargo test -p selectors
12:57mystornox: I would use match_substructs
12:57SimonSapinfroydnj: is there &quot;mach cargo <arbitrary sub-commands>&quot;? If not, do you think it makes sense to add?
12:58noxmystor: This doesn&#39;t give me the VariantData value,
12:58noxmystor: so I can&#39;t check for attributes on the variant.
12:58froydnjSimonSapin: there is not, and yes, I think so
12:58mystornox: That wouldn&#39;t be hard to add
12:58noxmystor: Breaking change or now function?
12:58noxnew*
12:59mystornox: I&#39;d make it a new function - maybe match_substructs_vd?
12:59mystornox: I don&#39;t like breaking changes if I can help it
12:59mystor(I take it back, that&#39;s a terrible name)
12:59noxmystor: each_variant?
13:00noxmystor: Then one can use match_pattern or whatever.
13:00mystornox: Sure, I&#39;ll add a struct Variant { pub bi: Vec<BindingInfo>, pub vi: &VariantInfo, __privconstruct: () }
13:01mystorand pass that around or similar
13:01noxmystor: Why?
13:01mystornox: just in case I realize I need to do this again?
13:01mystorIunno
13:02noxmystor: I mean, why bi?
13:02mystornox: oh, I shortened the names for IRC
13:02* mystor would actually write out the names in full
13:02noxmystor: each_variant should take a function that takes a value that contains a VariantData and the variant name that&#39;s all, AFAIK.
13:02noxmystor: I mean, why should that contain a vec of BindingInfo at all?
13:03mystorI was assuming it would be a superset of the current match_substructs method&#39;s abilities
13:03mystorso it would still generate the match code and such
13:03mystorso you get the VariantData, but you also get bindings to any fields from that variant
13:03noxWhat I had in mind is just something similar to each_field but which yields variants,
13:03noxand then you can call match_pattern if you need to.
13:04mystorOk, so it wouldn&#39;t build up a match expression for you at all?
13:05noxmystor: Oh I see what you mean. I think your idea is better.
13:05mystor\o/
13:08mystornox: IYO, should this by 0.5.1 or 0.6
13:08noxmystor: If it&#39;s breaking, 0.6; otherwise 0.5.1.
13:09mystornox: non-breaking pre-1.0 fix, so I&#39;m thinking 0.5.1
13:09mystors/fix/feature
13:10emilioxidorn: should I assume the &quot;Convert PseudoElement to an enum&quot; patch is r+&#39;d?
13:10xidornemilio: no
13:11xidornemilio: I&#39;m reviewing other parts of that patch now
13:11emilioxidorn: Oh, ok, thanks :)
13:16noxemilio: offscreen_gl_context epically broken.
13:16emilionox: oh, why&#39;s that?
13:16noxemilio: https://github.com/servo/servo/pull/16867#issuecomment-301455386
13:16crowbotPR #16867: Update offscreen_gl_context to 0.8.8 - https://github.com/servo/servo/pull/16867
13:18froydnjugh, the -m32/-m64 bindgen fix doesn&#39;t seem to have solved the linux32 problem for some reason
13:18noxemilio: Want to review my gradient stuff?
13:18mystornox: Do you actually want a syn::Variant rather than a syn::VariantData?
13:19noxmystor: Oh. Variant probably.
13:19noxmystor: I will need the attributes at some point.
13:19mystornox: I&#39;ll have to fabricate one for struct types
13:19emilionox: where&#39;s it?
13:19mystorBut I can do that
13:19noxmystor: I&#39;m trying to derive https://doc.servo.org/style/values/computed/trait.ToComputedValue.html.
13:19noxemilio: https://github.com/servo/servo/pull/16859
13:19crowbotPR #16859: Rewrite style images with a good dose of generics - https://github.com/servo/servo/pull/16859
13:20noxemilio: Next level PR title.
13:22emilionox: oh, I had taken a look at it this morning, heh
13:22noxemilio: Nice. :)
13:24cynicaldevilnox: I can&#39;t wrap my head around these Rooted DOM object types :/
13:24cynicaldevilThe doc says &quot;reference to a rooted DOM object&quot; in one part and &quot;rooted reference to a DOM object&quot; in another. Are these the same?
13:25noxcynicaldevil: What do you mean?
13:25emilionox: do we expect to add other compat modes which aren&#39;t WebKit?
13:25emilionox: i.e., Moz?
13:25noxemilio: No.
13:25emilionox: ok, then
13:25noxemilio: https://bugzilla.mozilla.org/show_bug.cgi?id=1337655
13:25firebotBug 1337655 NEW, nobody@mozilla.org Try disabling moz-prefixed gradient functions by default
13:26cynicaldevilnox: The Root<T> mentioned in https://doc.servo.org/script/dom/bindings/js/index.html refers to script::dom::bindings::js::Root, right?
13:26noxcynicaldevil: Yes.
13:27cynicaldevilnox: Thanks, there was a slight difference in its description in that page and the doc page for Root.
13:28jdmcrowbot: infrastructure report
13:29crowbotSurprise! no problems detected :<
13:29noxcrowbot: What&#39;s new?
13:29crowbotServo tied Chrome&#39;s DOM API by 38 minutes :)
13:32emilionox: r=me. I tried to get more than nits but I couldn&#39;t! :P
13:32noxAh ah.
13:33noxemilio: Will address.
13:33noxemilio: Can you land the expectations afterwards?
13:33emilionox: sure
13:35emiliobholley: did you manage to take a look at the restyle hints patch? That should be the last remaining bit
13:35bholleyemilio: yeah I know, sorry - I&#39;m in the Berlin office meeting with jseward
13:35emiliobholley: oh, ok, np :)
13:35bholleyemilio: I&#39;ll need to wait a few hours to finish the review
13:36emiliobholley: sounds fine, I have a bunch of other stuff to do :P
13:41noxkvark: Are you WR people working on supporting interpolation hints in WR btw?
13:47jdmajeffrey: now that the worklet infrastructure&#39;s not under active development, check your review queue?
13:57froydnjis there a way to make bindgen emit the command lines it&#39;s invoking clang with?
13:58emiliofroydnj: you should be able to dump http://searchfox.org/mozilla-central/source/third_party/rust/bindgen/src/ir/context.rs#225
13:58emilionox: I think interpolation hints can be done just adding more stops before display list processing
13:59noxemilio: I am not sure of that.
14:00noxemilio: But maybe you are right, I just did the parsing and was done with it. :P
14:00emilionox: https://hg.mozilla.org/integration/mozilla-inbound/rev/e430c434d9c7 suggests that that&#39;s how it&#39;s done in gecko
14:00emilionox: (replacing it with 9 stops: stops.ReplaceElementsAt(x, 1, newStops, 9);)
14:01noxemilio: Where are hints here?
14:01noxOh nevermind, found the code.
14:01noxemilio: Ok.
14:05noxSimonSapin: match_ignore_ascii_case isn&#39;t in its own crate, right?
14:09SimonSapinnox: no, cssparser
14:10noxSimonSapin: Never mind I misread the code where I thought I could have a use for it anyway. \_()_/
14:11kvarknox: I suppose your question is answered?
14:11avadacatavrahttp://searchfox.org/mozilla-central/source/__GENERATED__/dist/include/mozilla/dom/WindowBinding.h#784
14:11* avadacatavra needs a tea break to deal with seeing that
14:11noxkvark: Yes.
14:20mystornox: Does this look fine? https://github.com/mystor/synstructure
14:21noxmystor: Hah nice code reuse. :) https://github.com/mystor/synstructure/commit/7b1963be3fe48054a5b2309b103fef106312ab82#diff-b4aea3e418ccdb71239b96952d9cddb6R329
14:21noxmystor: Seems nice!
14:21mystornox: :P - I was considering marking match_substructs #[deprecated] but decided against it because it has better ergonomics in some cases
14:23mystornox: New version published if you want to use it
14:24noxmystor: Thanks!
14:30froydnjemilio: ok, thanks! for avoidance of doubt, you just meant println!() that thing, right? (and all the fun of modifying vendored packages on try...
14:30emiliofroydnj: yeah, println! should work, and... yeah :/
14:31* froydnj goes to re-read vendoring conversations with a new eye towards relevancy
14:33SimonSapinemilio: whats a pseudo element selector and how is it different from a pseudo element?
14:33emilioSimonSapin: a pseudo-element selector can store state. Gecko supports stuff like ::placeholder:hover
14:33SimonSapinugh
14:34noxemilio: Isn&#39;t that true in general?
14:34emilionox: what is true in general?
14:34noxCombining pseudo elements with pseudo classes.
14:34emilionox: it isn&#39;t
14:35nox&quot;Pseudo-classing Pseudo-elements&quot;
14:35noxis what selectors 4 call them.
14:35SimonSapinin selectors-3 a pseudo-element must be last
14:35SimonSapinnot in 4, apparently
14:35noxhttps://drafts.csswg.org/selectors/#pseudo-element-states
14:36SimonSapinemilio: should PseudoElement move to Component? (or does your patches already do that?)
14:36SimonSapinemilio: can reviewboard show the combined diff of a patch set?
14:36emilioSimonSapin: they don&#39;t, but there&#39;s a dependent bug that will do that
14:37emilioSimonSapin: and I don&#39;t know if reviewboard supports that :/
14:41jryansSimonSapin: click &quot;Review Summary&quot;, then click &quot;Squashed Diff&quot;
14:42SimonSapinthanks jryans
14:43emiliojryans: hey, one thing, do you know if you could land the SelectorMatchingContext bits before all the rest of the :visited patches?
14:43emiliojryans: I&#39;m going to need it relatively soon, and I don&#39;t really want to bitrot those
14:43jryansemilio: as in, just wrap existing args in the context without adding anything visited related?
14:44emiliojryans: yeah
14:44emiliojryans: nevermind if it&#39;s too much effort though
14:44jryansemilio: seems doable, i&#39;ll post a separate patch for you to review that just does that?
14:44emiliojryans: yes, please (and thanks!)
14:45emiliojryans: and sorry to delegate a lot to dbaron for the :visited stuff btw, those patches look terrible to rebase :(
14:46jryansemilio: no worries, that&#39;s just how it goes sometimes :)
14:48jryansemilio: i think there&#39;s value in continuing to look at the rest of visited... i don&#39;t think your comments so far would effect the later patches much, so it would be good to hear some thoughts on those (assuming you feel okay to review them!)
14:48emiliojryans: sure! I&#39;ll take a look later today :)
14:48jryansthanks :)
14:49jryansemilio: it&#39;s great to see your questions so far, i was quite unsure how visited should interact with snapshots, style sharing, etc.
14:50jryansso i&#39;ll investigate more and add some replies :)
14:50* jdm disappears for a while
14:50noxemilio: Soon you will be able to land https://hg.mozilla.org/try/rev/832db792f38eb04f7eb0d84dfb81852ccf1feec3 :)
14:51emiliojryans: yeah, it&#39;s... hard to interact with optimizations when the thing is prone to timing attacks :/
14:51emiliojryans: and awesome job, btw! I only skimmed over the other patches, but seems like a pretty solid job :)
14:52emilionox: did the parsing patch land already?
14:52emilionox: (in any case, yes)
14:52noxemilio: It&#39;s 37 minutes in the CI cyclem
14:52noxhence, &quot;soon&quot;.
14:52jryansemilio: thanks :) i wonder if we need to just disable optimizations once a link is found and not try to share anything (because of timing attacks...)
14:52jryans(i guess you already said this!)
14:53emiliojryans: yeah, it&#39;d be unfortunate though, that&#39;s why I&#39;m curious about why does Gecko do what it does (and what dbaron thinks)
14:53jryansyep!
14:54jryansemilio: do you have a bug that needs matching context? i can mark it as blocking the separate patch...
14:55emiliojryans: bug 1364850
14:55firebothttps://bugzil.la/1364850 NEW, emilio+bugs@crisal.io stylo: Store pseudo-element selectors as another Component.
14:55jryansthanks!
14:55emiliojryans: (need to pass some pseudo-element state around)
14:56SimonSapinemilio: r? https://github.com/servo/servo/pull/16870
14:56crowbotPR #16870: Add size_of tests for geckolib selectors - https://github.com/servo/servo/pull/16870
15:00emilioSimonSapin: should the other size_of/align_of tests also be converted to the use new crate?
15:01SimonSapinemilio: the one in script are kinda different
15:01emilioSimonSapin: yeah, I guess so, ok then :)
15:01SimonSapinI can change one of the two in s