• KiChjang
  • jmercouris
    KiChjang: thank you for the link, if I understand correctly, there are only Rust bindings for Servo yeah? Are there plans to make any others?
  • KiChjang
    i can't answer that question; paul would know best
  • mbrubeck
    jmercouris: There are also C bindings that implement the CEF API, but I think there's still a fair amount of work to make it usable in an actual product
  • KiChjang
    IIRC the CEF API hasn't been maintained for a while
  • jryans
    jmercouris: i believe this thread describes the current status groups.google.com/forum/#!topic/mozilla.dev.servo/20lkEsRI-ZI
  • jmercouris
    Thank you KiChjang jryans , it seems like servo is a still a bit immature for my application :\
  • mbrubeck
    jmercouris: Yes, we're still in the "brave early adopters" phase, if that
  • pcwalton
    mbrubeck: I think that just using “scope.spawn” for each kid might help
  • pcwalton
    and not using par_iter and friends
  • mbrubeck
    yeah
  • pcwalton
    the profiler says that the OS scheduler is under a lot of load
  • pcwalton
    we could implement a par_iter for flows that just uses scope.spawn under the hood
  • mbrubeck
    possibly with some (adaptive?) chunking with arrayvecs
  • pcwalton
    mbrubeck: yeah, we could probably just basically copy bholley’s code in style/parallel.rs
  • pcwalton
    or factor it out to be more generic, whatever
  • pcwalton
    it’s well tuned in its domain so it’s probably a good starting point
  • jmercouris
    mbrubeck: Just tried servo shell, and I got it to crash within a few minutes on OSX just be resizing from large to small
  • jmercouris
    s/be/by
  • jmercouris
    Looks like a really cool project you've all got, maybe sometime in the future I can try again :)
  • mbrubeck
    pcwalton: Well, the current `layout::parallel` is a rough port of bholley's `style::parallel`... This part of my experiment was to see if I could make it a bit *less* continuation-passing-style
  • mbrubeck
    with the goal of simplifying the traversal code, and possibly having more state in stack locals instead of in the flows themselves
  • pcwalton
    ah, ok
  • mbrubeck
    Fortunately the changes to float handling should somewhat orthogonal to this...
  • Mateon1
    I feel like there should be benchmarks for changes like #18793. True, we're potentially removing a memory access, but in most cases we are adding a comparison.
  • crowbot
    PR #18793: servo_arc: Try pointer equality test first when comparing two Arcs. - servo/servo #18793
  • mbrubeck
    Also it changes behavior for types like f32 that don't implement Eq
  • mbrubeck
    not that anyone actually cares that much about NaN
  • njn
    mbrubeck: how do I use a local copy of the `heapsize` crate in a local servo build?
  • njn
    I have some notes about the `[replace]` section in Cargo.toml, but I'm having trouble making it work
  • mbrubeck
    njn: [patch] is apparently the new hotness, though I haven't had a chance to try it yet: doc.crates.io/specifying-dependencies.html#overriding-dependencies
  • njn
    mbrubeck: oh, the top-level Cargo.toml actually has comments documenting this
  • njn
    mbrubeck: a bunch of other crates depend on heapsize as well. Will I need to make local copies of them and update the version they use too?
  • njn
    (I'm making backwards-incompatible changes)
  • mbrubeck
    njn: Yeah, sounds like it. :/
  • njn
    ugh, that sucks
  • njn
    this is part of the reason why I don't want this stuff on crates
  • mbrubeck
    If they are affected by the backward-incompatibility
  • mbrubeck
    Crates that don't need any source updates shouldn't need to be overridden.
  • njn
    ok, thanks
  • xidorn
    I hope there can be some kind of frozen hashmap which wraps around a hashmap, and allow you to query or change value of existing entry
  • xidorn
    and it provides the guarantee that key would always live as long as the frozen hashmap is alive
  • xidorn
    I think it is doable... but only inside the std
  • heycam
    Mateon1: mbrubeck: if you think it would be better to move the pointer comparison back out to all the callers I care about, I can do that
  • Mateon1
    I don't have much of an opinion on Servo code, but I feel this might be a premature optimization. Servo needs some benchmarking infrastructure.
  • Mateon1
    Now that I reported everything I found in a few rounds of fuzzing, I'll wait for more bugs to be fixed before I go on another bug hunt. I'm currently focusing on performance, and playing around with cargo bench and criterion.rs
  • heycam
    I do know that it helps with some large objects we were comparing through Arcs, but yes, I'm not sure how much of an impact it has on other users of Arc::eq
  • Manishearth
    emilio: btw, bugzilla.mozilla.org/1404545 might be a restyle/change hint bug
  • firebot
    Bug 1404545 — NEW, nobody⊙mozilla.org — stylo: support Fennec's "Honor system font sizes" setting
  • Manishearth
    not change hint
  • Manishearth
    restyle
  • Manishearth
    cc heycam
  • Manishearth
    I'm unassigning but would be nice if someone could look into it
  • xidorn
    emilio: hmmm, it seems your custom property change magically fixes my testcase :/
  • xidorn
    oh, nvm... it's just that I forgot how the test should be written
  • nox
    mfw switching a couple of things to USVString is actually too damn invasive
  • emilio
    heycam: r? ^
  • nox
    emilio: r? ^
  • emilio
    nox: looking
  • heycam
    oh, already r+ed
  • heycam
    (oh, misread the direction of r? :-) )
  • cpearce
    nox: turns out the mime crate can't handle mime types with more than one ';' character in them, for example "video/mp4; codecs=\"a,b\"; width=1024; Height=768; FrameRate=60; BITRATE=100000", which is of the format YouTube uses.
  • nox
    cpearce: Nice, that's not even being too strict, that's just being plain wrong.
  • nox
    cpearce: You sure that's the issue though?
  • nox
    cpearce: mime definitely can store multiple params.
  • cpearce
    nox: thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: FromStrError { inner: InvalidToken { pos: 23, byte: 59 } }
  • cpearce
    Pos 23 is the second ';' character
  • nox
    Weird.
  • emilio
    heycam: heh
  • nox
    seanmonstar: ^
  • nox
    cpearce: Did you want to port the mime stuff from the media stack to the mime crate in Rust?
  • cpearce
    nox: I need mime parsing. I was looking at calling the mime crate over FFI from gecko-media. I'll probably just have to import Gecko's mime parsing code I think.
  • nox
    cpearce: That's what I thought we would do, given we need to feed a mime type to the stack in a format it understands anyway.
  • nox
    emilio: That change I wanted to do kept on giving,
  • nox
    emilio: and in the end I didn't do the change. -_-
  • emilio
    nox: classic
  • heycam
    emilio: I couldn't work out how to use &s and *s to get rid of some function call. local_name.eq(name) works. not very satisfying though.
  • emilio
    heycam: hmm, I'm moderately sure it should be possible, try removing the `&` and just using `*name`?
  • heycam
    emilio: I tried all combinations of &s and *s on both sides :-)
  • emilio
    heycam: lol
  • nox
    What's local_name?
  • emilio
    heycam: unrelated though, do you know if we're supposed to use `#pragma once`?
  • nox
    And what's name?
  • emilio
    nox: this is Gecko's `WeakAtom` vs `&Atom`
  • heycam
    local_name is &WeakAtom, name is &Atom
  • emilio
    heycam: I think `*local_name == **name` should do
  • heycam
    wow ok
  • emilio
    heycam: kinda sucky though
  • nox
    *name == *local_name
  • heycam
    emilio: need two *s to first dereference the & and second to invoke the deref()?
  • emilio
    heycam: I think so, yeah
  • nox
    heycam: Need two * so that rustc tries to use the PartialEq<WeakAtom> impl for WeakAtom.
  • nox
    Otherwise it would try PartialEq<Atom> for WeakAtom, which doesn't exist.
  • heycam
    nox: I see, so there's no automatic deref call when the thing is used in an operator like that?
  • heycam
    (as opposed to a function call)
  • nox
    heycam: There is no automatic deref when the type is generic.
  • nox
    Like, a type parameter.
  • heycam
    oh, interesting, ok
  • heycam
    anyway, yes, two stars worked. thanks emilio/nox.
  • boris
    emilio: hi, I have some questions about this code and its function: searchfox.org/mozilla-central/rev/1…ervo/ports/geckolib/glue.rs#716-723
  • boris
    1) The element in this function is a generated element or original element?
  • boris
    2) and why we can make sure there is no animation rule in its pseudo element's rule tree?
  • nox
    2 star quality support
  • nox
    3) why isn’t this function unsafe? :’(
  • hiro
    boris: The answer for 1) is the parent element for the pseudo element.
  • boris
    hiro: ok, so we have to give StyleResolverForElement::new() the parent element
  • emilio
    boris: so, ok, what's the question?
  • emilio
    boris: hiro is right that for that to work, it needs to receive the parent element
  • emilio
    boris: and the reason why it doesn't have any animation already is because we process animations when restyling the pseudo-element itself
  • hiro
    emilio, boris: oh right. I think now I understand.
  • hiro
    clever way.
  • emilio
    hiro: boris: Anyway, if we could start passing the pseudo-element and remove that code, that'd be nice, I think
  • boris
    hiro emilio: Thanks. so where do we remove the animation rules for a pseudo eleement. It seems this function is "parent element only". Or actually we only have to GetBaseComputedValuesForElement for parent element?
  • hiro
    emilio: yeah, I think, once we drop gecko style system, we can do that.
  • boris
    emilio: ok ok. Isee
  • emilio
    boris: hiro: Both work, that is: if you pass the parent element with the proper pseudo-element type, or if you pass the pseudo-element with PseudoElementType::None, both should in theory give you the same style
  • emilio
    boris: hiro: That's why I think eventually removing that special-case would be nice
  • boris
    ok
  • hiro
    boris: anyway, the removing animation styles struff does not need for cumulative change hints.
  • hiro
    boris: We definitely need animation styles for cumulative change hints. e.g. font-size animation.
  • nox
    emilio: Ouch,
  • nox
    emilio: I broke some background tests,
  • nox
    emilio: but the test is intermittent,
  • nox
    so I noticed the failures by luck, just because I checked the results on Buildbot.
  • KiChjang
    why does github.com/servo/servo/blob/b1c7a2f…ts/net/resource_thread.rs#L347-L361 not take ownership of the request struct?
  • nox
    larsberg: Where?
  • nox
    KiChjang* UGH
  • KiChjang
    both http_redirect_fetch and fetch
  • KiChjang
    i don't see why they should take a mutable reference
  • nox
    Ain't that a remnant of that stuff going through Rc<T>?
  • KiChjang
    huh really now
  • nox
    What?
  • KiChjang
    in that case we can totally just refeactor it
  • nox
    Yes, feel free to do so.
  • nox
    KiChjang: Thought are you sure this is the only 'fetch' caller?
  • KiChjang
    IIRC when you reach the resource thread, that's exactly the place where it uses net::methods::fetch to do stuff
  • KiChjang
    i don't really know of any other place that calls it
  • nox
    Yeah that seems correct.
  • nox
    crowbot: tell jdm that it turns out <base> was pretty broken.
  • crowbot
    there's a phone right next to you, but okay
  • KiChjang
    nox: ugh, doesn't work as well as expected, http_redirect_fetch is called by http_fetch as well, so i'm screwed
  • nox
    KiChjang: Oh right, redirects of course.
  • eijebong
    Mhhh where is the servo-websocket repository ?
  • crowbot
    jdm: nox said that it turns out <base> was pretty broken.
  • SimonSapin
    eijebong: crates.io/crates/servo-websocket has a repository link
  • eijebong
    SimonSapin: Yeah, I know that but patch for `servo-websocket` in `github.com/rust-lang/crates.io-index` did not resolve to any crates
  • SimonSapin
    eijebong: although that link looks wrong… so it’s probably our fork github.com/servo/rust-websocket
  • eijebong
    :D
  • eijebong
    I don't know why I couldn't find it, sorry
  • eijebong
    If you want I can update the link in the Cargo.toml with the PR for bitflags
  • eijebong
    "Removing bitflags v0.7.03
  • eijebong
    Only 0.9 left (and sfackler doesn't want to update openssl to bitflags 1.0 yet as it's a breaking change)
  • nox
  • eijebong
    nox: Yeah, SimonSapin already told me
  • nox
  • eijebong
    /kick nox
  • jdm
  • jdm
    crowbot: tell eijebong github.com/servo/rust-websocket
  • crowbot
    *sigh*
  • eijebong
    /kick jdm "Don't try me, I'm crazy !"
  • » jdm tries to figure out how the arm builds sometimes choose a compiler that accepts -m64 and sometimes don't
  • eijebong
    jdm: That's an interesting one
  • eijebong
    Is it always on the same machine ?
  • jdm
    it is three different machines
  • eijebong
    Well there you go, there is one machine with a bad compiler :p
  • jdm
    er, no
  • jdm
    all three machines intermittently report this problem occurring
  • eijebong
    Oh
  • jdm
    SimonSapin: good news - I understand the arm failures. bad news - it's not intermittent, and it's an issue with using native dependencies in build scripts that hasn't been triggered on our build environments before.
  • » jdm -> office
  • pcwalton
    jrmuizel: there’s certainly code in LLVM that tries to do the comparison optimization
  • pcwalton
    SimplifyCFG.cpp has isImpliedCondition() -> ValueTracking.cpp isImpliedCondition() -> isImpliedCondICmps() -> isImpliedCondOperands()
  • pcwalton
    I wonder if this is a regression
  • jdm
  • larsberg
    jdm: omg so much better than my pile of hacks
  • » jdm bets it wouldn't be hard to add support to check all of the repos for an org
  • staticassert
    Hey, I'm curious about using the ipc-channel crate for sandboxing. It seems like you would create the queue, fork, and then talk between processes. The issue is that that means the second process has a copy of all of the memory of the parent proc, so secrets, memory addresses, etc would be shared. Is there a way to use a fork-exec approach with ipc-channel so
  • staticassert
    the child proc is a clean-slate?
  • staticassert
    When I say 'for sandboxing' I mean for communication between a broker and sandboxed process.
  • jdm
    pcwalton: ^
  • SimonSapin
    jdm, Manishearth, ajeffrey: is there GC hazard in using Box::new(x) instead of (box x) ?
  • jdm
    SimonSapin: no.
  • pcwalton
    staticassert: well, the right way to do this would be to exec with the FD still open and then recreate the IPC channel from the FD on the child process side
  • SimonSapin
    jdm: it’s defined as #[inline(always)] fn new(x: T) { box x } so it should compile to the same thing. I’m considring switching everything to check one more thing off servo/servo #5286
  • crowbot
    Issue #5286: Tracking: Unstable Rust feature gates used by Servo - servo/servo #5286
  • jdm
    SimonSapin: Box::new is a perf hazard for DOM code, however.
  • SimonSapin
    even with #[inline(always)] ?
  • pcwalton
    staticassert: I believe there’s an ipc-channel method for constructing an IPC channel from a raw FD
  • jdm
    SimonSapin: Box::new certainly showed up in profiles last year; no idea if something changed since then.
  • KiChjang
  • crowbot
    Issue #14897: Integrate websocket connection creation into fetch network stack - servo/servo #14897
  • jdm
    KiChjang: yeah, that sounds sensible
  • jdm
    KiChjang: thanks for looking into that!
  • KiChjang
    jdm: i aspire to be like nox, where i can leave commit messages like "Kill this data structure"
  • jryans
    emilio: why do StyleBuilder's for_inheritance / for_derived_style set rules to None?
  • emilio
    jryans: because for_inheritance is used for text and so on, though I think there should be a "this looks fishy" comment on for_derived_style
  • emilio
    jryans: anyway, what bug are you investigating?
  • emilio
    jryans: (I'm going to be semi-intermittently away btw, if I take a bit to reply)
  • jryans
    emilio: right, i saw the comment... :) reason i ask is that we clear out regular rules for those cases, but for visited, we clone the whole style, with rules still intact. i am thinking visited should also clear rules as well, does that seem logical?
  • ajeffrey
    SimonSapin: AFAIK Box::new is fine.
  • ajeffrey
    jdm: oh bah humbug perf issues? really?
  • jdm
    ajeffrey: yeah, it is created on the stack and then copied to the heap
  • SimonSapin
    in release mode it compiles to the same assembly: play.rust-lang.org/?gist=fd7d3cfde5…f53864d6e8cf51d32c6&version=nightly
  • ajeffrey
    jdm: oh pox
  • jdm
    interesting
  • emilio
    jryans: sorry for the lag... Why would you clear the rules for :visited?
  • jryans
    emilio: no worries... i don't see why we'd want different logic for unvisited vs. visited...
  • jryans
    emilio: anyway, i'll probably have you review, so you can think about it then :)
  • nox
    jdm: Can't wait to close all of them when they will be irrelevant in Fall 2018. ;)
  • jdm
    ha
  • Caspy7
    I don't write quotes in stone, but it helps when setting expectations as I work with users.
  • Caspy7
    Does anyone have a loose estimate on when Webrender might be expected to land in Firefox?
  • Caspy7
    Previously I'd seen 58-59, but based on some talk I've seen, I'm wondering if it might be later?
  • jdm
    Caspy7: I'm hearing may, which would be 60
  • Caspy7
    jdm: thanks
  • Caspy7
    I won't quote you :)
  • SimonSapin
    work in components/profile, recompile in 40 seconds
  • SimonSapin
    wait, wrong terminal
  • SimonSapin
    20 seconds
  • emilio
    jryans: yeah, I think for_derived_style isn't actually used to compute an actual ComputedValues instance
  • emilio
    jryans: anyway, happy to review, and will think about it :)
  • jryans
    emilio: yep, should be easier to think about with more context :)
  • nox
  • nox
    jdm: wtf? ^
  • jdm
    yeah, it's new
  • nox
    jdm: Pffft.
  • nox
  • jdm
    ha
  • nox
    jdm: So the label name is hardcoded or what?
  • jdm
    yes
  • nox
    …why
  • nox
    jdm: Should we rename ours?
  • jdm
    I dont' see a reason
  • nox
    jdm: To plug into that, nothing more.
  • jdm
    sure, but I don't know that we need another pipeline for new contributors at this point
  • SimonSapin
    what does github do with these?
  • nox
    jdm: Reminds me that GH doesn't know that Servo has a CoC.
  • nox
    jdm: We may get new ones if I do my job well btw!
  • jdm
    :plusone:
  • nox
    jdm: Will be at my univ on Friday, will mention Servo starters.
  • nox
    I've thought hard about showing them just a single Rust story with code,
  • nox
    and I think this will be about a #[derive]-introducing PR that removes hundreds of lines.
  • SimonSapin
    nox: r? ^
  • emilio
    jryans: reviewing now, are you around? Mozreview wen't down mid-review, but I remember the patch well enough, I guess
  • jryans
    emilio: yep, i am here
  • emilio
    jryans: just left a lengthy comment on the bug, can you go through it just to sanity-check?
  • emilio
    jryans: it's basically a suggestion which I think that makes the patch simpler (though relies on some stuff). It's kind of unexpected to synthesize the visited style there though, so wanted your opinion
  • jryans
    emilio: ah yeah, at first i went with the argument approach since i saw that only one for_inheritance used the styleadjuster, so this allowed visited to flow in the same way. but, i agree that adjust_for_text doesn't actually matter for visited properties.
  • jryans
    emilio: so, i'll try pulling it in to for_inheritance and ignoring the adjustment with visited like you suggest :)
  • emilio
    jryans: Yeah, I think given this stuff is easy to overlook (too easy to just pass `None` to that function :P), doing it in for_inheritance and do a simple inherited style from the parent makes sense.
  • emilio
    jryans: and be less repetitive too
  • jryans
    emilio: ok! thanks for the review :)
  • jryans
    emilio: (i assume separate test patch also looks good? not sure if you saw it before mozreview died...)
  • emilio
    jryans: no worries, nice catch! (I wasn't ccd on that bug, I could've helped out, maybe). I'm sorry I didn't got to the tests before mozreview went down :/
  • jryans
    emilio: it's fine, i learned more about reparenting and such with this one :)
  • emilio
    jryans: I can review them in the morning and land them if you want. Since they don't go through Servo code you can put the first patch on the servo queue meanwhile :P
  • emilio
    jryans: yeah, that stuff is nasty... :)
  • jryans
    emilio: here's the patch for tests jryans/gecko-dev 76a8cc4
  • emilio
    jryans: looks good. Maybe it'd be a good idea to try it with a nested visited link inside the first-line too, but feel free to not write that if you think it'd be too much churn.
  • jryans
    ok!
  • emilio
    jryans: oh, one question, is there something missing in `color-on-visited-text-1.html`? why does the `<span>` need an ID, did you intend to change its color?
  • jryans
    emilio: i just threw in the IDs while debugging, so it would be easier to identify them in some debug logs. can prune from the test, they aren't actually needed there.
  • emilio
    jryans: cool, r=me with that, will comment on the bug, thanks!
  • jryans
    great, thanks!
  • emilio
    jryans: no worries, thank _you_ for fixing that!