767738 - Reader Mode: Use site favicon

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Currently in reader mode the current favicon is the target build branded favicon (e.g, Nightly)

Comment 1

[Lucas Rocha (:lucasr)]

Agree. Ian, Madhava, confirm?

Comment 2

[Ian Barlow (:ibarlow)]

Make it so.

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Use favicon from the original site in Reader

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 646159 [details] [diff] [review]
Use favicon from the original site in Reader

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

Looks good, with one possible concern:

::: mobile/android/base/GeckoApp.java
@@ +868,5 @@
> +            public void onPostExecute(String faviconUrl) {
> +                if (faviconUrl != null) {
> +                    StringBuffer sb = new StringBuffer();
> +                    sb.append("{ \"url\" : \"").append(url).append("\"")
> +                      .append(", \"faviconUrl\" : \"").append(faviconUrl).append("\"")

Why are you building JSON using string manipulation rather than JSON objects here?  (Is the performance better?)  Are we quite certain that url and faviconUrl are sufficiently sanitized (no double-quotes)?

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Matt Brubeck (:mbrubeck) from comment #4)
> Comment on attachment 646159 [details] [diff] [review]
> Use favicon from the original site in Reader
> 
> Review of attachment 646159 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, with one possible concern:
> 
> ::: mobile/android/base/GeckoApp.java
> @@ +868,5 @@
> > +            public void onPostExecute(String faviconUrl) {
> > +                if (faviconUrl != null) {
> > +                    StringBuffer sb = new StringBuffer();
> > +                    sb.append("{ \"url\" : \"").append(url).append("\"")
> > +                      .append(", \"faviconUrl\" : \"").append(faviconUrl).append("\"")
> 
> Why are you building JSON using string manipulation rather than JSON objects
> here?  (Is the performance better?)  Are we quite certain that url and
> faviconUrl are sufficiently sanitized (no double-quotes)?

Just felt overkill to use the JSON API but your point about quoting in the URL is a good one. I'll send a new patch using JSON API.

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Use favicon from the original site in Reader

Comment 7

[Lucas Rocha (:lucasr)]

Pushed: http://hg.mozilla.org/integration/mozilla-inbound/rev/043783f87caa

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 646359 [details] [diff] [review]
Use favicon from the original site in Reader

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Reader won't show proper favicons from the original site
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): not risky, reader-only change.
String or UUID changes made by this patch: none

Reader on the Firefox 16 roadmap.

Comment 9

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/043783f87caa

Comment 10

[Lucas Rocha (:lucasr)]

Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/dff198fbc2ce

Comment 11

[Ed Morley [:emorley]]

(In reply to Lucas Rocha (:lucasr) from comment #10)
> Pushed to aurora:
> https://hg.mozilla.org/releases/mozilla-aurora/rev/dff198fbc2ce

Backed out for bustage, which persisted after clobber:
https://hg.mozilla.org/releases/mozilla-aurora/rev/81279c9aef08

Comment 12

[Ed Morley [:emorley]]

Sorry, forgot the log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=13966573&tree=Mozilla-Aurora

Also, please may you watch the tree when landing on aurora (or ask someone to watch it for you). Inbound is the only tree to have that exemption.

Comment 13

[Lucas Rocha (:lucasr)]

(In reply to Ed Morley [:edmorley] from comment #12)
> Sorry, forgot the log:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=13966573&tree=Mozilla-Aurora
> 
> Also, please may you watch the tree when landing on aurora (or ask someone
> to watch it for you). Inbound is the only tree to have that exemption.

I missed the dependency on the bug 774306 patch to land this in aurora. Requested approval now.

Comment 14

[Lucas Rocha (:lucasr)]

Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/906986f587b7