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

TIMOB-9897 Blackberry: address comments from Martin's review #95

Merged
merged 1 commit into from Jul 9, 2012

Conversation

jpl-mac
Copy link

@jpl-mac jpl-mac commented Jul 6, 2012

Reviewers: DC, Alex

This patch is to address comments from Martin. JIRA issues have been created to track omments that require more time to resolve.

Changelog:

  • created enum for NATIVE_ERROR for consistency with the rest of the file
  • added comment in NativeObjectFactory::createNativeObject stating where the created instances live and get deleted
  • added deleteInstance function to NativeStringInterface so we can delete it at shutdown time
  • added comment to empty methods onCreateStaticMembers and onSetProperty in TiObject class
  • added comment for overriding onSetProperty in .h
  • moved using namespace lines from TiV8EventContainer.h to cpp
  • removed unneeded forward declaration of TiInternalEventListener
  • moved TiInternalEventListener comment just before class declaration
  • release the TiRootObject at the end of script execution so it gets deleted properly
  • Added asset path to the js scripts for tibbtest

Tests:

  • Built and ran app.js (provided in JIRA ticket)
    => Made sure that the event handling still works
    => Used String.format to make sure the modification to delete the instance didn't break it
    => Verified that the deleteInstance code gets invoked when during the app shutdown, using debug logs

This patch is to address comments from Martin.  JIRA issues have been created to track omments that require more time to resolve.

Changelog:
- created enum for NATIVE_ERROR for consistency with the rest of the file
- added comment in NativeObjectFactory::createNativeObject stating where the created instances live and get deleted
- added deleteInstance function to NativeStringInterface so we can delete it at shutdown time
- added comment to empty methods onCreateStaticMembers and onSetProperty in TiObject class
- added comment for overriding onSetProperty in .h
- moved using namespace lines from TiV8EventContainer.h to cpp
- removed unneeded forward declaration of TiInternalEventListener
- moved TiInternalEventListener comment just before class declaration
- release the TiRootObject at the end of script execution so it gets deleted properly
- Added asset path to the js scripts for tibbtest

Tests:
- Built and ran app.js (provided in JIRA ticket)
=> Made sure that the event handling still works
=> Used String.format to make sure the modification to delete the instance didn't break it
=> Verified that the deleteInstance code gets invoked when during the app shutdown, using debug logs
@alexandergalstyan
Copy link

Approved

1 similar comment
@Larochelle
Copy link

Approved

Larochelle added a commit that referenced this pull request Jul 9, 2012
TIMOB-9897 Blackberry: address comments from Martin's review
@Larochelle Larochelle merged commit 2f7c353 into Macadamian:blackberry Jul 9, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants