mozilla :: #servo

9 Aug 2017
00:03hirois STYLO_FORCE_ENABLED always true for mochitest-chrome on linux x64 stylo? regardless of the preference value?
00:03hiroI did check the preference value in a mochitest-chrome, it did not work. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e608a4f7bbd52448acd6a7c6fb697a10671e6d8&selectedJob=121828814
00:05jryanshiro: yes all stylo test platforms should set it. you can see it is on if you search the raw log
00:05jryanshiro: https://public-artifacts.taskcluster.net/KRFDqzYKQQi-vuahNUX_Vg/0/public/logs/live_backing.log
00:06jryanshiro: oh, but STYLO_FORCE_ENABLED does _not_ set the pref at all.
00:06jryanshiro: you should use this instead of checking the pref http://searchfox.org/mozilla-central/source/layout/style/test/animation_utils.js#415
00:06hirojryans: thanks, so, what does it mean?
00:06hirojryans: oh nice, I will check the variable.
00:06jryanshiro: the DOMWindowUtils thing is correct for both pref and env var methods
00:07hirojryans: OK, I can do it in my test case. Actually the test is in dom/animation/test.
00:07hirojryans: thanks!
00:07jryanssure :)
01:31jdmcrowbot: infrastructure report
01:31crowbotit has been 85 milliseconds since the last incident
01:35gwsounds about right
01:45* jdm tries to figure out his next step for improving error reporting performance
01:51njnbholley: what's the type name for the frame tree?
01:52njn(e.g. nsFrameTree, or something like that, presumably)
01:52bholleynjn: nsIFrame is the frame type
01:53njnbholley: so you traverse it with GetNextSibling()?
01:54njnand each nsIFrame has mStyleContext, which is the same as ComputedValues?
01:55njnhow do I access to the top/start of the frame tree?
01:55* bholley njn: this is probably useful: http://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#7254
01:55bholleynjn: so you do shell->FrameManager()->GetRootFrame();
01:56bholleynjn: there must be an iterator class somewhere...
01:57njnso each presContext has a PresShell, which has a FrameManager, which owns frames
01:57bholleynjn: right
01:57njnwhat owns the presContext? the window?
01:57bholleynjn: the prescontext/presshell are 1-to-1 with window/document
01:57njnok
01:57bholleynjn: also with the ServoStyleSet FWIW
01:57bholleynjn: I'm not super familiar with frame tree walking, you may want to ask a layout hacker for the precise best way to traverse the frame tree
01:58njnok, thanks, this is a good start
01:58* bholley njn: ah, this looks like what you want: http://searchfox.org/mozilla-central/rev/0f16d437cce97733c6678d29982a6bcad49f817b/layout/base/ServoRestyleManager.cpp#1427
01:58njnbholley: BTW, on the HashMap front, of the 8(!) possibilities I considered, the least worst seems to be "add to jemalloc a function that can take an internal pointer and return a start pointer"
01:59njnbholley: so I will try that, but I thought I'd look at the ComputedValues stuff first, since that might be easier
01:59bholleynjn: yeah sounds good - and getting the style struct breakdown will be useful for our current theory on the memory regression
01:59njnbholley: can you point me at the code that provides the nsStyleContext==ComputedValue equivalence?
01:59bholleynjn: well, ServoStyleContext==ServoComputedValues
02:00bholleynjn: (ServoStyleContext is a subclass)
02:00njnok
02:00njnbholley: BTW, where do all the Gecko* things in https://bugzilla.mozilla.org/show_bug.cgi?id=1367854#c18 live?
02:00firebotBug 1367854 NEW, cam@mcc.id.au stylo: Memory usage seems to be higher than Gecko
02:00bholleynjn: the equivalence, is well, handled by bindgen. in C++ it's ServoStyleContext, in Rust it's ComputedValues
02:01bholleynjn: stuff like GeckoPosition is a flyweight Rust wrapper around nsStylePosition
02:02bholleynjn: the wrapper structs are set up in http://searchfox.org/mozilla-central/source/servo/components/style/properties/gecko.mako.rs
02:02bholleynjn: but the nice thing is that you basically don't need to go through Rust
02:02bholleynjn: except for ElementData I gues
02:02bholleynjn: but you can measure the rest directly in C++
02:03bholleynjn: i.e. I think it would be easiest for the measurement function of ComputedValues/ServoStyleContext to be in C++
02:03njnoh, ok
02:03njnthat does make life easier
02:03bholleynjn: because you've just got pointers to nsStyleFoo
02:04bholleynjn: you'll still need to bounce into Rust to grab the things hanging off the Element (mServoData)
02:04bholleynjn: but you can just bounce right back over FFI
02:04bholleynjn: and for the frame tree traversal, you already have it
02:04bholleynjn: so the only rust you should need here is the part where you grab the primary + pseudo ComputedValues from the Element
02:04bholleynjn: (gotta head out for a bit)
02:38paulajeffrey: https://github.com/servo/servo/pull/17425#discussion_r132078568 - does it make sense?
02:38crowbotPR #17425: cleanup embedder/compositor/constellation/script messages - https://github.com/servo/servo/pull/17425
03:09jdmcrowbot: what does the web need?
03:09crowbotjdm: web developers are clamouring for the xslt working group (https://www.w3.org/Consortium/activities#XSLT_Working_Group)
03:09jdmI bet they are
03:13jdmoof, I just saw https://github.com/servo/servo/issues/6908#issuecomment-321128973
03:13crowbotIssue #6908: Measure the memory usage of HashMap better - https://github.com/servo/servo/issues/6908
03:13jdmthat sucks
03:34njnjdm: it does suck
03:34njnjdm: if we'd just asked the Rust folks to add HashMap::as_ptr() back then, we'd have it now
03:35njnI'm sure they would have said yes without any qualms
03:36njnjdm: unfortunately, unlike Vec, there's not really anything interesting you can do with a raw HashMap storage pointer other than measure its size, because you don't know how the keys/values/hashes are laid out
04:21cpetersonManishearth or shinglyu: what does it mean for a reftest to be marked as fails-if(!styloVsGecko)? Do we need to do anything to tests? https://searchfox.org/mozilla-central/search?q=!styloVsGecko
04:22Manishearthcpeterson: it means it's a layout bug
04:22Manisheartha know layout bug
04:22Manishearthcpeterson: because basically both stylo and gecko produce the same , incorrect, rendering
04:22xidorncpeterson: it means we can ignore it
04:22Manishearthyeah
04:23xidornI actually think we should treat all "fails" as "fails-if(!styloVsGecko)" directly inside harness
04:23xidornrather than marking them individually
04:39cpetersonI see. thanks
04:48njnbholley: ping?
04:48bholleynjn: hi (briefly)
04:49njnbholley: you want ComputedValues measured separatelly. is it ok for the other bits of ElementData/ElementStyles to be included with that?
04:49bholleynjn: well, ElementStyles is just various other ComputedValues
04:50bholleynjn: or I guess you mean the ElementData allocation and the EagerPseudoStyles allocation?
04:50njnyeah
04:50njn(I'm not measuring ElementStyles.pseudo currently, BTW)
04:51bholleynjn: (ok, we should do that)
04:51njn(ok)
04:51bholleynjn: so, conceptually, I think it would make sense for ElementData and EagerPseudoStyles to either get tacked onto a "element style data" category under the style system, or get tacked onto the size of Element
04:52bholleynjn: (is that what we do for DOMSlots?)
04:52njnbholley: a more specific formulation: Servo_Element_SizeOfExcludingThis() currently returns a single number. Sounds like you want 2 numbers, one for ComputedValues and one for everything else
04:52bholleynjn: well, I'm saying that we should measure the ComputedValues not under Servo_Element_SizeOfExcludingThis
04:52njnyep
04:52bholleynjn: so perhaps what we should do
04:53njnexisting code has numerous buckets: https://pastebin.mozilla.org/9029181
04:53bholleynjn: is have Servo_Element_SizeOfExcludingThis measure ElementData and EagerPseudoStyles, but not the CVs themselves
04:53bholleynjn: and then do the separate traversal we've been discussing to measure the CVs, the style structs, and the rule nodes
04:54njnok, thanks
04:55bholleynjn: (assuming Servo_Element_SizeOfExcludingThis runs when measuring the DOM element. We should probably call it Servo_Element_SizeOfElementDataExcludingComputedValues_
04:55njnyes!
04:55njnI can reorder things on the C++ side so that element measurment happens before frame tree measurement
04:55njnand put big comments all over it
04:56bholleynjn: well, to be clear, there are two element measurements we're talking about
04:56njn?
04:56bholleynjn: I was assuming we'd do two separate traversals of the elements
04:56bholleynjn: one of them for the stuff we already have
04:56bholleynjn: and then one of them for the CVs
04:56bholleynjn: or can we do them both together?
04:57njnshould be able to do them together
04:57bholleynjn: what mechanism do we use to iterate the DOM?
04:57bholleynjn: for the existing traversal?
04:58* njn is looking
05:00njnbholley: http://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#12437-12471
05:00bholleynjn: ok, so that's going to miss stuff
05:00bholleynjn: specifically, anything anonymous
05:01bholleynjn: I wonder why this hasn't already shown up on DMD
05:01bholleynjn: you want an AllChildrenIterator, really
05:01njnbholley: I don't know what you mean by "anonymous", but there's a bunch of other code so maybe it's covered elsewhere
05:02bholleynjn: so the current code won't find shadow dom nodes, or XBL nodes, or native-anonymous nodes
05:02bholleynjn: to be fair, it looks like the current code predates any easy mechanism to find those things
05:02bholleynjn: but we now have AllChildrenIterator
05:03bholleynjn: so separately, you might want to switch to that over GetNextNode
05:03njnok
05:04bholleynjn: so does the memory reporting machinery let you accumulate separate categories during the same traversal?
05:05njnyes
05:05njnyou just pass a struct containing multiple size fields
05:05njnsuch as nsWindowSizes
05:05bholleynjn: ok. So if we just thread our CV measurements (which need multiple sub-categories) down into the DOM traversal, that should be fine
05:06njnbholley: and then do the frame tree traversal after to catch any leftovers, right?
05:06bholleynjn: that's right
05:06njnwhat are the sub-categories? I saw your comment in bugzilla but didn't understand it
05:06njninherited/non-inherited/rule-nodes/other
05:07bholleynjn: We can bill ElementData and EagerPseudoStyles to Element. As for the rest of the categories
05:07bholley...
05:08bholleynjn: We probably want: (1) ComputedValues itself. (2) the inherited style structs, (3) the non-inherited style structs
05:08bholleynjn: I mean, if it were feasible, I would love to track each style struct separately
05:08bholleynjn: I don't know how much trouble that would cause you
05:08bholleynjn: it would just mean having 20 fields
05:08bholleynjn: but if that's a lot we can make do with putting them in two buckets
05:09njnwhat is "each style struct" exactly?
05:09njnwhat are the 20?
05:09bholleynjn: http://searchfox.org/mozilla-central/source/layout/style/generate-stylestructlist.py
05:09bholleynjn: each of those ("Font", "Color", etc) corresponds to a style struct type in nsStyleStruct.h ("nsStyleFont", "nsStyleColor" etc)
05:09njndo these structs hang off CVs?
05:10bholleynjn: yes
05:10njncurrently I'm only measuring CVs itself
05:10* bholley njn: like this: http://searchfox.org/mozilla-central/rev/0f16d437cce97733c6678d29982a6bcad49f817b/layout/style/ServoTypes.h#216
05:10bholleynjn: right, and the whole point of that memory bug is that we realized we're hurting in the style structs, not the CVs
05:10njnso those are all the GeckoPosition, GeckoBox, etc
05:11bholleynjn: right, but that's the Rust name, which shouldn't concern you, since you can do this in C++
05:11njnis this all stuff that Gekco does in the arenas?
05:11bholleynjn: exactly
05:11bholleynjn: ServoComputedData (linked to you above) uses the macros to instantiate all the pointers
05:12bholleynjn: 9 inherited, 15 non-inherited ("reset") looks like
05:12bholleynjn: so you can just use the same macros to traverse all the pointers
05:12bholleynjn: and if it's possible, we should just count each type separately
05:12bholleyso report 24 different values for the style structs
05:12njnok
05:13njnwhere does ServoComputedData sit?
05:13bholleynjn: inline in ServoStyleContext
05:13bholleynjn: it's just how manish did it
05:13bholleynjn: effectively think of all those things as in SErvoStyleContext
05:13bholleynjn: gotta run for a bit
05:13bholleynjn: (thinking about this more, I think we can just measure the rule tree entirely separately - we can talk about that later when the style struct stuff is done)
05:13njnok
05:13bholleynjn: (if you ask more questions, I can respond async when back)
05:13bholleynjn: and thank you so much for working on this
05:14njnyw
05:14njnsorry it's so slow
05:18ajeffreypaul: replied on GitHub.
05:20paulajeffrey: only one LoadComplete message is supposed to arrive
05:21ajeffreypaul: so what happens if the embedder opens two documents in quick succession?
05:22paulajeffrey: Load A, Load B, HeadParsed A (browsing context pipeline is now A), HeadParsed B (browsing context pipeline is now B), LoadComplete A (not sent to embedder because not browsing context pipeline), LoadComplete B (sent to embedder)
05:22njnone day I'm gonna get to write a decent chunk of Rust code, rather than continually dealing with integration and plumbing issues
05:22paulajeffrey: There are 2 LoadStart, and only one LoadComplete.
05:23ajeffreyWhat if the constellation finished loading A?
05:23Manishearthnrc: ServoCOmputedData is the "servo-backed" side of ServoStyleContext
05:23Manishearther, njn
05:23paulajeffrey: well, then we get LoadStart, LoadComplete, LoadStart, LoadComplete,
05:24ajeffreySo how does the embedder know which scenario they're in?
05:24paulajeffrey: It doesn't need to know. Just think about "when to spin the throbber". But even it needs to know, it can be inferred from the HistoryChanged message
05:25ajeffreyYes, but do I stop spinning whenever I get a LoadComplete?
05:25paulajeffrey: Yes.
05:25paulajeffrey: you are not supposed to ever get LoadComplete if the current pipeline is still loading
05:26ajeffreyI don't understand how the embedder and constellation are meant to agree about the current pipeline.
05:27paulajeffrey: the embedder doesn't have concept of current pipeline
05:27paulajeffrey: or even current document
05:27paulajeffrey: the only it has access to right now is "is it loading or not, what is the current url"
05:28paulajeffrey: and the current url is inferred from the HistoryChanged event
05:28paulajeffrey: which is fired when the pipeline is not pending anymore
05:29paulajeffrey: and by "is it loading", _it_ is the tab / top level browsing context.
05:29ajeffreypaul: I still don't understand,
05:29paul:)
05:30ajeffreyis it okay if the spinner stops temporarily even though the most recent document is still loading?
05:30paulno. it's not ok.
05:30paulbut
05:30paulbut nothing
05:30paulsorry i'm not clear
05:31paulajeffrey: I am going to describe better how that works in a github comment
05:32njnManishearth: so ServoComputedData has ~25 Arcs to various nsStyleFoo structs, right? Do all those nsStyleFoo structs only contain scalars?
05:32ajeffreySo if the embedder opens A then B, gets back the LoadComplete for A (due to message delay) it's okay to stop the spinner?
05:32njnhmm, no, I see nsIAtoms
05:34paulajeffrey: let me think, I think I understand what's bothering you
05:36ajeffreypaul: it would be nice to get on the same page about this.
05:36paulajeffrey: you're saying it's possible to be in a situation where we get LoadComplete for A while B is loading. And my assumption was that when Load B is called, it's impossible to get a LoadComplete for A.
05:37ajeffreyIt's a silly point in some ways, but it seems to be suggesting we've got different models!
05:37ajeffreypaul: yes, but I don't see how to arrange this,
05:37ajeffreydue to message delay.
05:38paulajeffrey: hmm
05:38paulajeffrey: I see
05:38ajeffreyUnless we're going to make Load synchronous.
05:40paulajeffrey: I need to think a bit more about this
05:40paulajeffrey: I'll add a comment to the issue
05:41ajeffreypaul: remember to include a link to this IRC chat!
05:42ajeffreyWe can come back to this later.
05:42* ajeffrey of to sleep
07:35heycamI have an iterator over Option<T> values. what&#39;s the most idiomatic way to return the first Some value, or None if there are none?
07:35* heycam can think of a few different ways...
07:37noxheycam: iter.filter_map(|v| v).next()?
07:38heycamnox: nice, thanks!
08:03emilioheycam: looking, yay!
08:04heycamemilio: just the refactoring bit so far, will do the real rebuild skipping bit next
08:04heycam:)
08:12heycamemilio: btw is there any reason why font_faces and counter_styles are on PerDocumentStyleData and not the Stylist? (is it just because only Gecko needs this?)
08:18emilioheycam: yeah, I think so
08:19emilioheycam: and there&#39;s no CounterStyle in servo afaict
08:19emilioheycam: but it&#39;s probably worth to move them
08:19heycamemilio: great, will do that
08:19emilioheycam: just reviewed #18017 btw, looks pretty straight-forward
08:19crowbotPR #18017: style: Move all origin-specific cascade data to PerOriginCascadeData - https://github.com/servo/servo/pull/18017
08:19heycamemilio: thanks!!
08:57heycamemilio: I get a bunch of errors saying nsCSSFontFaceRule must implement ThreadSafeRefCounted, when I move it to Stylist
08:57heycampresumably because we can access the Stylist during traversal
09:01emilioheycam: huh... what?
09:01emilioheycam: Oh, I see...
09:01heycamemilio: guess I&#39;ll leave them where they are..
09:03emilioheycam: and I guess we&#39;ll need to rebuild the lists each time any origin changes? That&#39;d be somewhat unfortunate I guess
09:03heycamemilio: I&#39;ll still separate them out into per-origin lists
09:04emilioheycam: I suppose we could add a few more types to make the compiler understand that the stylist isn&#39;t cloned or anything during the parallel traversal
09:04heycamemilio: I don&#39;t know how to do that :-)
09:05noxemilio: Ping?
09:05noxemilio: https://github.com/servo/servo/blame/master/components/style/properties/helpers/animated_properties.mako.rs#L1128:L1132
09:05noxThis is a recursive call, right?
09:06noxAnd in the recursive call, you end up with neither of this and other being definitely 0,
09:06noxand thus calc() gets reintroduced, no?
09:07noxAh no never mind.
09:08emilioheycam: I can try real quick, do you have a patch where you tried to do that around?
09:08emilioheycam: (I&#39;m not sure it&#39;s easily doable btw, but I want to give it a shot, seems cleaner)
09:09heycamemilio: I already git resetted it ;)
09:10heycamemilio: just copy the font_faces declaration from PerDocBlahData onto PerOriginCascadeData (or Stylist)
09:10emilioheycam: yeah, doing that now
09:13noxSuch a pain to have to run tests from Gecko...
09:16noxhttp://searchfox.org/mozilla-central/source/layout/style/test/test_transitions_per_property.html#2471 I don&#39;t understand this test.
09:20noxIt makes no sense to me, why doesn&#39;t the that assertion have the same list as in line 2470?
09:20noxWhat&#39;s the difference between lines 2467-2468 and 2470-2471?
09:21noxemilio: Any clue? ^
09:23emilionox: looking
09:26emilionox: so, I think that test relies on a negative transition-delay
09:27noxHow, where?
09:27emilionox: so when the style changes, it gets interpolated immediately
09:27noxemilio: But why is the first &#39;is&#39; call with the input to setProperty,
09:27noxbut not the second?
09:27emilionox: line 1078
09:28noxemilio: Sorry, that doesn&#39;t answer my question, does it?
09:29noxemilio: Why is the property immediately &quot;10px 40px, 50px 50px, 30px 20px&quot; after div.style.setProperty(prop, &quot;10px 40px, 50px 50px, 30px 20px&quot;, &quot;&quot;); on line 2468?
09:29emilionox: no, that&#39;s the answer to &quot;How, where?&quot;. Now I&#39;m trying to see how does that line work without setting transition-property to &quot;none&quot; before the first check :)
09:30emilionox: I think that&#39;s because of how list interpolation works
09:30emilionox: birtles can confirm, but if the lists have different lengths, then the interpolated value for the extra values contains the lengths without any change (there&#39;s nothing to interpolate them with).
09:31noxWrong, AFAIK.
09:31emilionox: when you change them to another list of the same length, suddenly it gets interpolated...
09:31noxemilio: https://www.w3.org/TR/css3-transitions/#animtype-repeatable-list
09:31crowbotnox: that&#39;s probably not the spec you want. Please read https://github.com/servo/servo/wiki/Relevant-spec-links
09:31emilionox: then I don&#39;t know what&#39;s going on I guess? How do you interpolate &quot;none&quot; with &quot;list&quot;?
09:31noxcrowbot: Ssh.
09:32noxemilio: What I don&#39;t understand is that background-position wasn&#39;t &quot;none&quot; before line 2467.
09:32emilionox: probably it relies on the initial value not being interpolable with the list?
09:32noxemilio: The initial value is 0% 0%.
09:32emilionox: huh
09:32nox(For background-position.)
09:33noxAnd background-position is interpolated as a repeatable list, so it doesn&#39;t matter AFAIK.
09:34noxWhat even is the value of that property before line 2467?
09:34noxHow does this part of the test doesn&#39;t get influenced by past changes to the property?
09:35emilionox: right, it does... That test looks like a bit of a mess...
09:35noxHow do I do a code block on Bugzilla again?
09:35emilionox: just paste it, maybe indented a few spaces?
09:36noxemilio: Oh you can&#39;t, only quotes, meh.
09:36noxemilio: Anyway, https://bugzilla.mozilla.org/show_bug.cgi?id=1387946#c2
09:36firebotBug 1387946 NEW, nobody@mozilla.org stylo: The calc() computed values of {backgournd|mask}-{position|size} are not correct in test_trans
09:37noxbirtles: ^
09:40emilioSimonSapin: ping?
09:40SimonSapinhey emilio
09:40emilioSimonSapin: hey :)
09:41emilioSimonSapin: do you remember why we don&#39;t mark Locked<T> as Sync unconditionally?
09:43emilioSimonSapin: in particular, if you apply a patch like the following and try to build Geckolib:
09:43emiliohttps://www.irccloud.com/pastebin/aVMRnwud/font-face.patch
09:43emilioSimonSapin: you&#39;ll find the compiler complaining because Arc<Locked<T>> is not Sync
09:44SimonSapinemilio: Locked allows multiple readers on different threads
09:44emilioSimonSapin: and I _think_ it should be... but probably you have a better intuition than I for why it&#39;s not (and it&#39;s probably rightfully not sync), so I just wanted to check with you
09:44SimonSapinemilio: so the question is, can FontFaceRule be Sync
09:45noxemilio: It definitely seems weird that transition-property doesn&#39;t get reset during that test, right?
09:46emilioSimonSapin: right, and I think it can be sync, but not send.
09:46SimonSapinwhy not?
09:47emilioSimonSapin: hmm... actually... I didn&#39;t realise the font face rule in gecko is: |type FontFaceRule = RefPtr<nsCSSFontFaceRule>|
09:47SimonSapinwith refcounting? atomic?
09:47emilioSimonSapin: nope, non-atomic
09:49emilioSimonSapin: so, it&#39;s probably not Send at all, but I think we should be able to make it Sync...
09:49birtlesnox: I&#39;ll have a look at that test tomorrow (I didn&#39;t write it and I have to look it up every time to work out what it&#39;s trying to do)
09:49emilioSimonSapin: well, I guess not
09:49emilioSimonSapin: sigh
09:53emilioSimonSapin: I guess there&#39;s no easy way to convince rustc that we&#39;re really not cloning that refptr...
09:53SimonSapinemilio: make a wrapper type with limited API, and implement Sync on that?
09:57emilioSimonSapin: yeah, the problem is that we&#39;d need to access to the full thing from the same place... so probably it&#39;s just better to keep storing them away from the stylist
09:57emilioSimonSapin: thanks though :)
10:00noxbirtles: Ok.
10:00noxbirtles: Bed time for you?
10:55localhorsedoes servo run on raspberry pi?
10:55travis-ciServo failed to build with Rust nightly: https://travis-ci.org/servo/servo-with-rust-nightly/builds/262612313 CC nox, SimonSapin
11:27larsberglocalhorse: we&#39;ve had it running on rpi and pandaboard in the past, but I don&#39;t think anybody is currently working on it. Typically, the biggest blocker is if it has an old or buggy OpenGL driver that requires special fixes or support.
11:29localhorselarsberg: can servo be used like CEF to show a given website in fullscreen? (to render slideshows of images/video)
11:30cynicaldevilnox: Should we discuss https://github.com/servo/html5ever/issues/298 more, or can I create a PR for it?
11:30crowbotIssue #298: Pass insertion point to TreeSink&#39;s `associate_with_form` to avoid `same_tree` check - https://github.com/servo/html5ever/issues/298
11:31larsberglocalhorse: I would expect some work to be required to get it working - the CEF bindings exist for Servo, but are not currently tested and have a couple of issues logged against them. So, not out of the box, but it should be doable for someone familiar with Rust.
11:44localhorselarsberg: i just installed servo on windows, i get some rendering errors. is that normal?
11:45localhorselarsberg: https://i.imgur.com/sZterXM.png
11:57larsberglocalhorse: If there isn&#39;t already an issue, please log it!
12:36Tomcat|sheriffdutyemilio: thanks for fixing the bustage :)
12:36emilioTomcat|sheriffduty: you should not thank me for fixing self-bustage :P
12:36Tomcat|sheriffduty:)
12:40Tomcat|sheriffdutyemilio: seems there is still https://treeherder.mozilla.org/logviewer.html#?job_id=121973454&repo=autoland
12:41emilioTomcat|sheriffduty: is it windows-only?
12:41Tomcat|sheriffdutyyeah
12:41emilioTomcat|sheriffduty: gah, patch incoming
12:42Tomcat|sheriffdutyok :)
12:57jdmneha: how are the tests going?
12:57nehajdm : Hi I am working on it
12:57nehaI just updated my patch https://github.com/servo/servo/compare/master...iamneha:parseSrcset
12:59nehabut jdm : so my second test case is failing
12:59nehahttps://github.com/servo/servo/compare/master...iamneha:parseSrcset#diff-ba5c3a7ed5fdaac5b3680162a11ebcec
13:02jdmneha: one thing that might help you is adding `--nocapture` to the test-unit command
13:03jdmneha: that should allow you to see the output of println! from the code
13:03noxemilio: Is there a ticket for making shape-outside animatable?
13:03nehajdm : yes I used that for println! and that worked perfectly
13:04nehajdm : as you can see my second test case which is for one_value
13:04jdmneha: do you know why the test is failing yet?
13:05nehajdm : you can see here https://www.irccloud.com/pastebin/HrJ5p5eX/
13:07jdmneha: I mean, have you figured out the reason why the test does not pass?
13:07nehajdm : not yet
13:10jdmneha: I recommend walking through lines 1082 to 1171 in your head (or on paper) and figuring out what you expect each value to be
13:10jdmneha: and add println! that confirm if that&#39;s true or not
13:11nehajdm : ok
13:11jdmneha: you can also add a println! after the loop on 1172 that verifies that descriptors contains the expected descriptor, for example
13:20emilionox: huh, I don&#39;t think so
13:21noxemilio: There is a ticket for supporting compute_distance on BasicShape,
13:21noxbut AFAIK BasicShape doesn&#39;t even implement Animatable,
13:21noxisn&#39;t directly used by any property,
13:21noxand instead clip-path and shape-outside use it,
13:21noxbut their animation_value_type is &quot;none&quot;,
13:21noxeven though they are animated &quot;as specified&quot; in the spec.
13:21emilionox: probably those should become animatable
13:21noxYep.
13:22noxemilio: But I just discovered ComputedGreaterThanOrEqualToOneNumber,
13:22noxso am currently breathing in a paper bag.
13:23emilionox: d&#39;oh
13:23emilionox: it&#39;s just an alias though, right?
13:23noxNo.
13:24noxemilio: It was introduced for intermediate animation types that need clamping when getting back the computed value.
13:24noxemilio: But not in a way I expected it, that&#39;s all.
13:24emilionox: i see
13:24noxFor example, I don&#39;t understand why their ToAnimatedValue::AnimatedValue member is Self.
13:25noxI expected that associated type to be their unclamped equivalent type.
13:26noxemilio: Ah no right.
13:26noxpub type NonNegativeLengthOrPercentage = NonNegative<LengthOrPercentage>;
13:27noxBut I would expect some sort of impl ToAnimatedValue for NonNegative<T> where type AnimatedValue = T.
13:31cynicaldevilnox, jdm: https://bugzilla.mozilla.org/show_bug.cgi?id=1091425#c1
13:31firebotBug 1091425 FIXED, william@fastmail.com Create elements in the document on the &quot;intended parent&quot;
13:32jdmaha
13:32jdmgood find
13:32cynicaldevilfinding a solution for &#39;has_parent_node&#39; is going to be complicated
13:41noxManishearth: https://github.com/nox/servo/blame/master/components/style/gecko/conversions.rs#L524 Couldn&#39;t you implement Into here?
13:46noxemilio: The Gecko conversion code uses http://searchfox.org/mozilla-central/source/layout/style/ServoBindings.cpp#2027,
13:46noxemilio: what should I do for the opposite direction?
13:47noxIs there a precedent with a different type somewhere?
13:48emilionox: what does the filter stuff do?
13:48noxNo clue, will check.
13:51noxemilio: Anyone working on using proper unions in bindgen btw?
13:51emilionox: there&#39;s a patch in progress, yeah
13:54noxWould be nice if Gecko people didn&#39;t use |this notation| in doc blocks, it makes no sense in the context of rustdoc.
13:57noxemilio: SpecifiedUrl::from_url_value_data(&(**filter.__bindgen_anon_1.mURL.as_ref())._base).unwrap()
13:57noxemilio: Thanks for mentioning filter.
13:57emilionox: ick, nasty
13:57noxemilio: I&#39;ll take nasty over needing more Gecko glue.
13:58emilionox: but yeah, I just saw it had a pretty similar SetURL function, so I thought that it&#39;d be similar in the other direction too
13:58noxemilio: But for the nastiness, I think most, if not all, clone_* methods should be unsafe,
13:58noxand probably other Gecko methods too.
13:58emilionox: why is that? I don&#39;t think they should
13:59noxemilio: On what type are those methods?
13:59emilionox: on the style struct wrapper
13:59noxemilio: I can trivially construct a self value which says it&#39;s a URL but is in fact a basic shape.
13:59emilionox: sure, but that needs to happen on a method on the wrapper too
13:59emilionox: so it&#39;s relatively encapsulated
13:59noxWhat do you mean?
13:59nox&quot;Relatively&quot;,
13:59noxrustc doesn&#39;t care about your relativeness.
14:00noxIf I can build a bogus value from safe code and segfault, it should be unsafe, period.
14:00emilionox: I don&#39;t think you can construct a bogus style struct _wrapper_ on safe code
14:00emilionox: which is the point
14:01noxemilio: AFAIK these methods are on GeckoDisplay.
14:01emilionox: sure, which aren&#39;t nsStyleDisplay
14:02emilio*which isn&#39;t
14:02nox?
14:02emilionox: is the |gecko| field public in GeckoDisplay?
14:02noxYes?
14:02emilionox: wat
14:02noxHow would it work otherwise?
14:02noxemilio: But good try my friend. :P
14:02emilionox: how would what work?
14:03emilionox: I think we should be able to create a GeckoFoo without making the |gecko| field public
14:03noxAll the methods are defined in a separate sibling module,
14:03noxif the thing wasn&#39;t public how would you access the field?
14:03emilionox: a getter? Or make a private PhantomData field to forbid construction?
14:03SimonSapinContextualParseError has an inherent to_string method based on format!()
14:04SimonSapiny u no fmt::Display
14:04noxemilio: You would need a mut getter too,
14:04noxemilio: which means you could mem::replace.
14:05noxI don&#39;t have a solution for now, just saying that we have issues.
14:06emilionox: huh, since when the GeckoFoo stuff is defined in C++
14:06emilio*?
14:06noxNo clue?
14:06emilionox: ah, of course, bug 1367904...
14:06firebothttps://bugzil.la/1367904 FIXED, manishearth@gmail.com stylo: fuse ServoComputedValues and nsStyleContext into a single allocation
14:06noxHah.
14:06noxThe bug that keeps on giving.
14:07emilionox: so _that_ was the difference. Before that the gecko field was private
14:07noxUGH
14:07noxGreat
14:15noxemilio: An example of root::nsTArray<root::nsStyleCoord>-like values somewhere?
14:16noxFound one.
14:17noxemilio: In the unsoundness department, I also like how nsTArray implements Deref but all of its fields are pub.
14:26emilionox: yeah...
14:33noximpl<T: Borrow<StyleBasicShape>> From<T> for BasicShape
14:33noxwat
14:33noxwhy is everything so complicated
14:36noxThat being said, at least I can reuse that in the code for ShapeSource, which is nice.
14:40noxemilio: Lol, that impl isn&#39;t actually currently used.
14:47emilionox: lol
15:07linclarkhow do I pass in the dump_rule_tree flag? I tried `./mach run -d -- --dump_rule_tree`, but it says it&#39;s an unrecognized option
15:08noxlinclark: -Z or something like that.
15:09noxI doubt it&#39;s a -- long flag at least.
15:11linclarknox: ah, ok, I had to use that and switch underscores to hyphens... thanks!
15:13noxlinclark: You are welcome.
15:21* bz_dinner mutters about stylo assuming things about the structure of styles that are just false and the resulting contortions
15:38bzemilio: ping
15:48emiliobz: pong
15:51bzemilio: got a sec to talk about anon boxes?
15:51emiliobz: sure
15:52bzemilio: So I have stuff implemented, but still fighting with the various asserts
15:52bzemilio: mostly to do with the fact that for anonymous boxes gecko&#39;s styling is ... suspect
15:53emiliobz: huh, interesting, like which ones?
15:53bzemilio: So for example. Say you have text inside a scrollable flex container
15:53bzemilio: we create a block wrapper around it
15:54bzemilio: So in the frame tree we have the scrollframe, it has a scrolled-content flex container, and inside that is a block and inside that is text
15:55emiliobz: makes sense, the block is a :-moz-anon-flex-item, right?
15:55bzRight
15:55bzSo what is the parent style of the block?
15:56emiliobz: I&#39;d say it makes sense for it to be the scrolled-content container, but I wait impatiently for your reply showing sadness :)
15:56emiliobz: that is, the flex container
15:56* bz is poking at code for a sec
15:57bzSo during a restyle that is what it would be
15:57bzbecause for an anon box we use its parent frame style context as the parent
15:58bzhmm
15:58bzI guess during initial construction it is too
15:58bzbecause we base it on the parent frame, which in this case is the flex container
15:58bzho-hum
15:59bzAlright, thank you!
15:59emiliobz: I didn&#39;t do much tbh :)
15:59bzWell, you made me organize my thoughts
15:59emiliobz: just curious, which approach did you end up using to implement the anon-box restyling?
16:00bzhttps://hg.mozilla.org/try/rev/4eadd39c6387dede58a404c4c364b435bef8bd22#l2.100
16:00bzThat comment should explain it
16:23emiliobz: nice, thanks. Slightly tricky, will need to take a deeper look I guess
16:29bzemilio: fwiw, I was going to ask heycam for review, but that was last night when I figured you were asleep and he would be awake
16:30bzemilio: If you want to review once it&#39;s up, feel free to!
16:30* bz fixing asserts, about to push to try again
16:31emiliobz: heh... Probably heycam is also a good reviewer for that patch, so I don&#39;t really mind :)
16:32emiliobz: Just a nitpick in case I don&#39;t review it, I&#39;d leave the aAssertWrapperRestyleLength argument in release builds, but just leave it unused, just as aOwner. Looks slightly ugly having to have #ifdefs around in constructor calls :P
16:33bzemilio: ok, I can do that
16:35bzemilio: btw, with this and https://bugzilla.mozilla.org/show_bug.cgi?id=1388626 that&#39;s all the anon boxes
16:35firebotBug 1388626 NEW, bzbarsky@mit.edu stylo: implement dynamic restyling for ::-moz-xul-anonymous-block anon boxes
16:39SimonSapinemilio: r? https://github.com/servo/rust-cssparser/pull/177
16:39crowbotPR #177: Count line-numbers during tokenization - https://github.com/servo/rust-cssparser/pull/177
16:41jtagrawDoes anyone know right off if bugs in servo would be covered under the bug bounty program?
16:41jtagrawNot a major question, just thinking of doing some digging
16:42Manishearthjtagraw: no
16:42bzI would guess no
16:42Manishearthjtagraw: stylo bugs, perhaps
16:42bzhttps://www.mozilla.org/en-US/security/client-bug-bounty/ explicitly says &quot;Eligible security bugs may be present in any of the current main development or released versions of Firefox or Firefox for Android as released by Mozilla Corporation&quot;
16:43Manishearthjtagraw: servo has zero users, and we haven&#39;t done that much auditing ourselves
16:43bzAnd yes, stylo bugs that affect Firefox would be eligibl
16:43bzer, eligible
16:43bzAnd in fact, poking around stylo would be very much appreciated!
16:43* bz hope