mozilla :: #servo

9 Sep 2017
00:33mbrubeckbholley: has a working SmallBitVec type, though it's has only the most basic functionality so far (push, pop, get, set).
00:34bholleymbrubeck: awesome! Thanks for doing that so fast :_)
00:34mbrubeckbholley: When "spilled" onto the heap, it stores its length and capacity as u32, both because that's what Rust uses for bit shifting operators, and because I thought it might be a useful memory optimization versus usize.
00:35mbrubeckbholley: But it would be easy to change those to usize if we want it to have the same max capacity as a normal Vec.
00:35bholleymbrubeck: sgtm - we are very unlikely to need such capacity
01:17ajeffreystandups: Working on integrating the new JS bindings with JS's class mechanism.
01:17standupsOk, submitted #50711 for
01:55bzSince our one use case for BitVec so far is "whether CSS properties are important", being able to store a few thousand bits is plenty.
02:23jdmlinker errors that require updating common headers are just the worst
02:23jdmyou got so far before the error appeared
02:23jdmand then you need to do it all over again
02:24larsbergYou have just described android rust hacking, but like for every error
02:36bzWe really need to do something about this string impedance mismatch. :(
02:36bzIt was bad enough with just JS and C++, but at least we had non-copying ways of going between them...
02:36bz(if only in the C++-to-JS direction)
02:36bzNow we have copying between C++ and Rust. :(
02:42* bz tries to recall how to do cargo check in a stylo tree sanely
02:43bzI guess I have to do it inside the servo dir...
02:44bz"Python virtualenv is not installed. Please install it prior to running mach."
02:45* bz pokes package managers
02:45jdmI thought `./mach cargo check` worked
02:48bzusing the mach in the gecko tree?
02:49bzit exists, but...
02:49jdmthat was my impression, but i've not tried
02:49bzit runs check on gkrust
02:49bzwhich is succeeding immediately
02:49bz(I know I made rust code changes in style crate that are bogus)
02:49bzmach cargo check style says "Cannot locate crate style. Please check your spelling or add the crate information to the list."
02:57bzcompiler error messages...
02:59bzIf something says
02:59bz#[derive(Clone, PartialEq, ToComputedValue)]
02:59bzDoes that mean it's expected to somehow pick up a ToComputedValue impl?
02:59bzfrom somewhere?
02:59bzLooking at
03:00jdmbz: it means that its ToComputedValue implementation is just calling .to_computed_value() on each member contained in it
03:00* bz pokes
03:01jdmthat feeling when I finally don't have any compile errors in my change, and now I need to remember what I planned to do once I reached that stage
03:01* jdm was hoping to reach the point of being able to measure gmail before having to leave for the airport
03:06bzyou're working on the string thing, right?
03:06* bz is boxing stuff, hence the questions above... ;)
03:09bzSo wait
03:09jdmcorrect, string thing
03:09bzI tried implementing impl<T> ToComputedValue for Box<[T]>
03:10bzer, impl<T> ToComputedValue for Box<T>
03:10jdmoh good, my flight was delayed so this may still be possible
03:10bzAnd now the compiler complains that I have conflicting implementations of ToComputedValue for std::boxed::Box<_>
03:10bzIn particular, it says that my impl is one impl
03:10bzAnd impl<T> ToComputedValue for T where T: ComputedValueAsSpecified + Clone,
03:10bzis the other
03:11bzBut Box<T> doesn&#39;t have ComputedValueAsSpecified, I&#39;d think...
03:11jdm shows that there is already one for Box<[T]
03:11pcwaltonstandups: Yesterday, worked on the 3D demo and implemented ECAA subpixel antialiasing. Today, polished up the demos a bit and implemented rudimentary path stroking for SVG.
03:11standupsOk, submitted #50712 for
03:11jdm(where T: ToComputedValue)
03:11* jdm wish there were links to each one
03:12bzThere&#39;s one for Box<[T]>
03:12bzI want one for Box<T>
03:12bzWhere T is ComputedValue
03:12pcwalton subpixel antialiased text on GPU the kerning and hinting still need improvement but its a good first step
03:12jdmoh, I misread
03:13bzI mispasted at first
03:13pcwaltongw: ^ if youre interested this is all GPU rasterized using GL3
03:13jdmbz: I think the complaint is that Box<T> could overlap with T: ComputedValueAsSpecified, even if it currently does not?
03:14jdmbz: what happens if you use T: ToComputedValue in your impl?
03:14bzWhy is that not a problem for Vec<T>?
03:15bzMy impl looks like this:
03:16jdmI can reproduce that exact problem
03:16jdmeven down to Box vs. Vec
03:17bzyes, that is my reaction to
03:17bzer, too
03:17bzMaybe one of the many impl ComputedValueAsSpecified for SpecifiedValue {} we have has a SpecifiedValue that is a Box ?
03:17bzWell, that would probably exclude impl ComputedValueAsSpecified for SpecifiedValue as the problem. ;)
03:18bzMaybe it&#39;s because Box has deref?
03:18bzor something?
03:18jdmthat&#39;s plausible, since Vec&#39;s deref goes to [T] instead of T
03:18bzMy main problem is that I have a thing that does derive(ToComputedValue)
03:19jdmbut Rc also works
03:19bzbut if I make one of its members a Box, then there&#39;s suddenly no definition of ToComputedValue for it
03:19bzSo I figured fine, I&#39;d define ToComputedValue for Box
03:20* jdm wonders if it&#39;s an issue with the derivation
03:21jdmI guess it&#39;s that&#39;s triggering the Box check
03:22* bz reads that
03:22bzOh, derive is a macro?
03:22jdmI wonder if making that &#binding would make a difference
03:22bzlemme check
03:24* bz hacks line 40 too
03:24bzAlso no
03:25jdmyeah, that&#39;s from_computed_value
03:25bzI changed both line 30 and line 40
03:25bzto have &#binding
03:25bzNo change....
03:26bzI mean, I guess I could write a manual impl of ToComputedValue for my thing
03:26bzBut this is annoying
03:27bzOh, hmm
03:27bzDid you mean make that &#binding
03:27bzAnd not actually define ToComputedValue for Box?
03:28bzpcwalton: Do you have any idea what&#39;s going on here?
03:28bzoh, he left
03:28* bz mutters
03:29bzCan I have negative trait bounds?
03:29bzLike impl X for Y when Y does not implement some trait?
03:29jemonly for built in traits right now
03:29jemotherwise that would be the obvious solution
03:30bzI mean, the issue in your play testcase is that the compiler thinks that Box can implement T2
03:30bzor something
03:31jemspecifically, there is nothing preventing me from implementing T2 for a boxed type, which would lead to an overlap
03:31bzWhy does Rc not have that issue?
03:32jemI think Box is treated in a special way by the compiler, but I&#39;m not sure why
03:32* bz sighs
03:32jembz: however, if you implement the trait for the specific boxed type instead of a generic, it works
03:32bzI don&#39;t know what my specific boxed type is
03:32bzThis is my code
03:33bznote that Box<Gradient>
03:33bzBut Gradient is a type param...
03:33bzI mean, I could implement it at all the places that invoke Image
03:33bzMaybe I can do it that way....
03:33jemoh, you know why the derivation complains I bet?
03:34jemmaybe the type parameters aren&#39;t bounded by ToComputedValue?
03:34bzI could add that
03:34bzone sec
03:35bzpub enum Image<Gradient: ToComputedValue, MozImageRect: ToComputedValue, ImageUrl: ToComputedValue> {
03:35bzLike so?
03:36* jem isn&#39;t sure if rust cares about those or not, but worth a shot
03:36jembecause the real question is what the generated ToComputedValue impl looks like
03:36jemthat would need `impl<Gradient: ToComputedValue, ...> ToComputedValue for Image<Gradient, ...> {
03:36* bz isn&#39;t sure how to get his hands on that
03:37bzfwiw, if I do what I pasted above, I still get the &quot;conflicting implementation&quot; issue
03:37jemhonestly I would just write it by hand for now and ask nox for assistance deriving it if it pans out
03:38bzOK, let&#39;s try this
03:50Manishearthemilio: still need review on viewports
03:57bzOK, style question
03:57bzSay I have:
03:57bz GenericImage::Gradient(gradient) => {
03:57bz self.set_gradient(gradient)
03:57bz fn set_gradient(&mut self, gradient: Gradient) {
03:58bzBut now GenericImage::Gradient stores a Box<Gradient>
03:58bzShould I change set_gradient to take Box<Gradient>?
03:58bzOr change it to take &Gradient and pass gradient.borrow() ?
03:58bzOr something else?
04:00bzOr I guess I can set_gradient(*gradient)
04:00bzAnd leave it taking Gradient?
04:00mbrubeckI think is fairly complete now
04:00mbrubeckNot yet published to
04:00* bz is really not sure when moves/clones are happening here....
04:01* jem checks what set_gradient does
04:02* bz makes get_gradient definitely return Box<Gradient>
04:03jembz: it looks like you might be able to get away with a &Gradient argument
04:04Manishearthbz: set_gradient should take a Gradient, which you should box within the code
04:04bzManishearth: I don&#39;t follow
04:04jemManishearth: that does not make sense
04:04Manishearthwait, sorry
04:04ManishearthI misread
04:05Manishearthbz: it could go either way
04:05Manishearthgenerally you don&#39;t return boxed things but until we have placement new avoiding the copy is great
04:05Manishearthand set_gradient probably should just take a Gradient which you move out of the box when calling it
04:06Manishearthself.set_gradient(*gradient) will work
04:06bzI can do that...
04:06ManishearthBox is special and you can move out of a box
04:06bzBut doesn&#39;t that involve an extra large memmove?
04:06bzAs opposed to passing a Box<Gradient> to set_gradient?
04:06* bz tries to understand scoping rules.
04:06bz match *self {
04:06bz Image::Url(url) => Image::Url(url.to_computed_value(context)),
04:06bzExcept the thing before => is one Image (with one set of type params)
04:07bzAnd the thing after => has a different set of type params
04:07bzBut using Self::Url and Self::ComputedValue::Url, which is what I would have expected, does not work?
04:08Manishearthbz: nope :)
04:08jembz: you have you use the explicit type name
04:08Manishearthit should produce a Gradient rvalue
04:08Manishearthso it won&#39;t actually memmove
04:08bzjem: hmm?
04:08jembz: what does the type ComputedValue line say?
04:09bzManishearth: ok.... btw, if you have any bright ideas about our earlier problem with Box...
04:09bzjem: type ComputedValue = Image<<Gradient as ToComputedValue>::ComputedValue,
04:09bz <MozImageRect as ToComputedValue>::ComputedValue,
04:09bz <ImageUrl as ToComputedValue>::ComputedValue>;
04:09jembz: so wait, what does not work with your current code?
04:10bzno associated item named `Gradient` found for type `values::generics::image::Image<<Gradient as values::computed::ToComputedValue>::ComputedValue, <MozImageRect as values::computed::ToComputedValue>::ComputedValue, <ImageUrl as values::computed::ToComputedValue>::ComputedValue>` in the current scope
04:10Manishearthbz: oh, nope, it might memmove
04:10bzAnd it points to Self::ComputedValue::Gradient
04:10bzManishearth: heh
04:10jembz: don&#39;t you want Image::Gradient?
04:10Manishearthbz: what earlier problem with box?
04:11bzjem: well, I don&#39;t know what I want
04:11bzManishearth: when that commented-out line is uncommented, things don&#39;t compile
04:12bzManishearth: So I can&#39;t just impl<T> ToComputedValue for Box<T>
04:12bzManishearth: So I can&#39;t derive(ToComputedValue) for Image
04:12bzManishearth: which is why I&#39;m writing it by hand
04:12bzjem: It&#39;s weird to me to have Image::Url mean two different things in that line
04:12bzjem: As in for different instantiations of Image
04:13jembz: it&#39;s just type inference doing its thing
04:13Manishearthbz: Box<T> is #[fundamental]
04:13bzBut it&#39;s confusing that Self::Url did not work
04:13bzand Image::Url does
04:13bzManishearth: which means what?
04:14* bz points to the utter un-searchability of #[fundamental] :(
04:14crowbotIssue #24317: Rc and Arc should be marked #[fundamental] -
04:14Manishearthbz: it&#39;s an internal thing
04:14bzWhich is why Rc didn&#39;t fail the same way
04:15Manishearthbz: #[fundamental] isn&#39;t something non-std users can use
04:15Manishearthit&#39;s specifically for std to special case some types
04:15Manishearthbasically certain smart pointers
04:15Manishearthwhat it does is that it grants more freedom implementing traits for boxed/&borrowed types
04:15Manishearthhowever you lose the freedom to do blanket impls
04:15Manishearth> A #[fundamental] type Foo is one where implementing a blanket impl over Foo is a breaking change. As described, & and &mut are fundamental. This attribute would be applied to Box, making Box behave the same as & and &mut with respect to coherence.
04:16bzI see
04:16bzAlright, thanks
04:16bzSo there&#39;s no real way to make derive() work with members that are Box?
04:17Manishearthtweak the deriver to understand and punch through boz
04:17bzUnless I explicitly impl the trait I am deriving for box<stuff> ?
04:17Manishearthyou can do that too
04:17Manishearthbut I&#39;d tweak the deriver
04:17ManishearthI can do this for you if you need
04:17bzI have the manual impl written, I think
04:17bzBut it would be cleaner to not do that, of course
04:18bzSo how can the deriver detect a Box?
04:18bzIt&#39;s checking attrs.clone to detect clonable things....
04:25bzThe games with the trait were by far the hardest part of this
04:28bzOpt builds that involve rust-side stylo changes are just as slow on this box as on my laptop...
04:29* bz needs a better solution
04:29* jem wonders why runs for a good 10s at the start of his build when no ipdl files are changed
04:32bzjem: Because our build system hates you.
04:32bzjem: I mean, it hates all humans, but you&#39;re human, so....
04:32* bz assumes jem is human.
04:33* jem feels human most of the time
04:33bzI have no evidence whether our build system hates advanced AIs or not
04:35* bz slices a few megs off style-sheets
04:36Manishearthbz: just check if it&#39;s literally Box<T> :)
04:36jembz: so, when you said that style-sheets should go down if the url string thing is improved...
04:37* jem takes another measurement
04:37Manishearthoh, jem is jdm
04:37* bz realizes who jem is. ;)
04:37* jem has his alt nicknames just increase the middle initial by one letter
04:37bzDo you loop to jam once you hit jzm?
04:37Manishearthjem: in the event of a data breach, changing your middle name is a good security measure
04:38jemthere appears to be no change in these numbers
04:38* bz still needs to decide what to do about Equifax
04:38jemnow I need to figure out why
04:38bzjem: so.. it&#39;s possible this stuff is unreported. One sec.
04:39bzYou&#39;re changing the computed value, not the specified one, no?
04:40bzSo it won&#39;t be measured in style-sheets
04:40bz has a dmd stack
04:40firebotBug 1397971 NEW, stylo: lots of memory used for SpecifiedURLs relating to images for gmail
04:40bzwhich means it&#39;s part of heap-unclassified
04:40bzDid _that_ go down at all?
04:40bzBest would be to run dmd and check whether the thing goes away or gets smaller...
04:40jembah, I didn&#39;t record the memory sizes before applying this patch
04:41jemtime to go to the airport
04:42bzHave a good flight!
05:22* bz wonders where Parse is implemented for ImageLayer
05:43bzhow do I run servo/tests/unit/stylo/ ?
05:43bzmach cargo-geckolib test?
05:43bzSomething else?
05:44bzbecause &quot;mach cargo-geckolib test&quot; says &quot;running 0 tests&quot;
05:45bzah, mach test-stylo
05:53bzOh, crap
05:54bzchanging the parser involves cargo insanity...
05:54* bz tries to recall how to do this
06:01bzbholley: Do we have docs on how to do local overrides?
06:01bzbholley: of cargo stuff?
06:01bholleybz: it&#39;s actually pretty easy
06:01bholleybz: now
06:01bzbholley: e.g. if I need to edit rust-cssparser?
06:01* bz recalls seeing a doc, but isn&#39;t finding it
06:01bholleybz: cargo install cargo-edit-locally
06:01bholleybz: then cd into toolkit/library/rust
06:02bholleybz: then cargo edit-locally cssparser --path /path/to/third_party/rust/cssparser
06:02bzThis is new
06:02bholleybz: worked for me a few days ago
06:02bholleybz: (for cssparser)
06:02bzI haven&#39;t tried local edits in about 2 months. ;)
06:02bzlemme try
06:02* bz waits for cargo install to be done
06:02bholleybz: it generated the patch in bug 1397964
06:02firebot WORKSFORME, stylo: use a SmallVec in SelectorList
06:03bholleybz: in case it doesn&#39;t work for you for some reason
06:03* bz waiting for cargo to build ....
06:03bz&quot;Compiling cargo v0.18.0&quot;
06:06bzSo this will let me test locally?
06:07bzBut for actual landing I&#39;ll want to land on rust-cssparser, then do a version bump on it somehow, then update our stuff to pull in the new version?
06:14bholleybz: that&#39;s right
06:14bholleybz: probably best to review the unified patch that you test with
06:15bholleybz: because updates are an annoying round-trip
06:15* bholley -> bed
06:16bzbholley: Yeah
06:16bzbholley: btw, seems to be working fine, thank you!
06:36bz println!(&quot;Capacity: {}&quot;, values.capacity());
06:36bzCapacity: 18446744073709551615
06:45bzSo where is specified::ImageLayer::parse defined?
06:46bzAnd how would I find that info?
06:52emilioManishearth: i don&#39;t see viewport related patches either in the PR list or in my bugzilla queue?
06:52emiliobz: should be in components/style/values/specified/image iirc
06:54bzemilio: you&#39;d think
06:54bzemilio: I see it for Image there, but not ImageLayer...
06:55paulmbrubeck: you think the build error in #18410 is related to my change?
06:55crowbotPR #18410: harfbuzz-sys update -
06:56emiliobz: so `ImageLayer` is defined as pub type ImageLayer = Either<None_, Image>;`
06:56emiliobz: so it ends up using Either::parse
06:57emiliobz: which is at
06:57bzyes... which is...
06:57bzAh, I see
06:57emiliobz: can&#39;t link that fast ;)
06:57* bz didn&#39;t find it when searching because he&#39;s dumb
06:58bzok, that makes sense
06:58emilionp :)
06:58* bz is making progress on the background array thing
07:00bzI should measure gmail
07:04bz12.89 MB to 7.99 MB
07:04bzJust for the main gmail page
07:04bzAnd more savings in the subframes
07:06sewardjemilio: ping
07:07emiliosewardj: pong
07:07emiliobz: neat
07:08sewardjemilio: moin
07:08emiliosewardj: morning :)
07:08sewardjemilio: am looking at your comments from last night, but still a bit confused
07:09firebotBug 1395064 ASSIGNED, stylo: Add uses of fallible Vec, SmallVec and HashMap facilities
07:09sewardjemilio: you say &quot;should propagate up add_stylesheet&quot;, but the patch does that already
07:09bzemilio: Trying to decide how ambitious to get
07:09emiliosewardj: up to rebuild() I mean
07:09emiliosewardj: sorry
07:10sewardjemilio: yes. So the OOMs are caught in rebuild
07:10emiliosewardj: I mean making rebuild() return a result<..>
07:10emiliosewardj: so we only handle it once
07:10sewardjemilio: ok, so, propagate out of rebuilt, to its caller
07:10noxjem: Please don&#39;t suggest doing that.
07:10emiliosewardj: yeah, sorry :)
07:11noxYou are basically saying we should write derived impls by hand and then let me derive them again.
07:11emiliosewardj: so I overlooked it and the problem of keeping adding to the map, and with your patch we don&#39;t do it, but still seems worth to propagate it out of rebuild()
07:11noxThat can wait for Monday and I can help by then.
07:11sewardjemilio: ok. and in, I should propagate out of note_selector, yes?
07:12emiliosewardj: yeah, that should only be called from add_stylesheet, so should be trivial.
07:12* sewardj tries
07:14noxWhy can&#39;t you implement it for Box<T> bz?
07:14noxBox<[T]> and Box<T> where T: TCV should definitely be compatible impls.
07:15nox[T] is not TCV itself so it should work.
07:15noxIf it doesn&#39;t that means we have some impl for Box<T> already.
07:15noxPlease do not unship my derived code on a Saturday.
07:16noxbz: ^
07:17noxManishearth: Please do not tweak the derive macro for Box<T> either.
07:19bznox: hmm?
07:19bznox: I can&#39;t implement it for Box<T> because of the compiler error?
07:19bznox: did you see jdm&#39;s playground link above?
07:19noxBecause there is a conflicting impl.
07:19bzbecause Box is special like that
07:20noxNo, link?
07:20bzUncomment the commented-out line
07:20bzWatch it not build
07:20bzBecause of
07:21bzSo basically you can&#39;t generically define a trait for Box<T>
07:22bzMore precisely, if you have a trait implemented for all U that implement some other trait...
07:22bzThen you can&#39;t implement it for Box
07:22bzBecause apparently the compiler assumes Box might implement that other trait
07:22bzor something
07:23bzThe problem is not Box<[T]> vs Box<T>
07:24noxIf the problem is CVaS, just remove that impl.
07:24bzThe problem is that this:
07:25bzconflicts with &quot;impl<T> ToComputedValue for Box<T>&quot;
07:25noxWe should kill CVAS.
07:25bzBecause the compiler thinks that Box<T> might implement ComputedValueAsSpecified
07:25bzThat seems like a nontrivial project
07:25bzlonger than a few hours
07:25noxCVAS serves no purpose in a world without specialization.
07:32paulWhy do my &quot;@bors-servo retry&quot; don&#39;t trigger a retry? - I&#39;m in the try group:
07:32crowbotPR #18410: harfbuzz-sys update -
07:36Manishearthnox: why?
07:37sewardjemilio: how strictly does one follow rustfmt&#39;s suggestions?
07:37emiliosewardj: moderately strictly? If there&#39;s something that looks super-ugly, I&#39;d report it as an issue
07:37emiliosewardj: which piece of code are you looking at?
07:38Manishearthnox: and the reason it doesn&#39;t work for box is the confluence of CVAS and #[fundamental]
07:38Manishearth(it works for Vec)
07:39noxManishearth: Because we can just kill CVAS and special cases are bad.
07:40emiliosewardj: I think I&#39;d follow that suggestion, usually we don&#39;t have `if { } else { }`s where one of the branches is in the same line and the other isn&#39;t.
07:40sewardjemilio: ok, will do
07:40emiliosewardj: the alternative is defining a free function, which may look a bit more elegant? Not sure if worth it
07:42noxI would follow it too.
07:42noxI know a single place where I didn&#39;t follow that though, because of lack of `let ... else`.
07:47sewardjemilio: for this .. .. you asked, why didn&#39;t I use try_push there
07:48sewardjemilio: the compiler says there&#39;s no try_push for VL.
07:48sewardjah, you made some reference to making try_push concrete
07:49sewardjemilio: but I don&#39;t know what that means
07:49emiliosewardj: yes, that&#39;s because the `VecLike` trait doesn&#39;t define / require `try_push`, but I think we only call it with `SmallVec<[T; 1]>`
07:49sewardjIs there any simple fix?
07:50emiliosewardj: yes, I think this should work:
07:51emiliosewardj: though I guess we can just remove that function? It seems it has a single caller
07:56emiliosewardj: ^ removes the function
07:56emilionox: r? ^
07:57sewardjemilio: your revised version works.
07:57sewardjemilio: shall I leave the revised version in my current patch, then?
07:57sewardj(else it gets complex w.r.t rebasing)
07:57bzAnyone know whether simonsapin is generally around?
07:58bzi.e. whether it&#39;s ok to ask him for reviews
07:58emiliosewardj: as you wish, you can also remove it and just rebase on top and add try_entry and try_push to the code I added above
07:59emiliobz: he&#39;s generally around I think, just at not-so-regular schedules :)
07:59emiliobz: I think xidorn has been asking reviews from him, so I guess you can too without too much trouble
07:59* bz has a rust-cssparser patch....
08:00sewardjemilio: I&#39;ll leave my version in place for now.
08:00bzOf course I also need it to land before we branch stylo... ;)
08:00emiliobz: I can probably also review it and 301 to him if I&#39;m not 100% confident about the patch
08:01emiliosewardj: sounds good :)
08:01noxbz: You can ask for reviews any time you want, no? It&#39;s not like we asking for one means you get one immediately.
08:01bzIt&#39;s more like &quot;if he&#39;s out all next week&quot;...
08:02noxJust a Frenchman on a Saturday.
08:05bzI don&#39;t expect him to look at it until Monday
08:05bzWhich will be plenty early enough for me
08:05bzEsp because his Monday starts earlier than mine. ;)
08:05noxHeh. :)
08:05noxI may end up killing CVAS on Monday morning.
08:12bznox: well, if you do, then I can rip out the manual ToComputedValue impl forom my patches in bug 1397614
08:12firebot NEW, stylo: shrink background-images representation
08:42sewardjemilio: ok, I have a revised patch. Can I r? you on it?
08:45emiliosewardj: sure
08:46emiliobz: ping? Can I ask you a real quick (hopefully) question about Gecko&#39;s DOM?
08:46sewardjemilio: ok, will do, thx. Also, what try flags do you typically use?
08:47emiliobz: what&#39;s the deal with the NODE_FORCE_XBL_BINDINGS flag? I find it super-confusing
08:47emiliosewardj: I usually do `./mach try -b do -p linux64 -u all`
08:48sewardjemilio: ok. I will also do a talos run because I am worried that the extra checks might cause slowdowns
08:48emiliosewardj: cool, sounds good
08:48sewardjemilio: ok, out .. back later today
08:53bzemilio: it&#39;s simple
08:54bzemilio: XBL bindings are attached at the point when the node gets styled
08:54bzemilio: Or when a node in a docuemnt is touched from script
08:54bzemilio: with me so far?
08:54emiliobz: yes
08:55bzemilio: OK, so now consider some script that takes a node and clones it
08:55bzemilio: via cloneNode
08:55bzemilio: the clone is not in a document until you insert it somewhere
08:56bzemilio: So per my description above doesn&#39;t get XBL bound to it.
08:56bzemilio: And even if you then insert it in the document, still no XBL until styling happens
08:56emiliobz: yes, that makes sense so far
08:56bzemilio: From a theoretical point of view this is sucky if the XBL is exposing methods on the node
08:57bzemilio: in that you can&#39;t call the methods on the clone until ... sometime
08:57emiliobz: so instead of that we just force-attach them when the node is bound to the tree?
08:57bzemilio: from a practical point of view, long story short XUL elements when cloned would copy over XBL binding stuff back when we had XUL elements sharing stuff
08:57bzAnd we had UI code depending on that
08:58bzMore precisely, there used to be a time when XUL elements had an &quot;inner&quot; thiing
08:58bzAnd cloning just shared it
08:58bzAnd the inner thing knew about bindings
08:58bzSo clones would get the binding attached at clone time
08:58bzSo when we ripped that shared-inner thing out to make XUL elements a lot more like normal elements, we kept that behavior
08:59bzwe set the NODE_FORCE_XBL_BINDINGS flag while cloning.
08:59emiliobz: I see... So I was trying to address the fact that XBL anon content gets unbound via a runnable, because that leaves the flattened tree in an inconsistent stuff.
08:59bzAnd if that flag is set, wrapper creation attaches the XBL binding.
08:59emiliobz: (this is bug 1397983, fwiw)
08:59firebot NEW, stylo: Assertion failure: !mServoRestyleRoot || mServoRestyleRoot == aRoot || nsContentUtils::Conten
09:00emiliobz: and so far so good, except that we don&#39;t bind them back on BindToTree modulo when the force xbl bindings flag exists
09:00emiliobz: which causes us to iterate unbound content from frame construction, which is no good
09:01bzright, because if NODE_FORCE_XBL_BINDINGS then we can have the out-of-document clone that has a binding attached and hence anon content
09:01bzAnd then when we actually insert it somewhere we have to deal with the anon content
09:01emiliobz: I got a patch ( that gets the tree green, but it&#39;s pretty much a hack
09:01emiliobz: so I was trying to come up with the right fix for it
09:02emiliobz: I got, but I was wondering when / how I needed to handle the force xbl flag
09:02emiliobz: just pushed ^ to try, let&#39;s see...
09:02bzIt&#39;s 5am
09:02bzAnd I&#39;m still up
09:03emiliobz: nvm
09:03bzWhich means I&#39;m not going to be able to answer this right now
09:03emiliobz: sleep weel
09:03bzWill do.
09:03emiliobz: well*
09:03bzPlease needinfo me?
09:03bzAnd I&#39;ll look on Monday
09:03emiliobz: I was surprised you were still around :P
09:03bz_sleepYeah, well, I got stuck compiling rust opt code
09:03bz_sleep10 mins per compile....
09:03bz_sleepMany compiles
09:03bz_sleepAh, well
09:03emiliobz_sleep: heh, fair enough...
09:03emiliobz_sleep: anyway, good night!
09:25wcpanemilio: will we have any dirty bits for transitioning elements? (in servo)
09:54wcpanemilio: that&#39;s different from RestyleHint.has_animation_hint ?
09:55emiliowcpan: `has_animation_hint` is set on the element that is actually animating, `HAS_ANIMATION_DIRTY_DESCENDANTS` is set on the ancestors
09:56wcpanemilio: oh, i see, thanks! i&#39;ll give another try
10:32SimonSapinbz: I&#39;m not around this weekend, but otherwise yes I can do reviews
10:32SimonSapinbz_sleep: ^
10:38xidornnox: is it possible to have #[derive(ToComputedValue)] support boxed field?
10:44emilioxidorn: only if we remove ComputedValueAsSpecified (which we can do and nox was planning to do IIUC)
10:45xidornemilio: what&#39;s the plan to remove that? add some more annotation to the type?
10:46emilioxidorn: Derive `ToComputedValue` more, or add an annotation, I guess, yeah
10:47xidornemilio: currently ToComputedValue deriving only recognizes those in values::{specified,computed} IIUC
10:48xidornthat wouldn&#39;t work for ComputedValueAsSpecifieds which are mostly specified ad hoc in properties
10:48emilioxidorn: you can add a whole different path, that&#39;s trivial, since you can just use `Self`
10:49xidornemilio: but how would you recognize that a type wants to be ComputedValueAsSpecified?
10:49noxemilio: No need for annotations imo
10:50emilioxidorn: with a new attribute? You can hack a `derive(ToComputedValueAsSpecified)`, but not sure how nox plans to do it
10:50noxxidorn: Deriving works out of the box for CVAS.
10:50emilionox: you just plan to implement manually, maybe using a macro?
10:50noxI mean everywhere where there is CVAS,
10:50noxyou can just derive TCV instead.
10:51noxCVAS is just a silly optimisation in the absence of specialisation.
10:51emilionox: right, but how do you plan to do `CVAS` for a type? Just manually `impl TCV`?
10:51emilionox: I&#39;d just do a macro
10:51noxI don&#39;t understand.
10:51emilionox: `struct SpecifiedValue(i32)`
10:51noxJust derive TCV.
10:51noxYeah. That works already.
10:52xidornnox: so we can already remove CVAS?
10:52noxYou can derive TCV on that.
10:52xidornnox: how does that work?
10:52emilionox: do we have ToComputedValue for i32?
10:52noxYes. It&#39;s just grunt work mostly, and some refactoring around keyword enums.
10:52noxxidorn: Same as for any type with parameter types.
10:52noxExcept there are no parameter types.
10:54emilionox: oh, I see, neato
10:55emilionox: I can rip a few ones off in a bit
10:55noxemilio: Sure.
11:32emilionox: I&#39;m pretty sure even with specialisation we can do without CVAS fwiw
11:32emilionox: we could do something smart in the derive code instead, if it mattered
11:32noxemilio: Yes, but,
11:33noxemilio: with specialisation, we could have TCV for Vec<T> where T: TCV,
11:33noxand then CVAS for Vec<T> where T: CVAS,
11:33noxand have the latter use Clone for Vec<T>,
11:33noxwhich is itself specialised for T: Copy, which just does a memcpy.
11:34noxIn practice I think it&#39;s not important.
11:34emilionox: I see, that&#39;s nice... We could use derive(ToComputedValue) to output a marker trait if `computed_type == specified_type`
11:34emilionox: and just have that as an impl detail instead
11:35noxTrue, no need for two separate traits, if we can specialise on such a trait bound.
12:05sewardjemilio: around?
12:37emilioseanmonstar: now I am, sorry
12:54emiliosewardj: now i am, sorry
12:54emilio(seanmonstar: Sorry, wrong ping)
12:55sewardjemilio: np. There are some perf regressions with the patch (I think). Can you look at the talos link I put in the bug?
12:56emiliosewardj: looking
12:58emiliosewardj: hmm... bloom_basic seems super-noisy
12:58emiliosewardj: not sure about tp6_fb
12:59emilionox: r? ^
12:59sewardjemilio: what bit are you interpreting to mean &quot;super noisy&quot; ?
12:59emiliosewardj: that test in general, I know it is, I&#39;m not inferring it just from your push :)
12:59sewardjemilio: is it the +/- 14.95 % ?
13:00sewardjoh, ok
13:05emiliosewardj: the patch looks good to me
13:05sewardjemilio: well, let me see if I can reproduce any difference locally
13:05sewardjemilio: thanks.
13:06sewardjemilio: but I am not sure we can land it, with these potential perf regressions
13:07emiliosewardj: yeah, maybe wait for bholley to land it? I&#39;d say let&#39;s land it now and if there are any regressions we can just stub try_push() with push(); return Ok(())
13:07emiliosewardj: not sure what&#39;s better, I&#39;m fine with both :)
13:08sewardjemilio: /me not sure what to do.
13:09sewardjemilio: I would like to at least see if there are any obvious differences when profiling
13:09sewardjbut that will take at least 1.5 hours to find
13:09emiliosewardj: ok, that sounds fine, I think
13:09sewardjemilio: I can work till about 6 today; how long are you around?
13:09emiliosewardj: not much
13:53noxemilio: LOL at 1 commit per removed CVAS.
13:53noxYou sneaky little stat inflater. ;)
13:53emilionox: not really, I switched to 1 per file after a while :P
13:53noxAh ah, it gets boring fast indeed.
13:53emilionox: also grid was unexpectedly bogus
13:54emilionox: see the grid commit... I didn&#39;t remove the current serialization code because there were some discussions about that, but...
13:54noxI will.
13:54emilionox: I love how derive forces you to do the right thing :)
13:55noxemilio: Given I have a meeting about media stack next week,
13:55noxI&#39;ll probably be kidnapped away from style,