mozilla :: #servo

20 Mar 2017
00:20emilioManishearth: you mean the GetPossiblyStale thing?
00:20emilioManishearth: or GetDocumentState const?
00:22Manishearthemilio: yeah
00:23Manishearthhaving const and nonconst
00:23ManishearthI renamed it to ThreadSafeGetDocumentState
00:23Manishearththe stale thing no longer applies
00:34paulgw: question: after enabling the profiler, it is not drawn right away, I need to move the mouse over the page to trigger the drawing. What do I need to do to make it appear right away?
00:34gwpaul: calling generate_frame() should be enough
00:35gwpaul: The reason is that the main compositor thread blocks on UI events so that it's not spinning the cpu / wasting power when there's nothing to do. Calling generate_frame() is a simple way to say - just go and draw the frame from the current display list.
00:36paulgw: thanks - let me try
00:41paulgw: ok - it works. I've updated https://github.com/servo/servo/pull/16030 (need another review)
00:41crowbotPR #16030: Expose a method to toggle wr profiler - https://github.com/servo/servo/pull/16030
00:42gwpaul: r+
00:46jntrnrwoot
00:46jntrnrwindows nightly is close
00:46jntrnrthe build on my machine atm is probably good enough to throw the switch
01:54xidornso... stylo's bindgen crashes with clang 4.0 on windows :/
01:55xidorndmajor: I guess it is not just you. I see the same issue that "thread 'main' panicked at 'C'mon: Continue'" with clang 4.0
01:55emilioxidorn: only windows, right? I use clang trunk and it works for me
01:55emiliosigh
01:56xidornemilio: at least on windows
01:56xidornemilio: I upgraded the llvm install, and it doesn't work anymore.
01:56xidornI'll try to analyze after lunch
01:57dmajorxidorn: let me know if you can find any temporary workarounds that I could apply locally
01:59emiliodmajor: I believe we have a way of disabling build-time bindgen
02:00emiliodmajor: http://searchfox.org/mozilla-central/rev/557f236c19730116d3bf53c0deef36362cafafcd/toolkit/moz.configure#580
02:02dmajoremilio: what happens to my build if I disable the bindgen?
02:03emiliodmajor: you use the checked-in ones (servo/components/style/gecko_bindings/structs_{debug,release}.rs)
02:04emiliodmajor: those are in theory only for Servo CI, but had this option just in case :)
02:10emilioxidorn: I had ^ locally and forgot to upstream, otherwise the log is useless.
02:52xidornemilio: it seems you need "use std::env;"
02:53xidornemilio: and if I were you, I would probably create an array with three lambdas so I don't need to list them twice
02:57xidornprobably more loc, though
03:46ManishearthSimonSapin: there?
03:46Manishearthor emilio
03:49heycamManishearth: ping
03:50Manishearthheycam: pong
03:50heycamManishearth: hey, with the current state of mapped attribute support, should we be correctly restyling when the mapped attribute is modified?
03:51Manishearthheycam: we might not trigger restlyes properly
03:51ManishearthI'm not exactly sure how that works
03:51Manishearthi think there's a bug on file for it
03:52heycamManishearth: ok. I'll trace through a case I'm hitting and see what's happening.
03:53Manishearthheycam: what should we be doing to trigger restyles?
03:53Manishearthbecause I don't think we do anything special here
03:54heycamManishearth: well I'm not sure if we do need to add anything special -- there is existing code that generates restyle hints depending on the attribute that is changed
03:54Manishearthkay
03:54Manishearthyeah I didn't touch that
03:54Manishearthit seemed to work
03:54heycamManishearth: yeah. I'll check why it's not working in this case.
04:06Manishearthheycam: btw, got a moment to review https://github.com/servo/servo/pull/16016 ?
04:06crowbotPR #16016: Add separate specified value for keyword font sizes - https://github.com/servo/servo/pull/16016
04:11heycamManishearth: not immediately, but I can look at it later today if xidorn doesn't
04:12xidornManishearth: I'm going to have a look today
04:15Manishearththanks
04:15xidornManishearth: for the comment you left in bug 1341775, I wonder why we have early properties and late properties? there have been various dependencies between properties
04:15firebothttps://bugzil.la/1341775 NEW, manishearth@gmail.com stylo: need to do the right thing with "font-family: monospace" in terms of font size
04:15xidorne.g. float, position, and display
04:17Manishearthxidorn: float/position/display depend on *each other*?
04:18xidornIIRC position is a dependency of float, float and position are dependency of display
04:18xidornvery similar to the situation here, no?
04:18Manishearthxidorn: well, that's a bug then :)
04:19xidornyou mean we do it wrong at the moment?
04:19Manishearthxidorn: we compute float and position before display, but float and position can happen in any order
04:20xidornthere are several multiple dependencies, IIRC
04:20Manishearththe background ones have that too but we sidestepped that
04:20xidornbackground ones?
04:21xidornyou mean they need to share the same array?
04:21Manishearthno, there's the whole deal with filling the arrays cyclically
04:21Manishearthwe defer that to the end
04:22xidornManishearth: does servo eventually compute all properties in some order from compile time? or the order may change in runtime
04:23xidorn?
04:23Manishearthruntime dependent
04:23xidornit's parallel?
04:23Manishearthnot just because of that, our model is different
04:24Manishearthgecko stores specified values in away that they can be retrieved in O(1)
04:24Manishearthwe store them in a vector
04:24ManishearthO(n)
04:26xidornand gecko generates the style data lazily, but servo does so eagerly
04:26xidornbut how do you generate the computed style?
04:28Manishearthxidorn: hm?
04:28Manishearthxidorn: apply_declarations?
04:29Manishearthit gets an iterator of decl blocks in precedence order
04:29Manishearthand it just applies them
04:30xidornManishearth: oh...
04:30Manishearthxidorn: file a bug for the precedence thing for float
04:30xidornso for resolving multiple level dependency, you need to have them resolved level by level
04:31xidornohhh
04:31xidornhmmm
04:31Manishearthcurrently, in servo, float depends on position
04:31Manishearthso we have the dependence, but we don't apply it in the right oder
04:31Manishearthor, enforce order
04:31Manishearthwonderbar
04:32xidornI guess for early properties, you need to pull them out and have some order to resolve them
04:32xidornrather than having them get resolved together...
04:32Manishearthyes
04:32Manishearththat's what I'm doing :)
04:32xidornok, great :)
04:32Manishearth(I have patches already for font-size, I just haven't pushed because I need to write the base-to-thing conversion)
04:33ManishearthI'll probably piggyback the float/display thing on top of it
04:33Manishearther, float/position
04:40Manishearthxidorn: is GetDefaultFont expensive?
04:40xidornManishearth: which GetDefaultFont?
04:40Manishearthon prescontext
04:41xidorndoesn't seem to be expensive
04:41Manishearthcool
04:42xidornManishearth: what are you going to do?
04:43Manishearthxidorn: every time a keyword font-size is computed I have to hit GetDefaultFont
04:43Manishearthactually no this is fine
04:44xidornI guess this is fine
04:44xidornit seems to me gecko calls into that every time computing font data as well
04:45xidornit isn't that cheap... but...
04:45Manishearthyeah
04:46xidornbut it doesn't seem to be thread-safe, is that ok?
04:47Manishearthxidorn: oh, no
04:47Manishearthwhy isn't it threadsafe?
04:47xidornis preference access thread-safe?
04:48Manishearthxidorn: yes, we already do that off main thread
04:48Manishearthi think
04:48Manishearthno, we do it when parsing. hm
04:48xidornthe lang service call doesn't seem to be thread-safe anyway...
04:48Manishearthwhy not?
04:48xidornhttps://dxr.mozilla.org/mozilla-central/source/intl/locale/nsLanguageAtomService.cpp#65
04:49xidornthere are some hash table operation, and I suppose gecko's hash table isn't thread-safe by default?
04:51xidornthere is also mutation to nsPresContest's mLangGroupFontPrefs
04:51Manishearthxidorn: but aren't presentations global already?
04:51Manishearther, preferences
04:52xidornManishearth: some linked-list operation: https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/layout/base/StaticPresData.cpp#89-107
04:52Manishearthmultiple content processes will access them anyway
04:52xidornnsPresContext is not, it is some per-document thing I think
04:52Manishearthright, but the prefs are global
04:52xidornyep, but prefs is not the only problem
04:53xidornthere are unsafe linked-list operation, hashtable operations other than prefs from the call in GetDefaultFont
04:54Manishearthhm
04:55xidornManishearth: I think there was some discussion around getting this kind of things in main thread and have workers block on that?
04:56xidornManishearth: could we somehow cache that information somewhere? but that's language dependent, so we probably cannot compute it in advance
04:58Manishearthxidorn: yes
04:58Manishearthwe can't defer it
04:58Manishearthwell we could send to the main thread idk
05:01xidornManishearth: I think it doesn't really need to be in the main thread, it is just not concurrent-safe
05:01xidornso holding a global mutex should be enough
05:02xidorn... unless there is any assertion somewhere
05:11Manishearthlet's see, almost got this patch working
05:33Manishearthstandups: font-size base-size stuff (bug 1341775)
05:33standupsOk, submitted #43900 for https://www.standu.ps/user/Manishearth/
05:33firebothttps://bugzil.la/1341775 NEW, manishearth@gmail.com stylo: need to do the right thing with "font-family: monospace" in terms of font size
05:35xidornManishearth: I probably cannot get to your patches on bugzilla today. maybe tomorrow...
05:36Manishearthxidorn: no probs
05:36ManishearthI'll continue pushing fixes and stuff
05:36Manishearthand might drive-by fix the position thing
05:37Manishearthxidorn: "Could we have a function to share the code here and above? "
05:37Manishearthwhat do you mean by "above" there?
05:37Manishearththe number parsing that font uses?
05:37Manishearththat's different, HTML lets you do things like +5
05:38xidornManishearth: I mean parse_plain_attribute and SetSize
05:40xidornactually I guess you can put the extra logic into parse_size and make that function return AttrValue directly
05:40xidornmaybe that's not a good idea...
05:40Manishearthyeah
05:41xidornbut a helper function should reduce code redundant there
05:41Manishearthxidorn: I'm still not sure what two bits of code you consider redundant here
05:41Manishearthoh wait
05:41ManishearthI see
05:41xidornok
06:06waffleshuh Manishearth, strange, I'm unable to reproduce the error o.O
06:09Manishearthwaffles: right, of course you can't, because it works on the server too
06:10SimonSapinhey Manishearth
06:12ManishearthSimonSapin: priblem solved, see https://bugzilla.mozilla.org/show_bug.cgi?id=1341775#c3 and second patch on that bug
06:12firebotBug 1341775 NEW, manishearth@gmail.com stylo: need to do the right thing with "font-family: monospace" in terms of font size
06:14SimonSapinManishearth: I assume that "font-size: medium" now computes to something that can vary?
06:14Manishearthyes
06:14ManishearthSimonSapin:
06:14Manishearther
06:14Manishearthyes
06:14ManishearthI'm not sure what the implications are for the initial value
06:14SimonSapinthe initial value is "medium"
06:15SimonSapin"medium" used to be equivalent to 16px always, which was valid per spec but not what Firefox does
06:16SimonSapin(though perhaps it was replaced at parse time which might not be web compatible for serialization)
06:16ManishearthSimonSapin: yep
06:16ManishearthSimonSapin: but the initial value is a compiuted value concept, no?
06:17SimonSapinunclear
06:17SimonSapin(in specs)
06:17ManishearthSimonSapin: i mean, in servo
06:18Manishearthlike, my patches keep the initial computed value as FONT_MEDIUM_PX
06:18Manishearththat's not going to help much i guess
06:18Manishearthwe need to make the gecko setters handle this
06:19Manishearthand make the computed value Option<Au> or something
06:19SimonSapinwe use computed value types in servo to represent initial values because in most cases because theyre often used for defaulting in the cascade and we dont want to do a conversion every time there
06:19Manishearthyeah
06:19Manishearthcan we override that here?
06:20Manishearthwell, we don&#39;t want to call the font base size thing each time either. hmmm
06:20SimonSapinbut theres a couple cases where the computed value can be different from the initial value even without that property being specified
06:21SimonSapinfor example border-top-widths initial is medium, which means 3px, but computed to zero when border-top-style is none (which *is* the initial value there)
06:21Manishearth?
06:21Manishearthah
06:21SimonSapinwe deal with that at the end of cascade()
06:22SimonSapinfont-size sounds more tricky, though
06:22SimonSapinyou want to differentiate &quot;medium&quot; and &quot;a px value that happens to be what medium usually means&quot;
06:23Manishearthyes
06:23Manishearthalso that makes em units hard
06:28ManishearthSimonSapin: I guess ... I guess we want to special case it where `initial` (or not specifying a value) will always trigger a computation?
06:29SimonSapinIm not sure and Im not working this week :)
06:29SimonSapinpretend Im not here :)
06:29xidornSimonSapin: it doesn&#39;t seem to me the spec indicates medium should be 16px anywhere
06:29SimonSapinxidorn: it doesnt
06:30SimonSapinbut it doesnt say it should depend on font-face either
06:30xidornit doesn&#39;t say it must not
06:31SimonSapinanythings possible!
06:31xidornbehavior of those keywords are basically impl-dependent
06:34xidornand actually webkit and blink also have font-size depend on font-family
06:35SimonSapinright, and I chose to skip all of that for servo as a separate impl
06:35SimonSapinbut stylo is a different story
06:45ManishearthSimonSapin: xidorn this is what I came up with
06:45Manishearthhttps://manishearth.pastebin.mozilla.org/8982601
06:45Manishearthit doesn&#39;t handle the initial keyword, but that can be fixed similarly
06:45Manishearthwithout that patch a monospace without explicit font-size:medium will be the wrong size
06:45Manishearthyep, it works
06:50wafflesManishearth, could you check whether appending to sys.path works?
06:51Manishearthwaffles: prepending, yes
06:51Manishearthwaffles:
06:51Manishearthhttps://manishearth.pastebin.mozilla.org/8982602
06:51waffleswonderful
06:51Manisheartheh I&#39;ll just submit that
06:53wafflesManishearth, I already did
06:53Manishearthlol
06:53Manishearthwaffles: why the other changes?
06:54wafflesManishearth, wanted to do them a long time back
06:54Manishearthwaffles: you removed the stylo stuff
06:54wafflesManishearth, no, just tinkered a bit
06:54Manishearthoh
06:54wafflesit works the same
06:55Manishearthwaffles: it won&#39;t work
06:55wafflesManishearth, why not?
06:55Manishearthwaffles: the sys.path.remove is too early
06:55Manishearthyou&#39;re skipping the lint check
06:55Manishearthit will run, but will skip a check
06:55wafflesManishearth, the lint check is still there?
06:56xidornManishearth: I have no idea where does that patch mean
06:56xidornManishearth: that seems to be in inter-diff...
06:56xidorns/in/an
06:56Manishearthxidorn: it&#39;s based on my earlier patch that pulls out font_size and family early
06:56xidornk...
06:57Manishearthbasically, if it can&#39;t find a font_size being explicitly set, instead of letting it get inherited by copy, it will treat it as it was explicitly set to medium
06:57Manishearthpushing part 4 now
06:57Manishearthit works too
06:57Manisheartholder try push had lots of failures, let&#39;s try again
06:59wafflesManishearth, just to clarify, 1) once the module has been imported, the paths don&#39;t matter
06:59waffles2) we don&#39;t need stylo arg in filter_files
06:59Manishearthwaffles: please make sure the wpt lint gets run, then
06:59wafflesManishearth, why do you need the wpt lint for stylo?
06:59Manishearthstylo arg I get
07:00Manishearthwaffles: we don&#39;t>
07:00wafflesin general, the lint will run
07:00Manishearthwaffles: can you test that that is the case with your patch?
07:00wafflesbut since wpt lint returns on stylo=True, it doesn&#39;t
07:00wafflesI did :)
07:00Manishearthokay
07:00Manishearthr+ then
07:01wafflesthanks :)
09:59SimonSapinemilio: I used to pass &quot;-- -x c++ -std=gnu++11&quot; to the bindgen executable. Is there an equivalent with the bindgen library?
10:00SimonSapin(this is for a microcontroller, not servo)
10:00SimonSapinah, clang_arg sounds promising
10:05emilioSimonSapin: yeah, that&#39;s it
10:08SimonSapinemilio: Ive had to add `.clang_arg(&quot;-target&quot;).clang_arg(env::var(&quot;TARGET&quot;).unwrap())`, should that be the default?
10:09emilioSimonSapin: perhaps! Our handling for targets is not great right now, we have some platform-specific code that assumes host_pointer_size == target_pointer_size. I plan to make a target required and pas the clang flags magically, but I need to think through it and do it.
10:14SimonSapinemilio: I have generated stuff using ::std::os::raw::c_char despite me using .use_core(). I suppose this is because std::os::raw is not in core?
10:16emilioSimonSapin: You need to use libc, right? there&#39;s an option to use the right prefix (I believe it&#39;s called ctypes_prefix)
10:22travis-ciServo failed to build with Rust nightly: https://travis-ci.org/servo/servo-with-rust-nightly/builds/212920650 CC nox, SimonSapin
10:23xidornemilio: you mean backporting to 0.22.0?
10:23emilioxidorn: or 0.22.1
10:24emilioxidorn: to any version that doesn&#39;t have the template parameter refactor
10:25SimonSapinnox: rustup failure looks like a regression in name resolution, wanna look into it? ^
10:25xidornemilio: hmmm, let me try
10:27emilioSimonSapin: seems like they just renamed TryFrom::Err to TryFrom::Error, right?
10:27SimonSapinemilio: oh, thats a more likely explanation
10:27SimonSapinand less worrying :)
10:27emilioSimonSapin: https://github.com/rust-lang/rust/commit/2561dcddf9e61f5c52a65f1a42641e01bfabe3e2
10:42SimonSapinemilio: I have some &quot;inline static&quot; C++ methods missing from generated bindings, they used to be included
10:42SimonSapin(Id use -fkeep-inline-functions when compiling so that they could be linked)
10:44emilioSimonSapin: hmm... perhaps libclang stopped saying it&#39;s not inline when that happens? Could you open an issue with a test case?
10:47SimonSapinemilio: -fkeep-inline-functions for compiling, not for bindgen
10:48emilioSimonSapin: oh, try to pass it to `clang_args`
10:48SimonSapinwarning: optimization flag &#39;-fkeep-inline-functions&#39; is not supported [-Wignored-optimization-argument], err: false
10:49SimonSapinemilio: it doesnt help, and I get this warning
10:49xidornemilio: nope, doesn&#39;t work
10:50xidornemilio: since there is no template.rs, I tried to put the builtin skip in Type::from_clang_ty, but then there is an assertion in var.rs triggered
10:51emilioxidorn: which assertion?
10:51xidornCouldn&#39;t resolve constant type, and it wasn&#39;t an nondeductible auto type!&#39;, C:\mozilla-source\rust-bindgen\src\ir\var.rs:208
10:52xidornanyway, if you are not going to backout the refactor... I guess we should find solution on top of that
10:53xidornemilio: is that reproducible on linux as well or just windows?
10:56* emilio checks
10:56xidornemilio: the test case I added in that pr does crash on mac
10:57xidornemilio: but I failed to produce reproducible testcase for crashes after that
10:58emilioxidorn: yup, it crashes
10:58xidornemilio: I mean, does it crash linux&#39;s stylo build? I guess no?
10:58xidornI think you said you have llvm trunk with stylo, but doesn&#39;t see this issue
10:58emilioxidorn: nope, I guess it&#39;s some MSVC-specific header or something.
10:59xidornit is the std::__make_integer_seq
10:59xidornbuiltin template
10:59emilioxidorn: yeah, I definitely don&#39;t hit that in stylo
10:59emilioxidorn: *in stylo builds
11:00xidornemilio: if you can tell me how to read the bindgen log, I can probably locate where the crashes are from...
11:01emilioxidorn: Well, the bindgen log is like a trace of where bindgen is, so probably it&#39;s not super-hard to track down.
11:01xidornemilio: but, iirc, crashes after that are generally from codegen pass rather than parsing
11:01emilioxidorn: note that bindgen master does crash for me in an stylo build
11:01xidornemilio: oh...
11:02emilioxidorn: oh, really? Then we may be able to make it work
11:02xidornemilio: so I guess we would need master to work first, then figure out what&#39;s going on with windows build :/
11:03emilioxidorn: yeah, I tried to do that. IIRC the master issue I was fighting also had to do with builtin template parameters, let me try to find the IRC log
11:03emilioxidorn: so your patch may actually be the right fix (tm)
11:04SimonSapinemilio: in src/ir/function.rs, bindgen has if cursor.is_inlined_function() { return Err(ParseError::Continue); }
11:04SimonSapinso it looks liken inlines are always skipped?
11:04emilioSimonSapin: right, but clang _used_ to return the right thing for is_inlined_function if the function wasn&#39;t inlined
11:04emilioSimonSapin: I believe we have a test with -fno-inline-functions
11:05emilioSimonSapin: oh, though you&#39;re using -fkeep-inline-functions, which I guess still does inline the function
11:05emilioSimonSapin: We can add the equivalent option for bindgen I guess
11:05wafflescrowbot, is -ln-ht- intermittetn
11:05wafflescrowbot, is -ln-ht- intermittent
11:05crowbotNo intermittent issues filed with &#39;-ln-ht-&#39; in the title
11:05SimonSapin-fno-inline-functions doesnt help either
11:05emilioxidorn: http://logs.glob.uno/?c=mozilla%23servo&s=18+Mar+2017&e=18+Mar+2017&h=bindgen#c634122
11:05xidornemilio: so after my patch, the next crash is in BindgenContext::resolve_typerefs, that collect_typerefs collects a typeref with builtin template...?
11:06xidornwhat does collect_typerefs collect exactly?
11:06emilioxidorn: it collects types that may be incomplete when we first find them.
11:07emilioxidorn: for example, if we find a pointer to an incomplete type, we defer the resolution until later, when we have parsed the correct type
11:07xidornemilio: so should we skip builtin as well?
11:08scoopr intermittent
11:08scoopr130529 < crowbot> No intermittent issues filed with &#39;-ln-ht-&#39; in the title
11:08scoopr130535 <@SimonSapin> -fno-inline-functions doesnt help either
11:08scoopr130552 < emilio> xidorn:
11:08emilioxidorn: presumably, at least builtin templates, but those go through the same path as normal types, so they should be skipped already, what&#39;s the crash you&#39;re getting?
11:08scooprwoops, sorry
11:09xidornpanicked at &#39;What happened?: Recurse&#39;
11:09xidorn2:14.70 9: 0x7ff66131ca76 - bindgen::ir::context::BindgenContext::resolve_typerefs
11:09xidorn 2:14.70 at C:\mozilla-source\rust-bindgen\src\ir\context.rs:426
11:11emilioxidorn: so it&#39;s a typeref that we can&#39;t resolve... That&#39;s pretty much what I found (see the log above), and when I worked around it I got stuck in the monotone framework that fitzgen added, trying to reach a fixed point)
11:12xidornemilio: haha, that&#39;s what I hit eventually as well
11:13xidornso I guess after the crash fixed by my patch, issues are common among platforms
11:16emilioxidorn: yes, I think so. Oh well. I believe fitzgen may have an opinion there, because the first solution I see is backing out the template analysis, and I really don&#39;t want to.
11:17canaltinovaxidorn: hey, I&#39;m not sure about usages of SetCapacity and SetCount methods in bug 1342139. Can you check these? It&#39;s working currently but I want to be sure that I don&#39;t leak anything there
11:17firebothttps://bugzil.la/1342139 NEW, canaltinova@gmail.com stylo: Need will-change support
11:22xidorncanaltinova: I guess that is fine, but it seems to me you don&#39;t actually need to use insert element, you can simply set capacity then append element for each feature
11:22xidornoh wait
11:23xidorncanaltinova: the Copy function doesn&#39;t seem correct...
11:27canaltinovaxidorn: hm, ok. I&#39;ll fix that
11:27xidornI guess I should probably downgrade my llvm to make it work, otherwise I would be blocked until those issues get fixed :/
11:32bd339emilio: for issue 15754, should we do what Boris explained Gecko does, on the mailing list?
11:32crowbotIssue #15754: writing mode needs to affect computed display - https://github.com/servo/servo/issues/15754
11:33bd339emilio: the part about starting at the thing style is inherited from, and then walk up the inheritance chain until we find something with display != contents?
11:55emiliobd339: that&#39;s exactly what layout_parent_style is :)
11:58bd339emilio: ah. nice :) btw, is there some pref I have to set in order to activate display: contents?
11:58emiliobd339: nope, because servo itself doesn&#39;t support it. Display: contents is a Gecko-only thing for now
11:58emiliobd339: I suspect it won&#39;t be hard to implement, but I haven&#39;t tried yet
12:00bd339emilio: alright. so that would explain why layout_parent_style isn&#39;t the outer div in this test case? https://pastebin.mozilla.org/8982618
12:01emiliobd339: yeah, pretty much. If you do getComputedStyle(innerDiv).display, you&#39;d get &quot;block&quot; in servo
12:01emiliobd339: servo paints a border, right?
12:02bd339emilio: yes
12:02emiliobd339: yeah, the border shouldn&#39;t appear
12:02emiliobd339: that is, if servo supported display: contents :{)
12:02emilio*:)
12:04bd339emilio: yeah, I also couldn&#39;t find it in longhands::display::SpecifiedValue when I thought we had to do the &quot;inheritance walk&quot; ourselves.
12:06bd339emilio: anyway, any pointers on skipping text nodes then?
12:07emiliobd339: yes, because servo&#39;s style system is going to be used in Gecko, which does have display: contents support :)
12:07emiliobd339: the cool part is the following: In servo the text style is just a copy of the parent&#39;s, so it&#39;s fine
12:08emiliobd339: oh, sorry, I read &quot;any point&quot; :)
12:10emiliobd339: in gecko it&#39;s just the inherited style from the parent, so we need to add a &quot;is text node&quot; flag or something
12:10jdmgood morning
12:10emiliobd339: I&#39;d say don&#39;t worry about it for now, I can do that part when your patch lands :)
12:10* emilio waves at jdm :)
12:11wafflesjdm, o/
12:19wafflesemilio, it looks like bindgen missed two fields in nsStyleGridTemplate - https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/layout/style/nsStyleStruct.h#1691
12:19waffleshttps://dxr.mozilla.org/servo/source/components/style/gecko_bindings/structs_debug.rs#23802
12:19wafflesit doesn&#39;t support `bool field: 1` ?
12:19emiliowaffles: those are bitfields
12:20emiliowaffles: we once generated accessors for them, but they were quite broken
12:20wafflesemilio, so, how should I go about setting it for gecko?
12:20emiliowaffles: they should be correct now, but we&#39;re trying to figure out what the best way to update bindgen is right now :)
12:20wafflesoh okay
12:20emiliowaffles: you can do the math and file a FIXME: use bindgen-generated accessors or something like that
12:21emiliowaffles: they&#39;re the first two bits, should be trivial
12:21wafflesemilio, ok cool :)
12:22canaltinovaemilio: there is still no passing test in bug 1341642 :/ But :-moz-broken works in my local with that patch
12:22firebothttps://bugzil.la/1341642 FIXED, canaltinova@gmail.com stylo: Need support for the various -moz stuff used for alt text
12:23emiliocanaltinova: how can that be? Can you link a try run?
12:23canaltinovaemilio: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fdd7572c650
12:23canaltinovaemilio: I have no idea
12:23canaltinovaemilio: does try runs on top of inbound or central?
12:24emiliocanaltinova: try runs run on your branch. You seem to have that patch on top of central, but it seems that the servo patch is still on autoland
12:25emiliocanaltinova: please fetch autoland and apply your patch on top of it
12:26canaltinovaemilio: it landed to central: https://hg.mozilla.org/mozilla-central/rev/dacb6daafa3a
12:26emiliocanaltinova: huh, weird
12:26emiliocanaltinova: so if you run the tests yourself, it passes?
12:26canaltinovaemilio: I thought your test expectation update might not be in central but it&#39;s
12:27canaltinovaemilio: well, I didn&#39;t run all the test but test text looks green as it should be
12:40bd339emilio: hmm. I can&#39;t get &quot;Text&quot; to also be vertical in Boris&#39; 2nd test-case. The first test-case seems fine but unlike FF, servo still renders &quot;Text&quot; horizontally in the 2nd test-case.
12:41emiliobd339: yeah, I believe our writing-mode support is not perfect, that&#39;s why it is experimental. Don&#39;t worry about it for now :)
12:54emilioSimonSapin: ping? your inline functions were static inline, right?
13:18SimonSapinemilio: yes
13:18emilioSimonSapin: and could you call them? AFAIK |static inline| forces internal linkage when kept
13:18SimonSapinemilio: https://github.com/PaulStoffregen/SPI/blob/master/SPI.h#L170
13:19emilioSimonSapin: oh, static _members_
13:19SimonSapinemilio: a ~year ago, bindgen generated fn _ZN8SPIClass16beginTransactionE11SPISettings(settings: SPISettings);
13:20SimonSapinwe had been using that generated file (commited in git) since
13:20SimonSapinIm trying to switch to build-time bindgen now
13:20emilioSimonSapin: overloaded keywords ftw. Let me try to find a solution for that.
13:20SimonSapinemilio: so same C++ code, newer bindgen, and beginTransaction is nowhere to be found in generated bindings
13:21SimonSapinemilio: is &quot;inline static&quot; different from &quot;inline&quot; and also &quot;static&quot; ?
13:21emilioSimonSapin: yeah, sorry, I thought that you meant static free functions: |static inline int foo() { return 42; }|
13:22SimonSapinwell, what difference does it make?
13:22emilioSimonSapin: static in a free function is different, it affects linkage
13:22SimonSapinoh right
13:22SimonSapinstatic functions are sort of private, right?
13:23noxI removed inline functions from bindgen a long time ago because of undefined symbol errors.
13:23emilioSimonSapin: right, they force internal linkage IIRC
13:23noxSimonSapin: Aren&#39;t you supposed to be on PTO?
13:23emilionox: right, but they may be defined symbols if you compile your lib with -fkeep-inline-functions
13:23SimonSapinnox: yeah, Im playing with microcontrollers
13:23noxSimonSapin: Nice.
13:23noxSimonSapin: I am on PTO too with nothing to do.
13:24SimonSapinnox: this time around, counting the flashes that my electricity meter makes for every watt-hour and collecting data over wifi, to track electricity consumption
13:25SimonSapinprobably not that useful since Im unlikely to change habits, but who said side projects have to be useful
13:26noxAh ah.
13:31bzcanaltinova: ping
13:31SimonSapinnox: this tiny thing takes commands over a serial port on one side, and does wifi on the other http://letmeknow.fr/shop/device-programmable/306-module-esp8266-wifi-6104023306560.html
13:31noxAh ah.
13:31bzemilio: ping
13:31emiliobz: pong
13:32bzemilio: What&#39;s the landing status of 1341642?
13:32bzemilio: I just realized that https://hg.mozilla.org/integration/autoland/rev/f34476fa83fa did land
13:32bzemilio: So which parts are not landed?
13:33emiliobz: so as far as I know it hasn&#39;t landed. I think canaltinova was investigating why he didn&#39;t see failures on try when landing the NS_EVENT_STATE_XXX flags
13:33emiliobz: only the event state flag changes AFAIK
13:33bzhmm.
13:34bzI see
13:35bzAlright.
13:36bzSo anything I should do here?
13:36* bz pulls, for the moment
13:36emilioI don&#39;t think so. If canaltinova doesn&#39;t manage to figure out why he doesn&#39;t see new tests passing, I guess I&#39;ll pull and investigate with him
13:37bzok
13:37bzThanks!
13:37bzSo 315920-18b.html
13:38bzJust FYI, that depends on a few things
13:38bzIt depends on correct handling of the :-moz-broken
13:38bzAnd depending on how the timing stars align it depends on whether dynamic changes to ~ are handled correctly.
13:38canaltinovabz: pong
13:38emiliobz: yeah, I know, it also (may?) depend on the slow selector flags, etc etc
13:39canaltinovabz: yes, tests are landed but m-c side of -moz-* pseudo classes are not landed yet
13:39bzIt absolutely does, _if_ the image load failure loses the race to initial layout here
13:39bzSo it&#39;s timing-dependent in weird ways
13:39emiliobz: gah, you&#39;re faster writing ;). Anyway, it wasn&#39;t due to it, forcing a restyle of the label onload didn&#39;t help at all, I commented on the dependent bug
13:40bzhuh
13:40* bz reads
13:40emiliobz: I just wanted to came with a coherent reason for it failing :). Note that that was _without_ the event state flags
13:41emiliobz: i.e., before I figured out that it had not landed, so with those flags, results may vary
13:41bzI see
13:42bzSeems like we should land the event state flags
13:42bzbecause without that, things can&#39;t possibly work, right?
13:42emilioright
13:42bzAnd in fact now the servo side is checking random flags that are totally unrelated when doing selector matching
13:43bzok
13:43bzAnyway, if you decide you need me, please ping. ;)
13:45canaltinovabz: servo side actually is not checking the flags in the stylo side I guess since their parse functions are different
13:47emiliocanaltinova: what?
13:47emiliocanaltinova: I bet boris meant the servo side of the Gecko integration, which does check those flags
13:48canaltinovaemilio: ah, sorry. right. We should land the gecko side too
13:48* bz is really confused by what happened in the landing of this bug. ;)
13:49canaltinovaI thought he was talking about new added ElementState bitflags
13:50canaltinovabz: the tests are landed but the actual changeset that organizes the bitflags in the m-c side is not landed yet
13:51bzYes, I understand that. What&#39;s confusing me is how we got into that state. ;)
13:51bzAnyway.
13:51canaltinovawell, I dunno :)
13:53bzok
13:53bzWho&#39;s landing the event states? ;)
13:54bzDo we have a try run on hand with the event states?
13:55canaltinovabz: yes, I ran but there is no passing tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fdd7572c650b6fafd51a6e7ae1ef4b9394f5152
13:55canaltinovawhich is interesting
13:57bzAs in no unexpected passes?
13:58* bz is so confused about what ServoRestyleManager::ContentStateChanged is doing
13:59bzok
14:00bzBut testing actual images it works?
14:00bzAll the :-moz-broken reftests rely on correct handling of the slow selector flags
14:01canaltinovabz: ah, yes, I was expecting some unexpected passing reftest but if they are rely on something else, it&#39;s ok to land I guess
14:01SimonSapinemilio: whats the difference between -fkeep-inline-functions and -fno-inline-functions?
14:02emilioSimonSapin: I guess the later doesn&#39;t inline them in the C++ side
14:02bzDo you have builds with/without the flags change on hand?
14:02* SimonSapin nods
14:02bzI can write you a reftest that should get fixed by the flags change... ;)
14:05canaltinovabz: yep, I have a build with this change. It would be great :)
14:08canaltinovabz: can we test internal pseudo-classes in reftests? I can do others too I guess but I don&#39;t really know how to test them all
14:14bzcanaltinova: what do you mean by &quot;internal pseudo-classes&quot;?
14:16canaltinovabz: some of these pseudo-classes are enabled only ua stylesheet and chrome unlike :-moz-broken
14:16canaltinovabz: I assume we can&#39;t test them
14:16bzah
14:17bzYes, you can&#39;t test that in a reftest
14:17bztestcase: data:text/html,<!DOCTYPE html><style>:-moz-broken { color: green; }</style><img src=&quot;nosuch:url&quot; alt=&quot;This text should be green&quot;>
14:17bzreference: data:text/html,<!DOCTYPE html><style>img { color: green; }</style><img src=&quot;nosuch:url&quot; alt=&quot;This text should be green&quot;>
14:20canaltinovabz: thanks but we don&#39;t have complete alt support because we don&#39;t support :-moz-empty-except-children-with-localname yet. Manishearth told us he&#39;s working on it, so I dropped
14:21canaltinovahttps://bugzilla.mozilla.org/show_bug.cgi?id=1341642#c10
14:21bd339emilio: where in the tree should the new WPT for writing-mode go?
14:21firebotBug 1341642 REOPENED, canaltinova@gmail.com stylo: Need support for the various -moz stuff used for alt text
14:21bzah
14:21bzok, how about:
14:22bzdata:text/html,<!DOCTYPE html><style>:-moz-broken { border: 10px solid green; }</style><img src=&quot;nosuch:url&quot;>
14:22bzdata:text/html,<!DOCTYPE html><style>img { border: 10px solid green; }</style><img src=&quot;nosuch:url&quot;>
14:23canaltinovabz, yep it works
14:25canaltinovabz: so where should I put this reftest? :)
14:27canaltinovabz: I assumed we would want to add this to reftests but I don&#39;t know if you want
14:36bzYeah, please add it to the place where the other moz-broken reftests are?
14:36bzOh, hmm
14:36bzOne sec
14:37bzlayout/reftests/image
14:37bzis better
14:38bzName it moz-broken-matching-1.html and moz-broken-matching-1-ref.html
14:50canaltinovabz: ok, thanks!
14:56emiliobd339: let&#39;s put it on tests/wpt/mozilla/tests/css
14:57emiliocrowbot: is failed to fill intermittent?
14:57crowbot#14323 - Intermittent crash (&quot;failed to fill whole buffer&quot;) in various tests (https://github.com/servo/servo/issues/14323)
14:59canaltinovabz: added a reftest I think it&#39;s ready to land now
15:07bzDoes anyone here understand ServoRestyleManager::ContentStateChanged ?
15:07bzOr at least the exact behavior of when Servo_Element_GetSnapshot returns non-null
15:26emiliobz: I believe I do
15:26emiliobz: but that changed, recently, let me re-read
15:26bzemilio: My real question is why it bothers with the ContentStateChangedInternal call if !snapshot
15:26bzcanaltinova