| 13:14 | smaug____ | tries to understand https://github.com/whatwg/dom/pull/121 |
| 13:15 | <smaug____> | is that just the case when both params point to the same node? |
| 13:16 | <Ms2ger> | I think so |
| 13:32 | <smaug____> | nox: ping |
| 13:32 | <nox> | smaug____: Pong. |
| 13:32 | <smaug____> | nox: do you have testcase for 2nd comment in https://github.com/whatwg/dom/pull/121 |
| 13:32 | <smaug____> | trying to understand what it is about |
| 13:33 | <nox> | smaug____: There are already in the file I changed in the PR I linked. |
| 13:33 | <nox> | smaug____: Which case in particular would you like to see? |
| 13:33 | <smaug____> | like which mutationrecords you see in gecko |
| 13:34 | <nox> | For which case? |
| 13:34 | <nox> | Replacing a child by itself? |
| 13:34 | <smaug____> | you say in the comment " queuing 2 records in total." |
| 13:34 | <nox> | (removedNodes = [child]), (removedNodes = [child], addedNodes = [child]). |
| 13:34 | <smaug____> | and I'd like to understand in which case |
| 13:35 | <smaug____> | what is the testcase there |
| 13:35 | <nox> | In the case of replacing a child by itself. |
| 13:35 | <nox> | It's the test case I linked. My test case follows my spec change, so obviously Gecko fails it. |
| 13:35 | <nox> | In the case where child ≠ node, it queues: |
| 13:35 | smaug____ | doesn't know where the link to testcase is |
| 13:35 | <nox> | (removedNodes = [node]), (removedNodes = [child], addedNodes = [node]). |
| 13:36 | <smaug____> | oh, down there is somelink |
| 13:36 | <nox> | smaug____: https://github.com/whatwg/dom/pull/121#issuecomment-160137419 |
| 13:41 | <smaug____> | nox: ok, and you'd like to get rid of the first MutationRecord, right? |
| 13:41 | smaug____ | can see a bug in Gecko there |
| 13:41 | <nox> | smaug____: Yes. |
| 13:41 | <nox> | smaug____: In the case where child = node. |
| 13:42 | <smaug____> | we don't remove anything there, but queue record |
| 13:42 | <nox> | That's not a bug. |
| 13:42 | <smaug____> | hmm |
| 13:42 | <nox> | Actually, what do you mean? |
| 13:42 | <smaug____> | wait, we do remove there |
| 13:42 | <smaug____> | nm |
| 13:42 | <smaug____> | let me read |
| 13:42 | <nox> | Do you mean that no mutation should happen? |
| 13:42 | <nox> | Because that would be wrong too. Cf. ranges. |
| 13:43 | <nox> | parent.replaceChild(child, child) should move any range that starts or ends in child. |
| 13:44 | <smaug____> | nox: I don't mean no mutation should happen |
| 13:44 | <nox> | smaug____: Ok. :) |
| 13:45 | <smaug____> | I'm trying to understand where we get removedNodes in (removedNodes = [child], addedNodes = [node]). |
| 13:45 | <nox> | smaug____: Completely unrelated, could you paste somewhere your PrototypeList.cpp file? You are a Gecko developer, right? |
| 13:45 | <smaug____> | actually, I would totally expect 2 records here |
| 13:45 | <smaug____> | but I don't understand why (removedNodes = [child], addedNodes = [node]). |
| 13:45 | <nox> | Trying to fix something in Servo and I don't want to build Gecko. :P |
| 13:46 | <smaug____> | PrototypeList... |
| 13:46 | <nox> | I don't understand what you mean. Which records do you expect for replacing a child by itself? |
| 13:46 | <smaug____> | just a sec |
| 13:46 | <nox> | (removedNodes = [child]) (addedNones = [child])? |
| 13:47 | <nox> | You remove `node` from its previous parent, and then you replace `child` by node. That's why you end up with a mutation record with both removedNodes and addedNodes. |
| 13:47 | <nox> | In the case where child = node, we either need to avoid the record about removing from the previous parent, or the removedNodes part in the second mutation record. |
| 13:48 | <nox> | And my opinion is that the former is cleaner. |
| 13:48 | <smaug____> | fun, gedit becomes non-responsive after opening PrototypeList.cpp with it |
| 13:48 | <nox> | smaug____: Ah ah. :) |
| 13:48 | <smaug____> | https://pastebin.mozilla.org/8853215 |
| 13:49 | <nox> | smaug____: How come the namespaces are empty? :( |
| 13:49 | <nox> | (prototypes and constructors) |
| 13:50 | <smaug____> | nox: so, first you remove from a parent, so you get (removedNodes = [node]), and then you add, so you get (addedNodes = [node]) - that is what I'd expect at least now |
| 13:50 | <smaug____> | but why gecko gives (removedNodes = [child], addedNodes = [node] there |
| 13:50 | <smaug____> | looking... |
| 13:50 | <nox> | This isn't what I would expect at all. |
| 13:50 | <nox> | It's a replacing. |
| 13:51 | <nox> | smaug____: In the most basic case, replacing a node removes two elements and adds one. |
| 13:51 | <smaug____> | nox: those empty namespaces look like just some codegen artifact |
| 13:51 | <nox> | It must removes the replacing node from its previous parent, |
| 13:51 | <nox> | and it must removes the to-be-replaced child from the new parent of the replacing node. |
| 13:51 | <nox> | The removing of the to-be-replaced child is the removedNodes in the mutation record of the actual replacement. |
| 13:52 | <nox> | The removing of the replacing node is part of `node` being adopted. |
| 13:52 | <smaug____> | first you remove node from parent, you get record 1, (now node is already removed), then you can't remove it again so the next record has just added node |
| 13:52 | <nox> | All replacements end with a (removedNodes = [child] addedNodes = [node]), so that shouldn't be the thing that is changed. |
| 13:52 | <nox> | smaug____: Then that is a replacement that doesn't look like a replacement. |
| 13:53 | <nox> | Hence why I think it would be wrong. |
| 13:53 | <nox> | Case 1: node has no previous parent: (removedNodes = [child], addedNodes = [node]) |
| 13:53 | <nox> | Case 1: node has a previous parent: (removedNodes = [node]) (removedNodes = [child], addedNodes = [node]) |
| 13:53 | <nox> | s/1/2/ |
| 13:53 | <nox> | Case 3: node = child in current Gecko: (removedNodes = [child]) (removedNodes = [child], addedNodes = [child]) |
| 13:54 | <nox> | You suggest changing case 3 to (removedNodes = [child]) (addedNodes = [child]); |
| 13:54 | <nox> | I suggest (removedNodes = [child], addedNodes = [child]) |
| 13:54 | <smaug____> | and I think that is wrong |
| 13:54 | <smaug____> | first you remove from whatever parent |
| 13:54 | <nox> | smaug____: Your suggestion makes case 3 out of the place when comparing to case 1 and 2… |
| 13:54 | <smaug____> | so you need to get (removedNodes = [node]) |
| 13:55 | <smaug____> | and your suggestion makes case 2 out of place |
| 13:55 | <nox> | And it makes the algorithm way more complex than conditionally adopting. |
| 13:55 | <nox> | No it doesn't. |
| 13:55 | <smaug____> | you're missing one record |
| 13:55 | <nox> | No. |
| 13:55 | <nox> | It just makes it like case 1. |
| 13:55 | <smaug____> | which should always happen when something is replaced |
| 13:55 | <smaug____> | but it isn't like case 1 |
| 13:56 | <nox> | When something is replaced, there should always be a mutation record corresponding to a damn replacement. |
| 13:56 | <nox> | In your case 3, it doesn't. |
| 13:56 | <nox> | it looks like remove and insertBefore were called. |
| 13:56 | <smaug____> | node *does have* previous parent |
| 13:56 | <smaug____> | so it is not case 1 |
| 13:56 | <nox> | And it is a replacement, so no mutation record at all with both removedNodes and addedNodes is wrong too. |
| 13:56 | <nox> | And it makes the spec way more verbose than my suggestion. |
| 13:57 | <nox> | "Let nodes be node’s children if node is a DocumentFragment node, and a list containing solely node otherwise." becomes: |
| 13:57 | <nox> | Let nodes be the empty list if node is child, node’s children if node is a DocumentFragment node, and a list containing solely node otherwise. |
| 13:58 | <smaug____> | ( I don't understand why Gecko gives that (removedNodes = [child], addedNodes = [child]), I need to debug) |
| 13:58 | <nox> | It does it because it's a replacement… |
| 13:58 | <nox> | And because the spec says to queue that. |
| 13:59 | <nox> | Err, what I said about step 12 is wrong. |
| 13:59 | <smaug____> | I don't care what the spec says, I care what should happen ;) |
| 14:00 | <nox> | It's step 14 that needs to be changed for what you said. |
| 14:00 | <nox> | smaug____: Sure, but replacing a node should always queue a replacement record, |
| 14:00 | <nox> | a record with just addedNodes isn't, whatever we say. |
| 14:00 | <smaug____> | "replacement record" |
| 14:00 | <nox> | "removedNodes a list solely containing child" would need to become "removedNodes a list solely containing child if child is not node, and the empty list otherwise." |
| 14:00 | <smaug____> | what on earth that is :) |
| 14:00 | <smaug____> | there are just MutationRecords |
| 14:00 | <nox> | smaug____: A record with both removedNodes and addedNodes, obviously. |
| 14:01 | <smaug____> | but let me debug this some |
| 14:01 | <smaug____> | I need to understand why gecko does what it does now |
| 14:04 | <nox> | And with 2 mutation records, I will need to invalidate childNodes' cache more frequently in Servo. |
| 14:05 | <nox> | A mutation record with (removedNodes = [node], addedNodes = [node]) tells me that the list of children didn't change size, so my cache is still correct. |
| 14:09 | <smaug____> | nox: so in Gecko I'm getting the records I expect |
| 14:09 | <smaug____> | 2 records |
| 14:09 | <smaug____> | first one has the removal, 2nd one has the added node |
| 14:09 | <nox> | And why would that be more correct than what I said? |
| 14:09 | <smaug____> | nox: you use mutation records to invalidate childNodes cache? |
| 14:09 | <nox> | smaug____: Their machinery yes. |
| 14:10 | <nox> | That's written in the ticket, it would be nice if I didn't have to repeat myself. :) |
| 14:10 | <smaug____> | it is rather expected that replaceChild first removes the replacing node from its parent |
| 14:10 | <nox> | Mutation records in Servo will be queued somewhere in a children_changed method. That method is too used to update childNodes cache. |
| 14:10 | <smaug____> | so, you get one record for that |
| 14:11 | <nox> | It is rather expected that replaceChild will queue a record with both removedNodes and addedNodes. |
| 14:11 | <smaug____> | and then you create a record for the case when replacing node is added to context node |
| 14:11 | <nox> | What's wrong about what I'm saying? |
| 14:11 | <smaug____> | and whether or not something removed is just a side thing |
| 14:11 | <nox> | "replacement", i.e. something is removed and something is added. |
| 14:11 | <smaug____> | you somehow special case child == node |
| 14:12 | <nox> | You do too. |
| 14:12 | <smaug____> | not really |
| 14:12 | <annevk> | I think either way we'll have to special case that |
| 14:12 | <nox> | Yes you do. That case must be discriminated to not include child in removedNodes. |
| 14:12 | <nox> | Cf. step 14. |
| 14:12 | <smaug____> | if nothing is removed in the second phase, you don't get anything in the removedNodes |
| 14:13 | <nox> | And how do you initialise removedNodes? |
| 14:13 | <nox> | By saying it should be empty if child = node, |
| 14:13 | <nox> | that's discriminating. |
| 14:14 | <smaug____> | initialize removedNodes? it is empty list unless you've removed something from a node when the record is created |
| 14:15 | <nox> | That makes no sense. You are talking about this from the POV of implementation details of Gecko. |
| 14:15 | <smaug____> | I see Gecko's behavior, which is (removedNodes = [child], addedNodes = [child]), not (removedNodes = [child]), (removedNodes = [child], addedNodes = [child]) rather good one |
| 14:15 | <nox> | How do you *specify* what you are saying, if not by special-casing child = node? |
| 14:15 | <smaug____> | nox: I'm talking about from the point of view of how I see mutations in DOM should work ;) |
| 14:16 | <nox> | Me too. |
| 14:16 | <nox> | How do you specify it? |
| 14:16 | <smaug____> | and from a point of view of the MutationObserver API designer |
| 14:16 | <nox> | How do you write the prose that describe what *you* want? |
| 14:16 | <nox> | Me too. |
| 14:16 | <nox> | You say you aren't special-casing child == node, explain what would be the prose to do what you say without special-casing it. |
| 14:18 | <annevk> | Note also that we are special casing this case already elsewhere, in pre-insert |
| 14:18 | <nox> | annevk: Yeah, and insertion is not about removing something and putting something instead. |
| 14:19 | <nox> | From the POV of a MutationObserver API designer, I would expect the mutation records to strive to describe the actual mutations taking place, |
| 14:19 | <smaug____> | there is still special case sure, but different place. and I say removing the first "replacing node is removed from parent" in some case would be weirder that having just the case later that if there isn't anything to remove anymore, don't add anything to removedNodes |
| 14:19 | <nox> | that's why we have the suppress observers flag and whatnot, |
| 14:19 | <nox> | making two records is to me like not having the suppress observers flag, and queueing mutation records for each removal and insertion instead of in bulk. |
| 14:20 | <nox> | smaug____: And that's not special-casing? |
| 14:20 | <nox> | How is changing the removedNodes property of the second record not as special-casing as not queuing the first one? |
| 14:20 | <annevk> | smaug____: it seems your solution requires changes to step 11 and step 14, whereas nox' solution requires only a change to step 10 and creates cleaner records... |
| 14:21 | <smaug____> | I'm looking if my solution needs any changes |
| 14:22 | <smaug____> | step 11 wouldn't just do anything, and I'm not sure what it does here... reading |
| 14:22 | <nox> | To the spec? Of course it does, that's what I've been saying for 15 minutes. |
| 14:22 | <annevk> | smaug____: it segfaults |
| 14:22 | <nox> | Removing a node from its parent needs a parent. `node` doesn't have a parent anymore. |
| 14:22 | <nox> | That's why the spec is wrong. |
| 14:22 | <smaug____> | ah, 14 would need change |
| 14:23 | <nox> | And 11. |
| 14:23 | <smaug____> | yes |
| 14:23 | <nox> | Mine needs a change in step 10 and makes better records. |
| 14:23 | <smaug____> | nox' solution creates unexpected records, since it is missing the first one |
| 14:23 | <nox> | Expected records* |
| 14:24 | <smaug____> | unexpected :) |
| 14:24 | <nox> | Yours are unexpected, because the second one doesn't describe a replacement. |
| 14:24 | <smaug____> | because it is missing the first one |
| 14:24 | <nox> | See my test. |
| 14:24 | <nox> | Find a replaceChild that produces a record without a removedNodes property. |
| 14:24 | <nox> | Hint: there is none. |
| 14:24 | <annevk> | smaug____: not describing the replacement seems at least equally weird |
| 14:24 | <annevk> | or unexpected |
| 14:25 | <smaug____> | I'm trying to see why what is unexpected |
| 14:25 | <nox> | https://github.com/nox/web-platform-tests/blob/replacechild-mutation-records/dom/nodes/MutationObserver-childList.html#L258-L296 |
| 14:25 | <annevk> | smaug____: because in all cases, except for this one in Gecko, invoking replaceChild ends with a record that describes what node got removed and what it got replaced with |
| 14:26 | <nox> | {type: "childList", removedNodes: […], addedNodes: […]} |
| 14:28 | <smaug____> | can't see the light here |
| 14:29 | <nox> | Do you understand what annevk said? |
| 14:29 | <nox> | It's not rocket science. |
| 14:29 | <nox> | Do you not see the common element to all the tests I linked? |
| 14:32 | <smaug____> | 53 is missing one record, that is what I see |
| 14:32 | <nox> | Are you kidding me? |
| 14:32 | <smaug____> | I'm trying to understand reasoning for this proposed change |
| 14:32 | <smaug____> | we need a fix to the spec sure |
| 14:33 | <nox> | Oh, n51 is the same test I see. |
| 14:34 | <smaug____> | one of the initial steps in replaceChild has always been that the node is removed from its parent |
| 14:34 | <nox> | Ah no, d51 doesn't exist. Different test. |
| 14:34 | <smaug____> | so I'm trying to understand why that step should now change |
| 14:34 | <nox> | smaug____: One of the final steps in replaceChild has always been that the mutation record has always a removedNodes. |
| 14:34 | <smaug____> | no |
| 14:34 | <nox> | What no? |
| 14:35 | <smaug____> | MutationObservers are a new thing |
| 14:35 | <nox> | Ok. |
| 14:35 | <smaug____> | added to DOM recently |
| 14:35 | <nox> | Let me reformulate. |
| 14:35 | <nox> | One of the initial steps in replaceChild has not been that the node is removed from its parent, |
| 14:35 | <nox> | since WebKit always short-circuited everything in case child = node. |
| 14:36 | <smaug____> | that is implementation detail |
| 14:36 | <smaug____> | I was talking about spec here |
| 14:36 | <nox> | And if the node wasn't in a parent, it isn't removed from anywhere, so what you are saying is wrong too. |
| 14:37 | <nox> | About spec, mutation records of replacement operations always include a removedNodes entry. |
| 14:37 | <nox> | That's right there in the spec, isn't it? |
| 14:37 | <nox> | Replacing a child by a node with no parent: removedNodes present; |
| 14:37 | <nox> | replacing a child by nothing: removedNodes present; |
| 14:37 | <nox> | replacing a child by another node in the document: removedNodes present; |
| 14:37 | <nox> | replacing a child by another node in another document: removedNodes present; |
| 14:38 | <nox> | replacing a child by itself: you want no removedNodes present, I want one. |
| 14:38 | <smaug____> | .innerHTML creates "replacement record", and may not contain anything in removedNodes |
| 14:38 | <smaug____> | same with .textContent |
| 14:38 | <nox> | That's because it's akin to "replace all". |
| 14:38 | <nox> | That's not "replacing a single thing by something else". |
| 14:38 | <nox> | replaceWith does like I just described. |
| 14:39 | <caitp> | would there be any harm done if implementations which short circuited the actual replacement, still queued up the mutation records? |
| 14:39 | <nox> | caitp: Yes. |
| 14:39 | <nox> | caitp: Ranges not being updated. |
| 14:42 | <caitp> | in webkit, they could probably get all the side effects with Node::didReplace() or whatever it's called |
| 14:42 | <smaug____> | (anecdote, even DOM 1 spec from 1997 says "If the newChild is already in the tree, it is first removed. ") |
| 14:44 | <nox> | You can't argue that mutation observers are new, |
| 14:44 | <nox> | and then argue about how the newChild must be removed from its parent in 1997. |
| 14:44 | <nox> | Even with my suggestion, newChild is still removed from its parent. |
| 14:44 | <nox> | But it does so with observer suppressed. |
| 14:45 | <nox> | Whereas you want to keep removing it during adoption (which doesn't look mentioned in 1997), with the observers unsuppressed. |
| 14:49 | smaug____ | doesn't understand how the proposal would improve anything but thinking ... trying to understand |
| 14:50 | <annevk> | smaug____: well for one it'd keep the spec simpler |
| 14:50 | <nox> | And would queue less mutation records. |
| 14:52 | <smaug____> | and would change the behavior of shipping produces |
| 14:52 | <smaug____> | products |
| 14:52 | <smaug____> | nox: what does Edge do? |
| 14:53 | <nox> | I'm on a Mac. |
| 14:54 | <nox> | smaug____: I'm pretty sure Gecko has another bug anyway… |
| 14:55 | <nox> | http://mxr.mozilla.org/mozilla-central/source/dom/base/nsINode.cpp#1970 This, what is it? |
| 14:56 | <nox> | Isn't that about the child's next sibling being node? |
| 14:57 | <smaug____> | no |
| 14:57 | <smaug____> | that is the child |
| 14:58 | <nox> | Why could an error be thrown at that point? |
| 14:58 | <smaug____> | mostly because "this is mutation events, ensure we're some sane-ish state still" |
| 14:59 | <caitp> | "I'm pretty sure Gecko/AnyOtherEngine has another bug anyway" pretty safe bet =) |
| 14:59 | <smaug____> | well, mutation events aren't spec'ed |
| 14:59 | <nox> | Wha |
| 14:59 | <smaug____> | ++caitp |
| 15:00 | <gsnedder1> | nox: you do "something vaguely like this" |
| 15:01 | <nox> | gsnedder1: My patch is pristine clear. =) |
| 15:01 | <annevk> | I still hold some hopes we don't have to define mutation events |
| 15:01 | <annevk> | But... I'm getting close to the point of just trying to figure them out and get it over with |
| 15:01 | <gsnedders> | annevk: I think that's a vain hope at this stage :( |
| 15:01 | <annevk> | Although maybe if Servo gets away with sanity and prevails |
| 15:02 | <nox> | Justice shall prevail. |
| 15:02 | <gsnedders> | May the odds be forever in your favour. |
| 15:02 | <smaug____> | so in Gecko the current behavior comes rather naturally from the implementation, I admit. Since MutationObserver implementation uses internal nsIMutationObserver to get notifications about actual removals and additions, if some removal just doesn't happen, removedNodes will be empty |
| 15:03 | <nox> | smaug____: In Servo too. :) |
| 15:03 | <smaug____> | so it is like merging 14 to 11 |
| 15:04 | <smaug____> | partially |
| 15:04 | <nox> | smaug____: Following my DOM PR: https://github.com/nox/servo/blob/c63e8e62454a0ef2d1006c00a19d7aa62ba0777d/components/script/dom/node.rs#L2050-L2054 |
| 15:05 | <smaug____> | nox: and you could have that same null check around Node::remove(child, self, SuppressObserver::Suppressed); |
| 15:05 | <smaug____> | and get Gecko's behavior |
| 15:06 | <nox> | smaug____: No. I would then have to change step 14. |
| 15:06 | <smaug____> | ok, so Servo has quite different setup then |
| 15:06 | <nox> | Specifically, I would need to call either ChildrenMutation::replace or ChildrenMutation::insert. |
| 15:07 | <smaug____> | in Gecko it is step 11 effectively which tells whether mutation record has something in removedNodes |
| 15:07 | <smaug____> | since it nothing is removed, nothing will be added to removed nodes |
| 15:07 | <nox> | smaug____: In Servo, mutation records are built explicitly. |
| 15:07 | <nox> | s/are/will be/ |
| 15:07 | <nox> | From a ChildrenMutation struct, which we build in each method that queues a mutation record. |
| 15:08 | <nox> | That ChildrenMutation thing is what I use to update childNodes. |
| 15:08 | <nox> | https://github.com/nox/servo/blob/c63e8e62454a0ef2d1006c00a19d7aa62ba0777d/components/script/dom/nodelist.rs#L240-L267 |
| 15:09 | <nox> | With your suggestion, calling replaceChild will mean we reach https://github.com/nox/servo/blob/c63e8e62454a0ef2d1006c00a19d7aa62ba0777d/components/script/dom/nodelist.rs#L233 way more frequently. |
| 15:09 | <nox> | With mine, never. |
| 15:09 | <nox> | With mine, only the case where we replace a child by nothing* |
| 15:14 | <smaug____> | in Gecko the latter record gets both removed/addedNodes because of http://mxr.mozilla.org/mozilla-central/source/dom/base/nsINode.cpp?mark=2168-2168,2189-2189#2168 and it nsAutoMutationBatch isn't ever initialized, you just get a normal record with addedNodes |
| 15:14 | <smaug____> | s/it/if/ |
| 15:15 | <nox> | Someone should try my test on Edge. |
| 15:15 | <smaug____> | when no batching happens, all mutationrecord handling is based on actual insertions or removals |
| 15:18 | <nox> | I find the setup in Servo better because it follows closely the spec. |
| 15:20 | <smaug____> | oh, sure from readability point of view probably much better. Gecko just was there well before we had good DOM spec ;) |
| 15:20 | <smaug____> | and, nsIMutationObserver which we use for this too is super handy |
| 15:21 | <smaug____> | nsIMutationObserver has very little to do with MutationObserver, way lower level, C++ only notifications |
| 15:21 | <smaug____> | (nsIMutationObserver is used also to implement Range etc) |
| 15:22 | <nox> | smaug____: children_changed is Servo's nsIMutationObserver. |
| 15:23 | <smaug____> | sounds like it is a tad higher level thing |
| 15:23 | <nox> | For childList records that is. |
| 15:24 | <nox> | It is used for Rust-only things I mean. |
| 15:24 | <nox> | And will be usable for actual MutationRecord things. |
| 15:29 | <smaug____> | lunch |
| 15:30 | <Ms2ger> | 5:30pm? |
| 15:30 | <nox> | Ms2ger: Actually just went outside to buy a sandwich too. |
| 15:31 | <nox> | But I woke up at 11:30. |
| 15:41 | <nox> | :'( |
| 15:42 | <nox> | https://github.com/whatwg/dom/pull/121#issuecomment-160163345 |
| 17:01 | <smaug____> | 1) We should aim for consistency as long as possible. So if some step in an algorithm can be executed, it should be (by default) (step 10), and aim for adding special cases to steps which just can't be executed (step 11) |
| 17:02 | <smaug____> | 2) with your proposal we end up creating such mutation records which no other mutation creates, same node in removed and in added nodes |
| 17:02 | <smaug____> | nox: aha, even more reasons for Gecko's behavior :) |
| 17:02 | <nox> | Yeah. |
| 17:38 | <annevk> | Well glad that got sorted |
| 17:38 | <annevk> | I guess now we need to figure out what the steps actually need to say |
| 17:39 | <annevk> | And we should add some more tests |
| 17:45 | <nox> | annevk: Will do. |
| 17:46 | <smaug____> | annevk: just to verify, getAttributeNames is supposed to return attributes in the same order as .attributes, right? |
| 17:46 | <gsnedders> | We should probably try and make sure we have tests for every spec change going forward, somehow. |
| 19:27 | <smaug____> | annevk: FYI, I'm adding some getAttributeNames tests in a Gecko bug |
| 19:27 | <smaug____> | wpt tests |
| 19:54 | <annevk> | smaug____: yes and nice! |
| 19:54 | <annevk> | gsnedders: some kind of specification dashboard from which you can file bugs for commits would be great |
| 19:54 | <annevk> | gsnedders: so you can file bugs against browsers and wpt |
| 19:57 | <gsnedders> | annevk: like, updating an impl to the current spec from something a few years ago where the spec hasn't changed much is so much easier if there's tests for it rather than looking through the *whole* spec and impl |
| 19:57 | <gsnedders> | so we really need tests for everything |
| 19:57 | <gsnedders> | and people to write them |
| 19:57 | <gsnedders> | which is really the problem :P |
| 19:57 | <gsnedders> | TabAtkins: yt? |
| 20:01 | <astearns> | and people to review them |
| 20:02 | <gsnedders> | astearns: wpt has almost no problems getting test review |
| 20:03 | <gsnedders> | astearns: that's a csswg-test problem, pretty much |
| 20:03 | <astearns> | yep |
| 20:03 | <gsnedders> | astearns: really you guys just need to relax the requirements as to who can review |
| 20:03 | <gsnedders> | But there's not really much point in me rambling about this again :) |
| 20:03 | <astearns> | afaik, we have - we should be allowing anyone to review |
| 20:04 | <astearns> | if that's not the case, I'll push at it again |
| 20:05 | <gsnedders> | I have no idea. |
| 20:05 | <gsnedders> | There's no good written documentation saying what the policies are nowadays, and what there is is so out of date. |
| 20:07 | <gsnedders> | astearns: really I think a lot of the problem is half the time when there's any discussion almost nobody responds, and it's only the people who object respond |
| 20:07 | <gsnedders> | astearns: probably just need to take some large proposal of "here's what we want to do" to the whole group and see if there's consensus |
| 20:07 | <astearns> | gsnedders: on the mailing list, you mean? |
| 20:08 | <gsnedders> | astearns: yeah |
| 20:08 | <gsnedders> | I think there's much more explicit implicit agreement at F2Fs :) |
| 20:09 | <gsnedders> | (I presume I won't be showing up in Sydney, given costs and not having any funding in place for next year presently) |
| 20:09 | <astearns> | from people who don't subscribe to the testing mailing list, and so don't contribute to the discussions there :) |
| 20:09 | <gsnedders> | (FWIW) |
| 20:09 | <smaug____> | gsnedders: yet, even if wpt gets reviews, one should not rely on them in case implementing something you get test failures :) (I just fixed a broken MutationObserver test yesterday) |
| 20:09 | <gsnedders> | smaug____: oh, totally agreed |
| 20:09 | <smaug____> | but sure, usually things go well |
| 20:10 | <gsnedders> | smaug____: but even with far higher requirements for who can review you still end up with bad tests slipping through (probably fewer though), but then you also get far fewer tests reviewed too… |
| 20:11 | <smaug____> | very true. need some balance, yet mostly we need just more tests |
| 20:11 | <gsnedders> | my basic conclusion is we just need to swing policies in favour of more tests, because bad tests will be found when people debug failures |
| 20:11 | <gsnedders> | (though that does nothing for tests that bogusly pass) |
| 20:12 | <gsnedders> | and really the choice is between getting the major of tests vendors write v. very, very few |
| 20:12 | <gsnedders> | no policy will lead to any middle ground, AFAICT |
| 20:12 | <smaug____> | it would be great if we could get help from web devs to write tests, so that not only browser impls write them. It should be also interests for web devs to ensure browsers don't regress behavior |
| 20:13 | <gsnedders> | smaug____: FWIW, I know some major web companies have expressed interest in paying people to work on testing browsers (because long-term it's in there interest), but thus far nothing really has come of it |
| 20:13 | <astearns> | I'm going to be talking next week to a room full of web devs on that very topic - becoming responsible for the tech we use (writing tests, reporting bugs, etc.) |
| 20:14 | <gsnedders> | smaug____: the big problem with tests from TTWF and similar is that the devs will open PRs with what they've written, but then will never get back to respond to any sort of review comments |
| 20:14 | <gsnedders> | and quite frankly plenty of the tests are really poor |
| 20:14 | <gsnedders> | I think a lot of that is just the fact the majority of people are terrible at any sort of QA |
| 20:14 | <smaug____> | does that hint we need better documentation how to write good tests |
| 20:15 | <gsnedders> | smaug____: I think the issue is deeper than that: people don't know *how* to even start writing a test for a feature (not a web feature, but *any* feature) |
| 20:16 | <gsnedders> | like what we get is consistent with what I see in plenty of places: devs are simply terrible at testing |
| 20:16 | <smaug____> | hmm, I don't btw know how to run wpt tests if I'm not using mach from mozilla-central |
| 20:16 | <smaug____> | but like how to run wpt in other browsers |
| 20:17 | <smaug____> | looks like https://github.com/w3c/web-platform-tests/ explains |
| 20:17 | <gsnedders> | not really how to run all the tests in an automated manner |
| 20:17 | <gsnedders> | but how many people actually need to do that? |
| 20:17 | <smaug____> | jgraham_ et al have just make running tests so easy with gecko |
| 20:17 | <gsnedders> | like what matters is being able to run one test |
| 20:17 | <smaug____> | s/make/made/ |
| 20:17 | <gsnedders> | smaug____: fwiw, wptrunner works with more than just Gecko and Servo |
| 20:18 | <gsnedders> | at least in principle, it should be cross-browser, without too much setup code per browser |
| 20:18 | <gsnedders> | the harder thing is running reftests in some user-friendly way, IMO |
| 20:23 | <gsnedders> | astearns: do you have any views on how to clarify policies about testsuite? just bring it up on www-style? |
| 20:23 | <gsnedders> | (and telecons?) |
| 20:24 | <smaug____> | gsnedders: but do other vendors yet run wpt automatically? |
| 20:24 | <smaug____> | I thought blink has plans, but not doing it yet |
| 20:24 | <gsnedders> | smaug____: Edge do |
| 20:24 | <smaug____> | ah, great |
| 20:25 | <astearns> | telecons and ftf would probably be more effective than the mailing list(s) (as you noted, only the people who are comfortable with the status quo respond on the lists) |
| 20:25 | <astearns> | I'm happy to spend telcon time talking about testing |
| 20:25 | <gsnedders> | we should probably do this once I formally back in the group :) |
| 20:26 | <gsnedders> | smaug____: I think within six months we really hopefully can be at a point where everyone is running wpt+csswg-test. idk quite how realistic that is for Blink. |
| 20:26 | <astearns> | yep yep (and/or before the formalities are done, depending on how long that takes) |
| 20:27 | <astearns> | brb |
| 20:27 | <gsnedders> | as I said, I won't be at the next F2F (and idk really until next TPAC, given travel costs, depending on what happens about funding) |
| 20:27 | <gsnedders> | (I mean I /can/ self-fund, but long-haul travel starts having real effects on income fast) |
| 20:36 | <astearns> | gsnedders: where are you based? |
| 20:37 | <gsnedders> | astearns: Scotland |
| 20:39 | <astearns> | I think good progress can be made if we spend some time talking on the weekly calls about tests |
| 20:40 | <astearns> | prod people about the review queue, get some testing voids identified to spur test writing |
| 20:41 | <gsnedders> | I think stuff would be easier if we just viewed the tests in the repo as de-facto reviewed (by virtue of being run by some), and just kept on top of new tests |
| 20:42 | <gsnedders> | (I also don't think finding test voids really helps *that* much, because browser vendors will mostly write tests when it's in their interest and not much otherwise) |
| 20:42 | <astearns> | the review queue I'm concerned about is pending pull requests. I don't much care about the review "status" of tests already in the repo |
| 20:45 | <gsnedders> | I'll try and write something up as to what there's mostly consensus on (mostly those who've spoken except for Gérard) to www-style for the sake of somewhere to *start* discussions; I'll probably throw in a few things that I feel strongly about too :) |
| 20:48 | <gsnedders> | there wasn't anything more than "west coast US" said for the May F2F was there? |
| 21:01 | <astearns> | gsnedders: nothing more definite yet for May. I don't expect we'll have a destination until after the new year |
| 23:54 | <paxcoder> | Hello. annevk: Why don't you write something on your blog? Are you too busy? |