Closed
Bug 882319
Opened 9 years ago
Closed 9 years ago
Page load fixes for first release
Categories
(Firefox Health Report Graveyard :: Client: Android, defect)
Tracking
(firefox23+ fixed)
RESOLVED
FIXED
Firefox 24
People
(Reporter: rnewman, Assigned: rnewman)
References
Details
Attachments
(1 file, 1 obsolete file)
1.56 KB,
patch
|
nalexander
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
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•9 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•9 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•9 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•9 years ago
|
Depends on: 882464
Summary: Permissions and page load fixes for first release → Page load fixes for first release
Assignee | ||
Comment 7•9 years ago
|
||
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•9 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?
Assignee | ||
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c890f39dfa6
Target Milestone: --- → Firefox 24
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0c890f39dfa6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Attachment #761755 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
status-firefox23:
--- → affected
tracking-firefox23:
--- → +
Assignee | ||
Comment 13•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/71778324511a
Updated•4 years ago
|
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•