mozilla / memchaser

Firefox extension to chase the memory usage and garbage collector activity
https://wiki.mozilla.org/QA/Automation_Services/Projects/Addons/MemChaser
29 stars 23 forks source link

Upgrade from widget to toolbar api to support Australis #183

Closed whimboo closed 9 years ago

whimboo commented 10 years ago

As what I got today from the Addon SDK guys is that we have to upgrade memchaser so we really support the new toolbars. Widgets are deprecated and will be removed at some time. So we need support for the toolbar API but also should leave fallback code for widget in the tool if possible.

xabolcs commented 10 years ago

I played a bit with the toolbar API. sdk/ui/toolbar and sdk/ui/frame are not supported by 1.15 Add-on SDK but only by master. Interestingly I have no success displaying the - static - content of widgets/widget.html. The toolbar was displayed, but without content.

Will give it another try later.

whimboo commented 10 years ago

That's great to hear that you got started on it, @xabolcs! So what my understanding is, we have to special case and check if the toolbar API is available. If not we should fall back to the old method. So for the first step you might not have to worry about the SDK v1.5.x. It's old and not updated anymore. The SDK is part of Firefox for a while already.

xabolcs commented 10 years ago

It's still very interesting. I tried the toolbar-api example, but the toolbaris unable to receive messages from the frame, until customization. After that it's all OK. :)

The other main task in this issue is to upgrade the communication from port based to message based.

whimboo commented 10 years ago

I wonder if we should do the change from port to message first in another pull. I don't think that this is related to Australis, and we might be able to get this done earlier. Since when we have message support? Goes it back to Firefox 24?

xabolcs commented 10 years ago

Slicing that into another issue would be helpful. :+1:

In some way it's related: ui/frame and ui/toolbar modules don't implement the port object as panel and widget do. The Australis modules support only postMessage.

Thankfully postMessage is old enough, it is supported by the old modules.

xabolcs commented 10 years ago

Goes it back to Firefox 24?

Migrated update_tooltip communication to postMessage, deployed into Iceweasel 24.3.0 and it still updates the tooltips. :+1:

whimboo commented 10 years ago

That's great to hear that postMessage also works with Firefox 24. So lets focus on issue #185 then and get this implemented first. So we only need a fallback for toolbar vs. widget then. I love that.

whimboo commented 10 years ago

This issue will be part of the next 0.6 release of Memchaser.

xabolcs commented 10 years ago

ui/frame's content script is very interesting. let isn't allowed, for ... each is unknown, and sadly document.getElementById() doesn't find element.

whimboo commented 10 years ago

I don't really understand why those shouldn't work. Do you have code examples and the appropriate errors?

xabolcs commented 10 years ago

Do you have code examples and the appropriate errors?

See my hacks on my branch-issue-183-widget-to-toolbar-api branch. Please note it's just an experiment.

whimboo commented 10 years ago

Hm, I don't understand why this should be the case. Would you mind to ask some Add-on SDK folks for input? This looks kinda strange.

xabolcs commented 10 years ago

Note to self ...

[16:37] whimboo xabolcs: great. for now lets make sure that the current modules work with postMessage [16:37] whimboo any other refactoring we can do when the australis pr is in the works [16:38] xabolcs yeah, too much question raised by australis support [16:39] whimboo indeed [16:39] whimboo postMessage is just a pre-condition to fix before we can tackle australis [16:39] whimboo lets see when the first one complains about the widget in australis

[16:53] xabolcs whimboo: are there a place to talk about and note down the plan of the new ui for memchaser australis? [16:54] whimboo i would use the open issue [16:55] xabolcs ok [16:57] whimboo as first step we should just make it working [16:57] whimboo so we have our own toolbar [16:57] whimboo with the same look as now

xabolcs commented 10 years ago

Because of the new CustomizableUI modules we need to upgrade Addon SDK submodule to version 1.16. For the details see Bug 961846 comment 2!

whimboo commented 10 years ago

So as long as SDK v1.16 has not been released this issue is partly blocked. @xabolcs you could indeed already start to work on it given that a beta has already been released. We would only have to wait with v1.16 for the final release.

xabolcs commented 10 years ago

Yeah, 1.16b1 is out. I worked with master till now, but I will checkout 1.16 for now and commit it.

xabolcs commented 10 years ago

@whimboo, based on the work of travis guys, I could improve .travis.yml to stick firefox version to latest-beta or any other latest-*. Plus I could use wget's timestamping function to minimalize the load of ftp.mozilla.org. Sounds good? Could I split it into a separate issue?

Btw I'd like to ask you to revisit the thread in pull #173 about language again! Please read it through again.

xabolcs commented 10 years ago

I could improve .travis.yml to stick firefox version to latest-beta or any other latest-*. Plus I could use wget's timestamping function to minimalize the load of ftp.mozilla.org.

@whimboo, instead of hacking with complex install scripts we could also use 3rd party PPAs, as the doc states. This post describes well how to install Firefox from different channels.

xabolcs commented 10 years ago

@whimboo, I'm going to merge here the postMessage() PR's current state. After that I try to get let and for ... each work once more, but on failure I'm going to separate widget.js' logic into common and ui/frame/widget specific parts.

xabolcs commented 10 years ago

let and for ... each are working now, thanks to MDN and SO! But I'm facing with giant exceptions. So I'm going to postpone the debugging, and create new html and js files for the toolbar now.

@whimboo, you know that the toolbar can be switch on and off? Shouldn't we do something with that?

whimboo commented 10 years ago

based on the work of travis guys, I could improve .travis.yml to stick firefox version to latest-beta or any other latest-*. Plus I could use wget's timestamping function to minimalize the load of ftp.mozilla.org. Sounds good? Could I split it into a separate issue?

Separate issue would be good, but I still don't know on which work this is based. Please add more details and possible links to the new issue. But in general I think we can make use of mozdownload to download necessary Firefox releases.

Btw I'd like to ask you to revisit the thread in pull #173 about language again! Please read it through again.

We can do once a new issue and PR is up, we agreed on the updated behavior in .travis.yml.

whimboo commented 10 years ago

Btw. the version 1.6 of the Addon SDK should be out now.

whimboo commented 10 years ago

let and for ... each are working now, thanks to MDN and SO! But I'm facing with giant exceptions. So I'm going to postpone the debugging, and create new html and js files for the toolbar now.

I would need some code to have a look into. It might still be that there is a bug in Firefox for Australis. Lets get this investigated quickly, so a possible fix can go into Firefox 29.

@whimboo, you know that the toolbar can be switch on and off? Shouldn't we do something with that?

Well, toolbars can be shown and hidden, so I wouldn't be surprised if we can do this with the memchaser add-on too. So by default it will be shown after installation, right?

xabolcs commented 10 years ago

based on the work of travis guys, I could improve .travis.yml to stick firefox version to latest-beta or any other latest-*. Plus I could use wget's timestamping function to minimalize the load of ftp.mozilla.org. Sounds good? Could I split it into a separate issue?

Separate issue would be good, but I still don't know on which work this is based. Please add more details and possible links to the new issue. But in general I think we can make use of mozdownload to download necessary Firefox releases.

I referred to step install_firefox in the Travis build. And the day of automation training came, and I found the mozmill + travis-ci bug, so yes, mozdownload would be the easiest. :)

Btw I'd like to ask you to revisit the thread in pull #173 about language again! Please read it through again.

We can do once a new issue and PR is up, we agreed on the updated behavior in .travis.yml.

Will file a new issue for that, after this issue got fixed. We are good for a few weeks with Firefox 29, but a bump in .travis.yml is more than trivial.

Btw. the version 1.16 of the Addon SDK should be out now.

Yes, and it's the same as 1.16b1. No update is needed with the subrepo.

let and for ... each are working now, thanks to MDN and SO! But I'm facing with giant exceptions. So I'm going to postpone the debugging, and create new html and js files for the toolbar now.

I would need some code to have a look into. It might still be that there is a bug in Firefox for Australis. Lets get this investigated quickly, so a possible fix can go into Firefox 29.

My WIP work is at my toolbar-api branch. If you need a fix asap, then I'm sorry, I have very random free time nowadays. Feel free to take over.

@whimboo, you know that the toolbar can be switch on and off? Shouldn't we do something with that?

Well, toolbars can be shown and hidden, so I wouldn't be surprised if we can do this with the memchaser add-on too. So by default it will be shown after installation, right?

Yeah, it would be shown. But if the User toggles off the toolbar once, enabling/disabling won't do visually anything. So warn the User while he/she toggles the toolbar would be nice, IMHO. Or pushing a notificationbox on addon startup if toolbar is hidden, would be also nice.

whimboo commented 10 years ago

My WIP work is at my toolbar-api branch. If you need a fix asap, then I'm sorry, I have very random free time nowadays. Feel free to take over.

Would you mind to at least rebase this branch against master, so that the postMessage changes will be included? Thanks.

xabolcs commented 10 years ago

@whimboo could you check the gui? Checkout my branch and give it an ant package!

Please run Firefox with console and notice the bugs! :-/ If you want contextpanel to popup, you should open customizeUI once.

whimboo commented 10 years ago

Lets push this issue out to the 0.7 release.

whimboo commented 9 years ago

Sad news here... given that amount of work and that I will be away for some weeks soon, I won't be able to do any work on this issue before May. While I'm away maybe @KWierso can at least review a PR given that he has great knowledge about the Add-on SDK.

xabolcs commented 9 years ago

Sad news here...

Acknowledged.

xabolcs commented 9 years ago

From Issue #213.

I should have time for discussion and reviews if requested.

Please rethink / redesign the GUI of MemChaser! I could have time if you clearly specify what do you want for MemChaser!

As you could see there is no real successor of Widget module. I had to apply lot of workaround to mimic it with ui/toolbar module. IMHO ui/toolbar isn't for context menus and tooltips. Sorry.

See ui/toolbar documentation!

whimboo commented 9 years ago

That is great to hear @xabolcs!

What we would need here is a panel-only version without any fallback code to widget. So a toolbar is definitely the way to go. Including there we would most likely have to use a frame to an HTML file, similar to the example in the referenced toolbar documentation. We should be able to keep the UI identical to what we currently have. I assume that our backend modules will stay the same and we only have to communicate with them via postMessage? I would do that as the first step before caring about the popup and its actions. What do you think?

xabolcs commented 9 years ago

What we would need here is a panel-only version without any fallback code to widget. So a toolbar is definitely the way to go.

Yes, no fallback code here. Yes, toolbar is the way to go here. For button way there is Issue #176, as an alternate GUI.

Including there we would most likely have to use a frame to an HTML file, similar to the example in the referenced toolbar documentation.

Yeah, we could put frames and buttons on to toolbar, nothing else.

We should be able to keep the UI identical to what we currently have.

I'm on the opposite side. Design a whole new UI, for toolbar in mind. If needed, name it 1.0! The old UI isn't compatible with the new toolbar module. Take all the functionalities, prioritize them, put the TOP N on the toolbar and do something with the others. If the change is major, let it v1.0, if minor let it v0.7.

I assume that our backend modules will stay the same and we only have to communicate with them via postMessage?

Most coding work would happen in the frame's source and in main.js, others would be untouched.

I would do that as the first step before caring about the popup and its actions. What do you think?

You missed out the tooltips. ;-)

As I wrote above, imagine that you are making a new SDK addon with ui/toolbar module from some already written modules with logging, gc and memory reporting features. The new addon have some item on it's nice to have list, e.g. triggering memory related events, providing nice descriptions on the reported items, etc.

whimboo commented 9 years ago

Yes, no fallback code here. Yes, toolbar is the way to go here. For button way there is Issue #176 https://github.com/mozilla/memchaser/issues/176, as an alternate GUI.

This can definitely wait. First we should restore the behaviour we had before with the widget code, which allowed an easy way to always have the values visible.

We should be able to keep the UI identical to what we currently have.

I'm on the opposite side. Design a whole new UI, for toolbar in mind. If needed, name it 1.0!

I don't say that we should take the implementation but the layout in terms of what is displayed. The current code might be a bit nasty so a refactoring would always be good.

The old UI isn't compatible with the new |toolbar| module. Take all the functionalities, prioritize them, put the TOP N on the toolbar and do something with the others. If the change is major, let it v1.0, if minor let it v0.7.

Those are not that many features which would require a largish prioritization. But first should definitely come the memory usage, if that is the feedback you would like to hear. :)

I assume that our backend modules will stay the same and we only
have to communicate with them via postMessage?

Most coding work would happen in the frame's source and in |main.js|, others would be untouched.

Great to hear.

I would do that as the first step before caring about the popup and
its actions. What do you think?

You missed out the tooltips. ;-)

Those are kind of popup. :) But yes, those would be nice too.

As I wrote above, imagine that you are making a new SDK addon with |ui/toolbar| module from some already written modules with logging, gc and memory reporting features. The new addon have some item on it's nice to have list, e.g. triggering memory related events, providing nice descriptions on the reported items, etc.

Sounds fine with me. When you start implementing the ideas maybe lets do it in smaller steps and have review/feedback cycles more often? As smaller the code to review as faster I can do it.

Thanks a lot that you really wanna take it!

xabolcs commented 9 years ago

Sounds fine with me. When you start implementing the ideas maybe lets do it in smaller steps and have review/feedback cycles more often? As smaller the code to review as faster I can do it.

First of all, let land the logging PR. :)

I'll start soon the coding, and open a PR for it ... not to review, but to see how I imagine all this. Sure, I'll take small steps.

xabolcs commented 9 years ago

PR #220 is merged.