Closed Bug 1078353 Opened 7 years ago Closed 7 years ago

handleReaderListStatusRequest should use JS-Java callbacks

Categories

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

35 Branch
All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: wesj, Assigned: vivek, Mentored)

References

Details

(Whiteboard: [lang=java])

Attachments

(1 file, 3 obsolete files)

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
Component: General → Reader Mode
OS: Linux → Android
Hardware: x86_64 → All
See Also: → 1078365
Mentor: wjohnston
Whiteboard: [lang=java]
Attached patch 1078353.patch (obsolete) — Splinter Review
A new patch that uses Messaging.jsm promise callback
Attachment #8506421 - Flags: review?(wjohnston)
Comment on attachment 8506421 [details] [diff] [review]
1078353.patch

I can take this review, I'll get to this later today.
Attachment #8506421 - Flags: review?(wjohnston) → review?(margaret.leibovic)
Assignee: nobody → vivekb.balakrishnan
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
Attachment #8506421 - Flags: review?(margaret.leibovic) → feedback+
Attached patch 1078353.patch (obsolete) — Splinter Review
A new patch with review nits corrected
Attachment #8506421 - Attachment is obsolete: true
Attachment #8515275 - Flags: review?(wjohnston)
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.
Attachment #8515275 - Flags: review?(wjohnston) → feedback+
Attached patch 1078353.patch (obsolete) — Splinter Review
A new patch with indentation corrected.
Attachment #8515275 - Attachment is obsolete: true
Attachment #8517569 - Flags: review?(margaret.leibovic)
Attached patch 1078353.patchSplinter Review
I missed the tab spaces in my previous patch.
A new patch that actually corrects the indentation.
Attachment #8517569 - Attachment is obsolete: true
Attachment #8517569 - Flags: review?(margaret.leibovic)
Attachment #8517580 - Flags: review?(margaret.leibovic)
Comment on attachment 8517580 [details] [diff] [review]
1078353.patch

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

Looks good, thanks.
Attachment #8517580 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
Hi, would it be possible to provide a try link here ? 
Thanks!
Flags: needinfo?(vivekb.balakrishnan)
Keywords: checkin-needed
https://tbpl.mozilla.org/?tree=Try&rev=d1627427219a
Flags: needinfo?(vivekb.balakrishnan)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/8b7521add52b
Keywords: checkin-needed
Whiteboard: [lang=java] → [lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8b7521add52b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][fixed-in-fx-team] → [lang=java]
Target Milestone: --- → Firefox 36
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.