Closed Bug 989109 Opened 6 years ago Closed 6 years ago

WebAppRT expects return values from sendMessageToJava

Categories

(Firefox for Android :: Web Apps (PWAs), defect, P1, blocker)

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
firefox30 + fixed
firefox31 --- fixed

People

(Reporter: wesj, Assigned: myk)

References

Details

(Keywords: regression, Whiteboard: [WebRuntime])

Attachments

(1 file, 1 obsolete file)

I noticed this today. We don't return from this method anymore so this likely always fails.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/WebappRT.js#72
(In reply to Wesley Johnston (:wesj) from comment #0)
> I noticed this today. We don't return from this method anymore so this
> likely always fails.
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> WebappRT.js#72

It sure does!  Remote debugging is also broken by the server-side bug 988644, which ozten is working on.  But once he fixes that, we'll still need to fix this in order to get it working again.

I have a work in progress; I'll attach it shortly.
Assignee: nobody → myk
Blocks: 946344
Severity: normal → blocker
Status: NEW → ASSIGNED
Keywords: regression
OS: Linux → Android
Priority: -- → P1
Hardware: x86_64 → ARM
Whiteboard: [WebRuntime]
Attached patch patch v1: fixes problem (obsolete) — Splinter Review
Since EventDispatcher.sendResponse requires a JSONObject, we might as well return an actual boolean.

(Aside: I wish sendMessageToJava pre-parsed the response before passing it to the callback.)
Attachment #8399540 - Flags: review?(wjohnston)
Attachment #8399540 - Attachment is obsolete: true
Attachment #8399540 - Flags: review?(wjohnston)
Attachment #8399563 - Flags: review?(wjohnston)
Comment on attachment 8399563 [details] [diff] [review]
patch v2: be compatible with changes in bug 989098

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

I don't have a strong opinion on it, but if you want to simplify, you can avoid the extra JSONObject here.

::: mobile/android/base/GeckoApp.java
@@ +721,1 @@
>                  EventDispatcher.sendResponse(message, ret);

You can actually just pass this boolean straight to the sendResponse function now.

::: mobile/android/chrome/content/WebappRT.js
@@ +66,5 @@
>      }
>  
>  #ifdef MOZ_ANDROID_SYNTHAPKS
>      // If the app is in debug mode, configure and enable the remote debugger.
> +    sendMessageToJava({ type: "NativeApp:IsDebuggable" }, (response) => {

Then here response would just be the bool "isDebuggable".
Attachment #8399563 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #4)
> I don't have a strong opinion on it, but if you want to simplify, you can
> avoid the extra JSONObject here.

I do have a strong opinion, which is that I want to simplify!  But I also want to uplift to Fx30, since regressing bug 946344 landed there.  And bug 989098, which added the ability to pass a boolean to sendResponse, landed in Fx31.  So I'll leave this the way it is for now in the interest of simplifying the uplift request.
https://hg.mozilla.org/mozilla-central/rev/163a17a33f82
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Comment on attachment 8399563 [details] [diff] [review]
patch v2: be compatible with changes in bug 989098

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Bug 946344.

User impact if declined: 
  Webapp developers won't be able to debug their apps.

Testing completed (on m-c, etc.): 
  The fix landed on Central earlier today.

Risk to taking this patch (and alternatives if risky): 
  It's a small, low-risk change.

String or IDL/UUID changes made by this patch:
  None.
Attachment #8399563 - Flags: approval-mozilla-aurora?
Attachment #8399563 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.