1078353 - handleReaderListStatusRequest should use JS-Java callbacks

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Wesley Johnston (:wesj)]

Just looking at code, I noticed this message uses its own send-return message passing. We should move it to use the newer Messaging.jsm apis:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#434

Comment 1

[Vivek Balakrishnan[:vivek]]

Attachment: 1078353.patch

A new patch that uses Messaging.jsm promise callback

Comment 2

[:Margaret Leibovic]

Comment on attachment 8506421 [details] [diff] [review]
1078353.patch

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

Comment 3

[:Margaret Leibovic]

Comment on attachment 8506421 [details] [diff] [review]
1078353.patch

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

This is looking good, but unfortnately this code has changed since you wrote this patch, so you'll have to update your tree and update the patch. The Java code moved to ReadingListHelper.java.

::: mobile/android/chrome/content/aboutReader.js
@@ +294,4 @@
>        type: "Reader:ListStatusRequest",
>        url: this._article.url
> +    }).then(function(aData) {
> +      let args = JSON.parse(aData);

Nit: Drop the a prefix (so use `data` instead of `aData`).

@@ +306,5 @@
> +            this._setToolbarVisibility(true);
> +          }
> +        }
> +      }
> +    }.bind(this));

Instead of using bind, you should just use fat arrow syntax for the function you pass to `then`. Here's an example of what that looks like:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Prompt.jsm#174

Comment 4

[Vivek Balakrishnan[:vivek]]

Attachment: 1078353.patch

A new patch with review nits corrected

Comment 5

[:Margaret Leibovic]

Comment on attachment 8515275 [details] [diff] [review]
1078353.patch

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

This is looking good, but there's a JS problem.

(In the future, you can/should just request review from the same reviewer for future versions of a patch, since they've already taken the time to get to know the context of the patch :)

::: mobile/android/chrome/content/aboutReader.js
@@ +303,5 @@
> +
> +          // Display the toolbar when all its initial component states are known
> +          if (isInitialStateChange) {
> +            this._setToolbarVisibility(true);
> +          }

Please fix the indentation here. I also think you're missing a close brace.

Comment 6

[Vivek Balakrishnan[:vivek]]

Attachment: 1078353.patch

A new patch with indentation corrected.

Comment 7

[Vivek Balakrishnan[:vivek]]

Attachment: 1078353.patch

I missed the tab spaces in my previous patch.
A new patch that actually corrects the indentation.

Comment 8

[:Margaret Leibovic]

Comment on attachment 8517580 [details] [diff] [review]
1078353.patch

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

Looks good, thanks.

Comment 9

[Carsten Book [:Tomcat]]

Hi, would it be possible to provide a try link here ? 
Thanks!

Comment 10

[Vivek Balakrishnan[:vivek]]

https://tbpl.mozilla.org/?tree=Try&rev=d1627427219a

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/8b7521add52b

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8b7521add52b