John Lu [:mnjul][Please NI or I'll miss] Reporter
	Description • 10 years ago

When I was implementing the then-new lockscreen slider with bug 950884, I planted a useNewStyle option in lockscreen_slide.js to trigger the rendering of the new slider style. This was for the sake of backward compatibility because lockscreen_slide.js was shared with callscreen (as in 'incoming call when screen is locked'), and at that time, bug 951665 and its subsequent bug 1018283 hadn't been prepared.

There, a lot of code revolved around conditional branching dependent on the boolean value of useNewStyle.

Now that bug 1018283 has landed, and useNewStyle is always true from every caller, we could eliminate the usage of useNewStyle and dead code related to it.

	John Lu [:mnjul][Please NI or I'll miss] Reporter
	Comment 1 • 10 years ago

Getting it tested...

	John Lu [:mnjul][Please NI or I'll miss] Reporter
	Comment 2 • 10 years ago

Comment on attachment 8483339 [details]
Patch (PR @ GitHub)

Hi Greg and Germán,

I cleaned up the dead code in lockscreen_slide.js . Please, when you have spare time, take a look (lowest priority). Thanks!

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 3 • 10 years ago

Comment on attachment 8483339 [details]
Patch (PR @ GitHub)

I assume this doesn't cause any UX regression, which I can hardly discover from the code. Anyway, to see a workaround be eliminated is good. Thanks.

	Doug Sherk (:drs) (inactive)
	Comment 4 • 10 years ago

Comment on attachment 8483339 [details]
Patch (PR @ GitHub)

This must be reviewed by a dialer peer before landing, which Germán is not. Also, Germán is on PTO until the 15th.

	Doug Sherk (:drs) (inactive)
	Comment 5 • 10 years ago

Comment on attachment 8483339 [details]
Patch (PR @ GitHub)

Before reading this comment, please see bug 950884 comment 53.

I don't know enough about the lockscreen slider to do a truly good review on it, but I skimmed the changes to the shared code anyways. I noticed several things that I think Greg should have, and the fact that they weren't caught worries me. I don't think there was a thorough enough review done on this.

I want to ask for more tests, but since this is a code cleanup and there are no tests already, I don't think it's reasonable to. However, I would ask that you at least file a followup bug to add tests for this code.

Also, as established in bug 950884 comment 53, a system peer should review this, which Greg is not. I'm flagging Tim to decide or just give Greg the go-ahead here.

	Doug Sherk (:drs) (inactive)
	Comment 6 • 10 years ago

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #3)
> Comment on attachment 8483339 [details]
> Patch (PR @ GitHub)
> 
> I assume this doesn't cause any UX regression, which I can hardly discover
> from the code. Anyway, to see a workaround be eliminated is good. Thanks.

Also, I don't think it's a requirement, but reviewers should generally test patches. For a quick litmus test, if you have to state something like this, then it's probably a good idea to test it yourself.

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 7 • 10 years ago

Commented at https://bugzilla.mozilla.org/show_bug.cgi?id=950884#c54.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 8 • 10 years ago

Comment on attachment 8483339 [details]
Patch (PR @ GitHub)

:drs,

Thank you for raising the concern (and your careful review). Actually module ownership was never meant to be a rigid, unbroken rule -- if so we will be enforcing that with a Bugzilla bot already. It's often that a peer/owner generally redirect review to someone that is not a peer, in order to bringing in new peers for the module. For lock screen that's exactly what I am working on with Greg -- if you have concern with with that, I am more than happy to codify Greg as a peer of Gaia::System (lock screen) on the wiki.

As of the quality of the review, I looked at the comments and I agree many of the issues should have been caught. If there are more issues on that feel free to point it out again on the bug or bring it to me in private.

PS. Reviewer are not expected to try the patch -- AFAIK that's never been a requirement. In the perfect world of test-driven engineering we should have automation coverage for that (and QA coverage for the rest). That said, I agree we are not in the prefect world and there isn't an obvious way to test interactions on canvas -- in that case reviewer can make her/his own decisions on what to do with it.

I am setting review request flag to you -- you are free to complete the review or relinquish the right to do so.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 9 • 10 years ago

Actually, re-read bug 928740 comment 53, I realized many of the issues can be fixed in this bug, since you are one of the reviewer for the patch.

	Doug Sherk (:drs) (inactive)
	Comment 10 • 10 years ago

Thanks for the long and thoughtful reply, Tim.

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #8)
> Thank you for raising the concern (and your careful review). Actually module
> ownership was never meant to be a rigid, unbroken rule -- if so we will be
> enforcing that with a Bugzilla bot already. It's often that a peer/owner
> generally redirect review to someone that is not a peer, in order to
> bringing in new peers for the module. For lock screen that's exactly what I
> am working on with Greg -- if you have concern with with that, I am more
> than happy to codify Greg as a peer of Gaia::System (lock screen) on the
> wiki.

Yes, I'm aware of this. I was being trained to be a peer on the dialer this way until recently. Indeed, I don't have any problem with this. The issue was more that this wasn't being written out explicitly for onlookers. But now that you've stated this, I understand.

As I mentioned, I wasn't sure if you were giving an implicit go-ahead for Greg to do the review in bug 950884 or not. However, in this bug, you have had no involvement at all until now, but Greg still took the review. I personally think it's dangerous to allow someone to act as a peer until they're officially promoted to one, and should instead be explicitly redirected reviews by a peer. The reason I brought this up was because I thought that the other issues may be caused at least partially by this one. Perhaps you disagree, but that wasn't the focus of my comments, so I won't push this issue further.

> As of the quality of the review, I looked at the comments and I agree many
> of the issues should have been caught. If there are more issues on that feel
> free to point it out again on the bug or bring it to me in private.

I'm glad that you took a look. However, my point here wasn't to fix this one-time problem, or to cast blame to people. Sometimes, people miss things in review, or aren't aware of policies. It happens. But I think the issue here is a more systemic one, and I haven't seen anything in your comments that indicates that you agree with that. Basically, what I'm worried about is that we will fix the issue here, but then Greg will continue reviewing patches while being unaware of our policies. I think this is the issue that needs to be addressed. Note that I'm not at all saying that Greg is a poor reviewer, or a poor developer. Mozilla definitely has communication problems on issues like these, and not having someone tell you these things makes it a lot more stressful for you. I'd be happy to help with this if either of you would like.

> PS. Reviewer are not expected to try the patch -- AFAIK that's never been a
> requirement.

Thanks for clarifying that. I talked with some others, and they all agreed that testing anything but the smallest patches is strongly recommended, as it may even help with understanding the patch and preventing mistakes from slipping through which could have been easily caught. I mentioned this because comment 3 seemed to me to mean that Greg wasn't confident in this patch not breaking anything. But he has since clarified that.

> In the perfect world of test-driven engineering we should have
> automation coverage for that (and QA coverage for the rest). That said, I
> agree we are not in the prefect world and there isn't an obvious way to test
> interactions on canvas -- in that case reviewer can make her/his own
> decisions on what to do with it.

I think this is oversimplifying the issue, and it's reasonable to expect at least some test coverage here. Even if canvas was untestable, there is other functionality here that is. For example, event dispatching, touch listeners, and calling private functions.

FWIW, there are two ways I can think of to test canvas interactions, though I agree that neither is clean. 1) Hooking canvas functions with sinon, and 2) poor-man's reftests using dataURL's.

> I am setting review request flag to you -- you are free to complete the
> review or relinquish the right to do so.

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #9)
> Actually, re-read bug 928740 comment 53, I realized many of the issues can
> be fixed in this bug, since you are one of the reviewer for the patch.

Unfortunately, I think this is missing the issue. To me, it makes more sense for us to step back and think about where we should be. Would you agree that ideally, module owners, peers, and peers-in-training should:

1. have specialized knowledge in their components and should use this to do reviews on the code in their modules,
2. they should do thorough reviews that catch most mistakes and they should understand every line of code or pretty close to that,
3. push for at least some new tests, possibly covering every bit of new functionality?

If so, what makes the most sense to me here is this:

* I do a full review of the changes to the dialer component, and do a very shallow review of the shared code, since I don't truly understand it. If I were to do a full review of the shared code, I would have to spend hours learning about it, just to review it. See point 1 above.
* Greg does another review of this, though more thorough than last time, perhaps guided by you. He knows the most about this code, so it makes the most sense for him to review it. See point 2 above.
* Greg suggests some tests that should be added so that we can have at least some test coverage here. See point 3 above.

There's no rush here, since this is a code cleanup bug with no deadline. I think it's best if we use this time to get it right and teach people in the process.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 11 • 10 years ago

Right. I see what you meant and I agree with your proposal above. Let's work make the code better in this bug. John, I am available for feedback? if you need me.

	Doug Sherk (:drs) (inactive)
	Comment 12 • 10 years ago

Comment on attachment 8483339 [details]
Patch (PR @ GitHub)

Ok, in that case, I'm re-setting the review flag for Greg. I'll come back to this once it's more solidified, since there may be more changes from Greg's review.

I think we've covered all of the areas for improvement collectively in our posts, but if anyone has any questions I'd be happy to help or discuss this further.

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 13 • 10 years ago

Comment on attachment 8483339 [details]
Patch (PR @ GitHub)

While to be addressed with some failures is good for me, to be misunderstood and ignored in the following discussions is hard to accepted, especially when you're purposing some peace treaty without the country involved in.

1. If you think a man without peer title can still review patch but only under someone's guide (as you mentioned), please push this to all Gaia developers *publicly*. Or, I think you're personally pick me up as a prisoner under paroling, while you're now playing the police, judge and the jury at the same time. 

2. To find someone's defected review doesn't mean you have the absolute right to apply your opinions upon others. If this is the game we're playing for the throne, I'm glad to join it to spend most of my time finding other defected reviews, and to see if Gaia developers only blame whom in the lowest tier of this feudal society. Also, you don't trust me with one failure not means I failed at every patches I've reviewed. If this is what you're thinking about.

3. What I mentioned is to add new test *files*, you missed what I mean is to resolve this part in another bug as we usually do, since the purpose of this bug is to remove the workaround, not to add new test at all. If this bug involve modifying tests, it of course should be added in the patch. But to add a whole new tests should be in another bug.

4. I would like to remove the workaround with perhaps some test modifications *only*, and leave other code clean-up in other bugs. To do too much things in the same bug is one of the major cause we've failed in the past. I don't want this happen again.

5. It would be meaningless to let me review again, since you as another reviewer apparently don't think I'm a qualified one to review the patch, and you've pointed out the issues like indentations and dead comments. I don't like to be blamed again because you think I'm not trustable. So, please assign to whomever you trusted, with the shining title and the prestige you recognize.

Believe me, I don't type these words emotionally (see, my keyboard are still sticked on my MacBook Air), but to be an untitled developer with some failures doesn't mean he should be monitored as a silly child. What you've addressed is good for me to learn how to review a patch, as I discussed in the private Skype channel. But your other words don't, especially you consistently think I'm not qualified, because I need to do that under guidance. I'm not too pride to reject other's teaching and guidance, but it is actually poisoned if you insist it based on the untrusting and the failure.

	Doug Sherk (:drs) (inactive)
	Comment 14 • 10 years ago

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #13)
> While to be addressed with some failures is good for me, to be misunderstood
> and ignored in the following discussions is hard to accepted, especially
> when you're purposing some peace treaty without the country involved in.
> 
> 1. If you think a man without peer title can still review patch but only
> under someone's guide (as you mentioned), please push this to all Gaia
> developers *publicly*. Or, I think you're personally pick me up as a
> prisoner under paroling, while you're now playing the police, judge and the
> jury at the same time. 

> Believe me, I don't type these words emotionally (see, my keyboard are still
> sticked on my MacBook Air), but to be an untitled developer with some
> failures doesn't mean he should be monitored as a silly child. What you've
> addressed is good for me to learn how to review a patch, as I discussed in
> the private Skype channel. But your other words don't, especially you
> consistently think I'm not qualified, because I need to do that under
> guidance. I'm not too pride to reject other's teaching and guidance, but it
> is actually poisoned if you insist it based on the untrusting and the
> failure.

Greg, I can see why you feel this way. Perhaps you could tell us your side of this, as I'd like to hear your thoughts. You're right that it was inconsiderate of us to leave you out of the discussion before deciding on a course of action.

I'm no stranger to this either. I've had to have bugs redirected to me from the dialer peers since I'm not a dialer peer yet. When I'm not sure about something, I ask the other peers. It's just part of the learning process and Gaia policy. It doesn't mean anything about my or your competence and I think it's great that we're both working towards the same goal.

> 2. To find someone's defected review doesn't mean you have the absolute
> right to apply your opinions upon others. If this is the game we're playing
> for the throne, I'm glad to join it to spend most of my time finding other
> defected reviews, and to see if Gaia developers only blame whom in the
> lowest tier of this feudal society. Also, you don't trust me with one
> failure not means I failed at every patches I've reviewed. If this is what
> you're thinking about.

> 5. It would be meaningless to let me review again, since you as another
> reviewer apparently don't think I'm a qualified one to review the patch, and
> you've pointed out the issues like indentations and dead comments. I don't
> like to be blamed again because you think I'm not trustable. So, please
> assign to whomever you trusted, with the shining title and the prestige you
> recognize.

There's nothing wrong with not knowing something, especially when nobody tells you. Indeed, as I mentioned before, Mozilla tends to have communication problems with issues like this. It's unfortunate that this adds stress to you and others who are thrown into situations like this.

However, I think you may have misinterpreted what I said here. I really don't think you're a bad reviewer or developer. Instead, my understanding is that you were just not told about the Gaia policies. I wanted to correct this here by laying it out clearly so that we can try again. If you don't know what's expected, then nobody can blame you for what happens. Even if you missed some things, that's understandable if you didn't spend as much time on the review as is expected.

> 3. What I mentioned is to add new test *files*, you missed what I mean is to
> resolve this part in another bug as we usually do, since the purpose of this
> bug is to remove the workaround, not to add new test at all. If this bug
> involve modifying tests, it of course should be added in the patch. But to
> add a whole new tests should be in another bug.
>
> 4. I would like to remove the workaround with perhaps some test
> modifications *only*, and leave other code clean-up in other bugs. To do too
> much things in the same bug is one of the major cause we've failed in the
> past. I don't want this happen again.

Sure, that's a great point. I've filed bug 1064696 for this.

In closing, I really want the best for everyone here. I apologize if I came off as overly argumentative and blaming at first. If you're up for it, I'd like to talk on Vidyo or IRC sometime and hopefully resolve any lingering issues. What do you think we should do next?

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 15 • 10 years ago

I think it would be better if you and John updating the patch and reviewing it first. Then, if you think there are something you're not sure, you can set the review flag after you complete your review. Since I think you have caught the general issues, and the reset would be some cases about the slider itself, if they exist.

And, for the new test, what I'm worrying now is I have no time to take it at this stage (working on LockScreen-as-iframe now). So if we want it happen soon, we need to ask someone to take it. Or, it would be arranged at least after the as-iframe-bug.

	Doug Sherk (:drs) (inactive)
	Comment 16 • 10 years ago

Greg, your suggestion basically brings us back to where we started. The point here was for you to learn the Gaia policies, and it seems that your suggestions prevent us from actually doing that.

As I mentioned, I don't have specialized experience here on the lockscreen slider, but you do. I understand if you didn't spend a long time on the review and that's why you missed some things. But if you reviewed it again and allocated more time so that you could catch these things, then we'd be closer to the level of review that is expected.

Gaia has a minimum level of code quality that is expected. There's always the trade-off of quality vs. quantity. We shouldn't be sacrificing quality in order to get more quantity. In this case, a more thorough review should take you 1 hour at most. Reviewing bug 1064696, including suggesting tests, should also take 1-2 hours for you. I don't think this is unreasonable to ask, even if you have other commitments.

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 17 • 10 years ago

Yes, I know what you mean, what I concerned is I may do duplicated things after your opinions that already on the GitHub pages, if you set the flag now. So I suggest to let you finish your review which now seems started half way, and I can see the following things (until your review+ for John's new patch). I don't say it's improper to do the review, I just want to put the it in the right order. If you set the flag now, and I pick what John may update for your reviewing opinions, it's would be a nulled work.

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 18 • 10 years ago

And what I mean no time to take the test bug is to write the code, not to review it, and it's only for this time (while handling the bug I mentioned).

	John Lu [:mnjul][Please NI or I'll miss] Reporter
	Comment 19 • 10 years ago

Please give me some more time on this as I'll have to deal with comments and logics not written by me (i.e. just moved around by me when I was doing this and bug 950884).

	Doug Sherk (:drs) (inactive)
	Comment 20 • 10 years ago

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #17)
> Yes, I know what you mean, what I concerned is I may do duplicated things
> after your opinions that already on the GitHub pages, if you set the flag
> now. So I suggest to let you finish your review which now seems started half
> way, and I can see the following things (until your review+ for John's new
> patch). I don't say it's improper to do the review, I just want to put the
> it in the right order. If you set the flag now, and I pick what John may
> update for your reviewing opinions, it's would be a nulled work.

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #18)
> And what I mean no time to take the test bug is to write the code, not to
> review it, and it's only for this time (while handling the bug I mentioned).

Ok, that makes sense. Thanks for clarifying.

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #19)
> Please give me some more time on this as I'll have to deal with comments and
> logics not written by me (i.e. just moved around by me when I was doing this
> and bug 950884).

Sure, there's no rush here. Do you think you'll have time to take bug 1064696 after you come back to this?

	Doug Sherk (:drs) (inactive)
	Comment 21 • 10 years ago

Comment on attachment 8483339 [details]
Patch (PR @ GitHub)

Clearing my review flag for now. Please ask again once my previous round of comments are addressed.

	John Lu [:mnjul][Please NI or I'll miss] Reporter
	Comment 22 • 10 years ago

(In reply to Doug Sherk (:drs) from comment #20)
> Sure, there's no rush here. Do you think you'll have time to take bug
> 1064696 after you come back to this?

Not yet sure about that. What I'm afraid is that we might have to do some refactoring, or at least chop functions into smaller pieces, for efficiently-testable codes/blocks, since the codes out there are a bit ill-structured... I will try to assess such need when I do this bug.

For this bug, I intend to (only) deal with the removal of dead code, and address your concerns on obvious issues, such as obscure comments and ill-formed coding styles. We can file follow-up bugs if needed (for example, moving this from being gjslint-linted to jshint-linted).

	BMO Automation
	Comment 23 • 6 years ago

Firefox OS is not being worked on