mozilla :: #servo

16 May 2017
00:00hansihegw: thanks!
00:03hansiheHow does Frame relate to this? Is that internal, or is there some reason to use that?
00:04hansihehope all the questions are okay
00:07gwhansihe: The Frame in WR you mean? Frame is basically a representation of what is currently shown on screen - including scroll state, flattened display lists etc
00:11gwr? anyone based on the last comment here - https://github.com/servo/servo/pull/16860#issuecomment-301638921
00:11crowbotPR #16860: Update WR (preserve-3d, AA improvements) - https://github.com/servo/servo/pull/16860
00:11mbrubeckgw: looking
00:14gwmbrubeck: thanks!
00:15mbrubecktrivial doc-only r? anyone ^
00:16hansiheso where is scroll state really stored? where would I go if I wanted to get or set a scroll offset?
00:17gwmbrubeck: r+'ed
00:18gwhansihe: you can scroll a node with https://github.com/servo/webrender/blob/master/webrender_traits/src/api.rs#L335
00:18gwhansihe: and retrieve the scroll states with https://github.com/servo/webrender/blob/master/webrender_traits/src/api.rs#L381
00:24xidornhow can we kill in-tree binding files :/
00:25* xidorn has grumbled about this many times
00:50Manishearthxidorn: fix the CI
00:50Manishearthxidorn: make double landing happen
00:51xidornManishearth: as far as you still want to build geckolib without gecko code, that doesn't help
00:51ManishearthI think we can get more benefit around tooling for this than removing them, since to remove them there's a lot of stuff that needs doing first
00:51Manishearthxidorn: once you make double landing happen it's no longer needed to do that
00:52xidornhmmm, probably yeah...
00:52xidornManishearth: probably not soonish, though
00:52xidornI guess double landing without updating in-tree binding files automatically can actually make things worse
00:53Manishearthyes
00:54xidornbut auto-updating in-tree binding files is hard
00:54gwmbrubeck: would you mind r+'ing https://github.com/servo/servo/pull/16860 again? sorry to mess around - I squashed things incorrectly last time. I've just removed the commit that enables preserve-3d, so this is now a straight WR update - and then we can open the PR from kvark separately to switch on preserve-3d
00:54crowbotPR #16860: Update WR (preserve-3d, AA improvements) - https://github.com/servo/servo/pull/16860
00:54mbrubeckgw: looking
00:59Manishearthshinglyu: are you still working on bug 1355402 ? It blocks a bunch of reftests, and canaltinova wants to fix it
00:59firebothttps://bugzil.la/1355402 NEW, shing.lyu@gmail.com stylo: Support prefixed intrinsic size value for {min-,max-,}{width,height,{inline,block}-size}
01:01* avadacatavra looks at clock and goes to bed
01:08shinglyuManishearth: canaltinova: feel free to take it :)
01:49xidornI wonder whether emilio is still around...
01:49emilioxidorn: hey
01:50* emilio is at the library finishing some schoolwork
01:50emiliobut I can talk :P
01:50xidornemilio: hey, I'm just seeing your servo side patch for bug 1364412 is landing, and your mozreview patch hasn't been split
01:50firebothttps://bugzil.la/1364412 NEW, emilio+bugs@crisal.io stylo: Support state selectors in pseudo-elements.
01:50xidornemilio: and I have a feeling that it is not your active hours so...
01:51emilioxidorn: yeah, I usually split it right before pushing it, why?
01:51emilioxidorn: yeah, I just happen to be around for long today :)
01:51xidornok :)
01:51emilioxidorn: thanks for caring about it though! :P
02:53bz_awayManishearth: ping
02:53bz_awayManishearth: layout/reftests/bugs/349695-1a/b.html fails in Rs5 are due to missing :-moz-is-html support
02:53* bz_away fixes
03:00* bz_away is once again confused by how snapshots are supposed to work
03:00bz_awaye.g. how do we match :dir against them?
03:01bz_awayAh, I see
03:02bz_awayok
03:07bz_awaySo in a pattern match...
03:07bz_awayDoes the presence or absence of ',' after things mean anything?
03:13Manishearthbz_away: nope
03:14* bz_away is trying to figure out why it's present in some places in http://searchfox.org/mozilla-central/source/servo/components/style/gecko/wrapper.rs#1131 and not others
03:14bz_awaye.g. http://searchfox.org/mozilla-central/source/servo/components/style/gecko/wrapper.rs#1190
03:14bz_awayvs
03:14bz_awayhttp://searchfox.org/mozilla-central/source/servo/components/style/gecko/wrapper.rs#1201
03:15bz_awayI guess just noise....
03:15bz_awaymanishearth: I just did a push with an impl of :-moz-is-html; we'll see how much stuff turns unexpected pass
03:15bz_awaymanishearth: It's really only used for the "bogus form in table" UA rule
03:16* bz_away wonders whether we can restrict it to UA sheets
03:18bzmanishearth: The layout/reftests/bugs/467084-1.html failure marked unknown in your list was HasAuthorSpecifiedRules, I bet: it&#39;s <select> sizing. It&#39;s fixed on tip
03:29emiliobz: I thought you were used to inconsistent coding style ;)
03:29bzhmm?
03:29emiliobz: (the comma is harmless, and useless)
03:29emiliobz: It just depends on who writes it and how does he/she feel that day :P
03:29bzI&#39;m used to many things I don&#39;t like... ;)
03:30emilioheh
03:36* bz files bugs on unsound snapshot stuff
03:38* xidorn wonders why the servo queue is still that long...
03:39xidornI have a feeling that I may not be able to land counter-style things today...
03:40heycamxidorn: just bump the priority of your PR
03:40bzxidorn: It&#39;s that long because each test takes forever....
03:41bzxidorn: for example...
03:41bzhttps://github.com/servo/servo/pull/16877
03:41crowbotPR #16877: Implement :-moz-anonymous-content in stylo - https://github.com/servo/servo/pull/16877
03:41bzTest started at 5:38pm
03:41xidornheycam: hopefully there is no binding updates in other prs...
03:41bzmerge happened at 6:40pm
03:42xidornbz: that sounds a fair amount of time for servo pr
03:42bz(some tests before that one in the
03:42bzSome tests before that one in the queue took closer to 1.5 hours
03:42heycammaximum 24 merges per day
03:42bzRight
03:42heycamI wonder it&#39;ll be before that&#39;s not sustainable
03:42bzSo I&#39;m pretty sure I had 3 PRs today
03:42heycam*how long
03:43xidornheycam: my pr would change a large part of structs binding files, because of adding new classes
03:43xidornwhich pushes the id number of various things...
03:43bzheycam: http://logs.glob.uno/?c=mozilla%23servo&s=15+May+2017&e=16+May+2017#c672776
03:43heycam:)
03:44xidornso I fear any conflicts because of that...
03:44heycamxidorn: you fear causing other people conflicts if you jump the queue?
03:44heycamor that other people will make yours conflict
03:44stshineThe test time have a strange rise from about 47 min to 90 min recently
03:45xidornheycam: if I bump the priority, the former
03:45xidornotherwise, the latter :)
03:45xidornI guess no other pr is currently updating the bindings, so it is probably fine
03:46heycamxidorn: I think it&#39;s acceptable to bump your priority, if you have to coordinate gecko landings too
03:46xidornthe only remaining problem is how can I apply the binding file changes cleanly...
03:47bzManishearth: The &quot;unknown :lang selector bug&quot; thing at https://gist.github.com/Manishearth/086118c940ff86a6cfc573f53c508279#file-reftest-failures-L615 is https://bugzilla.mozilla.org/show_bug.cgi?id=1365162
03:47firebotBug 1365162 NEW, nobody@mozilla.org stylo: need to be able to match :lang on snapshots
03:48xidornthere are pushes touch binding files after m-c, so I cannot simply override the files with those from my try push
03:48xidornand I cannot use the diff trick because there are already conflicts :/
03:49bzManishearth: There&#39;s a good chance that https://gist.github.com/Manishearth/086118c940ff86a6cfc573f53c508279#file-reftest-failures-L685 is due to https://bugzilla.mozilla.org/show_bug.cgi?id=1339629
03:49firebotBug 1339629 NEW, bwerth@mozilla.com stylo: ServoStyleSheets don&#39;t handle modification in the presence of cloned inners correctly
03:50bzManishearth: https://gist.github.com/Manishearth/086118c940ff86a6cfc573f53c508279#file-reftest-failures-L688-L690 seem to be fixed on tip at first glance
03:50bzManishearth: By https://bugzilla.mozilla.org/show_bug.cgi?id=1356916 looks like?
03:50emiliobz: Manishearth: display: contents inheritance is due to the test using :scope selector, so the bug is against <style scope>
03:50firebotBug 1356916 FIXED, nobody@mozilla.org stylo: inline canvas width and height are not reflected
03:52* bz wonders what the flow-root issue is
03:53bzGenerally flow-root seems to work...
03:56bzOh, I see
03:56bzstylo ignores the pref
03:57bzManishearth: https://gist.github.com/Manishearth/086118c940ff86a6cfc573f53c508279#file-reftest-failures-L716-L717 is https://bugzilla.mozilla.org/show_bug.cgi?id=1365163
03:57firebotBug 1365163 NEW, nobody@mozilla.org stylo: Need to support the layout.css.display-flow-root.enabled preference
03:59heycambz: it is always the case that a content&#39;s primary frame will live in its parent frame&#39;s principal child list?
03:59heycam*is it
03:59bzheycam: no
04:00bzheycam: e.g. positioned/floated stuff
04:00heycambz: ok
04:01bzI still owe you a reply in that bug
04:01bzI need to think about it a bit....
04:01* bz has too many things in taht bucket. :(
04:01heycambz: ok. I&#39;m just going ahead with my suggestion there, and I&#39;ll stick it up for review
04:02bzok
04:02bzI&#39;ll try to say something useful there tomorrow
04:02heycamthanks :)
04:12gw\o/
04:20* emilio just spent 1h debugging something extremely stupid
04:20emiliogw++ :)
04:25stshineemilio: morning
04:26emiliostshine: hey! good morning :)
04:27stshineemilio: you get up early today! :)
04:27emiliostshine: more like going to bed late :P
04:27stshineemilio: well done!
04:36* bz finds the &quot;unknown text-overflow issue&quot;
04:39emiliocrowbot: tell bholley I hope the patch over bug 1364850 makes you happier ;)
04:39crowbotok, but I won&#39;t enjoy it :(
04:39firebothttps://bugzil.la/1364850 NEW, emilio+bugs@crisal.io stylo: Store pseudo-element selectors as another Component.
04:39emiliobz: curious, what&#39;s it?
04:40bzemilio: http://searchfox.org/mozilla-central/source/servo/components/style/properties/gecko.mako.rs#3646-3648
04:40bzemilio: that&#39;s buggy
04:40bzemilio: should be self.gecko.mTextOverflow.mLogicalDirections = v.second.is_none();
04:40bzemilio: because text-overflow inherits
04:40emiliobz: oh, I see
04:40bzemilio: in some cases
04:41bzemilio: And the current code never sets it to false...
04:41bzAlso, http://searchfox.org/mozilla-central/source/layout/style/nsStyleStruct.h#1830
04:41bzSo it always starts off true... ;)
04:41bzfor a new struct
04:42* bz will do a try push and post patch once he has expectation adjustments
04:42emiliobz++
04:42* emilio goes to rest a bit for today
04:43bzemilio: heh
04:43bzemilio: sleep!
04:43* bz is about to go to bed too; meant to do it an hour ago...
04:43emiliobz: I meant to do it ~7h ago... oh well :)
04:44* emilio thinks it was a fun bug to track down
04:46* bz_sleep watches hg push review corrupt his repo
04:46bz_sleepThis is not great
04:51bz_sleepManishearth: well :)
04:51bz_sleepManishearth: https://bugzilla.mozilla.org/show_bug.cgi?id=1365168 for the text-overflow issue
04:51firebotBug 1365168 NEW, nobody@mozilla.org stylo: fix two-value text-overflow to actually work
05:02bz_sleepManishearth: the font-feature-settings failures are presumably all https://bugzilla.mozilla.org/show_bug.cgi?id=1355366
05:02firebotBug 1355366 NEW, cku@mozilla.com stylo: Support font-feature-settings descriptor in @font-face rule
05:04bz_sleepManisheart: and font-language-override is https://bugzilla.mozilla.org/show_bug.cgi?id=1355364
05:04firebotBug 1355364 NEW, nobody@mozilla.org stylo: Support font-language-override descriptor in @font-face rule
05:04SimonSapinManishearth: pong
05:04* bz_sleep sleep for real
05:19gwmbrubeck: around by any chance?
06:01xidornwhat&#39;s going wrong with servo&#39;s CI?
06:01xidorn#16874 has been in pending for almost 2hrs
06:01crowbotPR #16874: Use a SmallVec when gathering applicable declarations - https://github.com/servo/servo/pull/16874
06:03xidornmaybe because the next task doesn&#39;t have &quot;yes&quot; in mergable column?
07:57stshinegw: around? got a question about webrender :)
08:01crowbotbholley: emilio said I hope the patch over bug 1364850 makes you happier ;)
08:01firebothttps://bugzil.la/1364850 NEW, emilio+bugs@crisal.io stylo: Store pseudo-element selectors as another Component.
08:11hansihedid I misunderstand something, or does webrender really create a new socketpair for every api call that requires a response?
08:12hansihewhen running in ipc mode that is
08:28noxIs the windows build box broken?
08:28noxI so damn hate when I take Servo commits, try to apply them on central and it utterly fails.
08:31noxwtf
08:37noxemilio: r? https://github.com/servo/servo/pull/16858
08:37crowbotPR #16858: Implement the hashless color quirk (fixes #15341) - https://github.com/servo/servo/pull/16858
08:44noxemilio: Why is -webkit prefix in media queries pref-gated?
08:48xidornnox: because that breaks sites iirc
08:49noxxidorn: If that breaks sites, how did it end up in compat spec?
08:50xidornnox: you can probably see bug 1237720 for some details
08:50firebothttps://bugzil.la/1237720 FIXED, dholbert@mozilla.com Give &quot;-webkit-min-device-pixel-ratio&quot;/&quot;-webkit-max-device-pixel-ratio&quot; media query its own pref, & d
08:51noxAh, I see.
08:53noxOh and https://bugzilla.mozilla.org/show_bug.cgi?id=215083 is still NEW of course.
08:53firebotBug 215083 NEW, nobody@mozilla.org Support CSS3 content property replacement of element (rather than pseudo-element)
08:54noxHow do prefs work in Stylo?
08:54noxDo we have infra for them yet?
08:55heycamwe do for properties, but I&#39;m not sure we do for other features
08:58noxWhere is the latest version of stylo-failures.md?
08:58noxOr alternatively asked, how do I use searchfox with autoland?
09:00noxhiro: Could you mark the tickets you steal as ASSIGNED?
09:00hironox: sure will do.
09:00heycamnox: searchfox only updates once a day or so, and probably based on m-c
09:00heycamnox: you could look at hg.mozilla.org/integration/autoland/
09:01noxhiro: I would have done https://bugzilla.mozilla.org/show_bug.cgi?id=1355402 if I knew no one was working on it... Do people often preemptively take tickets for some reason?
09:01firebotBug 1355402 NEW, hikezoe@mozilla.com stylo: Support prefixed intrinsic size value for {min-,max-,}{width,height,{inline,block}-size}
09:01heycamnox: https://hg.mozilla.org/integration/autoland/file/tip/layout/style/test/stylo-failures.md
09:01noxheycam: Thanks.
09:02emilionox: for -webkit-device-pixel-ratio, etc
09:02noxI thought searchfox was life, that&#39;s quite disappointing.
09:02noxlive* lol
09:02noxemilio: What about it?
09:03noxemilio: Oh that wasn&#39;t the question.
09:03emilionox: that we need to parse it? You asked why there was a WebKit prefix there :)
09:03noxemilio: I meant why are they pref-gated even though all this stuff is mandated by spec.
09:03noxemilio: The operative part was &quot;why pref-gated&quot;.
09:03emilionox: shrug?
09:04heycamsearchfox is life
09:04heycamit&#39;s just life from 24h ago...
09:04emilionox: ehh, sorry, dunno how to read apparently :)
09:44gwstshine: around for a few mins, yea
09:44gwhansihe: no - only for synchronous calls, which are relatively rare. we could improve this :)
09:51stshinegw: hi! I am trying to raster svg to a blob image
09:51stshinegw: so the characteristics for SVG is that we can not know its dimensions until layout
09:52stshinegw: while the update_image() of webrender api cannot update the dimensions of an image
09:53stshinegw: can you tell me the reason, and if it is possible to improve this?
09:55sewardjbholley: hi. I see bug 1364952 is already marked as fixed (!). But how do I know when it lands in m-c?
09:55firebothttps://bugzil.la/1364952 FIXED, bobbyholley@gmail.com stylo: Use a SmallVec when gathering applicable declarations
09:55bholleysewardj: yeah, our process isn&#39;t great for that
09:56bholleysewardj: I usually just pull mc, do git log central, and then grep for the PR #
09:58sewardjbholley: (confused) do you mean you are doing &quot;git log central&quot; at the root of a m-c tree? Or something else
09:58bholleysewardj: I&#39;m saying that I just fetch m-c tip with my version control tool, get a (paged) log of the commits, and then search for the # of the PR I&#39;m interested in
10:05bholleySimonSapin: what&#39;s the relative win we can get with and without making Components interdependent?
10:06SimonSapinbholley: my previous idea was boxing
10:06SimonSapinbholley: with Option<Box<>> being None in the hopefully-common case
10:06SimonSapinbholley: with that I think 5 words for Component
10:07bholleySimonSapin: what about just adding more discriminant values?
10:07SimonSapinwe can do that with combinatorial explosion
10:07SimonSapinin which case Id look into generating it with a macro
10:07gwstshine: the main reason for this is that most of the modern gfx APIs are moving towards immutable texture storage - where you can&#39;t modify the texture after initialization. it also has an effect on how the texture may be allocated as a sub-rect of an atlas for batching reasons. The normal way we deal with this for canvas etc, is to generate the image key upfront, then call add_image() on the first layout and update_image() after that. we could make the API
10:07gwa bit nicer to hide that detail, perhaps.
10:08bholleyhm...
10:08gwstshine: can&#39;t modify the texture size, that is
10:08SimonSapinbholley: theres already Attr*NeverMatch variants for another edge case
10:08bholleySimonSapin: what does that do?
10:09SimonSapinbholley: fast-path matching to false without looking up the attribute at all, and avoid a branch in the normal matching case
10:09SimonSapinbholley: and in the case of AttrIncludesNeverMatch, avoid scanning the selector value for whitespace
10:10stshinegw: but for canvas we know the dimensions from start, while for SVG we may need to change the dimensions for each reflow
10:11gwstshine: I don&#39;t know much about SVG - what would cause the image size to change per reflow?
10:12stshinegw: well the size of an svg is determined during layout as other replaced elements
10:12SimonSapinuhm, that scan may not be necessary
10:12stshinegw: which is why it is called &quot;scalable&quot;
10:12bholleySimonSapin: would boxing be less work?
10:12stshinegw: I am worrying about the performance and the resource management if we upload image for each reflow
10:12bholleySimonSapin: I&#39;m wondering if we should try that now, as the simpler option
10:13stshinegw: at least, when the element size changes
10:13SimonSapinbholley: *NeverMatch is a fast path but for bogus selectors that arent useful (they never match anything), so we can probably live without it (wed need to fix up matching to deal with theses cases)
10:13SimonSapinbholley: yes, boxing is likely less work
10:14SimonSapinok, Ill try that
10:14gwstshine: can&#39;t the size of a canvas also be determined during the layout? e.g. if width = 50%
10:14bholleySimonSapin: maybe make separate case-insensitive variants of all the existing values, and do boxing for the attribute namespaces?
10:14bholleySimonSapin: (is that what you had in mind?)
10:14gwstshine: we shouldn&#39;t need to upload per reflow - it should only be if the size and/or content has changed
10:15gwstshine: which might not be ideal for performance, but should be ok for a first proof of concept
10:15gwstshine: back in 5 mins
10:16stshinegw: the size of a canvas is set in script
10:16bholleySimonSapin: I&#39;m less confident that case-insensitive is as rare as explicitly-namespaced attrs
10:16* stshine thinks
10:17stshinegw: if the texture size cannot be changed I will try to store the image key in SVGFragmentInfo
10:18stshinegw: and when the size changed I will delete the previous image key and generate one with new dimensions
10:18stshinegw: how do you think about that?
10:19bholleySimonSapin: and I think that should still get us to five words
10:19* stshine is sure if implement Drop on SVGFragmentInfo is feasible
10:19stshineis not*
10:21gwstshine: I think that sounds fine, at least for a first prototype
10:21gwstshine: we can certainly revisit the API if/when we decide its annoying or slow :)
10:23stshinegw: okay
10:23stshinegw: if so I will clone the RenderApi from LayoutThread to LayoutContext
10:23stshinegw: thank you! let me have a try :)
10:24gwstshine: yup, cloning that render api interface is designed to be cheap - sounds good!
10:25stshinegw: great :)
10:26bholleyemilio: yt?
10:26emiliobholley: hey
10:27bholleyemilio: hey! Looking at your patch now - thanks for doing it :-)
10:27emiliobholley: no problem! I think I got some test failures this morning, but they seem minor
10:27bholleyemilio: one thing I&#39;m trying to figure out - when we&#39;re matching in the special pseudo mode, under what circumstances will we not have a pseudo on the right?
10:28emiliobholley: only when doing the restyle hints stuff, which is unified now, why?
10:29bholleyemilio: well, so the bigger picture is that I&#39;m kinda bothered by the whole &quot;matches_selector&quot; vs &quot;matches_selector_inner&quot; business, and I&#39;m wondering if we can get away with always matching on the inner
10:29bholleyemilio: right now the reason we have the distinction is because the HAS_PSEUDO bit lives on the outer
10:29bholleyemilio: but I&#39;m wondering if we really need to check has_pseudo, or if we can just look at the rightmost two simple selectors unconditionally in pseudo-mode
10:30bholleyemilio: (_when_ in pseudo-mode)
10:30emiliobholley: right. I looked into moving it to the inner, but didn&#39;t seem really straight-forward
10:30bholleyemilio: moving what to the inner
10:30emiliobholley: there can be arbitrarily to the right
10:30emiliobholley: i.e., you can have ::placeholder:hover:hover:hover:hover
10:31emiliobholley: and you&#39;d need to traverse it to figure out whether there&#39;s a pseudo or not once you pass the state pseudo-classes
10:31bholleymmm
10:31bholleyemilio: but aren&#39;t you saying that there&#39;s always a pseudo if we&#39;re matching in this mode?
10:32bholleyemilio: (the restyle hint stuff would presumably not pass this mode)
10:32emiliobholley: oh, you mean in IgnoreStatelessPseudos? If so, yes
10:32bholleyemilio: right
10:32emiliobholley: but seems quirky to force that
10:32emilio*s/quirky/flacky
10:32bholleyemilio: well, we could assert it
10:33bholleyemilio: I&#39;m just trying to simplify
10:33emiliobholley: I&#39;d prefer not to enforce that at the rust-selectors level, but I guess...
10:33bholleyemilio: this special mode is already pretty special, FWIW
10:33emiliobholley: yeah, to some extent
10:33bholleyemilio: ok, I&#39;ll note it in the review - thanks
10:33emiliobholley: np
10:45bholleyemilio: regarding the logic in SelectorIter. Do we really need to do it that way? Why can&#39;t we just make :: into a bonafide combinator at the parse level?
10:46emiliobholley: because then serialization would be incorrect
10:46emiliobholley: or we need to special-case it, or something like that
10:46bholleyemilio: because serialization inserts spaces on the left and right of the combinator?
10:46emiliobholley: serialization would serialize &quot;div::before&quot; as &quot;div > ::before&quot;, which is incorrect.
10:46bholleyemilio: I&#39;m suggesting adding a new combinator
10:47bholleycalled ::
10:47bholleynot reusing <
10:47bholleyer
10:47bholley>
10:48bholleyemilio: (we&#39;d still need to special-case to avoid the space to the right of the :: combinator, but special-casing serialization seems preferable to special-casing iteration)
10:48emiliobholley: oh, I see... I thought about doing that, but I can&#39;t remember why I didn&#39;t that for some reason
10:48bholleyemilio: ok - I think it&#39;d be a lot nicer! Will comment in the review
10:48emiliobholley: well, it complicates the matching code, that now has to deal with another combinator
10:49emiliobholley: but yeah, seems doable
10:49bholleyemilio: it can just treat it as >
10:49bholleylike > | ::
10:50emiliobholley: yeah, sure, it feels fishy though, I&#39;d ideally like the selector to remain as close as the original form as possible, though it&#39;s right we don&#39;t have a case right now for selector like ::shadow that wouldn&#39;t need this behavior
10:50emiliobholley: sounds ok
10:50bholleyk cool
11:10noxThe more we make the representations of selectors &quot;flat&quot; and distance them from the actual syntax, the more I think we should look into making a VM for selector matching.
12:18noxemilio: I&#39;ve been told you want to refactor stuff related to calc().
12:18emilionox: what happens now with calc?
12:18noxemilio: What do you mean?
12:19emilionox: Why would I want to refactor calc stuff now?
12:19noxI don&#39;t know, I&#39;ve been told that.
12:19emilionox: what, why? who? I definitely don&#39;t :)
12:19noxemilio: SimonSapin told me that but he wasn&#39;t sure, hence me confirming.
12:20noxI want to refactor all length types, so I was checking you weren&#39;t willing to do that already.
12:20SimonSapinemilio: didnt you want to generalize the data structure for arbitrary arithmetic rather than just sums?
12:20emilioSimonSapin: didn&#39;t that land already? :)
12:21emilioSimonSapin: https://github.com/servo/servo/pull/16728
12:21crowbotPR #16728: style: Rewrite calc to be cleaner and support arbitrary expressions. - https://github.com/servo/servo/pull/16728
12:21SimonSapinoh, perfect :)
12:21SimonSapincarry on :)
12:21noxOh yes that&#39;s not what I meant.
12:21noxI want to refactor stuff that contains calc expressions.
12:21noxI think I&#39;ll start by killing LengthOrPercentageOr<SomeKeyword>.
12:22SimonSapinyeah, they are two different things, I just though they might conflict with each other
12:22SimonSapinbut since emilios is already landed theres no problem
12:34emiliomrobinson: ping?
12:34emiliomrobinson: when can a ClipId correspond to different StackingContexts?
13:00cynicaldevilnox: I&#39;m unable to find the docs for the QualName type used here: https://github.com/servo/servo/blob/master/components/script/dom/servoparser/mod.rs#L731
13:01cynicaldevilI can&#39;t find the file where its defined in html5ever either.
13:06emiliobholley: if you could take a look to the updated patch, that&#39;d be neat
13:06mrobinsonemilio: Stacking contexts no longer match 1-to-1 with ClipIds. If you mean when can a ClipId for the clip and a scroll root differ, this will be able to happen once we properly assign CSS clips.
13:06emiliobholley: turns out we needed the different combinator anyway to handle the pseudo_element_parent stuff, so...
13:06mrobinsonemilio: The situation is when an element has a CSS clip, but a child is absolutely positioned.
13:06emiliomrobinson: I see
13:06mrobinsonemilio: Or perhaps this only happens with fixed position children.
13:07emiliomrobinson: thanks for explaining. I left a comment in the PR, perhaps it&#39;d be cool to document it in CLIPPING.md, or some of the other documentation. But r=me in that hit testing fix anyway :)
13:08mrobinsonemilio: Okay. Thanks! I plan to document all the clipping stuff at some point. Some of the documentation lives in WebRender now, but a good time to do it is after the API is a bit more stable.
13:08mrobinsonemilio: I think there is maybe one more major API change.
13:09emiliomrobinson: oh, ok, yeah, that&#39;s totally fine, thanks for doing that! :)
13:51jdmcrowbot: infrastructure report
13:51crowbotmacstadium can&#39;t be pinged since 59 seconds ago
13:54noxSimonSapin: r? ^
13:55bholleyemilio: back from lunch, looking now
13:55bz_sleepgr.....
13:56bholleyemilio: I don&#39;t see anything about function calls in https://github.com/rust-lang-nursery/fmt-rfcs/blob/master/guide/guide.md
13:56* bz_sleep can&#39;t even create a PR because the infra is so slow. :(
13:56bholleyemilio: am I missing it?
13:56bzIn that https://github.com/servo/servo/pull/16875 went into the queue 19 hours ago
13:56crowbotPR #16875: Don&#39;t require style sharing cache invalidation for :dir - https://github.com/servo/servo/pull/16875
13:56bzand is still not merged
13:56bzBut the new PR I want to create needs to be on top of it...
13:57bzI guess I could maybe do a branch on top of that one and push it?
13:57bzAnd hope that things work out correctly
13:59bholleybz: get it reviewed on bugzilla, and wait for the dependent one to merge before sticking the new thing in the queue
13:59bholleybz: sucks, I know
14:00bzbholley: I got it reviewed on bugzilla
14:00bzbholley: That&#39;s the point. ;)
14:00bholleybz: then you just have to wait, unfortunately :-(
14:00bzbholley: yes, I&#39;m just griping
14:01bzbholley: That said, if I do create a PR based on the other branch...
14:01bzbholley: seems like it should work, no?
14:01noxWe will still wait for the dependent branch to get merged first,
14:01bzbholley: or will we end up with silly identical changesets we really shouldn&#39;t have?
14:01bholleybz: it won&#39;t - but it&#39;s possible that they might both merge together
14:01noxbz: No duplicate commit, just a fancier commit graph.
14:01bholleybz: which is also probably fine
14:01noxbholley: In general, we wait, and don&#39;t r+ such PRs which include other PRs.
14:01bzok
14:02* bz will just wait
14:02bholleybz: i.e. if PR A hits an intermittent
14:02noxBut is it really *that* bad to have to wait btw?
14:02bzbholley: ah
14:02bznox: yes
14:02nox\_()_/
14:02bznox: because it increases cognitive load
14:02bznox: Now I have to remember all the things I&#39;m waiting on and not forget to actually land them
14:02bznox: And my context switching overhead goes up
14:02noxFile a PR?
14:02noxAnd be done with it.
14:02* nox never had that kind of problem.
14:03bznox: Then someone else has to rememeber to review it, right?
14:03noxNot sure what&#39;s your point.
14:03* bz has this problem all the time; spends 30+% of time on context-switching overhead and nudging that is needed because people forget stuff.
14:03bzI don&#39;t have a point.
14:03bzI had a question, which was what&#39;s the best way to handle this situation.
14:03bholleybz: you totally have a point :-)
14:03noxPRs get assigned a reviewer automatically.
14:04bzPlus a gripe, which is that it&#39;s taken 19 hours to merge a trivial one-line change
14:04bzMostly because of CI failures
14:04bzwhich were, needless to say, not caused by that change.
14:05bz19+ hours, I should say, since it&#39;s not merged yet and who knows when it will be...
14:05emiliobholley: that was not a function call, but a function definition though, right?
14:05bznox: So my problem is this. If I create a PR, it gets a reviewer, say emilio
14:06bznox: Now _he_ has the overhead of remembering to review it, but not until this other thing merges
14:06nox7 hours*
14:06bholleyemilio: er sorry, that&#39;s what I meant
14:06noxBetween 19 hours ago and 7 hours ago, the PR was just in queue.
14:06noxAnd GitHub has a dashboard to list what we are assigned to.
14:06bznox: From the point of view of the PR submitter, that&#39;s not relevant.
14:06bholleyemilio: but I still don&#39;t see it in the guide
14:06bznox: I am clearly failing to get something through.
14:07bznox: The problem with a dashboard that shows things that are your responsibility but you can&#39;t act on right now...
14:07bznox: is that you have to keep checking the damn things to see whether you can act on them yet
14:07noxYou could have asked for a p=3 so that it would have spent less time in queue.
14:07bznox: True. At the time I didn&#39;t expect to have other things depending on it.
14:07emiliobholley: https://github.com/rust-lang-nursery/fmt-rfcs/issues/39#issuecomment-278544439
14:07crowbotIssue #39: Function definitions - https://github.com/rust-lang-nursery/fmt-rfcs/issues/39
14:07emiliobholley: but anyway, nbd
14:07bznox: And I didn&#39;t, until review comments on another PR that made it depend on this one.
14:08noxWhat I fail to understand is how is it different with bugzilla and mozreview and whatever?
14:08noxApart from the fact that you can yolo-land stuff without it passing CI that is.
14:08bznox: That is the key difference.
14:08noxHah, ok.
14:08bznox: Intermittents do not block landing, for example
14:08bznox: Which is good, given how many of them there are.
14:08noxThey don&#39;t either in Servo.
14:08noxThey only block when we don&#39;t know yet they are an intermittent.
14:08bznox: mhm
14:09noxAnd if the intermittent is triggered by different tests, too; jdm&#39;s tooling isn&#39;t able to detect that.
14:09bznox: given how many retries for intermittents I&#39;ve had in the last month, that&#39;s moderately cold comfort
14:09noxThen let&#39;s push for improving that tool.
14:09bznox: Sure
14:10noxAnd let&#39;s not gripe every damn day saying the workflow sucks, because that&#39;s everything but motivating.
14:10bzIf you&#39;re looking to me for &quot;motivating&quot;, you have the wrong person.
14:10bzSorry. :(
14:10bzbtw, one of the intermittents in this case was mach filter-intermittents deciding to die
14:10bholleynox: I think bz is making a very valid point, and I don&#39;t think you of all people get to tell people to not complain :-)
14:10bzintermittently.
14:11bzWhich just adds delicious irony. ;)
14:11noxbholley: I don&#39;t go on #content and say stuff sucks or sigh about things being subpar.
14:11noxI complain on thread, about the topic of said thread.
14:11bzBtw, I&#39;m not saying stuff sucks...
14:12froydnjnox: why not? everybody else does ;)
14:12bholleyno comment
14:12jdmbz: &quot;filter-intermittents deciding to die&quot;?
14:12bzIf I were, it would be in a different venue and much louder, trust me
14:12noxfroydnj: That&#39;s unfortunate.
14:12bzjdm: http://build.servo.org/builders/linux-rel-wpt/builds/4062/steps/shell__2/logs/stdio
14:12jdmew
14:13bzI guess generally http://build.servo.org/builders/linux-rel-wpt/builds/4062/steps/shell__2
14:13bznox: but OK, point taken, I&#39;ll try to be more upbeat.
14:13jdmbz: oh, that&#39;s not filter-intermittents dying
14:13jdmbz: that worked as intended, but still sucks for you
14:13noxbz: It&#39;s not you as an individual,
14:14bznox: My initial question really was about what the best option I have is for getting these things landed with minimal cognitive overhead for both me and reviewers.
14:14noxit&#39;s just that it happens everyday and it&#39;s not very nice.
14:14bznox: Because I&#39;m not OK just pushing off my cognitive overhead onto &quot;reviewers&quot;
14:14noxbz: &quot;File and move on&quot; is quite standard around here, at least from my POV.
14:15noxbz: I p=5&#39;d your PR.
14:15bznox: Thanks
14:16* bz didn&#39;t realize you could change priority while already in queue
14:16noxbz: You can even set it before r+&#39;ing at all.
14:20bzemilio, bholley: does one of you have a sec for a question about how matching works?
14:20emiliobz: sure
14:21bholleybz: I&#39;m trying to finish a review for emilio before a call in 10 minutes, so I&#39;ll let emilio take it
14:21bzemilio: Ok, so the context here is that :lang bug I filed
14:21noxHah!
14:21noxthread &#39;size_of::test_size_of_fragment&#39; panicked at &#39;Your changes have decreased the stack size of layout::fragment::Fragment from 160 to 136. Good work! Please update the size in tests/unit/layout/size_of.rs&#39;, /Users/nox/src/servo/tests/unit/layout/size_of.rs:11
14:21emiliobz: ok, for snapshots, right?
14:21noxThat&#39;s with the clamping change, didn&#39;t expect that hah.
14:21bzemilio: yes
14:21bzemilio: So I&#39;m trying to understand what the matching entrypoints are and with what types
14:22bzemilio: I assume we enter at http://searchfox.org/mozilla-central/source/servo/components/selectors/matching.rs#276
14:22bzemilio: But with what E?
14:22emiliobz: so the only entry point for selector-matching is the selectors::Element trait. And that `E` is either a `GeckoElement`, or a `ElementSnapshot<GeckoElement>`
14:23bzemilio: And specifically, just for stylo purposes, E might be an ElementWrapper
14:23bzemilio: as defined at http://searchfox.org/mozilla-central/source/servo/components/style/restyle_hints.rs#317
14:23emiliobz: Err, yes, ElementWrapper
14:23bzok
14:23bzSo it might be either Element or ElementWrapper?
14:24bzHere&#39;s the reason I ask
14:24emilioyes. In servo it can also be a ThreadLayoutElement, but that just does a reduced amount of the work for internal pseudo-elements
14:24emilio*ThreadSafeLayoutElement
14:24bzThe obvious way to fix this is to implement :lang matching entirely in servo
14:24bzOr at least on the servo side of stylo
14:24bzAnd make it generic over the thing being matched
14:24emilioNote that for all the attribute stuff we already have a fair amount of boilerplate for snapshots
14:25bzAs long as that thing has a concept of parent and getAttribute
14:25bzwhich both snapshots and elements do
14:25bzI suppose we could just have a generic function and call it from both impls of match_non_ts_pseudo_class