mozilla :: #servo

17 Jul 2017
00:05Manishearthemilio: I like crypto, yeah
00:05Manishearthim not *doing* any crypto
00:05Manishearthjust reading some SIDH papers
01:10Manishearthemilio: my Hax(tm) fixed the leak
01:10Manishearthi think
03:07Manishearthbholley: we can't exactly do it at the top because UsesArena assumes that the ptr isn't null
03:07Manishearthso it gets uglier
03:08Manishearthbut i guess i could do that
03:08bholleyManishearth: it seems wrong to access ->Arena() if UsesArena() returns false
03:08bholleyManishearth: but we could just invert that order more locally in the checks I guess
03:09Manishearthyeah
03:11Manishearthbholley: i can stick a #cfg DEBUG if (mPtr && aPtr) MOZ_ASSERT(...) at the top i guess
03:12Manishearthbholley: actually, im not clear why we want to assert that it's the same?
03:13Manishearthcurrently it will be, because we'll never replace a GeckoStyleContext with a ServoStyleContext
03:13Manishearthbut future consumers of ArenaRefPtr may feel differently
05:21heycamxidorn: will your rebasing in bug 1379901 change it significantly?
05:21firebothttps://bugzil.la/1379901 NEW, xidorn+moz@upsuper.org stylo: Socorro Super Search page's form fields are aligned vertically instead of horizontally
05:21heycam(or should I review now)
05:21xidornheycam: it shouldn't change significantly
05:21heycamok thanks
05:30xidornheycam: thanks
05:30heycamnp
06:42heycamxidorn: are you looking at bug 1381357?
06:42firebothttps://bugzil.la/1381357 NEW, nobody@mozilla.org stylo: thread &#39;<unnamed>&#39; panicked at &#39;Resolving style on element without current styles&#39;
06:43xidornheycam: no
06:43heycamok
06:47emilioManishearth: why isn&#39;t it leaking now again?
07:08noxShouldn&#39;t we be trying to get enum flattening in rustc ready for when stylo lands?
07:31emilioxidorn: heycam: The assertion may be because of non-animation hints on an animation-only restyle? If so, hiro had a wip patch relaxing the assertion
07:32heycamemilio: ok. I&#39;m wondering if that&#39;s the cause of some of the other unstyled issues, e.g. on slack, fastmail
07:33heycamemilio: is the WIP patch there baked enough for me to try locally to see if I can still repro my fastmail unstyled/assertion?
07:33emilioheycam: presumably, yeah...
07:33* emilio tries to find the office
07:34heycamemilio: first day?
07:34emilioheycam: yup! If I manage to find it of course :)
07:34heycam:D
07:35* heycam notices emilio&#39;s phonebook office location is wrong
07:35emilioheycam: yeah, hopefully I don&#39;t need to do a transoceanic flight before 10am :)
07:36heycamemilio: btw that assertion fix is the bug 1378064 one?
07:36firebothttps://bugzil.la/1378064 ASSIGNED, hikezoe@mozilla.com stylo: site issue: when clicking to make a video fullscreen, sometimes the website and not the video
07:37emilioheycam: yup
07:37heycamcool thanks
08:00Manishearthemilio i feel like it works now because the SSC lives for shorter time
08:00Manishearthhave yet to investigate
08:00Manishearthbasically this bug only crops up with a certain ordering of events
08:00emilioManishearth: that doesn&#39;t make too much sense to me, specially if the leaked stuff was computed style data, but cool, thanks for investigating :)
08:02emilioheycam: if you&#39;re around, do you know what made keeping the data for display: none subtrees around until the next restyle hard?
08:02Manishearthemilio: i figure alloc addref addref+register decref+register decref free would not have the bug
08:02heycamemilio: I don&#39;t think that specific scenario was considered when we were discussing it back then. it was more about whether we kept them indefinitely or not.
08:03heycamemilio: although I might be misremembering
08:03Manishearthwhereas alloc addref addref+register decref decref+deregister free would
08:03emilioheycam: Oh, ok, then my approach should work quite well
08:03heycamemilio: I think it will be ok. on the face of it I&#39;m not sure what trickiness in tracking there should be.
08:24Manishearthemilio: ah, so I&#39;m more certain about my hunch
08:25Manishearthemilio: there&#39;s a static cast to GeckoStyleContext due to the arena stuff
08:25Manishearththis still *works* because GeckoStyleContext doesn&#39;t contain additional owned data, just a shared destructor
08:25Manishearthbut it tries to deallocate itself from the arena which does nothing since it&#39;s not in the arena
08:26Manishearthwhich makes the pres shell crash make more sense in this context
08:26Manishearthemilio: so, basically, somewhere there, in the past we were doing:
08:27emilioManishearth: why doesn&#39;t it use AsGecko()?
08:27* emilio shrugs
08:27emilioManishearth: can you point me to the cast?
08:28noxemilio: &quot;FIXME(emilio): why is this in length.rs?&quot;
08:28noxStory of my life
08:28emilionox: heh
08:29Manishearthemilio: http://searchfox.org/mozilla-central/source/layout/base/nsPresArena.cpp#45
08:29Manishearthit&#39;s just how the pres arena stuff works
08:29noxcynicaldevil: Ping?
08:30xidornnox: is there anyone working on enum flattening?
08:30emilioManishearth: Hmm... But then why doesn&#39;t it leak?
08:30noxxidorn: eddyb has a design in his mind,
08:30Manishearthemilio: because of the order
08:30noxxidorn: but he was abducted to work on something else, also Stylo-related IIRC.
08:31noxxidorn: And that something else, I&#39;m not sure the design exists only in his head, that&#39;s why I&#39;m trying to push for an enum flattening effort.
08:31Manishearthemilio: this will only leak if it&#39;s not the last thing to be cleared IIRC
08:31noxAlso because I want moar enums everywhere, and I don&#39;t want to be That Guy Who Made Our Memory Usage Worse.
08:31Manishearthemilio: basically, where we used to hold on to a SCV we hold onto an SSC now, which lengthens its lifetime long enough for the leak to be observable
08:33Manishearthemilio: you&#39;re clear that what we were doing was horribly wrong, yeah? :)
08:33Manishearthit only worked because we deregistered before the arena did
08:34cynicaldevilnox: ping
08:34cynicaldevil*pong
08:34noxcynicaldevil: How are things going?
08:35cynicaldevilnox: Yeah I figured out what I had to do. I&#39;ll update the PR in a few hours :)
08:35noxcynicaldevil: Nice!
08:35Manishearthemilio: I should probably insert an IsGecko() assert where we fetch the arena id
08:53xidornnox: ok, thanks for the info. hope that can happen soonish...
08:58emilioManishearth: ok, that makes sense, yeah
08:58emilioManishearth: I just wanted to know what was the underlying issue, thanks for doing that
08:59Manishearthemilio: thanks
08:59Manishearthemilio: I&#39;m not exactly sure where the ordering issue comes from, but there&#39;s plenty of swapping RefPtr<SCV> for SSC that it could happen
09:00Manishearthemilio: the animations_omta bug is that visited styles totally broke in a rebase ;p
09:02Manishearthhas nothing to do with animations, just that the animations test actually waits on visited styles happening
09:02Manishearthjryans: FWIW we need more visited style tests, I broke visited styles and the *only* test that failed was animations_omta.html
09:04emilioManishearth: Yeah, that happened to me too...
09:04emilioManishearth: note also bug 1381276
09:04firebothttps://bugzil.la/1381276 NEW, nobody@mozilla.org stylo: Wrong color of elements inside <a> tag.
09:04Manishearthand that test is like a single test with a million subtests with *one* subtest that tests visited style animations
09:05Manishearthbut I broke <a href=&quot;&quot;>
09:05Manishearthwe need simpler tests for that
09:05Manishearthto be clear, not saying it&#39;s your fault, just pointing it out since you&#39;ve looked into this more and may know of tests that *should* be there but are failing due to other reasons
09:06Manishearth:)
09:06Manishearthemilio: yeah i suspect we have tests, btu they fail due to other reasons rn
09:12noxInterpolating matrices is so messy. :(
09:13noxI&#39;m not even sure what it&#39;s doing.
09:15Manishearthnox: SLEEEERP
09:15noxWhat?
09:15Manishearthnox: Spherical Linear intERPolation
09:15noxNo idea what you mean.
09:17Manishearthnox: it&#39;s a trick that lets you avoid the gimbal lock situation (google it) when meaningfully interpolating rotations and transitions in 3d, which works by basically adding a redundant degree of freedom (thus obtaining a space in which gimbal lock does not occur), and then removing it later via a constraint
09:17Manishearthneat sleight of hand
09:17Manishearthbut the equations are gnarly
09:17noxPretty sure that has absolutely nothing to do with what I&#39;m talking about.
09:17noxI don&#39;t care about the equations.
09:18Manishearthnox: interpolating matrices uses the slerp algorithm
09:18noxSure.
09:18Manishearthnox: oh, you&#39;re dealign with the types
09:18Manishearthnot the code
09:21emilioheycam: ;_; https://treeherder.mozilla.org/#/jobs?repo=try&revision=882675411724edfee11ae5d5d376361b2cfc578a&selectedJob=114778646
09:21emilioheycam: I hate those tests
09:21noxSeems like we don&#39;t even do anything with these transform variants,
09:21heycamo_O
09:21noxexcept convert them to gecko stuff.
09:21noxGreat.
09:22heycamemilio: I have a suspicion that your changes last week didn&#39;t actually fix whatever the issue is there with them
09:22heycamemilio: but just happened to paper over it
09:23emilioheycam: I&#39;m pretty sure it did... This is, I think, about handling the XBL binding on the scrollbar... I _think_ the few HasServoData() checks that we do makes the difference here
09:24emilioheycam: I can try and debug them further I guess
09:24heycamemilio: mmmm
09:26noxSo we don&#39;t actually animate &#39;transform&#39; in Servo?
09:27emilionox: we should, but there are a few cases (with percentages I think) that transform needs layout info
09:27emilionox: and we don&#39;t do that in Servo I think
09:27Manishearthnox: no, because it requires layout support
09:27noxemilio: No no,
09:27Manishearthnox: the problem is, you can have percentages in transforms
09:28noxthat&#39;s again unrelated but ok.
09:28noxI mean the todo about InterpolateMatrix and AccumulateMatrix,
09:28noxin the middle of fragment.rs,
09:28Manishearthso if you have a composition of a translate with percentages with something else, you need layout info to interpolate, which we need support for in servo
09:28noxbecause these representations our own code never actually do anything with them,
09:28Manishearthyeah that&#39;s exactly it
09:28noxand instead we just convert that to Gecko.
09:28noxThat&#39;s exactly not it.
09:28noxThese variants have absolutely nothing to do with the weird -moz-transform matrices.
09:28Manishearthnox: and?
09:29noxSo why do you mention the translate percentages or whatever?
09:29Manishearthnox: you can have percentages in translate!
09:29noxOk.
09:29Manishearthyou can&#39;t have percentages in *matrix*
09:29noxHow does that relate?
09:29Manishearthnox: that&#39;s what I&#39;m trying to tell you
09:29noxThese two variants have to do with https://drafts.csswg.org/css-transforms/#other-animation
09:29Manishearthif you want to animate, say, translateX(50%)rotateY(20deg) with say translateZ(10px)
09:30Manishearthyou cannot use the existing animation machinery
09:30noxAnd https://github.com/nox/servo/blame/master/components/layout/fragment.rs#L2899 very much handle percentage translations.
09:30noxErr, stupid link,
09:30Manishearthnox: please listen
09:30noxhttps://github.com/nox/servo/blame/master/components/layout/fragment.rs#L2882
09:30Manishearthnox: it&#39;s the confluence of *composed* matrices where one of them is a percentage translation or otherwise
09:31Manishearthnox: you *can* animate `translateX(50%) ->translateX(20px) `
09:31Manishearthyou cannot easily animate `translateX(50%)rotate(20deg) -> translateX(10px)`
09:31Manishearthbecause the algorithm for arbitrary interpolation requires
09:31noxThat doesn&#39;t explain to me why we have these variants.
09:31Manishearthnox: let me finish
09:31Manishearthbecause the algorithm for arbitrary interpolation requires pixel values
09:32noxAs far as I read the spec, all you need in the end is a matrix() or a matrix3d().
09:32Manishearthnox: translate(50%)rotate(20deg) composes to a matrix() that uses calcs
09:33Manishearthsince we don&#39;t have layout info in the animation machinery, the way InterpolateMatrix works is that it notices that we can&#39;t yet do the full computation, and defers it by saying &quot;ok here are the before and after translate matrices, *you* deal with the calculations&quot;
09:33Manishearthwhich is what gecko does
09:33Manishearthservo doesn&#39;t yet do that last part, the &quot;you deal with the calculations&quot;
09:33Manishearthwe could
09:34Manishearthbut it&#39;s nontrivial IIRC so nobody has done it
09:35noxWhy can these intermediate matrices be nested?
09:36Manishearthnox: you can&#39;t put an InterpolateMatrix within an InterpolateMatrix if that&#39;s what you&#39;re asking
09:36noxYou can.
09:36Manishearthoh
09:36ManishearthI know why
09:36Manishearthinterrupted transitions
09:36noxThere is even a comment about it.
09:37Manishearthnox: if there&#39;s a transition and then you do a dynamic change that changes the &quot;to&quot; style, it doesn&#39;t restart, it continues from the current point
09:37Manishearthexcept the current point is an InterpolateMatrix
09:37Manishearthso it just nests them
09:37Manishearth*shrug*
09:37Manishearthat least, that&#39;s what I suspect it is
09:37ManishearthI haven&#39;t seen that code, link to the comment?
09:38nox&quot; nested &quot; in box.mako.rs.
09:38noxThese values are never observable outside animations?
09:39noxThe to_css code for them just use unreachable!.
09:39Manishearthnox: yes
09:39Manishearthnox: okay interesting
09:39noxOk.
09:39Manishearthnox: so i was under the impression they didn&#39;t nest and that we even had asserts preventing nesting (I could swear I reviewed code adding those asserts)
09:39Manishearthbut it makes sense that they would need to nest for interruptable animations to work
09:39noxWhy?
09:40Manishearthso perhaps someone removed those asserts, or i may have imagined their existence
09:40Manishearthnox: like I said
09:40noxWhen you anime the first time and the two lists have different lengths,
09:40noxyou end up with a list of a single InterpolateMatrix value,
09:40Manishearthnox: lets say you transition from pt A to pt B. But then halfway through the &quot;to&quot; point changes to pt C
09:40Manishearthwhat is the new &quot;from&quot; style?
09:40noxon interrupting, couldn&#39;t we see that there is a single InterpolateMatrix stored?
09:41Manishearthnox: huh
09:41noxAt which point can we have an InterpolateMatrix and something else at the same level of a list of transforms?
09:41Manishearthnox: well no it&#39;s an interpolatematrix with the wrong `progress` value
09:41Manishearthnox: but this looks like it can be cleaned up?
09:41Manishearthnox: you probably need to talk to whoever wrote that comment :)
09:41noxWell that&#39;s what I&#39;m asking about.
09:42noxAFAIK, whenever you have an InterpolateMatrix or an AccumulateMatrix,
09:42Manishearthyeah
09:42noxyou never ever have something else.
09:42Manishearthyes
09:42Manishearthnox: well
09:42noxSo we could introduce some sort of AnimatedTransformList,
09:42Manishearthin the current code interruptible transitions will need nesting of IMs
09:42noxwhich either stores a list of transforms,
09:42noxor the from/to/third thing I forgot the name of.
09:42Manishearthbut you can add another parameter to IM to make that unnecessary AFAICT
09:43noxWhat?
09:43Manishearthnox: again, the `progress` field of the IM will be wrong, you can&#39;t just reuse the IM for the same transition
09:43noxCan&#39;t we just see we already had the weird intermediate value and just keep on using that?
09:43Manishearthbut the weird intermediate value doesn&#39;t animate right?
09:44Manishearththe weird intermediate value animates from A to B
09:44Manishearthyou want to now animate from intermediate to C, but at progress point where you were between A and B
09:44nox&quot;Doesn&#39;t animate&quot;?
09:45Manishearthnox: it doesn&#39;t animate correctly
09:46Manishearthnox: you now need to animate between (A, B, progress-at-interruption-point) and C
09:47Manishearthyou can&#39;t reuse the IM because the IM is designed for going towards B, not C
09:47Manishearthyou can&#39;t swap the &quot;to&quot; part of the IM because that will jump the animation
09:47noxFrom where in the spec can I conclude that all this stuff cannot be interpolated without layout info btw?
09:47Manishearthnox: the spec is horrible
09:47Manishearthnox: it&#39;s from the implicit assumption made in the spec where they decide to use numbers for everything
09:47noxThat doesn&#39;t answer my question.
09:48Manisheartheven though (a) homogeneous space doesn&#39;t have the same units for each element and (b) percentages are not numbers
09:48noxtranslate(100px) isn&#39;t a number so now I&#39;m confused.
09:48Manishearthnox: no but the SLERP algorithm in the spec assumes you have numbers only
09:49Manishearthit assumes everything has been converted to matrices
09:49Manishearthwhich use a uniform unit
09:49Manishearththe end of the spec specifies how to convert each thing to a matrix
09:50Manishearthnox: the SLERP algorithm multiplies and divides by matrix elements which you can&#39;t do with percentages
09:50Manishearthwell you can multiply
09:50noxWhich percentages?
09:50noxLet me rephrase,
09:50noxif -moz-transform wasn&#39;t a thing, would we still need these intermediate lists?
09:50Manishearthnox: yes
09:50noxSo how does the spec cope with the lack of layout info?
09:51Manishearthnox: it ignores it and assumes you invent such a scheme
09:51Manishearthnox: -moz-transform does not fundamentally let you do anything more
09:51noxDid anyone ever raise that issue to the spec?
09:51Manishearthnox: -moz-transform lets you specify in matrix() form what you *could* have specified as a transform list
09:51noxManishearth: Ok that makes sense.
09:52Manishearthnox: well the spec also has correctness issuse (https://github.com/w3c/csswg-drafts/issues/483) so when I raised issues I focused on that
09:52crowbotIssue #483: [css-transforms] Bunch of bugs in the interpolation algorithms - https://github.com/w3c/csswg-drafts/issues/483
09:52Manishearthnox: really all CSS specs assume percentages get smoothed out at some point
09:53Manishearthand fundamentally CSS doesn&#39;t make transforms complex to handle, the way engines implement interpolation does
09:53Manishearthactually, no, I&#39;m wrong
09:54Manishearththe spec conflates computed and used value here, and as a result completely misses how we should deal with changes in things that affect the resolution of percentage values
09:54Manishearthe.g. what happens if you animate width and height
09:54noxOk.
09:55Manishearthi recall posing this question at that point
09:55Manishearth*some point
09:55Manishearththe spec has lots of issues and honestly we should clean room rewrite it
09:56noxAnd implement all that stuff in Servo,
09:56Manishearth(a) it doesn&#39;t specify *any* of the conventions used. row-major/column-major? Multiplication order?
09:56noxbecause given none of our own code actually uses the two intermediate values,
09:56noxit&#39;s hard to even know how they are used.
09:56Manishearth(b) It implicitly assumes its dealing with numbers. This has two problems:
09:57noxManishearth: So is there absolutely no way to flatten nested intermediate matrices?
09:57Manishearth (i) it hides the used value / computed value distinction, causing the problem we&#39;re dealing with
09:57Manishearth (ii) it hides the homogeneous space unit/unitless distinction, which means that we don&#39;t actually know what unit the spec is dealing with and this is important. In practice (i.e. browser implementation) IIRC the unit is px.
09:58Manishearth(c) The code is a code dump of pseudocode from Safari
09:58noxStill better than IDB spec.
09:58Manishearth(d) The algorithm in Safari/Webkit is broken and has discontinuities for certain complex transitions. The algorithms in Gecko/Blink are broken in the same way for *different* testcases
09:58noxManishearth: So, can we flatten that stuff or not?
09:59noxAnd if we did, would that require changes on Gecko&#39;s side?
09:59noxOh wait you told me to ask whoever wrote that comment.
09:59Manishearthnox: not as is, but perhaps with some changes to how this all works. idk. talk to boris
09:59noxYeah I&#39;ll find something else to do then.
09:59Manishearthheh
09:59Manishearthnox: once stylo is done pls rewrite the spec
09:59Manishearth:)
09:59noxIf I ever do a spec,
10:00noxit will be the Gecko Quirks Standard,
10:00noxdocumenting all proprietary stuff in Stylo,
10:00noxto shame us into unshipping them.
10:00Manishearthhaha
10:01Manishearthiirc most of it is also in chrome, just unspecced
10:01Manishearthwe need to push it into specs
10:01Manishearthespecially the font stuff
10:01Manishearthi plan to blog about the font stuff at least
10:01noxManishearth: https://html.spec.whatwg.org/#concept-navigator-compatibility-mode
10:01noxManishearth: My spec would start with,
10:01nox&quot;if the navigator compatibility mode is &#39;Gecko&#39;, handle all that stuff kthxbye:&quot;
10:02Manishearthnox: pls multipage
10:02noxWell I had to look up single page,
10:02noxbecause I grepped Gecko to find how that thing is called again...
10:02Manishearthheh
10:14noxManishearth: How can an InterpolateMatrix contain an AccumulateMatrix?
10:14noxManishearth: 302 Boris?
10:17borisnox: If we combine &quot;mismatched transform lists&quot; and &quot;accumulate composite&quot;, an InterpolateMatrix contains an AccumulateMatrix.
10:18noxboris: Could you give me more details?
10:18borisnox: IIRC, for example: http://searchfox.org/mozilla-central/rev/01d27fdd3946f7210da91b18fcccca01d7324fe2/dom/animation/test/crashtests/1335998-1.html#20-23
10:18borisThis is the example
10:18noxWhy must it be nested?
10:19noxboris: Cannot it be the AccumulateMatrix one directly?
10:20noxOh is that because it needs both &#39;progress&#39; and &#39;count&#39;?
10:20noxManishearth: Is that what you meant with an additional parameter?
10:21xidornnox: derive to_css doesn&#39;t check whether it is possible?
10:21noxxidorn: I don&#39;t understand the question.
10:21xidornnox: Either has #[derive(ToCss)], so when I use &quot;type SpecifiedValue = Either<..., ...>&quot;, it tells me that there is conflicting impl
10:22noxxidorn: I&#39;m not sure I understand.
10:22noxWhy would you implement ToCss for an alias of Either?
10:22xidornhowever, one of the variant of the Either doesn&#39;t impl ToCss
10:22noxImplement ToCss for that variant.
10:22xidornthat variant is a Vec
10:22noxWrap it.
10:22noxOr don&#39;t use Either.
10:22Manishearthnox: I meant more than that but Im not sure :)
10:22xidornnox: hmmm
10:22noxxidorn: What&#39;s the property?
10:23xidornstroke-dasharray
10:23noxAnd what&#39;s the non-Vec value?
10:23xidornEither<Vec<LengthOrPercentageOrNumber>, ContextValue>
10:23noxWhat&#39;s ContextValue?
10:23xidornnox: just a new keyword value
10:24nox&#39;context-value&#39;?
10:24xidornyep
10:24noxenum StrokeDashArray {Lengths(Vec<LengthOrPercentageOrNumber>), ContextValue }
10:24noxAlso has the benefit of being smaller than your Either.
10:24xidornnox: that shouldn&#39;t
10:24noxIt shouldn&#39;t, but it is.
10:25xidornit isn&#39;t
10:25nox?
10:26noxxidorn: What isn&#39;t?
10:26noxOh this has been fixed I guess.
10:26xidornnox: it isn&#39;t smaller than either
10:26noxIt used to be.
10:26noxAnyway, I wouldn&#39;t use Either.
10:27xidornnox: https://play.rust-lang.org/?gist=5e277c4bb068cec938da18f639a335a0&version=stable
10:27xidornnox: not using Either means you would need to implement Animatable
10:27noxYeah I just checked. I thought this had not been optimised yet.
10:27xidornand several others
10:27noxxidorn: Several others?
10:27SimonSapinnox: r? https://github.com/servo/servo/pull/17727
10:27crowbotPR #17727: Upgrade to rustc 1.20.0-nightly (ab91c70cc 2017-07-14), use non-&quot;alt&quot; std - https://github.com/servo/servo/pull/17727
10:27noxJust Animatable AFAIK.
10:27noxToComputedValue can be derived.
10:28xidornAnimatable is annoying enough, though
10:28noxxidorn: If you really want to use Either (but it causes more issues than it solves them IMO),
10:28noxthen you either need to wrap the Vec,
10:28noxor to use OneOrMoreSeparated or whatever is its name.
10:28xidornand you need to use a very long qualified name whenever you want to match it
10:28noxWhat?
10:29noxStrokeDasharray::Lengths(_)?
10:29xidornproperties::longhand::stroke_dasharray::computed_value::T
10:29noxxidorn: Just import that?
10:29noxThat issue exists for pretty much all values.
10:30noxAnd that can be solved by using a generic type,
10:30xidornyeah, that&#39;s one more line in every function
10:30noxwhere LengthOrPercentageOrNumber is its own type,
10:30noxand computes to whatever craziness is specific to stroke-dasharray,
10:30noxas I already commented in a PR about that, AFAIK.
10:31xidornanyway, not using Either would just make everything longer
10:31noxOr not.
10:31Manishearthlol stroke-dasharray
10:31noxxidorn: I removed a lot of Either<A, B> uses exactly to simplify things.
10:31xidornnot in this case where you need to impl Animatable...
10:31noxxidorn: Anyway, your base question was whether you can avoid the conflicting impls error: the answer is no.
10:32xidornok
10:32noxAnimatable can be derived in the future.
10:32noxxidorn: I would do enum StrokeDasharray<T> {Specified(Box<[T]>, ContextValue }.
10:32noxenum StrokeDasharray<T> {Specified(Box<[T]>), ContextValue } rather
10:33noxxidorn: You are aware of https://github.com/servo/servo/pull/17704 right?
10:33crowbotPR #17704: Add animation value related with stroke-dasharray / stroke-dashoffset / stroke-width. - https://github.com/servo/servo/pull/17704
10:33xidorn(we probably want to replace Vec<T> with Box<[T]> in values at some point...)
10:33xidornnox: no
10:34noxxidorn: The operative part was about the generic T not the boxed slice.
10:34xidornnox: I know. it just reminds me of that
10:35noxSequence values in general could use a refactoring, I have various ideas on how to improve them.
10:35xidornI tried this for stroke-dashoffset (which isn&#39;t a vec property), but it seems using either simplifies things a lot
10:35xidornI mean, tried doing a generic enum
10:36noxDepends your definition of simplification.
10:36xidornnumber of lines, basically?
10:36noxMore types with more code that will be able to be derived in the future is to me way better than less code that is completely ad-hoc.
10:36noxxidorn: No, that&#39;s not my definite metric.
10:36noxI don&#39;t mind more use statements,
10:36noxI don&#39;t mind more mechanical code that I will be able to derive later.
10:36xidornby using either, that removes half of the lines I added
10:37xidorn(yeah, because mechanical code added by others would just make your contribution looks good)
10:37noxWhat?
10:37xidorns/good/better
10:38xidornbecause you remove more code :)
10:38noxI also added such code in various refactoring I did.
10:38noxFor example my last changes to the transform property.
10:39noxxidorn: So currently the value for stroke-dasharray can only be a list of LoPoN, right?
10:39xidornnox: also there are places you cannot just use an import
10:39noxHow so?
10:39xidornlike in helpers.predefined_type?
10:39noxxidorn: Wouldn&#39;t it be just easier to change predefined_type a bit and just let it take a string for the empty value?
10:39noxCurrently if you say allow_empty, it will parse &#39;none&#39; as an empty vector,
10:39noxcan&#39;t you just make that tweakable and pass &quot;context-value&quot;?
10:40xidornthe initial value, I would need to write something like ::values::generics::SVGLength::Length(Either::First(0.0)) before I use Either
10:40noxNo.
10:40xidornnot for stroke-dasharray, but for stroke-width and dashoffset
10:40noxxidorn: I made a lot of properties use generics and this wasn&#39;t an issue.
10:40xidornso how did you solve that?
10:40noxIn some cases I implemented a method on the generic type,
10:40noxin other cases I implemented From<T> where T is the type of some variant in the enum,
10:41xidorna method... okay. that adds more useless code
10:41noxin that case that would be From<LoPoN>.
10:41xidorna From is probably reasonable
10:41noxMore self-contained code that can be reasoned about, as opposed to more stuff in mako files which produce confusing compile errors.
10:41noxA From is intrinsically more complex to write than a method,
10:41xidornit makes more sense than a random method
10:41xidornto me
10:42noxso I don&#39;t see how you can call a method &quot;more useless code&quot;.
10:42noxWhat&#39;s random is ad-hoc methods on Either<A, B>.
10:42xidorn?
10:42noximpl<T: Parse> Either<Length, T> fn parse_non_negative_length<&#39;i, &#39;t> makes me sad.
10:42noximpl RepeatableListAnimatable for Either<f32, LengthOrPercentage>
10:43xidornwell, yep
10:43noxAnyway,
10:43xidornI wouldn&#39;t add things like them
10:43noxfor context-value,
10:43noxjust tweaking the &#39;empty&#39; keyword value would solve your issue,
10:43noxno?
10:43xidornno
10:43noxWhy?
10:44xidornstroke-{width,dashoffset} are not vec properties
10:44noxBut stroke-dasharray is.
10:44noxThey don&#39;t need to all use the same solution, do they?
10:44xidornnox: for stroke-dasharray, none and context-value are different
10:45noxnone?
10:45xidornso you need to implement things manually anyway
10:45noxNow I&#39;m confused about something else...
10:45xidornwhat do you mean by &#39;empty&#39; keyword?
10:45noxDo we even parse &#39;none&#39; correctly?
10:45noxAs opposed to erroring out and thus using &#39;none&#39; by default?
10:45xidornwe should...
10:45noxallow_empty=False by default,
10:46xidornfor stroke-dasharray it is true
10:46xidornit was true
10:46xidornand we have test for that, and the test doesn&#39;t fail, so yes we parse &#39;none&#39; correctly
10:46noxSeems like it&#39;s not true anymore.
10:46xidornbut if we have an additional keyword, we would need to handle everything manually
10:47noxWhere do you see allow_empty=True for it?
10:47xidornwe don&#39;t?
10:47noxWe don&#39;t.
10:48noxxidorn: I broke it.
10:48noxxidorn: And it didn&#39;t make anything orange.
10:48xidorninteresting...
10:48noxSo that wasn&#39;t tested, or just the difference wasn&#39;t reachable.
10:48xidornwe do have test checking none
10:48noxxidorn: Does it test for serialisation of specified values?
10:48noxxidorn: Because that&#39;s the only observable change from Gecko AFAIK.
10:48noxSerialisation of computed values is still Gecko&#39;s code,
10:49xidornnox: that should do various roundtrip test
10:49noxand erroring out on &#39;none&#39; means we will just use the initial value, which is none.
10:49noxxidorn: The roundtrip shouldn&#39;t fail if we just serialise the empty slice to the empty string,
10:49noxwhich then errors out during parsing,
10:49noxand thus falls back again on the initial value.
10:49xidornnot sure
10:50xidornanyway...
10:50noxAH
10:50noxI remember now.
10:50xidornso your suggestion is to use generic enum rather than Either for all of them?
10:50noxxidorn: I made it such that allow_empty is implied if the initial computed value is None.
10:50xidorneven if that bloats code by 1x?
10:51noxDo whatever you think is best.
10:52xidornnox: in which case is ToComputedValue derivable?
10:52noxUsing generics made me able to derive a lot of ToCss, HasViewportPercentage and ToComputedValue impls.
10:52noxxidorn: Whenever all type parameters implement ToComputedValue,
10:53noxand fields that aren&#39;t parameterised compute as specified.
10:53xidornnox: do I need to put them inside values::{computed,specified}?
10:53noxWhat do you mean?
10:53xidornor I can just put them in the property mako files?
10:53noxOh.
10:53noxI would create new &#39;svg&#39; modules.
10:53xidorndoes it work with SpecifiedValue and computed_value::T?
10:54noxI don&#39;t understand that last question.
10:54xidornI&#39;ve created it
10:54noxxidorn: Deriving ToComputedValue works on code similar as https://github.com/servo/servo/pull/16973/files#diff-a91e60fdcf5c49930d82269c5458520fL30
10:54noxAnother example with an enum: https://github.com/servo/servo/pull/16973/files#diff-1e1cf6f12f9d474d213560ccb319edb9L459
10:55xidornnox: so it has to be in values mod?
10:55noxxidorn: I prefer to remove code from the Mako templates, if that&#39;s what you mean.
10:55xidornrather than a random SpecifiedValue / computed_value::T in properties/longhand/whatever.mako.rs
10:55noxNot sure what you mean by &quot;has to&quot;.
10:55noxxidorn: Well, to derive ToComputedValue, SpecifiedValue and computed_value::T need either to be identical, or to be the same generic type with different type parameters.
10:56xidorn:/
10:56xidornhmmm
10:56xidornok
10:56noxOur current convention for using predefined_type is that you have a type Foo in values::specified,
10:56noxand a type Foo in values::computed.
10:56noxWhen using generics, I create a Foo in values::generics::foo,
10:56noxand then add type aliases to values::{specified,computed}.
10:57xidornwhy couldn&#39;t we just put ad-hoc types in mako...
10:57noxBecause that sucks?
10:57xidornthey wouldn&#39;t ever be used anywhere else...
10:57noxBecause then you have errors on properties.rs line 3463463?
10:57xidornnox: that&#39;s a different problem we need to solve at some point
10:57noxYes, by not putting the code there.
10:58xidornnox: rust should introduce some kind of line indication like C++&#39;s #line
10:58noxLolnope.
10:58noxMy way of doing things will even let us remove computed_value::T in the end.
10:58xidornhow?
10:58noxtype Computed<T> = <T as ToComputedValue>::ComputedValue;
10:58noxComputed<specified::StrokeDasharray>
10:59xidornk
10:59xidornthat probably works
10:59noxAnd given at some point in the future, rustc will allow you to use these in paths
10:59noxyou will be able to say <C<StrokeDashArray>>::ContextValue,
10:59noxor whatever.
10:59xidornalthough I don&#39;t really like duplicating everything in multiple places
10:59noxIt&#39;s not duplicating given they are not the same thing.
11:00xidornyou would have every type in values mod in addition to the mako files?
11:00noxThey wouldn&#39;t be in the mako files?
11:00noxThe only thing the mako files would list is the property -> type mapping.
11:01xidornand many property would need to define its own type?
11:01xidornhmmm
11:01noxThe only actual boilerplate is the type aliases in computed/specified, but that can be fixed later too.
11:03noxxidorn: You just see a different duplication as I do.
11:03noxxidorn: I don&#39;t care about duplicating a couple of type names or type aliases,
11:03noxI care that we have very closely related computed/specified types when they could share the same generic type.
11:05noxxidorn: See for example https://github.com/servo/servo/commit/874885e235d8b4aafa957d461df14f60c7f0d023
11:07noxSee how previously there was two ToCss implementations, and now it&#39;s just derived.
11:07noxSee also how I still have to implement ToComputedValue myself, because the Steps and Frames variants store bare u32s,
11:08noxbut that implementation is boring, and if we improve that u32 situation it will be derivable.
11:08xidornanyway... not going to continue working on this today... need some anime :/
11:08nox(Err, ToCss isn&#39;t derived but there is only one impl still)
11:10noxemilio: Is it really useful that CubicBezier holds a pair of Point2D?
11:10noxemilio: If we made it store 4 values explicitly, we could derive ToCss.
11:12Manishearthemilio: so https://reviewboard-hg.mozilla.org/gecko/rev/986eb54bcebfda7f09674109add57c36595ce2d7#l1.35 made visited styles stop working
11:12ManishearthANy idea why?
11:12Manishearthwhen I add that style context duplication again visited styles work again
11:13noxSeems like this is a pointless use of Point2D, will kill.
11:25noxemilio: How does ascii_case_insensitive_phf_map and match_ignore_ascii_case_whatever_the_name_is_i_dont_remember relate?
11:25noxdo*
11:35SimonSapinnox: the former maps to const expressions that initialize a static item. The latter can be arbitrary code, including `continue`, `return`, etc.
11:36SimonSapinalso, the latter expands to `match`, which likely compiles to a series of `if else`
11:37SimonSapinso O(1) vs O(n)
11:37noxSimonSapin: Ok.
11:38noxSimonSapin: Could we make the latter use phf if given enough patterns?
11:39SimonSapinnox: itd have to detect if all the expressions can be const
11:39Manishearthemilio: there?
11:39noxSimonSapin: Oh right.
11:47cynicaldevilCould somebody share the link which shows the status of servo try jobs?
11:48SimonSapincynicaldevil: not sure if thats what you mean, but I use http://build.servo.org/grid
11:49cynicaldevilSimonSapin: That&#39;s the one. Thanks!
11:50emilioManishearth: yup
11:53Manishearthemilio: nvm i have an alternate solution
11:53noxManishearth: You mean a fake one?
11:53Manishearthnox: !! :D
11:54Manishearthemilio: visited styles broke because we don&#39;t always call GetContext(), so I&#39;m switchng SSC to use the rust-side visited style instead of mStyleIfVisited
11:54Manishearthbut that has a const getter, was wondering how bad a const cast would be
11:54Manishearthbut then I realized that because of the c++ side repr I can just fetch the visited style off it directly
11:54emilioManishearth: why do you need it to be non-const?
11:54emilioManishearth: right, that&#39;s what I expected :)
11:54Manishearthemilio: GetStyleIfVisited has callers which expect it to be non const
11:55Manishearthemilio: however I suspect they don&#39;t actually *need* it to be non const
11:55Manishearthjust that adding const to anything like this takes forever and was wondering if I should actually do that
11:55* Manishearth adds it to the followup list regardless
11:55Manishearthanyway, with this I&#39;m pretty sure GetContext can die in a fire
11:56Manishearthwell, no, we need the SetBodyTextCo