search

ericwoodruff / passwordhasherplus

Password Hasher Plus
http://goo.gl/LyLk3
Other
12 stars 21 forks source link

Commits necessary to upgrade manifest version #16

Closed ecbaldwin closed 10 years ago

ecbaldwin commented 10 years ago

I found these two commits necessary to get the extension to work correctly in recent Chrome versions where the new security restrictions are enforced.

I'm currently playing around with synchronizing config between browsers using Chrome Sync and later maybe some other synchronization method. The secret seeds won't be synchronized but will stay in local storage.

That work has required me to change all of the synchronous calls to localStorage to an asynchronous API (call storage api and pass a callback). I wrapped localStorage with an API that acts like an asynchronous API so that using localStorage is still the default option. The changes are rather intrusive but necessary in order to make use of Chrome's storage API. Are you interested in seeing and possibly consuming that work? I'm still testing and probably won't publish until it is working well.

ericwoodruff commented 10 years ago

Thanks I'll check it out and test it and I can then publish it. The Chrome Sync is needed in this plugin.

ericwoodruff commented 10 years ago

You removed the portable html page from the makefile?

ecbaldwin commented 10 years ago

I did not notice that as I don't normally use the portable html page. You could ping jeoffreybakker about that. He had done that work in his fork.

ericwoodruff commented 10 years ago

Ok, I'll massage the changes into my branch. The plugin should also allow exporting the portable page from the options page.

ericwoodruff commented 10 years ago

I've merged in the changes and republished the extension.

ecbaldwin commented 10 years ago

Nice! I'm pretty close to having chrome sync work. I'll let you know when I have something to look at.

ecbaldwin commented 10 years ago

I'm not too thrilled about you having reset the author of the commits to yourself. I'm reconsidering contributing any more new code.

ericwoodruff commented 10 years ago

Carl, there were a lot of stylistic changes to the files, and while they would have just merged it, it made it difficult to detect where the feature/logic changes were. So I needed to walk myself though the relocation changes so then I could filter the noise in the diff between our branches. I have not used a pull request before and I may need to read up on open source etiquette. Given the remaining non-stylistic changes, it seemed that giving credit in the commit message was enough, but I meant no offense by any means. Since I couldn't take the changes wholesale because there were some refactorings that would break the portable html page, the merge made the most sense. If there is a better way to use git like taking the commits wholesale than applying a patch on top of that, or making changes and then rebasing your commits, I can do that next time. If it will make a difference, I can start the whole thing over and try again given that there are probably very few folks that have pulled these changes.

ecbaldwin commented 10 years ago

Eric, when I submit code to an open source project I have come to expect some feedback and an opportunity to fix it up and resubmit it myself.

In this case, my contribution was merely a single line fixup on top of jeoffreybakker's original work. I don't care about retaining credit for me. I'm okay if you roll my simple fixup in to a commit with another author.

I think that you should take steps to restore authorship for jeoffreybakker. I don't believe giving credit in the changenotes is sufficient, it is much less visible than the author. What I might suggest is that you restore his author line to each of the commits that you broke out from his work. Any commits that use mostly his work with some of your fixups. Setting the author name and email to his and then using --reset-author option to commit should do it. Git rebase --interactive will probably be useful in order to amend previous history. You will be shown as the "committer" in the commits but that is normal and acceptable.

Thank you for your willingness to make it right.

ericwoodruff commented 10 years ago

Should be fixed now.

ericwoodruff commented 10 years ago

I now see that the changes were not just stylistic because of Chrome's new policy: Refused to execute inline script because it violates the following Content Security Policy directive: "script-src 'self' chrome-extension-resource:".

That means the portable html page needs a fundamentally different design, at least in the way it provides the Download link.