mozilla :: #devtools

14 Jul 2017
09:30soleSo I took this bug to replace sdk/core/heritage with plain ES6 class & extend. https://bugzilla.mozilla.org/show_bug.cgi?id=1378849
09:30firebotBug 1378849 NEW, sole@mozilla.com Stop using sdk/core/heritage in DevTools Web Audio Editor
09:31solebut I'm seeing a strange thing as the instances of the models seem to have the EventTarget methods but *NOT* their own methods
09:32solehttps://gist.github.com/sole/587db829cbf802842c37d51bfe10b584#file-models-js-L172
09:32soleWhen I open the Web Audio panel I get TypeError: gAudioNodes.get is not a function: _onNodeSet@chrome://devtools/content/webaudioeditor/views/properties.js:124:24
09:33solegAudioNodes is an instance of AudioNodesCollection
09:34soleI did a hacky thing and enumerated the methods of the prototype with dump(JSON.stringify(Object.getOwnPropertyNames(Object.getPrototypeOf(gAudioNodes)))); and it indeed just showed me the methods that came from the EventTarget
09:34soleMy only remaining guess is maybe this is because of ... XUL? is it maybe a different context?
09:35soleThe Web Audio editor uses XUL to include all the stuff, so I do not know if the rules for JS are 'different' in XUL-land
09:35solezer0: pbro ^ any other XUL expert in the room
09:36zer0sole: I'm in the middle of something, I'll take a better look after; but in the meantime I suggest you to don't use `getOwnPropertyNames` but `getOwnPropertyDescriptors`
09:37solewelp
09:37soleI used that to try to debug
09:37zer0that's because in the first case you won't get the fields that aren't enumerable
09:37soleI want the methods to be there to start with!
09:37zer0yes
09:37zer0but if the method are, for instance, classes methods,
09:37soleIf I dump (gAudioNodes.get) I get undefined
09:37zer0you won't see that in your "debug"
09:37soleNo I'm not seeing them anywhere
09:37soleto start with they are undefined
09:38soleif they weren't I would not need to print them - that is not my problem :P
09:38zer0sole, I understand, I'm just saying that the way you tried to debug won't work anyway, so it's not useful for debugging.
09:38soleok
09:39solezer0: do you know anything about XUL being a different context or something weird like that?
09:39soleWould things behave differently? I tried looking at patches Honza had submitted but his were simpler fixes
09:39zer0sole, to be clear, if I have: `class Foo { mymethod() {} }` you won't see "mymethod" in your debug code.
09:40solebut if I did var bla = new Foo() I would expect to be able to do bla.mymethod() and it to work
09:40sole^^^ that is my issue
09:40zer0I don't know anything about that sorry. I've to double check the code. Give me five minutes, I've to finish something
09:40solethat it is not there
09:40zer0yes, I understand.
09:40solesure! I find it annoying when engineers fixate on something that is not the relevant issue
09:42zer0sole, I wanted just to provide the proper tool to debug properly, now and in the future. :)
09:43soleI just tried that too, shows the same methods but with more data, still not the methods I want
09:44zer0sole: it's the only method you have in JS to have what you're looking for. Unless you're using `forin` that works too, but it give also the methods from the prototype.
09:44zer0anyway, the snippet you posted is not complete right?
09:44soleWhat else do you need? It's just the file I modified
09:44solemodels.js contains the class definitions
09:45soleit's this http://searchfox.org/mozilla-central/source/devtools/client/webaudioeditor/models.js
09:45sole(But modified to not use the sdk's Class)
09:46zer0weird, how this file is loaded? It uses the `EventTarget` but it's not defined.
09:47solelol that was me yesterday "how does this even work"?
09:47soleWell it somehow loads using XUUUUL
09:47solehttp://searchfox.org/mozilla-central/source/devtools/client/webaudioeditor/webaudioeditor.xul
09:47soleApparently the files are listed here? And magically things happen?
09:48zer0EventTarget it would be just part of the global scope.
09:48zer0It's like a web page.
09:48soleYes, but EventTarget is working beautifully, and so is extending it
09:48soleso odd, this 8-|
09:48zer0We should avoid that. Just saying, not part of the problem. Makes just harder to follow the code.
09:49zer0So, the original code works, and if you debug the original code the methods are there?
09:49soleI found the panel quite hard to understand because of that, but I tried to ignore that for now
09:49zer0Or it's already broken?
09:49soleuh, I didn't debug the original code because it worked, do you mean you want me to list the methods?!
09:49zer0Yeah.
09:49soleNo it isn't broken
09:49soleI started with something that worked :P
09:50zer0OK. Just debug the original code to check the main differences.
09:50sole?
09:50soleWhat do you want me to do?
09:50sole&quot;just&quot; &quot;debug&quot; &quot;main differences&quot; <- I don&#39;t understand any of that
09:51zer0sole: What I&#39;m saying is: list the methods in the same way you do in your patch
09:51zer0but in the original code. You&#39;ll probably see all the methods defined. Check also the constructor.
09:51soleWhat would that help us with?
09:51zer0In the meantime I&#39;ll take a look to the code.
09:51soleWe&#39;re using the Class() thing in the original code
09:52soleWhich is a library that simulates classes, not the ES6 version
09:52soleThe methods are not failing in the original version because it works... so yes, they will be there
09:53soleI don&#39;t think that&#39;s a useful path, sorry :-/
09:53zer0sole, I have an educated guess about what&#39;s happening.
09:53zer0sole: I know the methods are there.
09:54zer0sole: I want just to dump the whole objects.
09:54zer0clearly the two objects we got (the original, that it works, and the one with your patch) are different.
09:54solecan you share with me your educated guess? :)
09:54zer0can you dump for me the original object? :P Otherwise I can do by myself but you have to wait a bit.
09:57soleI&#39;m trying something else... hold on
09:59zer0np.
10:09soleI tried writing this class B that extends class A. And... class B doesn&#39;t seem to have access to class A methods: https://codepen.io/yetanotheraccount/pen/yXwaGK?editors=1111#
10:10soleA defines oneMethod and B extends A, yet B.oneMethod is undefined
10:10soleso it shouldn&#39;t be a XUL issue as this happens on normal websites. It rather is an issue with my understanding of ES6 classes... but this is very baffling anyway
10:11* sole googles
10:12zer0No,definitely is not a XUL issue.
10:12soleI understand there are no &#39;private&#39; or &#39;public&#39; methods in ES6, so why are things not being inherited?
10:13zer0There is nothing different from that.
10:13zer0So I dumped the original object and the methods are not there either.
10:14zer0sole, let me check your code.
10:15zer0Your pen code doesn&#39;t work since you&#39;re trying to access to a instance method as a static one.
10:16soleYeah I just realised that
10:16soleI came to say &quot;AHHH&quot;
10:16soleBUT
10:16solelet me see what happens if I run this on the other context...
10:18soleWell! this does work
10:18soleB has the method from A
10:19solewait that wasn&#39;t my issue, does it have its own method?
10:19* sole getting distracted
10:19zer0eh eh eh
10:22soleWell so the method from A is in the instance of B which is good... JS seems to be unaffected by the XUL environment
10:22soleSo my other guess is that when you extend a native class weird things happen? I might try that but after lunch
10:22soleBRB
10:25solecouldn&#39;t resist a quick codepen... extending EventTarget looks to be something the browser doesn&#39;t like 8-)
10:26solehttps://codepen.io/yetanotheraccount/pen/mwoOJO --> TypeError: Illegal constructor.
10:26soleMore later!
10:35zer0sole, that&#39;s because another reason; EventTarget is also the name of a Web API.
10:35zer0But in the webaudioeditor it should be the SDK&#39;s EventTarget (otherwise it won&#39;t work at all also the original code).
10:36zer0(That also I didn&#39;t like to have that in the global scope)
10:36zer0*also why
10:45zer0sole, my educated guess is that the `Class` magic is doing some weird stuff on adding the instance method, so when you call `super()` but you have to call it, otherwise you cannot access to `this` the subclasses too have issues.
10:46zer0It&#39;s probably not clear, but the point here is that `EventTarget` is still using `Class`. You can either refactor that, or just wait until we&#39;re finishing with the event emitter (since is going to be remove anyway, this `EventTarget`).
10:46zer0That&#39;s also why I said in &quot;the plan&quot; to give a lower priority to webaudioeditor if the things are complicated; I expected that to be (since it was made by Jordan, and it uses a lot SDK code and paradigms)
11:37solemmm, right, that would maybe explain why it does inherit the methods - they&#39;re quite possibly the ones from the SDK&#39;s eventemitter
11:39solezer0: I started with the web audio editor because out of all the bugs, it&#39;s the only one whose code I had ever looked at before :)
11:42zer0sole: but it also the most complicated one, and it should be handled when all the refactored code is done, IMVHO.
11:42soleGood stuff on the email from myk too, I didn&#39;t remember that you could do a try push using mach, that&#39;s coollll
11:42zer0basically once we&#39;ve completed the events and the basic classes, we should be able to do the webaudioeditor too.
11:43soleyep on that too, as I was looking at the code I was seeing plenty of event code
11:43soleWell if you look at the hardest thing first, then everything else seems EASY!
11:43zer0:)
11:44zer0yeah the email from myk was great; so we can push to try as I thought since it uses git-cinnabar. I think the issue you get about the SHA was with gecko-dev.
11:45zer0I have to work on some inspector stuff today, but as soon as I finished I&#39;m going to set up my new workfow!
11:45zer0*workflow
11:48soleYep I believe I attempted to add gecko-dev as a remote and cinnabar didn&#39;t like it, but gecko should be fine as it&#39;s a cinnabar-originated clone. niiice
11:48solemyk: you&#39;re the best!
11:54soleoh I had already tried to add my mozilla/gecko fork as a remote
11:55soleI wonder if the issue was I tried to upload too much data on the push, waiting to see what happens after I push my updated master back to my fork
11:58soleIt... worked?
12:13soleIt takes a while for github to display the changes on the UI, but I hadn&#39;t synced since March so maybe that&#39;s why
13:26zer0sole, I added a note in our snippets to highlight the importance to start to refactor SDK classes from the superclasses, so that no one else would have the same problem you had.
13:29zer0sole: let me know if it makes sense!
13:35soleah let me reload the page
13:36solezer0: can you link me to the specific section?
13:36soleI can&#39;t find it
13:36zer0https://github.com/devtools-html/snippets-for-removing-the-sdk/blob/master/README.md#how-to-replace-the-class-extends
13:36zer0The one related to the replace of SDK classes with the extend.
13:36zer0Maybe I&#39;ve in draft?
13:36zer0Wait
13:37zer0No, it&#39;s there, at least I see it.
13:37solezer0: do you mean if Pet is not an &#39;official&#39; ES6 class, you need to convert it to ES6 before you can extend from it?
13:37zer0I mean that if Pet is a SDK class.
13:37zer0Both Dog and Pet.
13:38zer0So Pet is a subclass of Dog.
13:38soleeh?
13:38zer0Take a look at the code in that section.
13:38zer0Pet is a subclass of Dog.
13:38zer0Dog is the superclass of Pet.
13:39soleThat doesn&#39;t make sense
13:39zer0What doesn&#39;t make sense?
13:39solethat Pet is a subclass of Dog
13:39solethe super class is more generic than the subclass
13:40zer0(Note: the example about Dog and Pet was taken by the official SDK doc is on MDN, to have a clear reference)
13:40solePet is more generic than Dog
13:40solehttps://en.wikipedia.org/wiki/Class_(computer_programming)#SUPERCLASS
13:40zer0As said, the specific example was taken by the official SDK documentation
13:41soleyeah but that doesn&#39;t mean it&#39;s correct
13:41solewe have, you know, brains :P
13:41zer0so that if you look at that, you have a directly reference.
13:41solewe can find if things do not make sense
13:41zer0I believe that when Irakli wrote that,
13:41soleexercise our own judgment etc
13:41zer0it thought Pet in term of &quot;your pet&quot; where &quot;dog&quot; is generic.
13:42zer0At least that what I can understand from the fact that the &quot;Pet&quot; has a name.
13:42zer0(in the example).
13:43soleOK, that example will not work as soon as you think of &quot;Pet&quot; as &quot;Domesticated Animal&quot;, in which case Dog is definitely not the superclass of &quot;Cat&quot; which is another type of Pet
13:43soleAnd semantics aside - I think if you read what I wrote above, can you confirm that what you mean is in order to extend from a class it has to be a real ES6 clas?
13:43soleand not a class created with Heritage or some other helper like that?
13:43zer0Not necessary.
13:44solebecause that&#39;s what I understand
13:44zer0But before proceed on that, if you think that the example is misleading, and it&#39;s more important to be clear than similar to what SDK docs says, I think we should change the example there.
13:45zer0(here the original documentation, btw: https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/core_heritage)
13:45soleyes it is misleading :) I wasn&#39;t reading the class names actually, just looking at the structure. But once I look at them they are misleading
13:46soleI think my brain was looking for the patterns and how to use the new one, and you could have written class Thingamajja extends Brouhaha and I wouldn&#39;t have noticed :)
13:48zer0are those example makes more sense to you: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes
13:49zer0(or `speak` as generic method for an animal doesn&#39;t make sense?)
13:50zer0sole: specifically, I&#39;m talk about: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes#Sub_classing_with_extends
13:51soleI was actually using that page - the geometric examples are nice, the animal examples bother me as they do things such as &#39;speak&#39; and also &#39;return this&#39; which does not speak at all
13:57zer0sole: the reason because I skip the geometric example is that unfortunately they don&#39;t apply well to explain our use cases; since they&#39;re more &quot;data structure&quot; than classes. I&#39;ll try to think a some other example that works for us.
13:58zer0sole: btw the point of the note I made was that if you have `const A = Class({extends: B})` you have to ensure that `B` is not `Class` (SDK) too. If it is, before refactor A you have to refactor B.
13:59zer0B can be either ES6 classes, or just the old prototypal inheritance; doesn&#39;t really matter.
14:20solezer0: I thought the SDK class was, after all, prototypical inheritance
14:21zer0sole: yes and no. There is some black magic behind and we know we hate black magic.
14:21soleWhat kind of black magic?
15:44mykzer0, sole: mixing gecko-dev with git-cinnabar is tricky, although it&#39;s doable; glandium describes two approaches in https://github.com/glandium/git-cinnabar/wiki/Mozilla:-Using-a-git-clone-of-gecko%E2%80%90dev-to-push-to-mercurial
15:46myki originally did that but eventually switched to a &quot;fresh clone&quot; of mozilla-central, which avoids the caveats that glandium describes in that document
15:48myka fresh clone also happens to be smaller, because it doesn&#39;t contain the full history in gecko-dev (nor all its various branches); i used to think that history was a good thing, but in practice, i rarely review it, so the repository size has more of an impact on my day-to-day development (and i can always use gecko-dev when i need to do some code archeology)
15:51solemyk: yeah, I just use my cinnabarified repo now
15:51soleeven that feels slow 8-)
15:51solebut hey...
15:51soleAnyway signing off now!
15:51mykwoot, happy weekend!
15:59zer0victoria, gl, I just added the meeting minutes to the inspector meeting document; feel free to modify / change them.
15 Jul 2017
No messages
   
Last message: 72 days and 3 hours ago