1) User RV's an item 2) User Bookmarks said item 3) See this helper UI about Bookmarks integration 4A) DISMISS -> press outside helper, see snackbar confirmation 4B) BOOKMARKS -> press big orange button, go to Bookmarks -> Reading List folder

^ re-uses our helper UI styles from https://bugzilla.mozilla.org/show_bug.cgi?id=1217174#c63 Will attach the graphic next.

Note: Copy TBD

Copy finalized.

This provides a basic helper UI that can be customised with images/text. We need a very similar helper for both reader-view offline bookmarking related helpers (Bug 1236328 and Bug 1247689), hence it's useful to have a common class implementing most of the required functionality. Most of the new helper is borrowed from the existing HomeScreenPrompt. I will extract the common functionality in a followup Bug. Review commit: https://reviewboard.mozilla.org/r/47999/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47999/

Review commit: https://reviewboard.mozilla.org/r/48001/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/48001/

Comment on attachment 8743717 [details] MozReview Request: Bug 1246238 - Pre: Implement SimpleHelperUI r?sebastian https://reviewboard.mozilla.org/r/47999/#review45155 It looks like you are trying to create one implementation for all helper UIs. I wonder if you are going to succeed with that: HomeScreenPrompt has additional code to load the icon (no "resource") and TabQueuePrompt currently has a lot of permission code in there (even though this could be simplified). Maybe it makes more sense to create a "base" activity that creates the basic layout, handles the animations, touching outside, clicking the close button, has helpful methods and the different prompts extend from it, inject their own layout in the shell provided by the base class and then do whatever they want to do. What's nice about this is that it is more flexible and you can handle the success/rejection case in the prompt (if possible) and do not necessarily need to return something and this is again handled somewhere else. However this is a step in the direction (r+) and we can transform this in a follow-up bug if you agree. ::: mobile/android/base/java/org/mozilla/gecko/promotion/SimpleHelperUI.java:100 (Diff revision 1) > + private void setupViews() { > + final Intent i = getIntent(); > + > + setContentView(R.layout.simple_helper_ui); > + > + ((ImageView) findViewById(R.id.image)).setImageResource(i.getIntExtra(EXTRA_IMAGE, -1)); I've seen 0 used as "no resource id" but never -1. Note that resource ids can be negative, so maybe -1 could even be a valid resource? Let's avoid settig something if there's nothing to set. ::: mobile/android/base/java/org/mozilla/gecko/promotion/SimpleHelperUI.java:110 (Diff revision 1) > + > + ((Button) findViewById(R.id.button)).setText(i.getIntExtra(EXTRA_BUTTON, -1)); > + > + containerView = findViewById(R.id.container); > + > + findViewById(R.id.button).setOnClickListener(new View.OnClickListener() { NIT: As soon as you search the same view by id twice it might be worth keeping a reference to the result.

Comment on attachment 8743718 [details] MozReview Request: Bug 1246238 - Show helper UI first time a reader view page is bookmarked r?sebastian https://reviewboard.mozilla.org/r/48001/#review45157 ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:378 (Diff revision 1) > + final SharedPreferences prefs = GeckoSharedPrefs.forProfile(this); > + > + final boolean hasFirstReaderViewPromptBeenShownBefore = prefs.getBoolean(SimpleHelperUI.PREF_FIRST_RVBP_SHOWN, false); > + > + if (hasFirstReaderViewPromptBeenShownBefore) { > - showReaderModeBookmarkAddedSnackbar(); > + showReaderModeBookmarkAddedSnackbar(); > + } else { > + SimpleHelperUI.show(this, > + SimpleHelperUI.FIRST_RVBP_SHOWN_TELEMETRYEXTRA, > + ACTIVITY_REQUEST_FIRST_READERVIEW_BOOKMARK, > + R.string.helper_first_offline_bookmark_title, R.string.helper_first_offline_bookmark_message, > + R.drawable.helper_first_readerview_bookmark, R.string.helper_first_offline_bookmark_button, > + ACTIVITY_RESULT_FIRST_READERVIEW_BOOKMARKS_GOTO_BOOKMARKS, > + ACTIVITY_RESULT_FIRST_READERVIEW_BOOKMARKS_IGNORE); > + > + GeckoSharedPrefs.forProfile(this) > + .edit() > + .putBoolean(SimpleHelperUI.PREF_FIRST_RVBP_SHOWN, true) > + .apply(); > + } (Like comment in bug 1247689): I would like to see this moved into its own class.

Since this hasn't closed yet, I just want to update this image. The other images will be replaced in bug 1266899.

https://reviewboard.mozilla.org/r/47999/#review45155 Yup: in this case I knew I was implementing 2 helper UIs that were identical except for the text in them, I doubt we'll be able to reuse the layout for any of the existing prompts. I'm guessing we *might* have more prompts in future in which case the SimpleHelperUI might be useful, but for now I just wanted to share code for these two prompts. My main aim in Bug 1266182 is to refactor out the actual common functionality (i.e. the "base" activity you mention), I'll copy your comments to there. I didn't want to do too much in this direction as part of this bug since I was focussing on getting this into 48!

(In reply to Sebastian Kaspari (:sebastian) from comment #8) > (Like comment in bug 1247689): I would like to see this moved into its own > class. Because of uplifts / string-freeze (which I think is tomorrow), and to ensure this makes into 48, I've decided to land as is (but with updated images). It'll be much easier to move this code into a separate class once the BrowserAppDelegate lands (which I'll need to extend with an API for handling onActivityResult), so I'll probably wait for that to land, and then cleanup as part of bug 1247689.

https://hg.mozilla.org/integration/fx-team/rev/d0bed027367312449a35fb4f3a50b68a250545d9 Bug 1246238 - Pre: Implement SimpleHelperUI r=sebastian https://hg.mozilla.org/integration/fx-team/rev/f9177e655c8c7c3250087d71c0b6fed5e335e1b5 Bug 1246238 - Show helper UI first time a reader view page is bookmarked r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/ecdcf1026a3ba4bcd21304627a0eceba8f285ba0 Bug 1246238 - Post: make checkstyle happy r=me

https://hg.mozilla.org/mozilla-central/rev/d0bed0273673 https://hg.mozilla.org/mozilla-central/rev/f9177e655c8c https://hg.mozilla.org/mozilla-central/rev/ecdcf1026a3b

And here's a screenshot from the version that landed!

Verified as fixed using: Device: Nexus 6 (Android 6.0) Build: Firefox for Android 48.0a2 (2016-04-28)