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)
Web Compatibility
Tooling & Investigations
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: miketaylr, Assigned: cchung)
Details
Attachments
(1 file, 7 obsolete files)
1.92 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•7 years ago
|
Assignee: nobody → cchung
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 3•7 years ago
|
||
> ###
> 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.
Reporter | ||
Comment 4•7 years ago
|
||
(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)
Assignee | ||
Comment 5•7 years ago
|
||
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 (?)
Comment hidden (obsolete) |
Assignee | ||
Comment 7•7 years ago
|
||
(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
Reporter | ||
Comment 8•7 years ago
|
||
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!
Assignee | ||
Comment 9•7 years ago
|
||
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.
Reporter | ||
Updated•7 years ago
|
Attachment #8887971 -
Attachment is patch: true
Attachment #8887971 -
Attachment mime type: text/x-csrc → text/plain
Reporter | ||
Comment 10•7 years ago
|
||
(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
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 17•7 years ago
|
||
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)
Assignee | ||
Comment 18•7 years ago
|
||
(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
Reporter | ||
Comment 19•7 years ago
|
||
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+
Assignee | ||
Comment 20•7 years ago
|
||
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)
Reporter | ||
Comment 21•7 years ago
|
||
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+
Assignee | ||
Comment 22•7 years ago
|
||
(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?
Assignee | ||
Comment 23•7 years ago
|
||
(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)...
Assignee | ||
Comment 24•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8889935 -
Flags: review?(gijskruitbosch+bugs) → review+
Reporter | ||
Comment 25•7 years ago
|
||
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)
Assignee | ||
Comment 26•7 years ago
|
||
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
Comment 27•7 years ago
|
||
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
Comment 28•7 years ago
|
||
bugherder |
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.
Description
•