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

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

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.

Comment 1

[:Margaret Leibovic]

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

[surabhi anand]

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.

Comment 3

[:Margaret Leibovic]

(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

Comment 4

[surabhi anand]

Thanx for the feedback,will start working on it.. :)

Comment 5

[:Margaret Leibovic]

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!

Comment 6

[:Margaret Leibovic]

Attachment: 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

Comment 7

[:Margaret Leibovic]

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

Comment 8

[:Margaret Leibovic]

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

[surabhi anand]

No problem at all, in fact my mistake that i got stuck in academics of new semester :)

Comment 10

[Richard Newman [:rnewman]]

(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:

Comment 11

[Richard Newman [:rnewman]]

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.

Comment 12

[:Margaret Leibovic]

(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 13

[Richard Newman [:rnewman]]

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

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

Ship It!

Comment 14

[:Margaret Leibovic]

(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.

Comment 15

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/033918fb7819

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/033918fb7819

Comment 18

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1117226 - Use sendRequestForResult for reader mode favicon and add-to-list requests. r=rnewman