mozilla :: #servo

11 Aug 2017
00:25njnbholley: I'm inside a ServoStyleContext method. Might |this| be an interior pointer?
00:25njni.e. what's the memory layout there?
00:26bholleynjn: same as style structs
00:26bholleynjn: arc-managed
00:27njnbholley: can you point me at the Arc in the code?
00:28bholleynjn: http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/servo/components/style/properties/gecko.mako.rs#236
00:28njnok, thanks
00:40njnbholley: yay, subtracting sizeof(size_t) works again
00:43njnbholley: how does this look for about:memory structure? https://pastebin.mozilla.org/9029433
01:15bznjn: Pretty spiffy
01:15njnbz: anything you'd change?
01:15bzSo on the Gecko side we have "style-contexts", which is comparable to computed-values-objects
01:16bzAnd we have style-structs, as a single bucket; it might be nice to have the breakdown in Gecko too so we can compare
01:16bzWhat is "sundries" under style-structs
01:16bz?
01:16njnbz: sum of all the style sturcts that were < 8KiB
01:17* bz is surprised by the amount of &quot;non-dom&quot; computed-values-objects
01:17bzAh
01:17njnbz: the tooltip explains, when you have that
01:17bzAny reason it&#39;s not done the way other about:memory small allocations are done?
01:17* bz looks up how that is
01:18njnbz: you might be conflating two strategies for small allocations
01:18njnbz: about:memory can collapse smaller branches
01:18bzright
01:18njnbut the memory reporting also does that in advance in some cases, to avoid super-bloat
01:18bzAh, alright
01:18njnbz: you might be able to answer a question I have
01:18njnI have these FFI functions: bz: you https://pastebin.mozilla.org/9029436
01:19njnfor getting the CVs of an Element from C++ code
01:19njnI&#39;m a bit unclear about the optionality of some of these things
01:19njnI call these after checking HasServoData()
01:21njnand I&#39;ve never seen Servo_Element_HasPrimaryComputedValues() return false
01:22njnoh!
01:22njn // FIXME(emilio): Remove the Option<>.
01:22bzHeh
01:22bzI would think the computed values can be null if the element hasn&#39;t been styled yet...
01:23bzBut as long as ElementStyles exists
01:23bzits .primary will exist
01:23njnElementData always has an ElementStyles
01:23bz(the Option might be needed right now if we allocate the ElementStyles first before we figure out the ComputedValues)
01:24njnbz: a quick check of the code suggests it&#39;s not needed
01:24njnbut I might have overlooked a spot
01:24bzRight, the ElementData might not exist....
01:26* bz has to run, will be back in a bit
01:30bholleyhiro: ping
01:42hirobholley: pong
01:42bholleyhiro: hey! Wanted to talk about bug 1389347
01:42firebothttps://bugzil.la/1389347 NEW, bobbyholley@gmail.com stylo: More refactoring of the traversal in preparation for restyle roots
01:42bholleyhiro: er
01:42bholleyhiro: wrong bug
01:43bholleyhiro: bug 1388031
01:43firebothttps://bugzil.la/1388031 ASSIGNED, hikezoe@mozilla.com stylo: crash in geckoservo::glue::Servo_ResolveStyle caused by FlushThrottledStyles
01:43hirobholley: yep.
01:44bholleyhiro: so the basic idea of the patch there is to also do initial styling on throttled animation traversals - is that right?
01:44hirobholley: right.
01:45bholleyhiro: and the reason we don&#39;t just do a normal traversal is because we don&#39;t want to process invalidations
01:45bholleyhiro: (non-animation invalidations)
01:45bholleyhiro: is that right?
01:46hirobholley: ah, i have thought about the normal traversal in case of throttled animation flush.
01:46hirohaven&#39;t thought
01:47bholleyhiro: if we could just do a normal traversal for throttled animation flushes things would sure get a lot easier!
01:47* bholley had assumed we couldn&#39;t for some reason
01:47hirobholley: yeah, right. but it seems to me that it causes performance issue.
01:48bholleyhiro: how so?
01:48hirothat process is called whenever user moves mouse cursor on a document.
01:48bholleybirtles: (you might also be interested in this conversation)
01:48birtlesbholley: yep, I&#39;m listening
01:48bholleybirtles: :-)
01:49bholleyhiro: but the only case when the regular traversal would do any work is if there are invalidations in the tree, right?
01:49hirobholley: but... hmm the approach is really attractive to me.
01:49hirobholley: yes, that&#39;s right.
01:50hirobholley: OK, I will try your approach anyway.
01:50hirobholley: another concern about the crash is
01:50bholleyhiro: I guess I don&#39;t understand if the performance issue you describe is &quot;slowdown because of the work required to process the regular invalidations&quot;, or &quot;slowdown because of other overhead that occurs even when there are no regular invalidations&quot;
01:50hirobholley: we have other crashes that were not caused by throttled animation flush.
01:51birtles(I think we already had the animation vs normal restyle machinery in Gecko and so when we discovered we needed to update compositor animations on mouse move we just grabbed the animation restyle -- I&#39;m not sure that skipping the regular restyle is necessarily an important optimization)
01:52bholleybirtles: ok, that&#39;s great to hear
01:52hirobholley: the former.
01:52bholleyhiro: but we would have to do that work very soon anyway, right?
01:52hirobholley: yes, right.
01:52bholleyhiro: so I guess it&#39;s the difference between doing the work when the mouse moves, or waiting for the next refresh driver tick
01:53hirobholley: correct.
01:53birtles(or the next time someone forces a style flush)
01:53bholleyright
01:53hirobholley: what I was concerned about the case is something like this
01:54bholleyis the reason we have to flush on mouse move so that hit detection works?
01:54hirobholley: in a callback of setTimeout, something is changed to affect invalidation,
01:54birtlesbholley: right
01:54hirobholley: in a next tick, in a callback requestAnimationFrame, something changed to affect invalidation,
01:55hirobholley: in such case, normally we do process both invalidation in a normal restyle.
01:55hirobholley: but anyway, it&#39;s rare case.
01:58bholleyok - so the impact of just doing ProcessPendingRestyles for throttled animation flushes is that, in the case of compositor animations, mouse move triggers restyle processing, whereas it otherwise wouldn&#39;t. Is that right?
01:58bholleyand the benefit is that we can get rid of all the ForThrottledAnimationFlush stuff, and just call ProcessPendingRestyles instead (which is a huge win in terms of complexity)
01:59hirobholley: it&#39;s true ideally, but we don&#39;t currently restrict it only for compositor running animations.
01:59bholleyhiro: oh I see - so we&#39;d do it all the time
02:00bholleyhiro: can we easily restrict it to the times when the compositor is running animations?
02:00hirobholley: I tried, it&#39;s not finished, I mean it&#39;s a bit difficult.
02:02bholleyhiro: what&#39;s the tricky part? Seems like we could just check a bit to see if any animations are throttled?
02:03birtleswe have EffectCompositor::HasThrottledStyleUpdates()
02:03hirobholley: the tricky part is in animation-only traverse. I don&#39;t recall it correctly but, it&#39;s hard to tell whether the element needs to be traversed at the start of the animation-only restyle.
02:04hirobholley: I am talking about the case where we have throtteld animations and normal animations there.
02:05bholleyhiro: but we only need to do any style flushing at all in the case where there&#39;s throttled animations, right?
02:06bholleyhiro: like, can&#39;t we just check EffectCompositor::HasThrottledStyleUpdates() here? http://searchfox.org/mozilla-central/source/layout/base/PresShell.cpp#6938
02:06hirobholley: I am going to do the check in other bug.
02:06njnbz: AFAICT, ElementStyles::primary is Option<> because ElementData implements the Default trait
02:07bholleyhiro: ok. If we do that check, it seems likely safer to just do a regular ProcessPendingRestyles from FlushThrottledStyles
02:07hirobholley: bug 1388692
02:07firebothttps://bugzil.la/1388692 NEW, nobody@mozilla.org stylo: check throttled animation instead of all type of animations for flushing throttled animations
02:08bholleyhiro: ah, perfect!
02:08birtleshiro: I will try to review that later today
02:08birtles(it&#39;s a public holiday here and I have a swimming race in a few hours&#39; time)
02:08hirobholley: I guess it does not fix the crash though.
02:09bholleyhiro: sure, but if we do that, then we can just do a regular ProcessPendingRestyles call
02:09bholleyhiro: and then all the throttled-animation-specific weirdness will go away
02:09hirobholley: Ok, I will try the approach in the crash bug.
02:09bholleyhiro: ok great
02:10hirobholley: thanks for the great suggestion!
02:10bholleyhiro: :-)
02:11bholleyhiro: birtles: Another question. I&#39;m trying to understand this whole animation setup a bit better. The first animation-only traversal is necessary because the spec requires us to cascade animation changes first before processing regular invalidations. The second traversal is the &quot;normal&quot; traversal. The third animation-only is necessary because the second traversal would have cascaded transitions with the after-change style, and we need to
02:11bholley reset the style to the current moment in the timeline. Is that right so far?
02:13hiroI don&#39;t get what the &#39;cascade animation change&#39; means, but the first animation-only restyle is for distinguish the case we don&#39;t need to trigger transitions.
02:14bholleyhiro: ah, because animations (and transitions) don&#39;t trigger transitions, but regular style changes do?
02:14hirobholley: yes, right.
02:14bholleyhiro: ok. Is that the only reason?
02:15hirobholley: as far as I know, it&#39;s the only one reason.
02:15bholleyhiro: ok. And is my reason for the third traversal correct?
02:15birtlesthe third animation restyle is because the second traversal could have generated new CSS animations/transitions
02:15hiroyep.
02:16bholleybirtles: I understrand why we need it for transitions (since we cascade the after-change style). But why animations?
02:17hirobholley: animation-name or other animation properties could be changed in the normal traversal.
02:17bholleyhiro: but why can&#39;t we just apply those animations directly in the normal traversal?
02:17birtlesbholley: because new animations don&#39;t necessarily start from the underlying style -- and we don&#39;t apply them as part of the second traversal because that produces incorrect results in some cases: in particular, if we do that they can trigger transitions which they shouldn&#39;t do
02:17bholleybirtles: oh right, ok
02:18bholleyok that makes sense
02:18bholleyso
02:18birtlesbholley: yeah, it&#39;s basically to avoid triggering transitions (animation-only restyle never triggers transitions)
02:18bholleywhy do we need the fourth?
02:18hirobholley: oh fourth? I don&#39;t think it&#39;s necessary.
02:19bholleyok
02:19birtlesyeah, I can&#39;t remember when that got added -- but I thought there was some bug that needed it
02:20birtlesI thought hiro would remember :)
02:20bholleybirtles: well, it seems to basically just kinda happen by accident
02:20bholleybirtles: because Servo_TraverseSubtree does both
02:20birtlesyeah, but I seem to recall someone convincing me we needed it :)
02:21birtleswe could always try skipping it and see what breaks :)
02:21birtlesanyway, I really have to get to this swimming race
02:21bholleyk
02:21bholleycan processing animations trigger regular invalidations? Perhaps with hiro&#39;s SMIL display:none thing?
02:22birtlesyeah, and perhaps animating &#39;content&#39; ? (yet to land, however)
02:22hirobholley: oh right. that triggeres a normal traversal.
02:22hirobholley: content animatable is processed in frame construction.
02:23hiroerr birtles
02:23birtlesbeing SMIL, however, we shouldn&#39;t be doing &#39;display&#39; in the second animation traversal
02:23hirothat&#39;s right.
02:23birtlesbholley: even animation to and from content: none ?
02:24hirobirtles: that case doen&#39;t work either for now.
02:24hirobirtles: even with the patch I posted in the content animatable bug.
02:24birtlesah, ok
02:28bholleyhiro: FYI, see the long comment in part 5 of bug 1389347
02:28firebothttps://bugzil.la/1389347 NEW, bobbyholley@gmail.com stylo: More refactoring of the traversal in preparation for restyle roots
02:28bholleyhiro: about how we don&#39;t handle animations correctly in StyleNewChildren
02:29hirobholley: thanks, reading.
02:46heycamxidorn: do you know whether modifications to existing WPT tests in m-c get upstreamed automatically? (i.e. not just whole, newly written tests?)
02:47xidornheycam: I think they do...
02:47xidornheycam: but not sure
02:47heycamxidorn: ok
02:48heycamI figure they must otherwise how could we not have conflicts all the time when we import more...
03:46njnbholley: is EAGER_PSEUDO_COUNTS visible to C++?
03:57bholleynjn: hm, I think not
03:57njnbholley: I thought you said something about having just exposed it, yesterday
03:58bholleynjn: this is what I did yesterday: https://hg.mozilla.org/mozilla-central/file/tip/layout/style/nsCSSPseudoElements.h#l90
03:58njnok
03:59bholleynjn: so I guess we can just define it to 4 right next to that function
03:59bholleynjn: since if it ever changes that function will need to change
03:59njnok
04:40bzDo we know the type of mReaction?
04:40bzer, wrong window
04:50njnoh look, it&#39;s downloading rustc again
05:23bz_sleepAnd that&#39;s that for anon boxes
05:26ajabz_sleep, somniloquy ?
05:44sewardjbholley: ping
05:44bholleysewardj: hi
05:44sewardjbholley: hi. thanks for your comment. that actually sounds like a better plan.
05:45* sewardj wonders why he finds this so confusing.
05:45sewardjbholley: so IIUC, what we&#39;re looking for is large calls to malloc originating from rust code. Yes?
05:46bholleysewardj: right
05:56bz_sleepheycam: ping
05:57heycambz_sleep: pong
05:57bz_sleepheycam: d&#39;you want to steal the review on https://bugzilla.mozilla.org/show_bug.cgi?id=1385656 ?
05:57firebotBug 1385656 NEW, bzbarsky@mit.edu stylo: RecoverLetterFrames doesn&#39;t play nicely with ::first-line
05:57* heycam looks
05:58bz_sleepheycam: And also, if I have a patch for https://bugzilla.mozilla.org/show_bug.cgi?id=1388877 ready soonish, would you be able to review it?
05:58firebotBug 1388877 NEW, nobody@mozilla.org stylo: Some failing first-line reftests
05:58heycamsure
05:58* bz_sleep is not sure about dumping more stuff on emilio, but also trying to make sure he can wrap all this stuff up by tomorrow....
05:58bz_sleepGreat, thanks
05:58* bz_sleep writes reftest
05:58heycamI didn&#39;t review all the other first-line patches, so if you can summarize what&#39;s going on in the patches that would be great
05:59heycamthanks for the CSP-related reviews too
05:59bz_sleepSure
06:00bz_sleepSo the basic problem with stylo and first-line is that stylo stores styles on elements
06:00bz_sleepAnd those styles know nothing about first-line
06:00bz_sleepSo we have to do after-the-fact fixup of styles on anything that goes into the first-line frame
06:00bz_sleepon the nsIFrames
06:00bz_sleepThis is done by ServoRestyleManager::ReparentStyleContext
06:00heycam(aha that&#39;s why we finally needed that funciont)
06:00bz_sleepSo for that ::first-letter case...
06:01bz_sleepWhen we mutate the kids of a block with a first-letter, we remove the letter frame, do the frame construction, then put back the first-letter frame
06:01bz_sleepThe &quot;put back&quot; operation, when first-line is involved, will get the wrong style for the letterframe
06:01bz_sleepand for the textframe inside it
06:02heycamok, and gecko gets this right because ...?
06:02bz_sleepbecause it gets it from the servo side eager pseudo stuff, where it was computed with no knowledge of first-line
06:02bz_sleepgecko gets it right because it uses the right parent style context, and in that case that&#39;s the first-line-affected one
06:02heycamok
06:02bz_sleepbecause gecko gets the parent style off the frame parents
06:02heycamand RecoverLetterFrames is the function that re-adds the letter frame after it was removed
06:03bz_sleepyep
06:03bz_sleepExactly
06:03heycamok cool, should be enough for me to go on
06:03bz_sleepGreat
06:03bz_sleepThe other patch will be similar: we&#39;re doing a DOM insert that lands inside the first-line
06:04bz_sleepAnd if it&#39;s an element, we get its style from the servo side, and it&#39;s off
06:04bz_sleepso needs fixing.
06:04heycamgot it
06:04heycamI suppose if we had something like DebugVerifyStyleTree for Servo we&#39;d know about this...
06:04* bz_sleep is finishing up the reftests
06:04bz_sleepmmm
06:04bz_sleepyes
06:04bz_sleepif we had testcases exercising it
06:05heycamheh, that too :-)
06:05bz_sleep(I mean, we do have them, sort of: that&#39;s why we get failures that I&#39;m fixing)
06:08heycamr=me on the first one
06:09ajaManishearth, so, what&#39;s the font-size of an non-compliant web font?
06:10ajaManishearth, nice post :)
06:11heycamServoTraversalFlags has some fun value names now :-)
06:12bz_sleepheycam: Thanks!
06:22bz_sleepheycam: https://reviewboard.mozilla.org/r/167404/diff/1#index_header is up
06:24* heycam looks
06:30* heycam never really thought about how caption and ::first-line interact before...
06:32bz_sleepnon-interoperably
06:32heycambz_sleep: is how ::first-line applies to table stuff spec-defined?
06:33bz_sleepdefine spec-defined?
06:33heycambz_sleep: I see chrome just shows everything red, for the first test in the patch
06:33heycamheh
06:33bz_sleephttps://bugs.chromium.org/p/chromium/issues/detail?id=707533&q=first-line%20block&colspec=ID%20Pri%20M%20Stars%20ReleaseBlock%20Component%20Status%20Owner%20Summary%20OS%20Modified
06:33bz_sleeper, https://bugs.chromium.org/p/chromium/issues/detail?id=707533
06:34heycamthough the static ones look right
06:34bz_sleepwhich I had filed a dup of at https://bugs.chromium.org/p/chromium/issues/detail?id=750268 for inline-block
06:34heycam(i.e. the references)
06:34bz_sleepWell, the references don&#39;t use first-line at all
06:35heycamoh yeah :p
06:35bz_sleepAnyway, spec says https://drafts.csswg.org/css-pseudo-4/#first-text-line
06:35bz_sleepwhich is not very much
06:36bz_sleepe.g. what happens to out-of-flows? There&#39;s no real interop there either
06:36bz_sleepalso, IE does weird stuff
06:36bz_sleepwhere color is affected by first-line but border color is not
06:36bz_sleephttps://twitter.com/bz_moz/status/890597438498721794
06:37bz_sleepAnd https://twitter.com/bz_moz/status/890614323697000448 of course
06:37heycambz_sleep: ok. well, I&#39;m happy to assume what you&#39;ve got there for the table-related ones is what we want.
06:37bz_sleepgiven that ::first-line works on the box tree, but display:contents only happens on the element tree.... the CSSWG should have dropped one of the other feature
06:37bz_sleepheycam: it matches Gecko behavior, at least
06:38heycamyeah, the minimal thing I&#39;m happy to make stylo do :-)
06:38bz_sleeper, one or the other feature
06:38heycamok r=me
06:38bz_sleep&quot;if your style sheet uses both ::first-line and display:contents, it&#39;s a parse error and the whole sheet is not applied&quot;
06:38* bz_sleep can dream
06:38heycamheh
06:39bz_sleepoh, hmm
06:39bz_sleepI could check that on the container, you mean?
06:40bz_sleephmm
06:40bz_sleepI bet my patches are wrong, actually
06:40heycamno on the frames being inserted
06:40bz_sleepbecause they stop at the float containing block
06:40bz_sleepthe frames being inserted would not have the pseudo-element data thing, why would they?
06:41bz_sleepso if I do an insert into an inline-block, I bet bad things happen
06:41* bz_sleep writes testcase
06:41heycamoops yeah only after they&#39;ve been reparented would they
06:41heycamso yes, the container
06:55bz_sleepheycam: fixed patch coming up, with more tests....
06:59* heycam awaits
07:01* bz_sleep tests to make sure all the tests are fixed
07:02bz_sleepok, posted
07:08heycambz_sleep: the change you reverted in nsCSSFrameConstructor::ContentRangeInserted
07:08heycamwas that wrong?
07:08heycam(was the original change wrong?)
07:09bz_sleepthe change to move where haveFirstLineStyle is computed?
07:09bz_sleepI reverted it because I&#39;m no longer using the haveFirstLineStyle boolean
07:09bz_sleepbecause it represents the state of the nearest block-inside
07:10bz_sleepbut for my purposes I need to know whether I have a first-line anywhere on my ancestor chain
07:10heycamah, understood
07:11njnoh look, it&#39;s downloading another rustc
07:11heycambz_sleep: re-r=me
07:13bz_sleepheycam: thank you!
07:14heycambz_sleep: these kind of frame constructor fixes make me really look forward to our new Rust-written frame constructor :-)
07:14bz_sleepmmm
07:14bz_sleepIt&#39;s not clear that it will be any better
07:14bz_sleepBut we&#39;ll see.
07:17ajanow, on to ::first-word
07:18* aja ducks
07:18bz_sleepaja: r-
07:18ajaheh.....figured :)
07:27noxemilio: Ping.
07:32emiliobholley: ping?
07:32emilionox: pong
07:33noxI don&#39;t remember, already...
07:33emiliololo
07:33emilio*lol
07:44noxemilio: Oh right,
07:44paulwhat&#39;s the proper way to pass options to the linker? I want to add `-rpath XXX`.
07:44noxdoing a last geckotry run with additional compute_distance methods to see if it changes test results,
07:44noxemilio: whether it does or not, r=you my PR once it&#39;s green?
07:46emilionox: yes
07:48bholley_mobileemilio: why is the propagated hint thing surprising?
07:48* bholley_mobile is on phone fwiw
07:49emiliobholley_mobile: not particularly surprising, but why do we need to special-case there? Is it just to not clear it below?
07:49bholley_mobileemilio: we&#39;re traversing without processing invalidations, so we don&#39;t want to clear the restyle hint
07:49bholley_mobilePropagate clears
07:50bholley_mobileOtherwise we&#39;ll drop the hints on the floor
07:51bholley_mobile(took me a while to realize that propagate mutates self)
07:51emiliobholley_mobile: fair... I don&#39;t know, the amount of code and special paths added on that bug makes me a bit uncomfortable :(
07:52bholley_mobileThere are already special paths, in just shuffling them around
07:52bholley_mobileI would have liked to remove more of them
07:53bholley_mobileBut it&#39;s hard. And right now we mask a lot of bugs Bunny just forcing everything through one callpath
07:53bholley_mobileAnyway
07:53bholley_mobileAs for why I did it in C++
07:54emiliobholley_mobile: wait a second, the RestyleHint::empty stuff can only matter for the root element, right?
07:54emiliobholley_mobile: the root of the traversal, I mean
07:54emiliobholley_mobile: otherwise there is no restyle hint before at all
07:55bholley_mobileNo, because we&#39;ll still follow dirty descendants down styled childrrn
07:55bholley_mobileAnd there might be arbitrary hints down there
07:55emiliobholley_mobile: why do we do that? There shouldn&#39;t be unstyled stuff down there
07:55bholley_mobileIt&#39;s rare but can happen as long as we enter the fc with invalidations pending
07:56emiliobholley_mobile: I guess just so we don&#39;t have to special-case the unstyled-only stuff, as we did in both sequential / parallel.rs?
07:57bholley_mobileThere isn&#39;t, but the price of getting rid of the arms length handling at the root and...Yes
07:57emiliobholley_mobile: :(
07:58bholley_mobileLook these constraints suck.
07:58bholley_mobileI would like them to be better but for now I just need them out of my way
07:58bholley_mobileParticularly the style new children stuff
07:58emiliobholley_mobile: yeah, I know...
08:00emiliobholley_mobile: but it seems we&#39;re spending more time working around the lack of lazy frame construction everywhere than doing stylo stuff... oh well
08:01bholley_mobileWell, I certainly have been spending more time on it, which is why I wanted to move it to a separate path that I can ignore and reduce the special handling for it in the servo sidr
08:02bholley_mobileAs for C++ vs rust, kinda water under the bridge at this point, though it seems reasonable enough to do dom manipulation on the gecko side
08:03bholley_mobileThe functionality get more complex with the restyle root patch, and touches presshell
08:06emiliobholley_mobile: I&#39;m kind of thinking we shouldn&#39;t do these kind of stuff unless it&#39;s absolutely, completely necessary. My patch at bug 1386045 is becoming equally gross, and I don&#39;t like that. I&#39;d rather work on kill all the random frame constructor paths we have
08:06firebothttps://bugzil.la/1386045 NEW, emilio+bugs@crisal.io stylo: Lots of time spent refreshing the stylist on facebook.com
08:06bholley_mobileWhat kind of stuff
08:07bholley_mobileStyle new children?
08:07bholley_mobileI would love for it to go
08:08emiliobholley_mobile: right. StyleNewSubtree is still needed for NAC, but StyleNewChildren is just awful in a lot of sense
08:08emilio*senses
08:08bholley_mobileNo, look at the callers, theres more
08:09emiliobholley_mobile: sure, we still need more StyleNewSubtree for insertions and such, but those we can (ideally) get rid of
08:09bholley_mobileAnyway we can discuss opitons to remove it, but ideally some time when im not on my phone, and I really need to get the restylr root stuff landed
08:10emiliobholley_mobile: yeah, just r+&#39;d the pending patch... still not super-happy about all this
08:11noxemilio: https://treeherder.mozilla.org/#/jobs?repo=try&revision=edc8169e789bb71d2e8b0a6558ff9c0d188ccc0d&selectedJob=122494235 What&#39;s this?
08:11emilionox: that is that the build failed
08:11noxHow can I know if it&#39;s my fault or not?
08:11emilionox: the log says:
08:11emiliohttps://www.irccloud.com/pastebin/IYFHdjnA/
08:11emilionox: so it looks like it&#39;s yours :)
08:11noxWait,
08:11noxwhere is that log?
08:12emilionox: when you click on the &quot;B&quot;, there&#39;s a toolbar, and the leftmost button in there is &quot;Log&quot;
08:13noxOh, never saw that button.
08:13noxTreeherder&#39;s UI in general makes little sense to me.
08:17SimonSapinnox: https://github.com/rtbo/rust-xcb/blob/v0.7.6/rs_client.py#L2098 lol cross-compilation
08:17bholley_mobileEmilio just keep in mind that im not happy about it either, and that the fact that the existing setup sucks shouldn&#39;t be a reason to complain about patches that shuffle it around. I spent a lot of time trying to make it better but it&#39;s hard to do without ratholing on stuff we don&#39;t have time to do.
08:17noxSimonSapin: Ouch.
08:17emiliobholley_mobile: yeah, I agree... Sorry if it seems I&#39;m complaining specifically about your patches, I&#39;m not
08:17bholley_mobileIf you have ideas on how to make it better file bugs and we can discuss send prioritize
08:18bholley_mobileSorry in grouchy I&#39;m just about to sleep and want to land these patches in the morning :-)
08:18bholley_mobile*im
08:18emiliobholley_mobile: one last question
08:18noxOn a similar note, it would be very nice if I never read again around here that a piece of code is stupid or dumb.
08:18* bholley_mobile sleeps
08:19emiliobholley_mobile: can you think of a reason why we need to mark the parent of a text node as dirty on the frame constructor?
08:19bholley_mobileUh
08:19emiliobholley_mobile: I think I can&#39;t, and I think the nsIContent::NoteDirtyForServo functions you just added can be removed
08:19emiliobholley_mobile: s/removed/moved to Element/
08:20emiliobholley_mobile: and actually that may explain the extra traversals in speedometer, just saying
08:20bholley_mobile?
08:21emiliobholley_mobile: speedometer does a lot of textnode insertion
08:21emiliobholley_mobile: and now we&#39;re probably going there during restyling just to find nothing changed
08:21bholley_mobileWe still need lazy FC no?
08:22emiliobholley_mobile: hmm... I guess so, yeah, but we wouldn&#39;t do the style traversal there at all
08:22bholley_mobileBut we use dd bit to find lazy fc
08:22emiliobholley_mobile: we don&#39;t?
08:22emiliobholley_mobile: we use the NODE_DESCENDANTS_NEED_FRAME
08:22bholley_mobileDo we ? I though we post reconstructs from the post traversal
08:23bholley_mobileOr does the post traversal use ndnf?
08:23emiliobholley_mobile: it does
08:23emiliobholley_mobile: http://searchfox.org/mozilla-central/source/layout/base/ServoRestyleManager.cpp#536
08:25bholley_mobileAh nice. I&#39;ll move to element in s following patch, same bug ?
08:26bholley_mobile(need to get to sleep for real now)
08:28emiliobholley_mobile: rest well :)
09:22heycamemilio: ran out of time today, but hopefully should have some cascade rebuild avoiding patches for you tomorrow
09:23emilioheycam++, awesome :)
09:23* emilio goes back to try to repro and figure out bug 1389300
09:23firebothttps://bugzil.la/1389300 NEW, nobody@mozilla.org stylo: Crash in nsRuleNode::nsRuleNode
09:24heycamemilio: if you want to look at my very rough patch and provide any feedback, you&#39;re welcome to http://mcc.id.au/temp/cascade.patch
09:25heycamemilio: though keep in mind there&#39;s a bunch of things to make look cleaner
09:25heycamemilio: and it only compiles, haven&#39;t tested ;)
09:25* heycam ducks out
09:25emilioheycam: cool, will do :)
09:28noxemilio: r? https://github.com/servo/servo/pull/18035#issuecomment-321756706
09:28crowbotPR #18035: Animate basic shapes - https://github.com/servo/servo/pull/18035
09:29emilionox: nice, r=me
09:29emilionox: is it just the change to &quot;discrete&quot;?
09:29noxemilio: Yeah.
09:30noxStill no passing test for clip-path but I&#39;ll follow up on that.
09:31emilionox: awesome :)
09:59paulI have a stupid question would using a VPN change anything in the way pages are loaded? I would assume that , beside load time, it should not impact anything, but for some reasons, pages just don&#39;t load if I use my VPN. And I have no idea how that&#39;s possible.
10:12noxjntrnr: &quot;After some heroic work on Quantum, (...)&quot;
10:16Aryxhi, is anybody working on fixing the failures from b.z&#39;s 1388877? https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6c23895588581092613c8d6a054bfe54478f1462&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel
10:18emilioAryx: looking, thanks
10:20emilioAryx: so it seems like that is basically bug 1388400
10:20firebothttps://bugzil.la/1388400 NEW, nobody@mozilla.org stylo: Assertion failure: f->IsInlineFrame() (Must only have inline frames between us and the first-
10:21noxManishearth: Why should ref_slice go back in libstd?
10:22emilioAryx: I&#39;d back out for now
10:22Aryxok
10:56travis-ciServo failed to build with Rust nightly: https://travis-ci.org/servo/servo-with-rust-nightly/builds/263443875 CC nox, SimonSapin
11:03mortimerquick question, what&#39;s the best way to pass a small DOMObject array from rust to javascript every frame? If I use the default Vec<> bindings I guess that it creates new GC values every frame. I usually create a ObjectList wrapper to avoid GC creation.
11:03mortimerBut Is there another way without that extra List boilerplate?
11:04noxWhat&#39;s a DOMObject array?
11:04noxWhat&#39;s &quot;the default Vec<> bindings&quot;?
11:05mortimerfor example this webidl: sequence<VRLayer> getLayers();
11:05mortimerThe generated binding is: fn GetLayers(&self) -> Vec<VRLayer> {
11:05noxYeah. What&#39;s wrong with that?
11:05mortimerDoes it create new arrays each time the function is called from JS?
11:06noxYes.
11:06mortimerOk, I was asking the best way toa void that GC allocation
11:06noxNot sure how it could do otherwise, given you need to create the Rust representations of the VRLayer dictionaries too.
11:06noxI don&#39;t know which GC allocation you mean.
11:07noxIt&#39;s a Vec<T>, that has nothing to do with GC, so I&#39;m confused.
11:07mortimerI mean the javascript array
11:07mortimerfor the vec<T>
11:07mortimervar array = display.getLayers()
11:07mortimerthat array
11:07noxmortimer: Pretty sure that&#39;s mandated by the WebIDL spec.
11:08noxAnd that returning the same one would be an error.
11:08noxmortimer: &quot;Sequences are always passed by value. In language bindings where a sequence is represented by an object of some kind, passing a sequence to a platform object will not result in a reference to the sequence being kept by that object. Similarly, any sequence returned from a platform object will be a copy and modifications made to it will not be
11:08noxvisible to the platform object.&quot;
11:08noxhttps://heycam.github.io/webidl/#idl-sequence
11:13xidornemilio: is it possible to have a new version of bindgen soonish? it seems to me that rust-lang-nursery/rust-bindgen#829 actually makes merging worse than before without rust-lang-nursery/rust-bindgen#872
11:15mortimerThanks for that info, the case I&#39;m working on now is a FrozenArray
11:16noxmortimer: AFAIK it&#39;s the same for FrozenArray.
11:16noxheycam|away: Could you confirm? ^
11:31fwiwcrowbot: what&#39;s new?
11:31crowbotIn a head-to-head comparison, Mosaic tied Servo in crypto implementation enforcement
11:31mortimernox: Yes, I checked an the spec allows to define &quot;[SameObject] FrozenArray&quot; in order to return the same array instance
11:40emilioxidorn: sure
11:41emilioxidorn: could you verify nothing has broken in stylo with master? There have been a bunch of PRs I haven&#39;t been involved with, and I&#39;d rather make sure
11:43xidornemilio: hmm, I can do a local build and see. I&#39;m not sure how can I do a try push with a not-yet-published version
11:43emilioxidorn: you can use git dependencies and ./mach vendor I think
11:43xidornoh, interesting
11:43xidornI can try
11:45emilioxidorn: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e624e1500190739b02ddb167c13ea1cee5b7cf43&selectedJob=122540910 :(
11:45emilioxidorn: I really wanted to get rid of our frame constructor hacks in stylo
11:45emilioxidorn: but look at all that orange :(
11:45xidornwell :/
11:46emilioxidorn: and that is with a few extra fixes, without them it&#39;s even worse
11:47xidornemilio: it seems mach vendor doesn&#39;t work well with git dependency
11:47emilioxidorn: oh well... You can verify it works on windows, and I can verify it works on linux, I guess
11:47emilioxidorn: I don&#39;t expect major breakage, I just want to sanity-check
11:48xidornand file a bug for cargo-vendor :)
11:48xidornI&#39;ll do that
11:54emiliok, thanks :)
11:58xidornemilio: actually... I guess we can still do a try push, with rust-bindgen vendored in-tree manually somewhere, and have style crate&#39;s Cargo.toml points to that
12:09xidornemilio: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5cf16842f1708ef103c670638edf9e2a1d7715c hopefully it works
12:10emilioxidorn: awesome, thanks :)
12:11xidornah, I should exclude tests from that copy I guess :/
12:17xidornemilio: oops
12:17xidornemilio: my change breaks stylo
12:17xidornfn __bindgen_test_layout_NotNull_open0_ptr_const mozilla::Encoding_close0_instantiation()
12:20emilioxidorn: that&#39;s why I wanted to do this beforehand :-)
12:20* xidorn doesn&#39;t know what he should do or feel at a time like this
12:20emilioxidorn: yeah, I can try to look into it
12:20noxemilio, SimonSapin: In https://doc.servo.org/style/properties/animated_properties/enum.AnimationValue.html, do we actually need to discriminate e.g. BorderTopRightRadius from BorderBottomRightRadius?
12:20noxCould the codegen use the same variant for the two of them?
12:21emilionox: presumably, yeah... Why do you want that?
12:21xidornemilio: thanks
12:21noxemilio: Less code.
12:21noxemilio: And thus less code size in the end I think.
12:21SimonSapinnox: I dont know
12:21emilionox: yeah, you can presumably build a map of (computed_value_type, animation_kind) -> property
12:22emilionox: or something like that
12:22SimonSapinnox: the uncompute method might need it
12:22noxSimonSapin: Good call.
12:22noxActually no.
12:22noxSimonSapin: ToComputedValue is derived for AnimationValue.
12:23noxSo it doesn&#39;t pass any info about which property it is to the inner ToComputedValue impl that gets called.
12:23SimonSapinimpl AnimationValue fn uncompute(&self) -> PropertyDeclaration
12:23noxMmh.
12:23noxTIL
12:23SimonSapinone of the two call sites also has nsCSSPropertyID
12:24SimonSapinthe other has AnimationValueMap, which is FnvHashMap<AnimatableLonghand, AnimationValue>
12:25SimonSapinnox: so it looks like we can know what property it is from context
12:25noxSimonSapin: What about Servo_AnimationValue_Uncompute?
12:27SimonSapinhmm, missed that one
12:28SimonSapinhttp://searchfox.org/mozilla-central/source/layout/style/nsTransitionManager.cpp#798 has a nsCSSPropertyID
12:28noxInteresting.
12:28noxUnrelated, I wonder if we could merge add_weighted, add, interpolate and accumulate.
12:28SimonSapinbut not http://searchfox.org/mozilla-central/source/layout/style/nsTransitionManager.cpp#132
12:28SimonSapinat least not obviously
12:30noxHaving a single method for all of them, taking a other: &Self and an enum Op {Interpolate(f64), Add, Accumulate(u64) }.
12:30noxMost implementations would then just need to pass that damn enum value.
12:30noxInstead of having two similar methods that just calls either add or interpolate in the same places with the same arguments.
12:31* nox checks if we ever use add_weighted directly...
12:32noxAFAIK, we don&#39;t.
12:42Aryxxidorn: hi, can you request the landing of bug 1388255, please? the stylo part has landed and busted autoland
12:42firebothttps://bugzil.la/1388255 NEW, xidorn+moz@upsuper.org Move mGridTemplate{Columns,Rows} into a UniquePtr
12:43xidornAryx: that has landed
12:43Aryxoh sorry, thanks. was under the impression that i was at the top of the th page
12:46xidornAryx: btw, if that ends up causing bustage somewhere and you need to back out, you ne