Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

port to firefox devtools addon #60

Merged
merged 34 commits into from
Nov 6, 2013

Conversation

rpl
Copy link
Contributor

@rpl rpl commented Sep 3, 2013

This pull request is not intended to be merged as it is, I’m opening it to start evaluating if there's interest to integrate it into the project and, in that case to work on it until it will be ready to be merged (and then fix #59).

TODO LIST:

  • build firefox and chrome extension from the same sourcecode
  • firefox extension seems to work correctly, but there’re some CSS issues to work out.
  • integrate xpi build
  • test "grunt build_xpi" on OSX
  • support DOM inspection request
  • how to integrate "images" linked from handlebars templates
    SOLUTION: using relative image urls (e.g. "../images/send.png")
  • merge port-firefox and port-chrome modules into port modules (app, ember_debug)
  • fix bug ember app not detected from file url or from localhost (NOTE: unable to reproduce)
  • remove findCssSelector
  • fix vertical arrow (app css)
  • fix main section vertical scrollbar (should be 100% and scrollbar disabled)
  • rebase to firefox-adapter
  • update README
  • integrate build_xpi into build_and_upload (upload last build xpi using ember-s3)

@rpl rpl mentioned this pull request Sep 3, 2013
@locks
Copy link
Contributor

locks commented Sep 3, 2013

You can use the Task List syntax GitHub provides to have a nice Todo list.
Interesting work!

@stefanpenner
Copy link
Member

awesome!

@teddyzeenny
Copy link
Contributor

👍 Awesome!!

@stefanpenner
Copy link
Member

Just a note, I'm likely preaching to the choir, but this effort should strive to prevent too much platform specific pollution. We should aim to keep platform specific code isolated to some adapter layer as we want to be able to continue development at an agile pace.

Obviously supporting firefox is awesome, and I am excited to see it move forward!

@teddyzeenny
Copy link
Contributor

The CSS issues are likely the -webkit prefixes. But iirc Firefox now supports the new flexbox so it shouldn't be too complicated.

@ccarruitero
Copy link
Contributor

@rpl for integrate xpi are you think in add addon-sdk like submodule?

@rpl
Copy link
Contributor Author

rpl commented Sep 4, 2013

@stefanpenner I completely agree with you, I wouldn't have been able to achieve a working prototype in a so short time if the code had not been so well organized, and chrome specific code isolated in a couple of modules (which were the "app/main", "app/port" and "ember_debug/port" modules).

I tried to maintain the same clean approach, moving chrome specific code from the "app/main" and "app/port" into the "app/port-chrome" module and creating a "app/port-firefox" module with firefox counterparts (and I've done mostly the same in the "ember_debug/port" module) and tweak the gruntfile to be able chrome and firefox extensions from the same codebase.

Obviously I'm open to discuss how to achieve an even more clean integration, so we can merge the firefox extension without slowing the original chrome extension development.

@rpl
Copy link
Contributor Author

rpl commented Sep 4, 2013

@teddyzeenny for sure! and @ccarruitero was so kind to start working on it already

I'm testing these CSS changes on both browsers and add it to this pull request asap

@rpl
Copy link
Contributor Author

rpl commented Sep 4, 2013

@ccarruitero on my extensions I usually do so, because it helps to ensure that all developers have a working development environment (with python installed) and the same addon-sdk release.

In this case I think we should ask to the upstream maintainers how they prefer to integrate the xpi building into the repo (e.g. if they see any problem in adding addon-sdk as a submodule).

@rpl
Copy link
Contributor Author

rpl commented Sep 4, 2013

@ccarruitero I've merged your changes manually to include only needed "prefix removing changes" from the CSS changes (to simplify review of the overall pull request, but I've preserved author metadata in the merged commits ;-))

@rpl rpl closed this Sep 4, 2013
@rpl rpl reopened this Sep 4, 2013
@rpl
Copy link
Contributor Author

rpl commented Sep 4, 2013

reopened (closed by accident)

@jeffgca
Copy link

jeffgca commented Sep 4, 2013

@rpl: this is awesome! On nightly on OS X I get comically large icons in the toolbox tab header:

https://www.evernote.com/shard/s1/sh/937701e0-9c2d-4260-ad71-276e8ace4ac8/08fbf3862d3fd5fe9a9495d037b2ffe7/deep/0/Ember.js%20-%20About.png

...not sure if this is a devtools bug or an ember inspector bug...

@rpl
Copy link
Contributor Author

rpl commented Sep 5, 2013

@canuckistani Thanks a lot! it was my fault, I chosen the wrong icon as devtool panel :-P

I've just pushed a fix ;-) (using a 16x16 icon as other firefox devtools panel icons)

I've updated the experimental xpi build too (including @ccarruitero css changes)

@jeffgca
Copy link

jeffgca commented Sep 5, 2013

@rpl: looks great: http://goo.gl/NrvB1R

@stefanpenner
Copy link
Member

im blown away, this is just straight up awesome.

@ccarruitero
Copy link
Contributor

We can add addon-sdk like submodule for integrate xpi building? cc @stefanpenner @teddyzeenny @wycats

@wycats
Copy link
Member

wycats commented Sep 13, 2013

@ccarruitero I'd think so. @teddyzeenny what do you think?

@rpl
Copy link
Contributor Author

rpl commented Sep 13, 2013

@wycats @ccarruitero @teddyzeenny @stefanpenner I'm working on another solution, which I think could be cleaner and better integrated to the current build tools (and doesn't need a full addon-sdk clone as in case we choose to integrate it as git submodule):

I'm putting together a grunt plugin to be able to download and run a defined mozilla addon-sdk revision / tag from the github archive url.

If this new strategy seems interesting to you, I should be able to complete a working prototype during this weekend.

P.S. and I've a couple of minor css tweaks in my local branch which I will push asap

@wycats
Copy link
Member

wycats commented Sep 13, 2013

@rpl sounds good to me. Excited to get this done and ready to be regularly pushed as a Firefox addon.

@rpl
Copy link
Contributor Author

rpl commented Sep 16, 2013

I've pushed a small grunt plugin on: https://github.com/rpl/grunt-mozilla-addon-sdk
and published as regular npm package.

In the last 2 commits there are the needed changes (a the new plugin config fragment and a new
"build_xpi" task to streamline its use into the Gruntfile.js):

I can't test it on OSX (but it should work) and on Windows (where it "could" work using a bash shell, or needed changes to integrate support for a windows batch file in the grunt-mozilla-addon-sdk package)

If anyone has a chance to try it, I'll be happy to receive feedback and try to fix it (on OSX and on a Windows running a bash shell at least)

@ccarruitero
Copy link
Contributor

I tried to build xpi with grunt and with addon-sdk in ubuntu and generate xpi but not working.

But when run with addon sdk all is ok. So I think maybe is a problem with my fx nightly. I try to test this tonight again.

@zoghal
Copy link
Contributor

zoghal commented Sep 19, 2013

@rpl nice job, It works much better than Chrome on Windows. At least the good rtl support., But Chrome is a problem with rtl.
but I think firebug extension is popular and more stable than Firefox devtools.
I suggest port to firebug

@rpl
Copy link
Contributor Author

rpl commented Sep 20, 2013

@ccarruitero good catch!

Using a "Mozilla Addon SDK" released branch, the xpi will be build correctly, but it doesn't contains chrome dir and chrome.manifest (needed to register the new devtool panel)

I've pushed a fix (which will download "Mozilla Addon SDK" firefox26 branch instead, which include http://bugzil.la/559306 and works correctly)

NOTE: you will need to remove the old "tmp/mozilla-addon-sdk" to get the new 'firefox26' branch correctly downloaded
(asap I'll try to update the grunt-mozilla-addon-sdk package to detect by itself if it needs to update the downloaded sdk)

@rpl
Copy link
Contributor Author

rpl commented Sep 20, 2013

@zoghal Thanks!

This port uses only the simple "add this panel" feature from the integrated firefox devtools APIs, so it shouldn't be affected by most of the changes or issues from the "firefox integrate devtools".

I've just looked to the Firebug extensions APIs, and I'm evaluating how we can hook both (firefox integrated devtools and firebug if installed) from the same addon (and it seems really possible, but I'll need to look into the changes more deeply).

@teddyzeenny
Copy link
Contributor

@rpl what's the state of this?

@rpl
Copy link
Contributor Author

rpl commented Sep 24, 2013

@teddyzeenny I've just rebased this pull request on master to solve any conflicts in advance (there was only 1 on a css files)

follows an updated todo list:

  • "grunt build_xpi" needs to be tested on OSX ("build_xpi" should be called after a "grunt build")
  • there are a couple of handlebars templates which use image resources (and their urls will not work on firefox, but currently I don't know where they are in the ui)

@rpl
Copy link
Contributor Author

rpl commented Sep 29, 2013

@teddyzeenny last changes applied to this pull request:

  • rebased on master
  • changed image urls to be relative (e.g. "../images/send.png") in css files and handlebars templates
  • polyfill "inspect()" function used to activate DOM inspection on emberjs views

I've also managed to try to run "grunt build build_xpi" on OSX and it did complete successfully (tried on OSX 10.8.4, node 0.10.19, npm 1.3.11, firefox 23.0.1)

I think now it's ready to be reviewed.

@teddyzeenny
Copy link
Contributor

src="../images/calculate.svg" on this line

@rpl
Copy link
Contributor Author

rpl commented Nov 2, 2013

@teddyzeenny I've pushed last tweaks
+1
on the firefox side it's ready

@rpl
Copy link
Contributor Author

rpl commented Nov 3, 2013

@teddyzeenny I've just merged and pushed your fixes

}
};

let trackTabWorkers = new WeakMap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@teddyzeenny yep, removed in the last commit pushed

@stefanpenner
Copy link
Member

:)

@rpl
Copy link
Contributor Author

rpl commented Nov 4, 2013

@teddyzeenny merged and pushed "Fix view pinning in firefox"

@teddyzeenny
Copy link
Contributor

This looks ready to merge. @wycats any comments?

@teddyzeenny
Copy link
Contributor

@rpl is there a way to not trigger a page reload when opening the inspector for the first time?

@rpl
Copy link
Contributor Author

rpl commented Nov 4, 2013

@teddyzeenny I've looked into it, pushed a commit to prevents the page reload

teddyzeenny added a commit that referenced this pull request Nov 6, 2013
@teddyzeenny teddyzeenny merged commit eaa6546 into emberjs:master Nov 6, 2013
@wagenet
Copy link
Member

wagenet commented Nov 6, 2013

Great work!

@stefanpenner
Copy link
Member

super awesome!

@arashm
Copy link

arashm commented Nov 6, 2013

Awesome!
Probably the link to latest XPI on readme should be: http://ember-extension.s3.amazonaws.com/xpi-latest.xpi

@teddyzeenny
Copy link
Contributor

@arashm thanks! 5e85eca

@bkCDL
Copy link

bkCDL commented Dec 5, 2013

This is great! I'm so glad to see this on Firefox. . It does seem to conflict with FireBug a little bit, in that I can't have both FireBug and FF's dev tools windows open at the same time for this to work. It'd be nice if it was a tab in Firebug. I'm using FF on Ubuntu 13.10

@simonweil
Copy link

Also for me they don't work well together.

The only way they work together is when I first turn on the Ember inspector and only after it recognizes the application, only then I can turn on FireBug.

Any other way - the ember inspector doesn't recognize the ember app.

👍 for having the inspector as a tab in FireBug.

@jeffgca
Copy link

jeffgca commented Dec 9, 2013

I think it would be very difficult to port this extension to work with Firebug. jetpack and chrome extensions use similar techniques with regular html/js/css whereas Firebug uses DOMPlate to inject UI. I could be wrong!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

port to firefox?