apache / cordova-browser

Apache Cordova
Apache License 2.0
170 stars 85 forks source link

CB-8206: Browser platform: Add support for update #6

Closed nikhilkh closed 9 years ago

nikhilkh commented 9 years ago

This change add supports for updating the browser platform. I ended up deleting a couple of log messages from create as there is no way to retrieve and log them on update.

jsoref commented 9 years ago

@nikhilkh: please use git rebase to fold the fixes for 2d77f2b into aad4c25

jsoref commented 9 years ago

well, err, specifically: for lines you're adding in aad4c25 with bad whitespace, you should fold in the fixes from 2d77f2b. If you're fixing the entire file, you can do that as a distinct commit.

nikhilkh commented 9 years ago

@jsoref Thanks for reviewing this and the instructions for doing the git magic. It was useful. I have made the changes that you requested.

nikhilkh commented 9 years ago

Looking for anyone who can review this and commit it? It's quite a simple change.

jsoref commented 9 years ago

I'd like someone else to review this -- but please address my other comments. Sorry for the multipass review.

And, I need to see about ensuring there's a checklist (on the wiki or in coho or something) for most of these points...

nikhilkh commented 9 years ago

@jsoref Your comments have been addressed.

I will wait for someone else to review this. A checklist would be great - looks like there is a doc here: https://github.com/apache/cordova-coho/blob/master/docs/code-reviews.md but lacks details.

mmocny commented 9 years ago

I reviewed this in a previous iteration, and the latest changes look fine.

Since this feature does not exist yet anyway, and I'm not too concerned about inline updates to browser platform, I think this is more than ready to ship.

mmocny commented 9 years ago

..Thanks!

mmocny commented 9 years ago

(Josh I'll let you have the last word on landing, you have been running this)

jsoref commented 9 years ago

ok, one last final thing:

Then, someone can push this to cordova-browser.

nikhilkh commented 9 years ago

@jsoref Updated the commit message. Please review and help merge to master.