1078365 - "Feedback:LastUrl" should use JS-Java callbacks

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

"Feedback:LastUrl" in BrowserApp.java is using the old way of sending callbacks. We should upgrade them to the new Messaging.jsm style.

Comment 1

[:Margaret Leibovic]

vivek, you might be interested in this bug. No pressure, just wanted to let you know this is here :)

Comment 2

[Vivek Balakrishnan[:vivek]]

Sure, I'll take it

Comment 3

[Vivek Balakrishnan[:vivek]]

Attachment: 1078365.patch

A new patch that uses Messaging.jsm callbacks to retrieve LastUrl for feedback form

Comment 4

[:Margaret Leibovic]

Comment on attachment 8506289 [details] [diff] [review]
1078365.patch

I can take this review, I'll get to this later today.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8506289 [details] [diff] [review]
1078365.patch

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

Looks good. r+ with one small change to always call sendSuccess in getLastUrl.

::: mobile/android/base/BrowserApp.java
@@ +3147,5 @@
>              @Override
>              public void onPostExecute(String url) {
>                  // Don't bother sending a message if there is no URL.
>                  if (url.length() > 0)
> +                    callback.sendSuccess(url);

If we don't call sendSuccess or sendError, the promise on the other side will never be fulfilled, and under the hood it looks like we'll have an observer in Messaging.jsm that never gets removed. To work around this, I think we should call sendSuccess no matter what. We already check the length of the URL in JS, so we shouldn't need to change the logic there.

Comment 6

[Vivek Balakrishnan[:vivek]]

Attachment: 1078365.patch

Review nit corrected. Rebased with latest tree

Comment 7

[:Margaret Leibovic]

Comment on attachment 8515235 [details] [diff] [review]
1078365.patch

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

Looks good, thanks!

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/556fa6395bd2

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/556fa6395bd2