Closed Bug 840124 Opened 7 years ago Closed 7 years ago

implement postMessage API for remote report

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect, P1)

defect

Tracking

(firefox20 unaffected, firefox21+ fixed, firefox22 fixed)

RESOLVED FIXED
Firefox 22
Tracking Status
firefox20 --- unaffected
firefox21 + fixed
firefox22 --- fixed

People

(Reporter: mconnor, Assigned: mconnor)

Details

Attachments

(4 files, 4 obsolete files)

This needs to get finished.
Comment on attachment 714540 [details] [diff] [review]
v1 - postMessage most recent JSON payload on load of iframe content

Gavin for review, dveditz for sec-review to make sure I"m doing this correctly (I looked at the original patch comments in bug 719484)
Attachment #714540 - Flags: review?(gavin.sharp)
Attachment #714540 - Flags: review?(dveditz)
Attachment #714540 - Flags: review?
Comment on attachment 714540 [details] [diff] [review]
v1 - postMessage most recent JSON payload on load of iframe content

>diff --git a/browser/base/content/abouthealthreport/abouthealth.js b/browser/base/content/abouthealthreport/abouthealth.js

>+function getReportURI() {
>+  return Services.io.newURI(prefs.get("reportUrl"), null, null);
>+}
>+
>+function getIFrame() {
>+  return document.getElementById("remote-report");
>+}

nit: these seem small enough and not called enough that they are not useful as helpers (in the case of getIFrame() you call it twice in the same function inefficiently).

>+function doJSONInjection(data) {
>+  getIFrame().contentWindow.postMessage(JSON.stringify(data), getReportURI().scheme);

postMessage takes a URI, not a scheme, so I imagine you want reportURI.spec (or .prePath, though it shouldn't matter).

Seems like it would be reasonably easy to test this by pointing the report URI to a test-shipped file.
Attachment #714540 - Flags: review?(gavin.sharp) → review-
Attached patch v1.0.1 (obsolete) — Splinter Review
comments address, minus the bit about testing, since I don't want to go down the rabbit's hole on FHR vs. test infra while we're still sorting that out elsewhere.  That said, it's fairly trivial to test manually in the meantime, I'll attach a test HTML file.

Note on the changes to allow postMessage to any origin if the pref is pointing at a file URI: I did this to facilitate local testing for the webdevs building the feature, as postMessage currently can't limit on file URIs, per MDN.

Note to webdev types working on this, you can use the following snippet to process the received message.

<script>
window.addEventListener("message", receiveMessage, false);
var reportJSON = null;
function receiveMessage(event) {
  reportJSON = JSON.parse(event.data);
}
</script>
Attachment #714540 - Attachment is obsolete: true
Attachment #714540 - Flags: review?(dveditz)
Attachment #715584 - Flags: review?(gavin.sharp)
(In reply to Mike Connor [:mconnor] from comment #4)
> comments address, minus the bit about testing, since I don't want to go down
> the rabbit's hole on FHR vs. test infra while we're still sorting that out
> elsewhere.  That said, it's fairly trivial to test manually in the meantime,
> I'll attach a test HTML file.

Where is it being sorted out? I don't understand why FHR would be particularly complicated to test using our infra.
Comment on attachment 715584 [details] [diff] [review]
v1.0.1

>diff --git a/browser/base/content/abouthealthreport/abouthealth.js b/browser/base/content/abouthealthreport/abouthealth.js

> function refreshJSONPayload() {
>   reporter.getLastPayload().then(refreshDataView);
> }
> 
>+function injectJSON() {
>+  reporter.getLastPayload().then(doJSONInjection);
>+}

Seems like maybe you could combine these since they're doing essentially the same thing (also would guarantee that the data we're postMessaging matches exactly the data we're showing in the "raw data" frame - I don't know how often it gets updated).

> function onShowReportClick() {

>+  iframe.addEventListener("load", injectJSON, false);

You probably want to remove this listener after it first fires? If the iframe navigates itself (or fires multiple load events for sub-resources), you don't want to postMessage the data multiple times AIUI.

r=me with those addressed.
Attachment #715584 - Flags: review?(gavin.sharp) → review+
(and a test added as discussed on IRC)
New approach incoming
Summary: implement postMessage API for remote report to get content → implement postMessage API for remote report
Modifies about:healthreport to have 100% remote UI.

Pseudo-API docs and sample HTML are at https://mconnor.etherpad.mozilla.org/aboutHealthReportAPI

To-dos:
* observe pref changes and update the in-content pages
* make the JSON fetch more robust against early init.
Attachment #715584 - Attachment is obsolete: true
There will be tests for everything before this lands, but I want to get reviews on the approach ASAP, I'll attach them separately tomorrow.  There's a few pieces that are more robust in this approach that will make testing much more efficient and effective, so it should be less of a pain than it was being previously.

https://mconnor.etherpad.mozilla.org/aboutHealthReportAPI describes the API and has sample JS for the webdev folk to use.
Attachment #723788 - Attachment is obsolete: true
Attachment #723812 - Flags: review?(gavin.sharp)
Attachment #723812 - Flags: review?(gps)
Comment on attachment 723812 [details] [diff] [review]
part 1: make the report fully remote with a small wrapper (v1.0)

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

Not granting r+ because of lacking of promised tests.

I'd like to see some more rigor around promise rejection. We could probably just install a new "error" event and send all errors through it. It's probably best to not use the error strings from the rejected promise because they will likely be very low-level and not very user-friendly.

::: browser/base/content/abouthealthreport/abouthealth.js
@@ +23,3 @@
>  
> +
> +var healthReportWrapper = {

r- for using var ;)

@@ +23,5 @@
>  
> +
> +var healthReportWrapper = {
> +  init: function () {
> +    reporter.onInit().then(healthReportWrapper.refreshPayload);

We probably want to install a 2nd handler to then() to handle the case where init fails (it will happen in the wild).

@@ +27,5 @@
> +    reporter.onInit().then(healthReportWrapper.refreshPayload);
> +
> +    let iframe = document.getElementById("remote-report");
> +    iframe.addEventListener("load", healthReportWrapper.initRemotePage, false);
> +    let report = Services.io.newURI(prefs.get("about.reportUrl"), null, null);

If the pref is not a URI this will throw. GIGO?

@@ +29,5 @@
> +    let iframe = document.getElementById("remote-report");
> +    iframe.addEventListener("load", healthReportWrapper.initRemotePage, false);
> +    let report = Services.io.newURI(prefs.get("about.reportUrl"), null, null);
> +    iframe.src = report.spec;
> +    prefs.observe("uploadEnabled", this.updatePrefState, healthReportWrapper);

I /think/ we'll leak this observer.

@@ +52,5 @@
> +    this.injectData("prefs", prefs);
> +  },
> +
> +  refreshPayload: function () {
> +    reporter.collectAndObtainJSONPayload().then(healthReportWrapper.updatePayload);

We should handle the case where the promise is rejected.
Attachment #723812 - Flags: review?(gps) → feedback+
Priority: -- → P1
Attached patch v1.1Splinter Review
* var in let in global scope is silly.  but ok.
* if the pref is mangled the UI will break.  I am not worried about what happens in that case, since it should never happen, and if it does we want to know about it immediately.
* the observer should be fine, the pref wrapper implements nsISupportsWeakReference for this reason.
* added error handlers on the promises, and an error handler for the prefs check in case this blows up for some reason.  Please double check this stuff, I'm not 100%

Today got away from me, but tests will be forthcoming before this lands.
Attachment #723812 - Attachment is obsolete: true
Attachment #723812 - Flags: review?(gavin.sharp)
Attachment #724223 - Flags: feedback?(gps)
Comment on attachment 724223 [details] [diff] [review]
v1.1

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

::: browser/base/content/abouthealthreport/abouthealth.js
@@ +23,5 @@
>  
> +
> +let healthReportWrapper = {
> +  init: function () {
> +    reporter.onInit().then(healthReportWrapper.refreshPayload, 

Nit: trailing whitespace

@@ +57,5 @@
> +    }
> +  },
> +
> +  refreshPayload: function () {
> +    reporter.collectAndObtainJSONPayload().then(healthReportWrapper.updatePayload, 

Nit: trailing whitespace

@@ +67,5 @@
> +  },
> +
> +  injectData: function (type, content) {
> +    let report = Services.io.newURI(prefs.get("about.reportUrl"), null, null);
> +    let reportUrl = report.scheme == "file" ? "*" : report.spec;

I'm guessing there's a security restriction you're trying to get around to facilitate local testing? Could you please comment what exactly.

@@ +84,5 @@
> +      case "DisableDataSubmission":
> +        this.onOptOutClick();
> +        break;
> +      case "EnableDataSubmission":
> +        this.onOptInClick();

We don't know exactly how the page is asking for opt in or opt out. So, perhaps its best if the event pass in the reason and we make the names of the local functions generic?

@@ +93,5 @@
> +      case "RequestCurrentPayload":
> +        this.refreshPayload();
> +        break;
> +      default:
> +        Cu.reportError("Unexpected remote command received: " + evt.detail.command + ". Ignoring command.");

Will the Error Console attribute this to about:healthreport or should the message say something?
Attachment #724223 - Flags: feedback?(gps) → feedback+
Comment on attachment 724223 [details] [diff] [review]
v1.1

Basically moves everything external.  Tests coming up in 30s.
Attachment #724223 - Flags: review?(dolske)
Attached patch v1.1 (tests)Splinter Review
Attachment #725244 - Flags: review?(gps)
Comment on attachment 725244 [details] [diff] [review]
v1.1 (tests)

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

While this seems to look good, I'm no expert when it comes to testing these types of things. I'd feel much better if you got r+ from a browser peer.

::: browser/base/content/test/browser_aboutHealthReport.js
@@ +15,5 @@
> +                 .getService(Ci.nsISupports)
> +                 .wrappedJSObject
> +                 .policy;
> +        policy.recordHealthReportUploadEnabled(true,
> +                                           "Resetting after tests.");

We should also revert the policy URL, no? Shouldn't we instead clear user preferences?

@@ +37,5 @@
> +{
> +  desc: "Test the postMessage API",
> +  setup: function ()
> +  {
> +    Services.prefs.setCharPref("datareporting.healthreport.about.reportUrl", 

Nit: Trailing whitespace.

@@ +58,5 @@
> +        }
> +      }, false, true);
> +
> +    } catch(e) {
> +      deferred.resolve()

Shouldn't we reject if this occurs?

@@ +68,5 @@
> +{
> +  desc: "Test the remote commands",
> +  setup: function ()
> +  {
> +    Services.prefs.setCharPref("datareporting.healthreport.about.reportUrl", 

Nit: Trailing whitespace.

@@ +85,5 @@
> +    try {
> +      let win = gBrowser.contentWindow;
> +      win.addEventListener("message", function testLoad(e) {
> +        if (!e.data.command)
> +          return;

Nit: Always use braces.

@@ +135,5 @@
> +    }
> +
> +    finish();
> +  });
> +}

It pains me that we don't have a sane test framework for mochitests.
Attachment #725244 - Flags: review?(gps) → feedback+
Comment on attachment 724223 [details] [diff] [review]
v1.1

>diff --git a/browser/base/content/abouthealthreport/abouthealth.css b/browser/base/content/abouthealthreport/abouthealth.css

>+#remote-report {
>   width: 100%;
>   height: 100%;
>+  border: 0;
>+  display: flex;
> }

Why is display:flex needed?

>diff --git a/browser/base/content/abouthealthreport/abouthealth.js b/browser/base/content/abouthealthreport/abouthealth.js

I didn't review this file. I think any Firefox reviewer could.
Attachment #724223 - Flags: feedback+
Attachment #724223 - Flags: review?(dolske) → review?(jAwS)
Comment on attachment 724223 [details] [diff] [review]
v1.1

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

::: browser/base/content/abouthealthreport/abouthealth.css
@@ +11,4 @@
>    width: 100%;
>    height: 100%;
> +  border: 0;
> +  display: flex;

What is the display:flex needed for?
Attachment #724223 - Flags: review?(jAwS) → review+
Backed out because it made mochitest browser-chrome time out in the health report test https://hg.mozilla.org/integration/mozilla-inbound/rev/5c46d8839b7f
Target Milestone: mozilla22 → ---
This doesn't block a meta bug, comment 0 isn't very descriptive, and the tracking nom didn't have an associated comment. Why do we need to fix this in FF21?
Sorry for the confusion (the meta bug is out of date, we'll fix it)

This is the bug that allows the remote report to request the user's data in order to display the report.  The feature will not work without this patch.  We had to significantly refactor the original implementation in order to support the final visual design that came in after the original patch was written.  The patch is entirely contained within about:healthreport and includes tests, so risk outside of FHR should be zero.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: FHR will fail to function in final user-facing form.
Testing completed (on m-c, etc.): baking on Nightly since yesterday, in use by webdev people building the web side.
Risk to taking this patch (and alternatives if risky): no risk to non-FHR.
String or IDL/UUID changes made by this patch: Removes no-longer-used strings, adds nothing at all.
Attachment #730855 - Flags: approval-mozilla-aurora?
See previous comment.
Attachment #730857 - Flags: approval-mozilla-aurora?
Attachment #730855 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 730857 [details] [diff] [review]
v1.1 (tests) for aurora

This is a blocker for FHR to go live and helps with user facing part needed for Beta 1.
Attachment #730857 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: Metrics and Firefox Health Report → Client: Desktop
Flags: in-testsuite+
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla22 → Firefox 22
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.