Closed Bug 1381545 Opened 7 years ago Closed 7 years ago

Capture if servo prefs are enabled when user submits report via Report Site Issue button

Categories

(Web Compatibility :: Tooling & Investigations, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: miketaylr, Assigned: cchung)

Details

Attachments

(1 file, 7 obsolete files)

We should capture if a user has the servo pref enabled, and set a label accordingly in webcompat.com (and possibly somewhere in the bug report itself).

"layout.css.servo.enabled" is the relevant pref.
Assignee: nobody → cchung
> ###
> high level questions:
>   * I'm not totally clear whether updating the url string like above will
> have any impact from when the image gets added to text area (i.e. timing
> conflict or data override). my understanding is that a blob is a file format
> for storage so I do not feel the blob is going to get added to the text area
> but eventually it seems that the text area gets updated for the image and it
> is not obvious when/how that happens.

probably my concerns were addressed in your webcompat.com changes for github #1306. but I'll review the app and webcompat.com code to see if that answers these questions.
(In reply to Carol Chung from comment #3)
> > ###
> > high level questions:
> >   * I'm not totally clear whether updating the url string like above will
> > have any impact from when the image gets added to text area (i.e. timing
> > conflict or data override). my understanding is that a blob is a file format
> > for storage so I do not feel the blob is going to get added to the text area
> > but eventually it seems that the text area gets updated for the image and it
> > is not obvious when/how that happens.
> 
> probably my concerns were addressed in your webcompat.com changes for github
> #1306. but I'll review the app and webcompat.com code to see if that answers
> these questions.

The changes here won't have any impact on screenshots.

(The blob never really gets attached to the textarea. What happens is before form submission we do an xhr POST to the /upload/ endpoint, which sends back a URL where the uploaded file lives on the webserver -- we just stick that URL in the textarea)
thanks Mike. I've made the changes you requested this morning. I've having a couple of challenges with testing.

* yesterday I got the latest src and made a new build, this seemed to remove the layout.css.servo.enabled preference. I could add it back manually but think a couple mochi tests are failing due to the missing preference.

* I also tried an end to end test with the latest webcompat.com src. the text area prepopulation and bug form url look good to me (with preference enabled/disabled) but I think there is a pending code change from one of my pr's (#1634) which prevent the issue from getting saved currently (one _bind() got removed accidentally?).

since my tests are not passing, not sure whether to submit a patch for review (?)
(In reply to Mike Taylor [:miketaylr] (55 Regression Engineering Owner) from comment #6)
> (In reply to Carol Chung from comment #5)
> > * yesterday I got the latest src and made a new build, this seemed to remove
> > the layout.css.servo.enabled preference. I could add it back manually but
> > think a couple mochi tests are failing due to the missing preference.
> 
> I guess if you started with a new profile, it should be expected that the
> pref isn't there (I think). Which mochitests are failing? (That sounds
> surprising)

23 INFO Console message: [JavaScript Error: "[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://webcompat-reporter/content/WebCompatReporter.jsm :: openWebCompatTab :: line 124"  data: no]"]
openWebCompatTab@chrome://webcompat-reporter/content/WebCompatReporter.jsm:124:19
>this is the line where the code gets the preference value

promise callback*reportIssue@chrome://webcompat-reporter/content/WebCompatReporter.jsm:175:5
onCommand@chrome://webcompat-reporter/content/WebCompatReporter.jsm:49:25
handleWidgetCommand@resource:///modules/CustomizableUI.jsm:1525:11
test_screenshot@chrome://mochitests/content/browser/browser/extensions/webcompat-reporter/test/browser/browser_report_site_issue.js:16:3
Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:763:21
TaskImpl_run@resource://gre/modules/Task.jsm:331:42
TaskImpl@resource://gre/modules/Task.jsm:280:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
Task_spawn@resource://gre/modules/Task.jsm:166:12
Tester_execTest@chrome://mochikit/content/browser-test.js:758:9
Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:670:7
SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59

24 INFO Longer timeout required, waiting longer...  Remaining timeouts: 1
Failed to retrieve MOZ_UPLOAD_DIR env var
25 INFO TEST-UNEXPECTED-FAIL | browser/extensions/webcompat-reporter/test/browser/browser_report_site_issue.js | Test timed out - 
GECKO(69201) | MEMORY STAT | vsize 4941MB | residentFast 555MB | heapAllocated 128MB
26 INFO TEST-OK | browser/extensions/webcompat-reporter/test/browser/browser_report_site_issue.js | took 100024ms
27 INFO TEST-UNEXPECTED-FAIL | browser/extensions/webcompat-reporter/test/browser/browser_report_site_issue.js | Found a tab after previous test timed out: http://example.com/browser/browser/extensions/webcompat-reporter/test/browser/test.html
All the failures probably boil down to that first exception. It'll be easier to help debug if you can attach a WIP patch -- thanks!
Attached patch patch file (obsolete) — Splinter Review
hi Mike, this is my current reporter file. this should include change requests from Tues. in my build Tues, the layout.css.servo.enabled was missing so I added it back manually and was able to run manual tests ok against a local webcompat.com instance.

today I ran another build with the latest src but running mochi tests still gives the same errors as yesterday. I believe it's related to the missing preference but do not understand why the preference was missing yesterday (user error or ?).

also my current dev setup using using just mercurial (not git b/c that way was giving too many setup issues). let me know if that is a problem for patch reviews.
Attachment #8887971 - Attachment is patch: true
Attachment #8887971 - Attachment mime type: text/x-csrc → text/plain
(In reply to Carol Chung from comment #9)
> also my current dev setup using using just mercurial (not git b/c that way
> was giving too many setup issues). let me know if that is a problem for
> patch reviews.

No, not an issue. But you'll have to attach a patch that can be checked in, rather than an entire file: 

https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Attached patch patch for rebased commits (obsolete) — Splinter Review
replacing last 2 patch files from yesterday with one combo
(mochi test fix and changes for feedback 072017)

hi Mike, was not sure what the next steps should be but here's the patch based on the rebased commits. do you want to do another feedback or ?
Attachment #8888562 - Attachment is obsolete: true
Attachment #8888564 - Attachment is obsolete: true
Attachment #8888562 - Flags: feedback?(miket)
Attachment #8888564 - Flags: feedback?(miket)
Attachment #8888848 - Flags: feedback?(miket)
(sorry for spam) for the record, these were the 3 test cases used:

assumes following changes
  >local webcompat.com server running
  > update preferences:
    browser.photon.structure.enabled = false
    about:config?filter=newIssueEndpoint => reset to "http://localhost:5000"

1 update preferences: layout.css.servo.enabled = true
  open a webpage 
  from Nightly hamburger menu, click Report Site Issue
  expect url string to change: + "details=layout.css.servo.enabled%3A+true&label=type-stylo"
  also text area to have appended str: "layout.css.servo.enabled = true"
	
2 update preferences: layout.css.servo.enabled = false
  open a webpage 
  from Nightly hamburger menu, click Report Site Issue
  expect url string not to change
  also text area not to change

3 remove the pref: layout.css.servo.enabled
  open a webpage 
  from Nightly hamburger menu, click Report Site Issue
  expect url string not to change
  also text area not to change
  also there should be no errors like:
  INFO Console message: [JavaScript Error: "[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://webcompat-reporter/content/WebCompatReporter.jsm :: openWebCompatTab :: line 124"  data: no]"]
openWebCompatTab@chrome://webcompat-reporter/content/WebCompatReporter.jsm:124:19
Comment on attachment 8888848 [details] [diff] [review]
patch for rebased commits

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

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm
@@ +130,5 @@
> +        params.append("details", "layout.css.servo.enabled: true");
> +        params.append("label", "type-stylo");
> +    }
> +
> +    let WEBCOMPAT_URL = `${WebCompatReporter.endpoint}?${params}`;

Can we get ride of this variable and just use the template string directly?

gBrowser.loadOneTab(`${WebCompatReporter.endpoint}?${params}`,	
      {inBackground: false, triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal()});	

(whitespace will probably be terrible in this example... so with not terrible whitespace.:))

Let me see it once more time after that and we'll request review (and I can toss the patch at the try servers).
Attachment #8888848 - Flags: feedback?(miket) → feedback+
Attached patch var cleanup (obsolete) — Splinter Review
hi Mike, this is for the latest change request from today. let me know if I need to do anything after this (any manual review requests)... thx
Attachment #8888848 - Attachment is obsolete: true
Attachment #8888914 - Flags: feedback?(miket)
Comment on attachment 8888914 [details] [diff] [review]
var cleanup

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

Looks good! Just a few tiny things to change then you can request review from someone like :gijs

When you do that, please make sure the commit message of the patch uses a format like "Bug 123456. Does this thing. r?person"

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm
@@ +11,4 @@
>  
>  const TABLISTENER_JSM = "chrome://webcompat-reporter/content/TabListener.jsm";
>  const WIDGET_ID = "webcompat-reporter-button";
> +const PREF_SERVO_ENABLED = "layout.css.servo.enabled";

nit: maybe PREF_STYLO_ENABLED is a better name for this variable (even though the pref does say `stylo`....). Dunno, up to you. :)

@@ +123,5 @@
>      const WEBCOMPAT_ORIGIN = new win.URL(WebCompatReporter.endpoint).origin;
> +    let styloEnabled = Services.prefs.getBoolPref(PREF_SERVO_ENABLED, false);
> +
> +    let params = new URLSearchParams();
> +    params.append("url", `${tabData.url}`);

Sorry I missed this before, you can use use `tabData.url` directly here:

params.append("url", tabData.url);
Attachment #8888914 - Flags: feedback?(miket) → feedback+
(In reply to Mike Taylor [:miketaylr] (55 Regression Engineering Owner) from comment #21)
> Comment on attachment 8888914 [details] [diff] [review]
> var cleanup
> 
> Review of attachment 8888914 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! Just a few tiny things to change then you can request review
> from someone like :gijs
> 

> @@ +123,5 @@
> >      const WEBCOMPAT_ORIGIN = new win.URL(WebCompatReporter.endpoint).origin;
> > +    let styloEnabled = Services.prefs.getBoolPref(PREF_SERVO_ENABLED, false);
> > +
> > +    let params = new URLSearchParams();
> > +    params.append("url", `${tabData.url}`);
> 
> Sorry I missed this before, you can use use `tabData.url` directly here:
> 
> params.append("url", tabData.url);

hi Mike, I tested the tabData.url var both ways and when I changed it (without the template literal syntax), I got the bad url format again (additional "25" gets inserted in the url) so I kept this item as is.

should I still create a separate patch file (for merged commits) and create a new review request?
(In reply to Carol Chung from comment #18)
> (sorry for spam) for the record, these were the 3 test cases used:
> 
> assumes following changes
>   >local webcompat.com server running
>   > update preferences:
>     browser.photon.structure.enabled = false
>     about:config?filter=newIssueEndpoint => reset to "http://localhost:5000"
> 
> 1 update preferences: layout.css.servo.enabled = true
>   open a webpage 
>   from Nightly hamburger menu, click Report Site Issue
>   expect url string to change: +
> "details=layout.css.servo.enabled%3A+true&label=type-stylo"
>   also text area to have appended str: "layout.css.servo.enabled = true"

fixing a typo
text area in webcompat.com tab should have the appended str: "layout.css.servo.enabled: true"

(typos - the bane of my existence)...
hi, I may have a mistake with the commit message but let me know if there are change requests. thanks.
Attachment #8888914 - Attachment is obsolete: true
Attachment #8889935 - Flags: review?(gijskruitbosch+bugs)
Attachment #8889935 - Flags: review?(gijskruitbosch+bugs) → review+
Nice work Carol! Can you update your commit message to reflect r=gijs, then add the checkin-needed keyword?

(if you click "edit bug" there's a tracking section: https://cloudup.com/cCJLwODm-1x)
Flags: needinfo?(cchung)
hi Mike, between the last patch file and this latest commit, I had done one final test of that tabData.url and so in the latest commit it shows as a change (although the file content is the same as the patch file).

your other changes requests were done.
Flags: needinfo?(cchung)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ad1af89c807
Send stylo prefs via Report Site Issue. r=Gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2ad1af89c807
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.