This is an enhancement idea I'm breaking off of bug 961523. We may want this if we ever want to add telemetry to promo banners to see how often users hit the close button.

This will let us do things like never show a particular snippet again if a user hits the close button while that message is showing. Could also be useful for metrics! I'll add a test case for this, too.

This builds on top of my patch for bug 935264, so I won't ask for review until that bug is done. My one concern with this is that if we include logic to make the close button semi-permanently dismiss the banner, the test cases I included below to make sure the message is removed won't actually be effective... maybe I should re-add a message, then only test this close button behavior at the very end of the test.

Rebased on top of my new patch for bug 935264. I also decided to put this at the bottom of the test, since in the near future, clicking the close button will result in the banner going away for an extended period of time (bug 961523). I added a call to remove the banner message again at the end, not sure if we really need to do cleanup like this for robocop tests, but I figure it's better to be safe than sorry (although this does take a tiny amount of time).

Comment on attachment 8375860 [details] [diff] [review] test (v2) Review of attachment 8375860 [details] [diff] [review]: ----------------------------------------------------------------- r+ after the cleanup code issue is cleared up. ::: mobile/android/base/tests/components/AboutHomeComponent.java @@ +120,5 @@ > + public AboutHomeComponent dismissBanner() { > + assertBannerVisible(); > + > + mTestContext.dumpLog(LOGTAG, "Clicking on HomeBanner close button."); > + mSolo.clickOnView(getHomeBannerView().findViewById(R.id.close)); If the code to manipulate the home banner gets any more extensive, consider putting it into an inner class in future tests (or a mentored bug ;). ::: mobile/android/base/tests/testHomeBanner.java @@ +53,5 @@ > + > + NavigationHelper.enterAndLoadUrl("about:home"); > + mAboutHome.assertVisible(); > + > + // Test to make sure the ondismiss handler is called when the close button is clicked. Explain that we attach an ondismiss handler in js that sends the "MessageDismissed" message (so it's more apparent that this is not built-in and this is actually testing the handler). @@ +61,5 @@ > + > + mAboutHome.assertBannerNotVisible(); > + > + // Remove the banner message to clean up. > + removeBannerMessage(); Why do you want to clean up? Is that just so if we add more to this test, we are reset to the initial state? If so, consider commenting it out and add a comment explaining why the next developer may want to use it.

Sorry, I need to learn to read. (In reply to :Margaret Leibovic from comment #3) > Rebased on top of my new patch for bug 935264. I also decided to put this at > the bottom of the test, since in the near future, clicking the close button > will result in the banner going away for an extended period of time (bug > 961523). Add a comment to the bottom saying you may want to add additional code to the middle (and probably comment where the final test starts). Additionally, you may want to throw each thing we test on the home banner into helper methods (e.g. `closeBannerTest()`) so it's really obvious what's going on and it makes these comments easier to follow. > I added a call to remove the banner message again at the end, not sure if we > really need to do cleanup like this for robocop tests, but I figure it's > better to be safe than sorry (although this does take a tiny amount of time). It shouldn't be necessary (we already completely explode the profile and restart Fennec for each new test) and considering how long it takes for the suite to run, I'd prefer to avoid adding unnecessary cleanup. If later we need the cleanup, we'll cross that bridge once when we get to it.

Comment on attachment 8375860 [details] [diff] [review] test (v2) Review of attachment 8375860 [details] [diff] [review]: ----------------------------------------------------------------- Switching to r+ if you remove the cleanup code. I really hope you'll consider using the helper methods too, though! I think it will greatly improve readability of the test and makes it obvious what facets we've already tested (making it easier to add new targetted tests later).

I removed the cleanup code. I think cleanup code is just a force of habit from desktop tests. Here's a follow-up patch to split up testHomeBanner into smaller methods. What do you think?

Comment on attachment 8376470 [details] [diff] [review] Split testHomeBanner into smaller test methods Oops, it doesn't even build! I'll post a new version shortly.

Comment on attachment 8376479 [details] [diff] [review] Split testHomeBanner into smaller test methods Review of attachment 8376479 [details] [diff] [review]: ----------------------------------------------------------------- lgtm w/ nits. remove* and dismiss* are a tad ambigous - I lean towards vebosity over ambiguity (e.g. `removeHomeBannerThroughJSTest()`). ::: mobile/android/base/tests/testHomeBanner.java @@ +23,5 @@ > + // These test methods depend on being run in this order. > + addBannerTest(); > + removeBannerTest(); > + > + // Always run dismissBannerTest last, since it will have a semi-permanent effect on the banner. Explain what this effect is (if it's likely to change, be really general about it or give an example as to one possibility for dismissal) so developers are better informed as to whether or not they should disobey this warning. :P nit: Rather than explicitly use the test name, I'd say, "Always run the dismiss banner test..." - the name is more likely to change than the functionality. @@ +55,2 @@ > > + private void removeBannerTest() { Add a javadoc comment mentioning the assumed initial state of the system at the beginning of this test (and the others). @@ +68,5 @@ > > NavigationHelper.enterAndLoadUrl("about:home"); > mAboutHome.assertVisible(); > > // Test to make sure the ondismiss handler is called when the close button is clicked. Just wanted to point out (again) that this doesn't mention that we attach the ondismiss handler in js for this test because I guess it bothers me. :P

(In reply to Michael Comella (:mcomella) from comment #10) > Comment on attachment 8376479 [details] [diff] [review] > Split testHomeBanner into smaller test methods > > Review of attachment 8376479 [details] [diff] [review]: > ----------------------------------------------------------------- > > lgtm w/ nits. > > remove* and dismiss* are a tad ambigous - I lean towards vebosity over > ambiguity (e.g. `removeHomeBannerThroughJSTest()`). > > ::: mobile/android/base/tests/testHomeBanner.java > @@ +23,5 @@ > > + // These test methods depend on being run in this order. > > + addBannerTest(); > > + removeBannerTest(); > > + > > + // Always run dismissBannerTest last, since it will have a semi-permanent effect on the banner. > > Explain what this effect is (if it's likely to change, be really general > about it or give an example as to one possibility for dismissal) so > developers are better informed as to whether or not they should disobey this > warning. :P Basically the effect is that it's dismissed semi-permanently, I suppose I can update the comment to say just that. > @@ +68,5 @@ > > > > NavigationHelper.enterAndLoadUrl("about:home"); > > mAboutHome.assertVisible(); > > > > // Test to make sure the ondismiss handler is called when the close button is clicked. > > Just wanted to point out (again) that this doesn't mention that we attach > the ondismiss handler in js for this test because I guess it bothers me. :P I would hope any developer looking at this test would also be looking at (and understand) robocop_home_banner.html as well, since that's a critical part of how the test actually does anything at all. Also, noticing a general theme with review comments: maybe I put too much faith in institutional knowledge and our review system, but I feel that those will always be more effective than trying to document everything. But I do appreciate your nagging, and I will try to be better :)

Added more test comments, and rebased the original patch to account for bug 968378. https://hg.mozilla.org/integration/fx-team/rev/15f403785e01 https://hg.mozilla.org/integration/fx-team/rev/c626d2abe2e8 https://hg.mozilla.org/integration/fx-team/rev/293c35d95ab9

https://hg.mozilla.org/mozilla-central/rev/15f403785e01 https://hg.mozilla.org/mozilla-central/rev/c626d2abe2e8 https://hg.mozilla.org/mozilla-central/rev/293c35d95ab9

We'll need to uplift this in order to uplift bug 961523 and bug 975423.

Comment on attachment 8374470 [details] [diff] [review] add "ondismiss" to Home.banner.add options [Approval Request Comment] Bug caused by (feature/regressing bug #): we need to uplift this for bug 961523 and bug 975423 to work User impact if declined: messages will re-appear after users dismissed them Testing completed (on m-c, etc.): landed on m-c 2/14 Risk to taking this patch (and alternatives if risky): low-risk, this patch just adds some hooks to the Home.banner API String or IDL/UUID changes made by this patch: none

Also uplifted tests: https://hg.mozilla.org/releases/mozilla-aurora/rev/478823afeb53 https://hg.mozilla.org/releases/mozilla-aurora/rev/385c21263734