Use sendRequestForResult for reader mode favicon and add-to-list requests

RESOLVED FIXED in Firefox 38

Status

()

Firefox for Android
Reader View
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

Trunk
Firefox 38
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
From bug 1111142 comment 8:


>diff --git a/mobile/android/chrome/content/Reader.js b/mobile/android/chrome/content/Reader.js

>+  receiveMessage: function(message) {

>+      case "Reader:FaviconRequest": {
>+        let observer = (s, t, d) => {
>+          Services.obs.removeObserver(observer, "Reader:FaviconReturn", false);
>+          message.target.messageManager.sendAsyncMessage("Reader:FaviconReturn", JSON.parse(d));
>+        };
>+        Services.obs.addObserver(observer, "Reader:FaviconReturn", false);
>+        Messaging.sendRequest({
>+          type: "Reader:FaviconRequest",
>+          url: message.data.url
>+        });

I think you want Messaging.sendRequestForResult, like here:

http://hg.mozilla.org/mozilla-central/file/57e4e9c33bef/toolkit/components/reader/content/aboutReader.js#l287

That way you can skip the explicit observer.

Updated

3 years ago
Flags: needinfo?(talinpaul)
(Assignee)

Comment 1

3 years ago
talinpaul, are you trying to indicate that you are interested in working on this bug? Usually we use the needinfo flag to request information from someone else, or set it for ourselves as a reminder to look into a bug in the future. I just want to make sure that you're not left hanging due to confusing bugzilla UI :)

Comment 2

3 years ago
Hello ,I tried to work on the suggestion mentioned in this bug and getting info from bug 1111142 and from the patch there,i reached to following conclusion-

case "Reader:FaviconRequest": {
Messaging.sendRequestForResult({

         type: "Reader:ListStatusRequest",
          url: this._article.url
        }).then((s,t,d) => {
	message.target.messageManager.sendAsyncMessage("Reader:FaviconReturn", JSON.parse(d));
	});
}

Please,give feedback on this above solution.And i din't submitted a patch because i am not sure if that patch has landed or not.
(Assignee)

Comment 3

3 years ago
(In reply to surabhi anand from comment #2)
> Hello ,I tried to work on the suggestion mentioned in this bug and getting
> info from bug 1111142 and from the patch there,i reached to following
> conclusion-
> 
> case "Reader:FaviconRequest": {
> Messaging.sendRequestForResult({
> 
>          type: "Reader:ListStatusRequest",
>           url: this._article.url
>         }).then((s,t,d) => {
> 	message.target.messageManager.sendAsyncMessage("Reader:FaviconReturn",
> JSON.parse(d));
> 	});
> }
> 
> Please,give feedback on this above solution.And i din't submitted a patch
> because i am not sure if that patch has landed or not.

The patch for bug 1111142 just landed, so you should be able to 'hg pull' to get the latest changes, and then build your patch on top of that.

This looks like the right start on the JS side, but you should still send "Reader:FaviconRequest" (not "Reader:ListStatusRequest"), and you'll also have to change the "Reader:FaviconRequest" handler on the Java side for this to work:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ReadingListHelper.java#56

You can follow the logic here to see how "Reader:ListStatusRequest" does it:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ReadingListHelper.java#72
Flags: needinfo?(talinpaul)
(Assignee)

Updated

3 years ago
Assignee: nobody → san13692

Comment 4

3 years ago
Thanx for the feedback,will start working on it.. :)
(Assignee)

Comment 5

3 years ago
I'm sorry, surabhi, I just wrote a patch for this while working on my patch for bug 1113454, since I needed to do some refactoring to make that work easier. Please let me know if I can help you find another bug to work on!
Assignee: san13692 → margaret.leibovic
Blocks: 1113454
Summary: Use sendRequestForResult for reader mode favicon request → Use sendRequestForResult for reader mode favicon and add-to-list requests
(Assignee)

Comment 6

3 years ago
Created attachment 8555291 [details]
MozReview Request: bz://1117226/margaret

/r/3039 - Bug 1117226 - Use sendRequestForResult for reader mode favicon and add-to-list requests. r=rnewman

Pull down this commit:

hg pull review -r a6ed35e802108f197216ebc34572b605035dca4f
(Assignee)

Comment 7

3 years ago
Comment on attachment 8555291 [details]
MozReview Request: bz://1117226/margaret

/r/3039 - Bug 1117226 - Use sendRequestForResult for reader mode favicon and add-to-list requests. r=rnewman

Pull down this commit:

hg pull review -r a6ed35e802108f197216ebc34572b605035dca4f
Attachment #8555291 - Flags: review?(rnewman)
(Assignee)

Comment 8

3 years ago
rnewman, a few notes:

* I think it's wrong that we're currently truncating the URL/title for reading list items. Looking at the code comments for those max length values, those are intended to be used for data to udpate the UI. Storing a truncated URL in the reading list is most likely going to cause problems, and if we really want to enforce a max length, we should do that somewhere more central.

* Following our previous refactorings, the Reader.addArticleToReadingList method is now only called in 2 places, both of which we closely control, so I think it's acceptable to make more assumptions about what's getting passed in there. As part of this, we do always know what the status of the content is, so I decided we should just be explicit about passing that along.

In bug 1113454, I'll work on a separate code path for updating reading list items with new metadata.

Comment 9

3 years ago
No problem at all, in fact my mistake that i got stuck in academics of new semester :)
(In reply to :Margaret Leibovic from comment #8)
> rnewman, a few notes:

Thanks for being thorough :)


> * I think it's wrong that we're currently truncating the URL/title for
> reading list items. Looking at the code comments for those max length
> values, those are intended to be used for data to udpate the UI. Storing a
> truncated URL in the reading list is most likely going to cause problems,
> and if we really want to enforce a max length, we should do that somewhere
> more central.

Maybe.

(For the record, this truncation was added in Bug 1037476.)

There are lots of reasons we truncate, and why that truncation applies to reader items:

* Android UI code doesn't like it, mainly for perf reasons. This is a big one, but not the only reason.

* The bridge doesn't like it -- copying a lot of data is expensive and can OOM. The limit is probably higher than the UI limitation.

* Storage doesn't like it -- storing multi-megabyte strings in sqlite is a bad idea.

* Services don't like it. Uploading a 100,000-char title is a bad idea. This limit is arbitrary.


Bug 1037476 decided -- for the sake of expedience -- to simply truncate at the point of messaging. After all, Gecko has already parsed the long string, and we're still around, so we're just trying to avoid passing it on. (See Bug 1105880 for a case where it dies before that.)

We ideally will check for, and fix, over-long values at *each* of those stages; yes, a more central place for the reading list itself, but I'd argue that we also still need to do it here, and perhaps also in UI code if we change the limit.

In short: consider bumping the limit and adding code elsewhere, rather than removing this entirely.



> * Following our previous refactorings, the Reader.addArticleToReadingList
> method is now only called in 2 places, both of which we closely control, so
> I think it's acceptable to make more assumptions about what's getting passed
> in there. As part of this, we do always know what the status of the content
> is, so I decided we should just be explicit about passing that along.

:thumbsup:
Mentor: margaret.leibovic
Status: NEW → ASSIGNED
Whiteboard: [lang=java][lang=js]
Version: Firefox 35 → Trunk
https://reviewboard.mozilla.org/r/3039/#review2403

This looks good, I think. Would rather see a separate bug to address truncation if you feel strongly, and leave it in here.

::: mobile/android/chrome/content/Reader.js
(Diff revision 1)
>          });

Unhandled rejection…

::: mobile/android/base/ReadingListHelper.java
(Diff revision 1)
> +        // values here, even if we may not use them to insert an item into the DB.

Excellent comment.
(Assignee)

Comment 12

3 years ago
(In reply to Richard Newman [:rnewman] from comment #11)
> https://reviewboard.mozilla.org/r/3039/#review2403
> 
> This looks good, I think. Would rather see a separate bug to address
> truncation if you feel strongly, and leave it in here.

Okay, I will update and move the truncation to another bug. I do think we need to audit our code to think through what would happen if a URL is indeed truncated. It's good to have some sane limits, but if in that case reader mode just totally fails, we need to show something reasonable to the user (I'll admit the case of truncating titles doesn't really matter, since they get cut off in the UI anyway, so maybe we can just leave that alone).
Comment on attachment 8555291 [details]
MozReview Request: bz://1117226/margaret

https://reviewboard.mozilla.org/r/3037/#review2537

Ship It!
Attachment #8555291 - Flags: review?(rnewman) → review+
(Assignee)

Comment 14

3 years ago
(In reply to :Margaret Leibovic from comment #12)
> (In reply to Richard Newman [:rnewman] from comment #11)
> > https://reviewboard.mozilla.org/r/3039/#review2403
> > 
> > This looks good, I think. Would rather see a separate bug to address
> > truncation if you feel strongly, and leave it in here.
> 
> Okay, I will update and move the truncation to another bug. I do think we
> need to audit our code to think through what would happen if a URL is indeed
> truncated. It's good to have some sane limits, but if in that case reader
> mode just totally fails, we need to show something reasonable to the user
> (I'll admit the case of truncating titles doesn't really matter, since they
> get cut off in the UI anyway, so maybe we can just leave that alone).

Actually, this current limit is 25000 characters... I can't imagine a valid case where we'd hit that. If the UI fails in that case, I'm okay with that.
https://hg.mozilla.org/mozilla-central/rev/033918fb7819
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
(Assignee)

Comment 17

3 years ago
Comment on attachment 8555291 [details]
MozReview Request: bz://1117226/margaret
Attachment #8555291 - Attachment is obsolete: true
Attachment #8619047 - Flags: review+
(Assignee)

Comment 18

3 years ago
Created attachment 8619047 [details]
MozReview Request: Bug 1117226 - Use sendRequestForResult for reader mode favicon and add-to-list requests. r=rnewman
You need to log in before you can comment on or make changes to this bug.