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 10463:Android: Implement new bubbling architecture as prescribed in TIMOB-10372 #3124

Merged
merged 10 commits into from Oct 10, 2012

Conversation

pingwang2011
Copy link
Contributor

http://jira.appcelerator.org/browse/TIMOB-10463
Test steps in JIRA.
The doc is updated in #3114

@ghost ghost assigned ayeung Oct 8, 2012
if (!handler.listener || !(handler.listener.call)) {
if (kroll.DBG) {
kroll.log(TAG, "handler for event '" + type + "' is " + (typeof handler.listener) + " and cannot be called.");
}
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the handler for the event can't be called, shouldn't we just return? Do we even want to bubble it up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, eg. a view in the window, only window has the listener, when we click the view, the window's listener should be invoked.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, makes sense. Then we should probably merge the kroll.DBG condition with the previous if statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's better not merging them because there is "else" block after that.

@ayeung
Copy link
Contributor

ayeung commented Oct 9, 2012

Code reviewed. Please address comments.

@pingwang2011
Copy link
Contributor Author

Updated. Please review.

@pingwang2011
Copy link
Contributor Author

Updated. Please review.

// Set the "bubbles" and "cancelBubble" properties for event data.
if (data !== null && typeof data == "object") {
if (data.bubbles) {
data.bubbles = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having an if-else statement, you can just do data.bubbles = !!data.bubbles

@billdawson
Copy link
Contributor

I'm acting as second reviewer.

listeners;

// Set the "bubbles" and "cancelBubble" properties for event data.
if (data !== null && typeof data == "object") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be typeof data ===

Copy link
Contributor Author

Choose a reason for hiding this comment

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

typeof will always return a string. So it is fine to use "==" here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point!! :)

@billdawson
Copy link
Contributor

CR accepted

@ayeung
Copy link
Contributor

ayeung commented Oct 10, 2012

Code reviewed and functionally tested. Ran KS and Anvil on both runtimes. Also ran all of the TIMOB tickets mentioned in the test steps on both runtimes. Request Accepted

ayeung pushed a commit that referenced this pull request Oct 10, 2012
Timob 10463:Android: Implement new bubbling architecture as prescribed in TIMOB-10372
@ayeung ayeung merged commit 50b1079 into tidev:master Oct 10, 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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants