Page load fixes for first release

RESOLVED FIXED in Firefox 23

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

unspecified
Firefox 24
All
Android
Points:
---

Firefox Tracking Flags

(firefox23+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 761543 [details] [diff] [review]
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.
Attachment #761543 - Flags: review?(nalexander)
(Assignee)

Comment 2

5 years ago
Note that we don't ship PER_ANDROID_PACKAGE restrictions in 23, so that part doesn't apply to Aurora.
(Assignee)

Comment 3

5 years ago
Scratch that; we need Bug 811358 to land in Aurora for the uploader, so this patch applies cleanly.
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.
Attachment #761543 - Flags: review?(nalexander) → review-
(Assignee)

Comment 5

5 years ago
(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.
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.
(Assignee)

Updated

5 years ago
Depends on: 882464
Summary: Permissions and page load fixes for first release → Page load fixes for first release
(Assignee)

Comment 7

5 years ago
Created attachment 761755 [details] [diff] [review]
Proposed patch. v2
Attachment #761543 - Attachment is obsolete: true
Attachment #761755 - Flags: review?(nalexander)
Comment on attachment 761755 [details] [diff] [review]
Proposed patch. v2

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

short and sweet, works for me.
Attachment #761755 - Flags: review?(nalexander) → review+
Comment on attachment 761755 [details] [diff] [review]
Proposed patch. v2

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

short and sweet, works for me.
(Assignee)

Comment 10

5 years ago
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.
Attachment #761755 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/0c890f39dfa6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Attachment #761755 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
status-firefox23: --- → affected
tracking-firefox23: --- → +
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/71778324511a
status-firefox23: affected → fixed
You need to log in before you can comment on or make changes to this bug.