Closed Bug 1024807 Opened 10 years ago Closed 10 years ago

ship the webcompat.com reporter to nightly users

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(fennecNightly+)

VERIFIED FIXED
Firefox 36
Tracking Status
fennec Nightly+ ---

People

(Reporter: blassey, Assigned: miketaylr, Mentored)

References

Details

(Keywords: feature)

Attachments

(4 files, 6 obsolete files)

this addon makes it easy to report site compatability issues from inside fennec by pre-populating feilds and guiding you through the process. The issues wind up looking like this: 
https://github.com/webcompat/web-bugs/issues/129

and are actively managed by the web compat team
Assignee: nobody → miket
Mentor: wjohnston
tracking-fennec: ? → 33+
Flags: needinfo?(miket)
Pong?
Man. This totally slipped under my radar... even though snorp NI'd me. Wes, are there any other examples of add-ons shipped in Fennec that I can peek at?
Ah, just found https://github.com/mozilla/gecko-dev/commit/04503ec93afc47dbd3b7960f12bce2595cba70b8. Would it be inappropriate to put the extension files under /mobile/android/extensions, or is it better to stick the mobile-only add-on in /browser/extensions next to shumway and pdfjs?
We used to have some extensions in there. If it works for you I'd be fine with it in mobile/android/extensions (in fact, you're more likely to get help from our devs with it there).
Flags: needinfo?(wjohnston)
Cool, will jam it in there and try to get it running.
OK, so I'm closer to understanding this but still not quite there. My current level of understanding of what I need to do:

* Add add-on files to /mobile/android/extensions, in webcompat-reporter/ (bootstrap.js, install.rdf, etc) 
* In mobile/android/extensions/Makefile.in, package up the add-on files into an XPI and stick it in the right place (@BINPATH@/extensions/ maybe?)
* Modify /mobile/android/installer/package-manifest.in, so the installer picks it up

> #ifdef NIGHTLY_BUILD
> @BINPATH@/extensions/webcompat-reporter.xpi
> #endif
 
Unsure what the next step it, but I'll see if I can get this far.
OK, I've got Make mostly under control: 

https://github.com/miketaylr/gecko-dev/commit/120e135ea304f007fcc9d16aa0909107553e45bc

This patch will build the xpi and put it in /bin/extensions, but it's not visible from about:addons when I install fennec. 

Wes, any idea what I'm doing wrong here? I was suspicious it was my lazy `<em:maxVersion>*</em:maxVersion>` in install.rdf, but if I try to install the XPI that the Makefile generates, everything works as expected.

Unrelated, the nativeWindow.menu's icon doesn't seem to work anymore (?): https://github.com/miketaylr/gecko-dev/commit/120e135ea304f007fcc9d16aa0909107553e45bc#diff-e40d31426cedc4bbc7ed1c71cae7b706R56. Should probably just remove it to make the patch smaller. And should file a bug.
tracking-fennec: 33+ → +
(In reply to Mike Taylor ~*:miketaylr*~ from comment #7)
> Unrelated, the nativeWindow.menu's icon doesn't seem to work anymore (?):
> https://github.com/miketaylr/gecko-dev/commit/
> 120e135ea304f007fcc9d16aa0909107553e45bc#diff-
> e40d31426cedc4bbc7ed1c71cae7b706R56. Should probably just remove it to make
> the patch smaller. And should file a bug.
Looking at the rest, but you should leave this. It will still appear on Gingerbread Android devices. We just removed icons starting in Honeycomb+.
Flags: needinfo?(wjohnston)
Sorry, I talked to Mossop a bit about this (he might be a better person to ask). He said that on upgrades Firefox looks in the distribution directory for addons. In our case, that's from:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/DirectoryProvider.js#52

i.e. "/system/@ANDROID_PACKAGE_NAME@/distribution" We'll have to move the xpi there to have it installed, or move that directory to where your file is. I'd sorta prefer the later. I don't have a github mc pull to use your stuff easily, but I can if you want me to try.
Attached patch wip.patch (obsolete) — Splinter Review
Uploading my WIP patch that *seems* to do what Mossop suggested, but doesn't do work.
cc'ing Mossop because he'll be more help than me.
So there are two places you could put this add-on with slightly different behaviours:

@BINPATH@/extensions will force the add-on on all users, they won't be able to uninstall it but they will be able to disable it.

/system/@ANDROID_PACKAGE_NAME@/distribution/extensions is scanned whenever the app version of Fennec changes (i.e. a new install or an upgrade to a newer version) and any extensions there are installed into the user profile. Users can then uninstall this add-on and they won't get it pushed on them again if they do so.

Pick which behaviour you want.

In either case the file MUST be named <id>.xpi. So "mobile-reporter@webcompat.com.xpi" in this case. I'm guessing that is where your patch is falling down right now, that and since you're using the distribution directory you'll have to be wiping your profile or increasing the app version in order to see an effect.

If that is still giving you issues then getting a startup log with extensions.logging.enabled should give more details about what the add-ons manager sees and what might be wrong.
(In reply to Dave Townsend [:mossop] from comment #12)

Thanks mossop. 

> In either case the file MUST be named <id>.xpi. So
> "mobile-reporter@webcompat.com.xpi" in this case. I'm guessing that is where
> your patch is falling down right now, 

Yeah, I'm doing that:

> XPI_NAME = 'mobile-reporter@webcompat.com'
> XPI_PKGNAME = $(XPI_NAME)
> $(PYTHON) $(topsrcdir)/config/nsinstall.py $(XPI_PKGNAME).xpi $(DIST)/bin/distribution/extensionsaw

> that and since you're using the
> distribution directory you'll have to be wiping your profile or increasing
> the app version in order to see an effect.

But totally not doing this.

Talking with blassey, I guess there's some code that was removed that works with the XPI once it's in the right place (mfinkle might be able to clarify here).

I've got a patch now to add the functionality as a content script and skip the add-on route, but perhaps we can circle back to the add-on stuff if that gets shot down.
Wes, before I ask for a review I assume I need tests for this. Is Robocop the best way to test a NativeWindow.menu item?
Attachment #8494765 - Attachment is obsolete: true
It is. Testing JS-Java interaction is a bit of a pain though. I wouldn't hold this up if you get caught in it. You can write simple xpcshell style tests by extending JavascriptTest in mobile/android/base/tests and handing out a js filename. See testJNI for example:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testJNI.java
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testJNI.js
Flags: needinfo?(wjohnston)
Comment on attachment 8495460 [details] [diff] [review]
0001-Bug-1024807-Add-Report-Site-Issue-menu-item-for-Nigh.patch

Review of attachment 8495460 [details] [diff] [review]:
-----------------------------------------------------------------

Mike asked me to give some side comments on this to improve his patch.

::: mobile/android/chrome/content/WebcompatReporter.js
@@ +4,5 @@
> +const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +var WebcompatReporter = {

Add module description JSDOC style. example: http://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsOAuthClient.jsm#5

@@ +9,5 @@
> +  menuItem: null,
> +  init: function() {
> +    this.desktopModeListener.init();
> +    this.menuItem = NativeWindow.menu.add({
> +      name: "Report Site Issue",

Translations.

@@ +11,5 @@
> +    this.desktopModeListener.init();
> +    this.menuItem = NativeWindow.menu.add({
> +      name: "Report Site Issue",
> +      icon: "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAADUUlEQVR42sVXXU/TYBjlJ/injAoCguD8QmNUYrwxaiRRE/VC77xSb4x6YdRoYkQDEYOK4hc6cAxlIEScAgobH7KWbmu7tsfndDU0JCau3fQkT/I2a/ec9/k4z/tWKQNb1oj1iuEfWy99V4Vy3t8ktjkUiarSP4qI00Zk3m5AZqCJz7KugRKt57pkEqURoLPoJqjvt0GfaYedn4Gtp2HO90AdakXmXS3fqRwB7lx5vx1WbhKrYZsLUOP7oAjByhHg7mMtyH+7jPzkNRiznShkhgDHAaHPdiDTt5ZRqAABFltsJ4x0NwrKEGyJgp3/AUv7IiTiMBdeQJ++yXf4biUINEId3OM6Nxdfw0h1wZx7KutXRULZr7CNeWTHzkgt1FQmAmpslzj7ILuflt1/h5VNwlISRULpRxKF59ASR1ioFSIwuFscddGk8p+J4zew1FG3E5yCAsdcgjZ6kt1QmRQwAsy9Y2niNIWCmnALMZe8BG3kmOR/hzivr1ARRuuw/PEgPEj+n0jVt6OwFJOuuApL+yw2gez4OQpTuQlE2F5S5Td8BLrhGHPFdfqhRCUHwlLiQmC9fNNcPgIZyqyE3zEzIBwrK7u/T/kBUfgZlVaM4Teyn055JCLlINBM3XcL7jcsdVieX66ooLEIPdWx8qzPQh3Yikx/Q1gCDP06t8j8YMtRhPzQZx+4neCBXcIohCPgDp74Xi+/HmRNAqtBfbBEkPzIjp+VrqgOToA7MFKd8IOql5s4L3nvg0mb6ynK8Mw95Keuwwd2hSdKzaUToPBw8tnmIvyg8BSWBoTAO2RFdPQfd0V8jnMmCJnHkDzADy3RRmEqmYB8VAdt+Aj+BKqePn3LnYb699v+X+ADJyeLOAiBjdJOpxEWJOe1ZAACY+UgcCcogVosJ44iLHLfrpBAkCJsFPVrkVwrCANt9ESgIvROu9XujA8KHlqVfrZgUwACXicsDx8CHDtY+JMXvA6IBJdi5o+tVCIYORYyRSj8NOQucsmLriT/DYxUB88PrKPw09BPQo23uspn56Y4kr1x7EiGdPeEZMghRRtp47tU0tDjeHU6eC9gSvjnQmY/60PsMNQPB0S2I3Rc0nmQFuByGilWdbSBBxUaiQW9pPb+9+v5L/5IMZ9vMn5CAAAAAElFTkSuQmCC",
> +      callback: () => this.reportIssue()

Add `,` commas to avoid future git blame collisions.

@@ +21,5 @@
> +    NativeWindow.menu.remove(this.menuItem);
> +    this.menuItem = null;
> +  },
> +  reportIssue: function() {
> +    let prefix = "http://webcompat.com/?open=1&url=";

Maybe use searchParams and URL. example: http://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsOAuthClient.jsm#57

@@ +34,5 @@
> +    },
> +    uninit: function() {
> +      Services.obs.removeObserver(this, "DesktopMode:Change");
> +    },
> +    observe: function(subject, topic, data) {

Possible to make `observe:`, `uninit`, `init` part of `WebcompatReporter`? like some other modules, example: http://lxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#755

@@ +36,5 @@
> +      Services.obs.removeObserver(this, "DesktopMode:Change");
> +    },
> +    observe: function(subject, topic, data) {
> +      if (topic == "DesktopMode:Change") {
> +        let args = JSON.parse(data);

Make JSON.parse(data); safe

@@ +45,5 @@
> +      }
> +    },
> +    showMessage: function() {
> +      let buttons = [{
> +          label: "Yes",

Translations.

@@ +48,5 @@
> +      let buttons = [{
> +          label: "Yes",
> +          callback: WebcompatReporter.reportIssue
> +        }, {
> +          label: "No thanks",

Translations.

@@ +49,5 @@
> +          label: "Yes",
> +          callback: WebcompatReporter.reportIssue
> +        }, {
> +          label: "No thanks",
> +          callback: () => {}

Add `,` commas to avoid future git blame collisions.

@@ +51,5 @@
> +        }, {
> +          label: "No thanks",
> +          callback: () => {}
> +      }];
> +      let message = "Would you like to report an issue with the mobile site at webcompat.com?";

Translations.

@@ +54,5 @@
> +      }];
> +      let message = "Would you like to report an issue with the mobile site at webcompat.com?";
> +      let options = {
> +        timeout: 3000,
> +        persistence: 0

Add `,` commas to avoid future git blame collisions.
Attached patch 1024807-2.patch (obsolete) — Splinter Review
Uploading patch addressing Vlad's comments. Gonna see if I can write a simple robotium test or two before asking for review. The JS-Java interaction might be a bit over my head for now...
Attachment #8495460 - Attachment is obsolete: true
OK, I think I'm just spinning wheels--it's not clear to me what I would be able to test with xpcshell tests since the WebcompatReporter adds to the native UI menu and testing the menu via Java seems tricky since it was added via JS. 

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testAppMenuPathways.java seems like the logical place to test, but http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/components/AppMenuComponent.java seems like most of it was written with the "Page" menu in mind.

The other difficulty is part of testing this would be to load webcompat over the network, which is discouraged in robocop docs.

Wes, who would the best person be to ask for review for the non-test bits?
I stashed my janky robocop tests here, just to create a record: https://github.com/miketaylr/gecko-dev/commit/60e9436456da29289b2ab03d6f3cc61247618ffc

(I guess I can test at least test isReportableURL and reportIssue with a mocked out addTab as xpcshell tests.)
Attachment #8496383 - Flags: review?(margaret.leibovic)
Comment on attachment 8496383 [details] [diff] [review]
1024807-2.patch

Review of attachment 8496383 [details] [diff] [review]:
-----------------------------------------------------------------

I don't mind reviewing it. In fact, I forgot the question and reviewed it anyway :) mfinkle would be better though :)

If this isn't going to be an add-on, how much do you want to bring it into Fennec code style? How are we going to handle maintenance updates. I don't mind if its the Fennec teams job. Just want to make sure its clear.

::: mobile/android/chrome/content/WebcompatReporter.js
@@ +7,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +var WebcompatReporter = {
> +  menuItem: null,
> +  strings: Services.strings.createBundle("chrome://browser/locale/webcompatReporter.properties"),

We should lazy load these Strings, to avoid hitting startup:

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

@@ +12,5 @@
> +  init: function() {
> +    Services.obs.addObserver(this, "DesktopMode:Change", false);
> +    Services.obs.addObserver(this, "content-page-shown", false);
> +    this.addMenuItem();
> +  },

Our style would add a blank line between methods (and basically between any block of code).

@@ +15,5 @@
> +    this.addMenuItem();
> +  },
> +  uninit: function() {
> +    Services.obs.removeObserver(this, "DesktopMode:Change");
> +    NativeWindow.menu.remove(this.menuItem);

Do you need to check for this.menuItem first?

@@ +28,5 @@
> +        NativeWindow.menu.update(this.menuItem, {enabled: false});
> +      }
> +    }
> +
> +    if (topic === "DesktopMode:Change") {

I'd probably make this an else if and tie the two together.

@@ +38,5 @@
> +    }
> +  },
> +  addMenuItem: function() {
> +    this.menuItem = NativeWindow.menu.add({
> +      name: this.strings.GetStringFromName("webcompat.menu.name"),

We may want to put this in the Page menu. We have a way to do that for tools, but not Page. We should fix that:

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

@@ +39,5 @@
> +  },
> +  addMenuItem: function() {
> +    this.menuItem = NativeWindow.menu.add({
> +      name: this.strings.GetStringFromName("webcompat.menu.name"),
> +      icon: "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAADUUlEQVR42sVXXU/TYBjlJ/injAoCguD8QmNUYrwxaiRRE/VC77xSb4x6YdRoYkQDEYOK4hc6cAxlIEScAgobH7KWbmu7tsfndDU0JCau3fQkT/I2a/ec9/k4z/tWKQNb1oj1iuEfWy99V4Vy3t8ktjkUiarSP4qI00Zk3m5AZqCJz7KugRKt57pkEqURoLPoJqjvt0GfaYedn4Gtp2HO90AdakXmXS3fqRwB7lx5vx1WbhKrYZsLUOP7oAjByhHg7mMtyH+7jPzkNRiznShkhgDHAaHPdiDTt5ZRqAABFltsJ4x0NwrKEGyJgp3/AUv7IiTiMBdeQJ++yXf4biUINEId3OM6Nxdfw0h1wZx7KutXRULZr7CNeWTHzkgt1FQmAmpslzj7ILuflt1/h5VNwlISRULpRxKF59ASR1ioFSIwuFscddGk8p+J4zew1FG3E5yCAsdcgjZ6kt1QmRQwAsy9Y2niNIWCmnALMZe8BG3kmOR/hzivr1ARRuuw/PEgPEj+n0jVt6OwFJOuuApL+yw2gez4OQpTuQlE2F5S5Td8BLrhGHPFdfqhRCUHwlLiQmC9fNNcPgIZyqyE3zEzIBwrK7u/T/kBUfgZlVaM4Teyn055JCLlINBM3XcL7jcsdVieX66ooLEIPdWx8qzPQh3Yikx/Q1gCDP06t8j8YMtRhPzQZx+4neCBXcIohCPgDp74Xi+/HmRNAqtBfbBEkPzIjp+VrqgOToA7MFKd8IOql5s4L3nvg0mb6ynK8Mw95Keuwwd2hSdKzaUToPBw8tnmIvyg8BSWBoTAO2RFdPQfd0V8jnMmCJnHkDzADy3RRmEqmYB8VAdt+Aj+BKqePn3LnYb699v+X+ADJyeLOAiBjdJOpxEWJOe1ZAACY+UgcCcogVosJ44iLHLfrpBAkCJsFPVrkVwrCANt9ESgIvROu9XujA8KHlqVfrZgUwACXicsDx8CHDtY+JMXvA6IBJdi5o+tVCIYORYyRSj8NOQucsmLriT/DYxUB88PrKPw09BPQo23uspn56Y4kr1x7EiGdPeEZMghRRtp47tU0tDjeHU6eC9gSvjnQmY/60PsMNQPB0S2I3Rc0nmQFuByGilWdbSBBxUaiQW9pPb+9+v5L/5IMZ9vMn5CAAAAAElFTkSuQmCC",

If we're putting this in our code, we might as well just put this image in our java resources and reference it from there.

@@ +68,5 @@
> +    }];
> +    let options = {
> +      timeout: 3000,
> +      persistence: 1,
> +    };

I think this will be pretty obtrusive. Maybe we can use a Button Toast here. i.e.

Toast.show({
  label: "Desktop page requested",
  button: {
    label: "Report",
    icon: <optional>,
    callback: () => { }
  }
});

@@ +72,5 @@
> +    };
> +    NativeWindow.doorhanger.show(message, "webcompat-prompt", buttons, BrowserApp.selectedTab.id, options);
> +  },
> +  reportIssue: function(url) {
> +    let webcompatURL = new URL("http://webcompat.com/");

Looks like this is from the add-on sdk. Careful with it?
Attachment #8496383 - Flags: review?(mark.finkle)
Comment on attachment 8496383 [details] [diff] [review]
1024807-2.patch

Review of attachment 8496383 [details] [diff] [review]:
-----------------------------------------------------------------

Wes made a lot of good comments, I don't have much more to add.

This seems generally fine to me, especially if this is just something for Nightly users, but we should check with our UX team about the best way to prompt users. I like wesj's suggestion of a button toast.

::: mobile/android/chrome/content/WebcompatReporter.js
@@ +55,5 @@
> +                             url.startsWith("resource"));
> +  },
> +  reportDesktopModePrompt: function() {
> +    var currentURI = BrowserApp.selectedTab.browser.currentURI.spec;
> +    let getString = (name) => this.strings.GetStringFromName(name);

Couldn't you just pass the function directly like this?

let getString = this.strings.GetStringFromName;
Attachment #8496383 - Flags: review?(margaret.leibovic) → feedback+
+yuan for UX.
(In reply to Wesley Johnston (:wesj) from comment #20) 
> If this isn't going to be an add-on, how much do you want to bring it into
> Fennec code style? How are we going to handle maintenance updates. I don't
> mind if its the Fennec teams job. Just want to make sure its clear.

We should probably align it with Fennec style as much as possible, I think. I can help with maintenance, but am not emotionally attached to the code if others jump in.

> We may want to put this in the Page menu. We have a way to do that for
> tools, but not Page. We should fix that:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#1958

Agreed, that seems like a nice place to put it. Should I open a bug for that?

> @@ +68,5 @@
> > +    }];
> > +    let options = {
> > +      timeout: 3000,
> > +      persistence: 1,
> > +    };
> 
> I think this will be pretty obtrusive. Maybe we can use a Button Toast here.
> i.e.

Yeah, good suggestion. I didn't realize Toasts can have buttons.

> 
> @@ +72,5 @@
> > +    };
> > +    NativeWindow.doorhanger.show(message, "webcompat-prompt", buttons, BrowserApp.selectedTab.id, options);
> > +  },
> > +  reportIssue: function(url) {
> > +    let webcompatURL = new URL("http://webcompat.com/");
> 
> Looks like this is from the add-on sdk. Careful with it?

Not quite, but I see they have something similar. See https://developer.mozilla.org/en-US/docs/Web/API/URL / https://url.spec.whatwg.org/#api
(In reply to :Margaret Leibovic from comment #21)
> Couldn't you just pass the function directly like this?
> 
> let getString = this.strings.GetStringFromName;

Yeah, getting a little too fat arrow fn happy it seems.
Attached patch 1024807-3.patch (obsolete) — Splinter Review
Addressed all bits of feedback (with the exception of putting the menu item in Page sub-menu).
Attachment #8496383 - Attachment is obsolete: true
See Also: → 1077123
Ping. :)
Comment on attachment 8499160 [details] [diff] [review]
1024807-3.patch

Just noticed when I re-uploaded patch it removed r? for mfinkle. Sorry about that.
Comment on attachment 8499160 [details] [diff] [review]
1024807-3.patch

Review of attachment 8499160 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good. String question.

::: mobile/android/base/resources/menu/browser_app_menu.xml
@@ +74,5 @@
>  
> +#ifdef NIGHTLY_BUILD
> +    <item android:id="@+id/webcompat_icon"
> +          android:icon="@drawable/ic_menu_webcompat"/>
> +#endif

I don't think you want/need this.

::: mobile/android/chrome/content/WebcompatReporter.js
@@ +27,5 @@
> +  observe: function(subject, topic, data) {
> +    if (topic === "content-page-shown") {
> +      let currentURI = subject.documentURI;
> +      if (this.isReportableUrl(currentURI)) {
> +        NativeWindow.menu.update(this.menuItem, {enabled: true});

You might want to avoid calling update if there is no change. We're not that smart. I'm not sure what affect it will have on performance.

::: mobile/android/locales/en-US/chrome/webcompatReporter.properties
@@ +7,5 @@
> +webcompat.menu.name=Report Site Issue
> +
> +# LOCALIZATION NOTE (webcompat.reportDesktopMode.message): A " site issue" is a
> +# bug, display, or functionality problem with a webpage in the browser.
> +webcompat.reportDesktopMode.message=Report site issue?

This string bothers me a bit. I'd rather just say "Desktop mode requested", but maybe my toast idea was a bad one. Lets see what finkle thinks. A screenshot might help UX give comments too. You can take them with the ddms or Monitor tools in the android-sdk (in the sdk/tools folder).
Attachment #8499160 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #28)

> Looks pretty good. String question.
> 
> ::: mobile/android/base/resources/menu/browser_app_menu.xml
> @@ +74,5 @@
> >  
> > +#ifdef NIGHTLY_BUILD
> > +    <item android:id="@+id/webcompat_icon"
> > +          android:icon="@drawable/ic_menu_webcompat"/>
> > +#endif
> 
> I don't think you want/need this.

Could be. I don't know much about adding android resources--just trying to emulate what else was going on.
 
> ::: mobile/android/chrome/content/WebcompatReporter.js
> @@ +27,5 @@
> > +  observe: function(subject, topic, data) {
> > +    if (topic === "content-page-shown") {
> > +      let currentURI = subject.documentURI;
> > +      if (this.isReportableUrl(currentURI)) {
> > +        NativeWindow.menu.update(this.menuItem, {enabled: true});
> 
> You might want to avoid calling update if there is no change. We're not that
> smart. I'm not sure what affect it will have on performance.

Good point. Will tweak.

> ::: mobile/android/locales/en-US/chrome/webcompatReporter.properties
> @@ +7,5 @@
> > +webcompat.menu.name=Report Site Issue
> > +
> > +# LOCALIZATION NOTE (webcompat.reportDesktopMode.message): A " site issue" is a
> > +# bug, display, or functionality problem with a webpage in the browser.
> > +webcompat.reportDesktopMode.message=Report site issue?
> 
> This string bothers me a bit. I'd rather just say "Desktop mode requested",
> but maybe my toast idea was a bad one. Lets see what finkle thinks. A
> screenshot might help UX give comments too. You can take them with the ddms
> or Monitor tools in the android-sdk (in the sdk/tools folder).

OK, will upload some screenshots for UX. IMO, I think "Desktop mode requested" -> Report is a bit vague. 
I think the toast is nicer than the doorhanger, but it does constrain how much text you can get away with.
Attached image after-clicking.png
Attached image toast-message.png
Attached image menu-placement.png
Attached patch 1024807-4.patch (obsolete) — Splinter Review
Updated patch.
Yuan, can you give some feedback on UX/strings? (see screenshots)
(In reply to Mike Taylor ~*:miketaylr*~ from comment #30)
> Created attachment 8504857 [details]
> after-clicking.png

Hi Mike,

I think using "Report Site Issue" makes sense. And the placement in the context menu is reasonable too.

Could you clarify how can a user trigger this toast message? I assume the toast message is a confirmation of the action of tapping "Report Site Issue" from the menu. 

If that's correct, I don't think a confirmation of the "Report Site Issue" action is necessary in this case. Based on Android guidelines, toast dialogs are designed for offering feedback of an action. They shouldn't be used for performing an action. 

So A flow I would suggest is
1. Tap the "Report Site Issue" from context menu on a site
2. Report action being initiated and once it's done. Show a toast message "Issue reported by Webcompat" or "Issue reported to Firefox Nightly". 

The format of the toast should follow the Android standards. http://developer.android.com/guide/topics/ui/notifiers/toasts.html No need to have an icon.
Flags: needinfo?(ywang)
(In reply to Yuan Wang(:Yuan) – Mobile Firefox Design Lead from comment #35)
> (In reply to Mike Taylor ~*:miketaylr*~ from comment #30)
> Could you clarify how can a user trigger this toast message? I assume the
> toast message is a confirmation of the action of tapping "Report Site Issue"
> from the menu. 

Thanks Yuan.

This patch adds two possible ways to land at webcompat.com (via a new tab) to report some kind of compatibility site issue.

1) user notices bug/problem/issue with any site, 
2) clicks "Report Site Issue" 
3) new tab at webcompat.com is opened, with some bug form details pre-filled

OR

1) user notices bug/problem/issue with mobile site
2) user clicks "Request Desktop Site" from the application menu
3) user sees toast message prompting them to report whatever site issues prompted them to request desktop content
4) new tab at webcompat.com is opened, with some bug form details pre-filled

> If that's correct, I don't think a confirmation of the "Report Site Issue"
> action is necessary in this case. Based on Android guidelines, toast dialogs
> are designed for offering feedback of an action. They shouldn't be used for
> performing an action. 

It sounds like if a user is explicitly requesting desktop content (via the menu item), asking a user to report the issue... is more like asking for feedback. Unsure if that's OK, or if it should be a doorhanger or what.

> The format of the toast should follow the Android standards.
> http://developer.android.com/guide/topics/ui/notifiers/toasts.html No need
> to have an icon.

OK, I can remove the icon. That'll make the patch much smaller.
Attached patch 1024807-5.patch (obsolete) — Splinter Review
Removed icons and fixed how I was checking if the menu item needs to be updated.
Attachment #8499160 - Attachment is obsolete: true
Attachment #8504944 - Attachment is obsolete: true
Comment on attachment 8505059 [details] [diff] [review]
1024807-5.patch

Review of attachment 8505059 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! We can iterate with UX on the design in nightly if they want.
Attachment #8505059 - Flags: review?(wjohnston) → review+
Attached patch 1024807-5.patchSplinter Review
Yay!
Attachment #8505059 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bb58a5fc381b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Flags: in-moztrap?(fennec)
Keywords: feature
OS: Mac OS X → Android
Hardware: x86 → ARM
Device: Samsung S5 (Android 4.4)
Build: Firefox for Android 36.0a1 (2014-10-19)
Verified as fixed, based on the following steps:
1.Going to news.google.com. Open Menu -> "Report Site Issue". A new tab(webcompat.com) is opened to report a bug. 

2.Going to news.google.com. Open Menu -> "Request Desktop Site". A toast message appears to report site issues (Report site issue? | Report). After tapping "Report", a new tab(webcompat.com) is opened to report a bug.

Note:
When filling the Site URL, even one letter is accepted: http://i.imgur.com/Ei9IgNx.png . I think that a message should be displayed "Please enter a valid URL" or the green check symbol should be displayed only after filling in the valid URL.
Status: RESOLVED → VERIFIED
(In reply to Teodora Vermesan (:TeoVermesan) from comment #42)
> Note:
> When filling the Site URL, even one letter is accepted:
> http://i.imgur.com/Ei9IgNx.png . I think that a message should be displayed
> "Please enter a valid URL" or the green check symbol should be displayed
> only after filling in the valid URL.

Thanks Teodora. I filed https://github.com/webcompat/webcompat.com/issues/306 to track that.
Depends on: 1089376
Depends on: 1092447
Depends on: 1098037
tracking-fennec: + → Nightly+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.