mozilla :: #servo

15 Jul 2017
00:01bholleyjdm: anyway - if you think it might be some days we should probably back out, but otherwise we can just fix in-place
00:02jdmbholley: it shouldn't be; I just need to find someone willing to do the straightforward review.
00:03bholleyjdm: tell whoever it is that I said it's urgent :-)
00:12bholleyhiro: what do you think your ETA is for landing bug 1371450?
00:12firebot NEW, stylo: panic in geckoservo::glue::Servo_ResolveStyle call from APZ
00:13jdmbholley: another option - I have a one line patch that removes the performance problem which you could review right now:
00:13bholleyNightly pref experiments are ready to start on monday, but I think we probably want to hold off until bug 1371450 lands
00:13jdmit's just a more targeted version of the actual change the other patch makes
00:13hirobholley: once all pathes got r+ I can land as soon as possible. It depends on when emilio wakes up. :)
00:14bholleyjdm: r=me
00:14bholleyjdm: (Add comment explaining what's up)
00:15bholleyhiro: ok great! Hopefully emilio gets to it soon
00:16bholleyhiro: thanks for making that fix happen :-)
00:16hirohiro: np, it's my pleasure.
00:54Manishearthemilio: bholley why would resolve_primary_style be called for the ::placeholder pseudo?
01:03Manishearthemilio: I've changed resolve_primary_style to pass down pseudos
01:06bholleyManishearth: hm, placeholder isn't NAC-backed, right?
01:08Manishearthbholley: idk :)
01:09bholleyManishearth: oh wait, it is
01:09bholleyManishearth: so yes, that would make sense
01:09bholleyManishearth: so
01:09Manishearthbholley: so passing down a PseudoElement to resolve_primary_style is ok, yes?
01:10bholleyManishearth: basically, for pseudos that are implemented by native-anonymous-content, servo traverses them during the parallel traversal, and then notices that they implement a pseudo for an ancestor element, and then does a separate computation
01:10bholleyManishearth: no, that seems wrong
01:10bholleyManishearth: the primary style already knows if it's resolving for a pseudo
01:10* bholley Manishearth:
01:11bholleyManishearth: so in the caller, you can just assert that the primary style we're trying to resolve is the same thing the element implements
01:11bholleyManishearth: if I understand your situation correctly, which maybe I don't
01:13Manishearthbholley: I have a situation where the style for ::placeholder is being handled in resolve_primary_style
01:13ManishearthI need the pseudo info
01:13Manishearthshould I pass pseudo info separately?
01:13Manishearthi can do that i guess
01:13bholleyManishearth: this is during the traversal?
01:13bholleyManishearth: or when C++ is asking for the style?
01:14bholleyManishearth: where do you need the pseudo info? In the cascade?
01:14Manishearthbholley: to construct the style context
01:14bholleyright ok
01:15bholleyManishearth: we currently get this information here:
01:15bholleyManishearth: so you're going to want that in cascade_style
01:16bholleyManishearth: you could just get it again. But you might as well cache it on the StyleResolver
01:18Manishearthbholley: hm, yeah
01:18Manishearthbholley: or i can pass it down
01:18Manishearthill just pass it down as an extra arg
01:32bholleyManishearth: Looking at Part 13 - I see us removing the Arc type - where do we add the RefPtrTraits?
01:33bholley(For ServoStyleContext)
01:33bholleywas that in a previous patch?
01:37Manishearthbholley yeah
01:38Manishearthbholley the arcing for SSC is weird bc we use the type on both sides
01:38Manishearthwith the full struct def, not opaque
01:38Manishearthalso its namespaced
01:38Manishearthwhich breaks things
01:39bholleyManishearth: ok
01:39bholleyManishearth: r+, but please fix up the Forgotten stuff
01:39Manishearthbholley: didnt i fix Forget()?
01:39bholleyManishearth: (described in review comments)
01:39bholleyManishearth: yes, but what you did wasn't what I meant
01:39bholleyManishearth: basically do the raw ptr -> newtype conversion directly in the FFI entry point, and use the newtype everywhere else
01:40bholleyManishearth: and make the newtype not have an implicit constructor
01:41bholleyManishearth: |const nsStyleEffects* effects = computedValues->ComputedValues()->GetStyleEffects();| I'm confused - that looks like we still have two pointer dereferences?
01:41bholleyManishearth: I guess ComputedValues() returns a pointer to the inline structs?
01:41ManishearthComputedValues() is just ptr addition. and should inline
01:43bholleyManishearth: you moved ResolveSameStructs out of line again...?
02:07Manishearthbholley yeah, because the style struct funcs dont work otherwise
02:08Manishearthfor the style struct stuff to work you need nsStyleFoo to be fully defined
02:08Manishearthi can remove that but that involves removing the gecko: nsStyleFoo indirection, which is a large patch
02:21* jdm -> out
04:44ManishearthFireBlood: fwiw,
04:44FireBloodahhhh hahah. Manishearth
04:44ManishearthTLDR a significant fraction of us know the states in order bc of that song
04:45FireBloodits not that long
05:29paulWe don't have to only have one commit by PR, right? If so, is it fine if Servo doesn't compile between commits? I have 8 commits in my PR, it only compiles if the 8 of them are applied.
05:29Manishearthpaul: yes, but we prefer if it compiles per commit
05:29Manishearthbut not a hard requirement
05:30paulwouldn't that break bisecting?
05:30Manishearthagain, we prefer if not
05:31paulUnderstood. Thank you.
05:31Manishearthyou can bisect on merge commits though so it's fine if you have to, but try to avoid it
05:59jdmemilio: could you push the patch in bug 1381045 to autoland?
05:59firebot ASSIGNED, stylo: Significant Tp5 regression between rev 0e41d07a703f and rev b07db5d650b7
09:30cynicaldevilnox: ping?
11:12noxbholley: Please at least file a follow up for the code in #17741 where emilio preferred the previous version.
11:12crowbotPR #17741: Trim some fat from the traversal -
11:13noxOr just implement your suggestion from the bugzilla ticket, especially given your PR iS red it seems anyway.
11:20noxcynicaldevil: Pong
12:27noxSimonSapin: I wonder why a normal build of Servo requires building encoding_index_tests.
12:28SimonSapin# TODO consider using dev-dependencies instead
12:28SimonSapinnox: ^
12:29noxNow I wonder why it's a todo.
12:29SimonSapinthe crate is 229 lines and only has macros, so fixing that doesnt feel worth the bother to me
12:30noxajeffrey: Why does swapper makes servo require building a markdown parser?
12:30SimonSapinnox: the comment also says "(Cargo issue #860)"
12:30crowbotIssue #860: *** glibc detected *** ./servo: double free or corruption (!prev): 0x0000000002851920 *** -
12:30crowbotIssue #860: `Dev-dependencies` are not linked when testing subpackages -
12:31noxI will stop looking into the pointless dependencies we have because it makes me miserable.
12:32SimonSapinsounds like a good idea
12:32noxajeffrey: But if you could consider not depending on skeptic just for a 10 lines snippet in swapper's README, that would make me happy.
12:32SimonSapinnox: well likely switch to encoding-rs at some point anyway
12:32noxPut that snippet in a doc block if you want.
12:40ajeffreynox: oh that's annoying, using skeptic drags in the md parser.
12:40noxajeffrey: Just put the example in and point to the documentation in
12:40ajeffreynox: can we use a feature to stop this?
12:40noxWe can not use skeptic to stop this.
12:41ajeffreynox: I really like being able to test the readme.
12:41noxajeffrey: is tested.
12:41noxREADME doesn't need to contain code,
12:41noxit just needs to contain a link to documentation which includes the examples and whatever else.
12:41ajeffreyYes, but the readme is the top-level doc that everyone reads.
12:42nox[citation needed]?
12:42noxI won't speak for everyone, but I most often just check and never look at readmes.
12:44ajeffreyPerhaps brson knows how to avoid this?
12:44noxAvoid what?
12:45ajeffreyHmm, what's brson's irc Nick?
12:45noxNo code.
12:45ajeffreyAvoid having skeptic drag in the md parsner into any client build.
12:46noxThat sounds like making things more complicated,
12:46noxinstead of just replacing a sample with a documentation link.
12:46noxI wouldn't mind if we had dozens of crates relying on skeptic, but that's not the case.
12:47noxIn any case, the readme should contain a link to the documentation, even if you keep skeptic.
12:49noxajeffrey: Unrelated, can't we remove the T: Send bound on the swap method?
12:49noxNot that it would be useful,
12:49noxbut AFAIK it&#39;s ok to swap values that are not Send, given it will happen in the same thread, given the Swapper<T> itself ends up not Send.
12:51ajeffreynox: belt and braces.
12:52ajeffreynox: I&#39;m also worried about having safety properties that rely on the magical auto derived Send and Sync.
12:52noxIf you are saying &quot;moar seatbelts&quot;, libstd itself doesn&#39;t say T: Send for the send method.
12:52noxIt&#39;s like, the whole purpose of Rust?
12:52noxThat sounds like saying &quot;I&#39;m afraid &T is a null pointer&quot;.
12:55ajeffreyFor example, this would be unsafe if Swapper was made Sync.
12:55cynicaldevilnox: I wanted to run something by you. It&#39;s kinda obvious, but still,
12:55noxWhy would it be made Sync?
12:55ajeffreyIt&#39;s annoying that channels aren&#39;t Sync.
12:56ajeffreynox: because at some point in the future someone might change the API.
12:56cynicaldevilnox: I used the sync tokenizer to verify that treesink operations are indeed being called after the feed function returns. I modified the tokenizer in servoparser/ to look like this:
12:57noxajeffrey: As in channels becoming Sync?
12:57ajeffreyI was thinking of Swapper, but yes, channels too.
12:57cynicaldevilnox: And then added a few debug statements in the treesink methods. Now if these debug statements appear after the one I wrote in feed function, then that means TreeSink methods are called after them, right?
12:58noxSwapper uses channels and I don&#39;t see how it could be Sync anyway.
12:58ajeffreyChannels not being sync is annoying.
12:58noxGiven there is no T: Send on the send method, channels cannot become Sync.
12:58noxAnd I don&#39;t see how it&#39;s annoying anyway but shrug.
12:58ajeffreyMy nice lock-free implementation of worklets needs a lock because of this.
12:58noxIt&#39;s not lock-free then.
12:59ajeffreyIndeed :(
12:59noxI mean that&#39;s not channels&#39; fault. :)
13:00noxajeffrey: If it relies on something not lock-free, it&#39;s not a nice lock-free impl. :)
13:00cynicaldevilnox: well?
13:01noxSorry, reading now.
13:02noxcynicaldevil: AFAICT yes.
13:03cynicaldevilnox: Moreover, the test does not have a body element. The TreeSink operations which appear after the function returns are concerned with creating a body element, appending it, and then popping it.
13:03cynicaldevilnox: Any idea what might be the cause of this?
13:03noxcynicaldevil: What do you mean by &quot;does not have a body element&quot;?
13:04cynicaldevilnox: This is the test in question:
13:05noxcynicaldevil: And your thing doesn&#39;t print anything related to the creation of the body element, is that it?
13:05cynicaldevilnox: Exactly.
13:06noxcynicaldevil: Are you building a debug release, for starters?
13:06noxAre some debug! printed out at all?
13:07cynicaldevilnox: debug version, yes. All the debug statements before &#39;feed&#39; returns are printed normally. I added a few extra debug statements to verify this.
13:10cynicaldevilnox: I don&#39;t get why the TreeSink methods to create the body element are generated at all.
13:10noxcynicaldevil: That&#39;s part of HTML.
13:11noxcynicaldevil: All HTML documents have a body element.
13:13noxcynicaldevil: Ultimately when parsing such a HTML document,
13:13noxcynicaldevil: at some point it arrives in this mode
13:13noxcynicaldevil: And that mode&#39;s last rule says: &quot;Anything else: Insert an HTML element for a &quot;body&quot; start tag token with no attributes. ...&quot;
13:14cynicaldevilnox: But this all happens as part of the feed function, right?
13:14noxcynicaldevil: Yes.
13:14noxcynicaldevil: Oh!
13:14noxcynicaldevil: Did you account for the parser yielding back to caller on </script>?
13:15cynicaldevilnox: Yes, but why would that matter?
13:25noxcynicaldevil: We call into the parser again through resume_with_pending_parsing_blocking_script at that point.
13:25noxcynicaldevil: ...Which ultimately calls back feed, disregard me.
13:25cynicaldevilnox: yup exactly.
13:27cynicaldevilnox: Does servo call h5e tokenizer&#39;s feed function anywhere except through the tokenizers in and
13:28noxcynicaldevil: In
13:28noxcynicaldevil: From resume_with_pending_parsing_blocking_script.
13:29cynicaldevilnox: It calls h5e&#39;s tokenizer directly?
13:29noxcynicaldevil: It calls parse_sync,
13:29noxwhich calls do_parse_sync,
13:30noxwhich calls tokenize,
13:30noxwhich calls Tokenizer::feed,
13:30noxwhich calls h5e&#39;s feed method AFAIK.
13:31cynicaldevilyeah, that&#39;s through the tokenizer defined in
13:31cynicaldevilso no
13:35cynicaldevilnox: Could you point me to the code in h5e which deals with after-head-insertion mode thing?
13:37noxcynicaldevil: insert_phantom may not be doing some short-circuiting!
13:38noxcynicaldevil: Maybe we should always push and pop?
13:39cynicaldevilnox: does append call push?
13:39noxmay be doing some short-circuiting* btw.
13:39cynicaldevilnox: What do you mean by short-circuiting?
13:39noxcynicaldevil: Doing less work than if we pushed and then popped.
13:40noxThus not calling some of your callbacks or whatever.
13:41noxcynicaldevil: What do you mean by append?
13:42cynicaldevilnox: One of the treesink methods called after the feed function returns was append, so I thought append might be a result of insert_element
13:42noxcynicaldevil: Where append?
13:42noxThe only append method I see is a method on some RcDom stuff.
13:42cynicaldevilthe append in TreeSink
13:43noxCan you link to it?
13:44noxI guess I&#39;m blind.
13:45noxOh! In Servo, ok.
13:45noxcynicaldevil: Append is called from feed,
13:45noxby h5e.
13:46cynicaldevilnox: I know that.
13:47cynicaldevilThis whole thing is extremely confusing :/
13:49cynicaldevilnox: Let me try adding a few debug statements to my h5e fork and using it in servo.
13:50cynicaldevilnox: Keep in mind that if you appear online tomorrow, I might trouble you some more with this ;)
13:50noxcynicaldevil: No problem.
13:50noxcynicaldevil: Btw just decided on my Summer PTO,
13:51noxcynicaldevil: Will be away during last week of July and first week of August.
13:51cynicaldevilnox: Noted.
13:59emilionox: care to take a quick look to ^? Does it look reasonable to you?
14:00emilionox: it definitely allows less hacks on style, but I have to expose a limited part of the dom module, so I thought you may have an opinion
14:00noxemilio: You just move around stuff, right?
14:06emilionox: yes, pretty much
14:06noxemilio: LGTM
14:06emilionox: cool, thanks!
14:41* emilio mutters something about unit tests and recalls nox&#39;s position on that
14:45noxemilio: Death to unit tests.
17:03emilior? anyone ^
17:05emiliowaffles: thanks!
17:05wafflesemilio: no problem :)
17:16emiliowaffles: care to stamp another easy one? ^ :)
17:16wafflesemilio: lol
17:17emiliowaffles: I&#39;m just doing some spring cleanup :P
17:17wafflesheh :D
17:28bholley(and bitrotting me ;-) )
17:28bholleyit&#39;s ok though
17:30emiliobholley: whoops... Hope you don&#39;t mind too much I removed D::clear_element_data and such
17:30emiliobholley: they were pretty much a hack :)
20:26Manishearthlol I did a rebase and then did mach build binaries. bad idea
21:22Manishearthhuh i think the leak fixed itself
22:22bzbholley: ping
22:25bzbholley: unping; just commented in the bug instead
16 Jul 2017
No messages
Last message: 9 days and 5 minutes ago