882319 - Page load fixes for first release

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Android, defect)

Comment 1

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

There's a git bit here that I'll commit after review.

Unlike the PER_ACCOUNT_TYPE permission, we didn't automatically request the PER_ANDROID_PACKAGE permission where it was defined.

This patch also requests FHR document on page load.

Comment 2

[Richard Newman [:rnewman]]

Note that we don't ship PER_ANDROID_PACKAGE restrictions in 23, so that part doesn't apply to Aurora.

Comment 3

[Richard Newman [:rnewman]]

Scratch that; we need Bug 811358 to land in Aurora for the uploader, so this patch applies cleanly.

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 761543 [details] [diff] [review]
Proposed patch. v1

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

I believe that neither of these interventions are correct.

::: mobile/android/chrome/content/aboutHealthReport.js
@@ +142,5 @@
>      iframe.addEventListener("RemoteHealthReportCommand",
>                              function onCommand(e) {healthReportWrapper.handleRemoteCommand(e);},
>                              false);
>      healthReportWrapper.updatePrefState();
> +    healthReportWrapper.refreshPayload();

The original desktop site requested a payload on init, so doing this will request two documents.  That's lame.  I think we should make the page request on initialize, because it needs to know if there is no data available anyway.

@@ +151,5 @@
>    ERROR_PAYLOAD_FAILED: 2,
>    ERROR_PREFS_FAILED:   3,
>  
>    reportFailure: function (error) {
> +    console.log("Reporting FHR failure: " + error);

If we're going to log anywhere, log everywhere.

::: mobile/android/services/manifests/SyncAndroidManifest_permissions.xml.in
@@ +30,5 @@
>      <permission
>          android:name="@ANDROID_PACKAGE_NAME@.permission.PER_ANDROID_PACKAGE"
>          android:protectionLevel="signature"/>
> +
> +    <uses-permission android:name="@ANDROID_PACKAGE_NAME@.permission.PER_ANDROID_PACKAGE" />

This should not be here.  It should be in whatever manifest snippets actually use it.  And after Bug 828654, it is.

Comment 5

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #4)
> Comment on attachment 761543 [details] [diff] [review]
> Proposed patch. v1
> 
> Review of attachment 761543 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I believe that neither of these interventions are correct.
> 
> ::: mobile/android/chrome/content/aboutHealthReport.js
> @@ +142,5 @@
> >      iframe.addEventListener("RemoteHealthReportCommand",
> >                              function onCommand(e) {healthReportWrapper.handleRemoteCommand(e);},
> >                              false);
> >      healthReportWrapper.updatePrefState();
> > +    healthReportWrapper.refreshPayload();
> 
> The original desktop site requested a payload on init, so doing this will
> request two documents.

We no longer request a payload on init:

https://bug876473.bugzilla.mozilla.org/attachment.cgi?id=755638

 let healthReportWrapper = {
   init: function () {
-    reporter.onInit().then(healthReportWrapper.refreshPayload,
-                           healthReportWrapper.handleInitFailure);
-

This is the replacement for that call, pushed far enough down the lifecycle that it makes sense.

> That's lame.  I think we should make the page
> request on initialize, because it needs to know if there is no data
> available anyway.

This is what we're talking about in IRC: whether the page actually does that. espressive thought it did, but given that the wrapper doesn't get the message, apparently it does not.

If we can fix the page, great.

> >    reportFailure: function (error) {
> > +    console.log("Reporting FHR failure: " + error);
> 
> If we're going to log anywhere, log everywhere.

Disagree: we should log on failures, not routinely.

> >      <permission
> >          android:name="@ANDROID_PACKAGE_NAME@.permission.PER_ANDROID_PACKAGE"
> >          android:protectionLevel="signature"/>
> > +
> > +    <uses-permission android:name="@ANDROID_PACKAGE_NAME@.permission.PER_ANDROID_PACKAGE" />
> 
> This should not be here.  It should be in whatever manifest snippets
> actually use it.  And after Bug 828654, it is.

Gently disagree: if we introduce a custom permission, it makes sense to automatically use it, otherwise we'll cause confusion. As apparently we did, because current m-c is throwing an error because it doesn't use its own custom permission.

See the uses-permission earlier in that same file for the per-account-type permission.

Comment 6

[Nick Alexander :nalexander [he/him]]

I don't understand how the permission is missing at all: it landed as part of the original commit

https://hg.mozilla.org/mozilla-central/rev/fd95f49deeda#l3.13

I don't care where the perm lives; we should probably coalesce our permissions manifests for the next feature or two.  As for logging on failure: so be it.

Comment 7

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 761755 [details] [diff] [review]
Proposed patch. v2

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

short and sweet, works for me.

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 761755 [details] [diff] [review]
Proposed patch. v2

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

short and sweet, works for me.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 761755 [details] [diff] [review]
Proposed patch. v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Web-side FHR.

User impact if declined: 
  No FHR web!

Testing completed (on m-c, etc.): 
  Local. About to land.

Risk to taking this patch (and alternatives if risky): 
  Nada.

String or IDL/UUID changes made by this patch:
  None.

Comment 11

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c890f39dfa6

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0c890f39dfa6

Comment 13

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/71778324511a