1235902 - Support actions from links in hosted feedback page

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

We specifically need something to happen when…

a) Someone clicks on “Maybe later”
b) Someone clicks on “No thanks”
c) Someone clicks on “Go to Play Store”

Comment 1

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1235902 - Support actions from links in hosted feedback page. r=mfinkle

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

Comment 2

[:Margaret Leibovic]

This works with my test page here:
http://people.mozilla.org/~mleibovic/feedback.html

This patch isn't great because it adds these event listeners to the browser deck no matter what... maybe I should try to be lazier about it.

I tried adding the listeners in the old "Feedback:Show" observer, but the problem with that is that in that case the links would only work when we prompt the user for feedback, vs. them going to the feedback page directly.

I'll also want to write a follow-up patch to read from this URL pref instead of loading about:feedback directly from the "Feedback" item in settings.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8703002 [details]
MozReview Request: Bug 1235902 - Support actions from links in hosted feedback page. r=mfinkle

https://reviewboard.mozilla.org/r/29243/#review26043

::: mobile/android/chrome/content/browser.js:3272
(Diff revision 1)
> +var Feedback = {

Why not continue to use Feedback.js and lazy load it? We want less in browser.js, right?

::: mobile/android/chrome/content/browser.js:3282
(Diff revision 1)
> +    BrowserApp.deck.addEventListener("FeedbackClose", this, false, true);

You could add these directly to the tab.browser when adding it, and remove them when closing it.

::: mobile/android/chrome/content/browser.js:3288
(Diff revision 1)
> +    if (aTopic !== "Feedback:Show")

\{ \}

::: mobile/android/chrome/content/browser.js:3298
(Diff revision 1)
> +    BrowserApp.addTab(url, { selected: true, parentId: BrowserApp.selectedTab.id }).browser;

selectOrAddTab might be better, just in case

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6003

Comment 4

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Comment on attachment 8703002 [details]
> MozReview Request: Bug 1235902 - Support actions from links in hosted
> feedback page. r=mfinkle
> 
> https://reviewboard.mozilla.org/r/29243/#review26043
> 
> ::: mobile/android/chrome/content/browser.js:3272
> (Diff revision 1)
> > +var Feedback = {
> 
> Why not continue to use Feedback.js and lazy load it? We want less in
> browser.js, right?

There's not much point to lazy-loading it if we need to add listeners on startup.

> ::: mobile/android/chrome/content/browser.js:3282
> (Diff revision 1)
> > +    BrowserApp.deck.addEventListener("FeedbackClose", this, false, true);
> 
> You could add these directly to the tab.browser when adding it, and remove
> them when closing it.

So this is what I mentioned in my last comment - if we do this, these links will only work when we prompt them directly here. It mainly depends on how we expect people to be getting at this feedback page. If this is the case, we should probably use a URL parameter to only show these links when the user is prompted.

Alternately, we could try adding this logic in content.js, where we already listen for the "AboutReaderContentLoaded" event, and create a similar "FeedbackContentLoaded" event that adds the additional listeners. But then we're trading off one listener on the deck vs. a different listener on every content window.

I'll keep thinking about this.

Comment 5

[Will Kahn-Greene [:willkg] ET needinfo? me]

So, the current plan is that the form that Justin is building on Input will become *the* Firefox for Android feedback form. 

People come from various places to leave feedback currently, like the release notes page, peoples' blog posts, etc. If a user was coming from the releasenotes page, it seems weird to be greeted with a form that has "Maybe later" and "No thanks".

I think a querystring param is probably the easiest way to control whether those links show where "easiest" is defined as "easy to implement and easy to test the various possibilities".

Comment 6

[:Margaret Leibovic]

Comment on attachment 8703002 [details]
MozReview Request: Bug 1235902 - Support actions from links in hosted feedback page. r=mfinkle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29243/diff/1-2/

Comment 7

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1235902 - (Part 2) Update feedback settings item to read URL from gecko prefs. r=mfinkle

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

Comment 8

[:Margaret Leibovic]

I updated the patch to only add the listeners in the case where we open the tab directly ourselves.

In order to test this with a development page, you'll need to use the remote console to trigger the browser to open the page directly. To do this, in the main process, run the following code:

Services.obs.notifyObservers(null, "Feedback:Show", null);

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8703002 [details]
MozReview Request: Bug 1235902 - Support actions from links in hosted feedback page. r=mfinkle

https://reviewboard.mozilla.org/r/29243/#review26637

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8703202 [details]
MozReview Request: Bug 1235902 - (Part 2) Update feedback settings item to read URL from gecko prefs. r=mfinkle

https://reviewboard.mozilla.org/r/29317/#review26639

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
(Diff revision 1)
> -    private static final String PREFS_ADVANCED = NON_PREF_PREFIX + "advanced.enabled";

Did you want to remove these?

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
(Diff revision 1)
> -    private static final String PREFS_CATEGORY_HOMEPAGE = NON_PREF_PREFIX + "category_homepage";

And this one

Comment 11

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #10)
> Comment on attachment 8703202 [details]
> MozReview Request: Bug 1235902 - (Part 2) Update feedback settings item to
> read URL from gecko prefs. r=mfinkle
> 
> https://reviewboard.mozilla.org/r/29317/#review26639
> 
> :::
> mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
> (Diff revision 1)
> > -    private static final String PREFS_ADVANCED = NON_PREF_PREFIX + "advanced.enabled";
> 
> Did you want to remove these?
> 
> :::
> mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
> (Diff revision 1)
> > -    private static final String PREFS_CATEGORY_HOMEPAGE = NON_PREF_PREFIX + "category_homepage";
> 
> And this one

They're unused, so I was just doing some clean-up while I was in here.

Comment 12

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/f2c54cd55ed5ee6baecfe6b195292ad682167f5f
Bug 1235902 - Support actions from links in hosted feedback page. r=mfinkle

https://hg.mozilla.org/integration/fx-team/rev/deaf711a7bc8a8cb604666b0e4e7a91edc90018e
Bug 1235902 - (Part 2) Update feedback settings item to read URL from gecko prefs. r=mfinkle

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f2c54cd55ed5
https://hg.mozilla.org/mozilla-central/rev/deaf711a7bc8