918917 - Favicons are blank sometimes

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Sriram Ramasubramanian [:sriram]]

Favicons are blank sometimes when the TwoLinePageRow is recycled.

Comment 1

[Chris Kitching [:ckitching]]

Attachment: Always show default favicon view.

Perhaps the vanishment of the default favicon potenitally caused by the change undone by this patch is what you're after?

If not - steps to reproduce would be nice.

Comment 2

[:Margaret Leibovic]

Comment on attachment 807873 [details] [diff] [review]
Always show default favicon view.

This looks good. Hopefully this is the last regression from this refactor!

Comment 3

[Lucas Rocha (:lucasr)]

(In reply to Chris Kitching [:ckitching] from comment #1)
> Created attachment 807873 [details] [diff] [review]
> Always show default favicon view.
> 
> Perhaps the vanishment of the default favicon potenitally caused by the
> change undone by this patch is what you're after?
> 
> If not - steps to reproduce would be nice.

Won't this show the default favicon while favicons are loading in about:home? That is not the intended behaviour. I actually want us to show an empty image.

Comment 4

[Chris Kitching [:ckitching]]

(In reply to Lucas Rocha (:lucasr) from comment #3)
> Won't this show the default favicon while favicons are loading in
> about:home? That is not the intended behaviour. I actually want us to show
> an empty image.

This patch restores the original behaviour as it was before 888326.

The new favicon system is to show blank until the favicon loads, or the default if there is no favicon found. 

As for what happens before that lands - I have now U-turned on this behaviour twice. I am not a fan of repeatedly submitting patches that do the opposite of earlier patches in a way which seems potentially nonterminating.

Please handle the intermediate step yourself. I will of course fix any regressions my earlier patches have created - but when such fixes contradict with what you want in the intermediate solution I don't think it's a good use of my time to play patch ping-pong with different people's opinions.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Chris Kitching [:ckitching] from comment #4)

> As for what happens before that lands - I have now U-turned on this
> behaviour twice. I am not a fan of repeatedly submitting patches that do the
> opposite of earlier patches in a way which seems potentially nonterminating.
> 
> Please handle the intermediate step yourself. I will of course fix any
> regressions my earlier patches have created - but when such fixes contradict
> with what you want in the intermediate solution I don't think it's a good
> use of my time to play patch ping-pong with different people's opinions.

I think we can do a better job of communicating here. There is an aggressive, condescending tone which I hope is unintended.

As for the "ping-pong" and "use of time" points, you probably know that software is sometimes an iterative process and this point in the process is actually the least expensive time to make changes. Once a behavior gets into Aurora or Beta, changes become much more expensive. But yes, this can be frustrating.

Comment 6

[Chris Kitching [:ckitching]]

Attachment: Show default favicon view only after an unsuccessful load.

(In reply to Mark Finkle (:mfinkle) from comment #5)
> I think we can do a better job of communicating here. There is an
> aggressive, condescending tone which I hope is unintended.
> 
> As for the "ping-pong" and "use of time" points, you probably know that
> software is sometimes an iterative process and this point in the process is
> actually the least expensive time to make changes. Once a behavior gets into
> Aurora or Beta, changes become much more expensive. But yes, this can be
> frustrating.

Of course this wasn't intended to be aggressive or condecending.

I realise software often works this way - I've had similar back-and-forth with far more iterations in other patches, and it's usually productive - each new iteration at the very least advances the ongoing argument about what's the best way of doing something, so while sometimes we end up settling on the original version of a particular patch the work done making the binned ones wasn't pointless.

But we're not in that least-expensive period of change. There exist a collection of small bugs, such as Bug 905685 or Bug 915347, which have appeared after my favicon rewrite and solve a small number of the same problems, but in less tidy ways. That's fine - we can have a nice simple low-risk patch or two to uplift to 26.
But each time one of these lands it bitrots my existing work, which is mildly annoying. In fact, I got so tired of waiting for this to happen I ended up rebasing one of these myself after waiting almost a week for it to land so I could get my other patch into a reviewable state.

And now this single-line bug report that provides almost no information about the problem, no steps to reproduce, not even a screenshot, appears and is assigned to me. It claims that Bug 888326 has created a regression. Making a guess as to what is actually being reported, it seems that the "regression" is actually an omission from Bug 905685 because it apparently is also trying to solve Bug 873243. Or something.

In the face of no well-defined problem to solve, at a time when the solution to this bug either needs uplift or will be wasted, when the change will bitrot another series of patches, and when nobody can seem to make up their minds about what is actually needed, the back-and-forth that is usually so useful is just a recipie for wasted effort and frustration.

I'm sorry if my prior comment caused offense. Here is a patch that probably does what I think you currently want.

Comment 7

[:Margaret Leibovic]

Comment on attachment 808230 [details] [diff] [review]
Show default favicon view only after an unsuccessful load.

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

I think the main source of this perceived back-and-forth is just a lack of clear communication about what exactly all of these patches have been doing. For example, in a previous bug, we weren't on the same page about when to show the default favicon or not. It's also hard to keep track of what changes are where when you're trying to land a lot of different patches, so it requires diligence to make sure you keep mozilla-central (and now mozilla-aurora) in a good state.

Can you explain to me the different cases where updateImage is called, and what is actually causing the issue at hand here?

::: mobile/android/base/widget/FaviconView.java
@@ +143,5 @@
>       */
>      private void updateImageInternal(Bitmap bitmap, String key, boolean allowScaling) {
> +        if (bitmap == null) {
> +            showDefaultFavicon();
> +            return;

How is this different than the current behavior that goes through formatImage?

Currently, if bitmap is null, we should be setting mIconBitmap to null, and then formatImage should call setImageResource(R.drawable.favicon);.

Can you explain more about what you're trying to do here? Instead of adding on more individual cases, I'd like to see us think through all the different cases here.

@@ +160,5 @@
>          formatImage();
>      }
>  
> +    private void showDefaultFavicon() {
> +        setImageResource(R.drawable.favicon);

Do you also need a hideBackground call in here? Also, if you're making this helper method, we should use it in the other places where we currently decide to show the default favicon.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Why a single line-of-code requires a private method?

Comment 9

[Chris Kitching [:ckitching]]

Attachment: Show no favicon until load fails, then show default favicon, or load result.

(In reply to :Margaret Leibovic from comment #7)
> Comment on attachment 808230 [details] [diff] [review]
> Show default favicon view only after an unsuccessful load.
> 
> Review of attachment 808230 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the main source of this perceived back-and-forth is just a lack of
> clear communication about what exactly all of these patches have been doing.
> For example, in a previous bug, we weren't on the same page about when to
> show the default favicon or not. It's also hard to keep track of what
> changes are where when you're trying to land a lot of different patches, so
> it requires diligence to make sure you keep mozilla-central (and now
> mozilla-aurora) in a good state.

The patch from 888326 did not create this problem. This problem appeared after lucasr's patch to implement concurrent loading landed, because it attempted to also solve the bug about displaying the default favicons at the proper time as well, and because my patch did not make any effort to solve this problem.

When two competing groups are trying to work on the same region of code at the same time solving the same problems and both are landing patches it is no surprise when things start going wrong.

I *still* have no idea what sriram was trying to report when he filed this bug. The problem I'm now fixing here (The "We are showing default favicons at the wrong time" problem which really has a bug all to itself elsewhere, and has already been addressed in the other favicon rewrite bug) doesn't seem to have much to do with the very brief report that was provided.

I would greatly appreciate it if you would clarify your report so I don't have to keep taking educated guesses about what you mean. 

> Can you explain to me the different cases where updateImage is called, and
> what is actually causing the issue at hand here?

updateImage has one call site, at TwoLinePageRow:121.
The containing method has two call sites.

TwoLinePageRow:271 - the conclusion of lucasr's async task to load favicons (Which will return null if no favicon is available locally).
TwoLinePageRow:198 - in updateFromCursor when the memory cache hits. The value passed is always non-null.

updateAndScaleImage is, currently, exclusively used from search-preferences land, so is largely uninteresting for the purposes of this discussion.


As you can see, the only time that updateImage should be called with a null bitmap is when the attempt to get a bitmap has failed.
Assuming the desired behaviour is "Blank until we have either loaded a favicon or have tried and failed to load a favicon, in the latter case displaying the default favicon", then we can see that what we want to do is display the default favicon whenever updateImage is given a null value.

My initial implementation did this check in formatImage, but this no longer sufficient - the default value of mUnscaledBitmap is null, so when updateImageInternal is called for the very first time with a null bitmap it just returns after doing no work, since null == null. (And all refernence fields of a java object are implicitly initialised to null at construction time).

It is not sufficient to just remove the check from formatImage and put it in updateImageInternal, as this is potentially called from onSizeChanged before any bitmap is set. (If the layout takes place before our code provides a favicon for the view to display - something I encountered during testing of the earlier patches.). This necessary stupidity is approximately why I didn't make an attempt to solve this problem in the initial implementation, but left it for the larger patch which can solve it without needing to be so untidy.

> ::: mobile/android/base/widget/FaviconView.java
> @@ +143,5 @@
> >       */
> >      private void updateImageInternal(Bitmap bitmap, String key, boolean allowScaling) {
> > +        if (bitmap == null) {
> > +            showDefaultFavicon();
> > +            return;
> 
> How is this different than the current behavior that goes through
> formatImage?

See above. There's a stupid edgecase when the very first call to updateImageInternal is a null which is expected to display the default favicon. (Wasn't a supported feature before, suddenly a "regression" when it doesn't after refactoring...)

> Currently, if bitmap is null, we should be setting mIconBitmap to null, and
> then formatImage should call setImageResource(R.drawable.favicon);.

Quite right, let me tweak that.

> 
> Can you explain more about what you're trying to do here? Instead of adding
> on more individual cases, I'd like to see us think through all the different
> cases here.

See largeish paragraph above.

> 
> @@ +160,5 @@
> >          formatImage();
> >      }
> >  
> > +    private void showDefaultFavicon() {
> > +        setImageResource(R.drawable.favicon);
> 
> Do you also need a hideBackground call in here? Also, if you're making this
> helper method, we should use it in the other places where we currently
> decide to show the default favicon.

Looks like it, aye.

So, yes, a few flaws in the last approach. Here's a revised patch that certainly appears to do what we now want w.r.t lucasr's wishes about blank favicons (So we just fixed Bug 873243, then?)


(In reply to Sriram Ramasubramanian [:sriram] from comment #8)
> Why does a single line of code require a private method?

A single line of code can warrant its own helper method for a variety of reasons, usually clarity.
For example, if we inlined all the calls of this particular method then we now have to make sure that all the many different places where we do `setImageResource(R.drawable.favicon)` are kept up to date if we ever rename that resource. Readers of the code also have to do the extra thinking required to understand that `R.drawable.favicon` is the default favicon - something which isn't immediately obvious from its name. The name "favicon" might mean the branding favicon, or the empty favicon, or something else - it's not definite enough to be obvious. Sure, you can go digging through the resources directory and find this, but it just slows people down.

In other situations, the single line of code being placed in the helper function might be very long - it is not wise to duplicate a long, complicated line of code in many places throughout your program. If you ever wind up updating it, you're almost certainly going to miss one and introduce a bug that's both tricky to find and rage-inducingly frustrating when you do finally spot it.

Comment 10

[Sriram Ramasubramanian [:sriram]]

I personally showed the phone, showed that the favicons are blank, explained when it happens to ckitching. He *understood* the problem. And then asked me to *file a bug* and *assign it* to him.

Now a complete U-turn of "where is the STR?" (Comment 1) and "I *still* have no idea what sriram was trying to report when he filed this bug." (Comment 9) doesn't really sound right!

Or atleast, if ckitching is not sure about what the bug is all about.. then why repeated iteration of patches without knowing what the problem is? :D :D

Comment 11

[Sriram Ramasubramanian [:sriram]]

If STR is still needed:

1. Open Fennec.
2. Make sure there are atleast 50 entries in the list.
2a. Visit more and more sites to generate 50+ entries, if needed.
3. Open about:home (if in some other website).
4. Scroll the list in Top Sites, Bookmarks or History pages.

Expected: Either a favicon, a favicon with dominant color background or a default favicon is shown.

Actual: Few entries are *blank*.

Comment 12

[:Margaret Leibovic]

Comment on attachment 808937 [details] [diff] [review]
Show no favicon until load fails, then show default favicon, or load result.

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

Sriram, can you apply this patch and let us know if it fixes the problem you described?

::: mobile/android/base/widget/FaviconView.java
@@ +165,5 @@
>      /**
>       * Clear image and background shown by this view.
>       */
>      public void clearImage() {
> +        showEmptyFavicon();

Do we need this showEmptyFavicon() method, or could we just include its body in here, then just use clearImage() in formatImage().

I would prefer to avoid similar but slightly different logic paths if possible.

Comment 13

[Sriram Ramasubramanian [:sriram]]

I tried this patch. It didn't work.

Comment 14

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 808937 [details] [diff] [review]
Show no favicon until load fails, then show default favicon, or load result.

Didn't work. Hence f-.

Comment 15

[:Margaret Leibovic]

Comment on attachment 808937 [details] [diff] [review]
Show no favicon until load fails, then show default favicon, or load result.

Clearing review until we can sort out what's going wrong with this patch.

Comment 16

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 808937 [details] [diff] [review]
Show no favicon until load fails, then show default favicon, or load result.

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

This actually works fine. My patch queue was wrong. Oops.

Comment 17

[:Margaret Leibovic]

That's good to hear :)

I'd like to see a response to my question in comment 12 before giving this an r+.

Comment 18

[Chris Kitching [:ckitching]]

Attachment: V3 - Show no favicon until load fails, then show default favicon, or load result.

(In reply to :Margaret Leibovic from comment #12)
> ::: mobile/android/base/widget/FaviconView.java
> @@ +165,5 @@
> >      /**
> >       * Clear image and background shown by this view.
> >       */
> >      public void clearImage() {
> > +        showEmptyFavicon();
> 
> Do we need this showEmptyFavicon() method, or could we just include its body
> in here, then just use clearImage() in formatImage().
> 
> I would prefer to avoid similar but slightly different logic paths if
> possible.

The problem is that clearImage nulls out the various fields of the class, whereas showEmptyFavicon just blanks what is displayed without clearing what is stored.

That said, this class isn't thread safe anyway, and in a single threaded environment your suggestion should be fine. RNewman's got me all threading-ified from his reviews of my other patch :P.


(In reply to Sriram Ramasubramanian [:sriram] from comment #10)
> I personally showed the phone, showed that the favicons are blank, explained
> when it happens to ckitching. He *understood* the problem. And then asked me
> to *file a bug* and *assign it* to him.

Human memory is imperfect. If a report depends on information delivered like this you risk misinterpretation and wasted effort or, in this case, a complete failure to recall. At best, it slows things down by an extra communication roundtrip. There's a reason someone wrote the bug reporting guidelines article!

It should never be a struggle to get the information needed from the reporter.

> Now a complete U-turn of "where is the STR?" (Comment 1) and "I *still* have
> no idea what sriram was trying to report when he filed this bug." (Comment
> 9) doesn't really sound right!
>
> Or atleast, if ckitching is not sure about what the bug is all about.. then
> why repeated iteration of patches without knowing what the problem is? :D :D

That I have been trying to maximise progress by filing speculative patches does not mean what you did was right, and certainly does not warrant childish teasing.

Comment 19

[Chris Kitching [:ckitching]]

Comment on attachment 810323 [details] [diff] [review]
V3 - Show no favicon until load fails, then show default favicon, or load result.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 888326 and friends.
User impact if declined: Randomly vanishing favicons while scrolling up and down.
Testing completed (on m-c, etc.): Try run at https://tbpl.mozilla.org/?tree=Try&rev=28391135a633 - ought to be fine.
Risk to taking this patch (and alternatives if risky): Low risk. Just handling some slightly strange edgecases that weren't considered as they're covered by upcoming patches.
String or IDL/UUID changes made by this patch: None.

Comment 20

[Lucas Rocha (:lucasr)]

Pushed: https://hg.mozilla.org/integration/fx-team/rev/061c17bbab44

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Doesn't apply to fx-team. Please rebase.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Oh, comment 20. Lucas, please don't forget to clear checkin-needed when pushing, lest someone else waste time trying to do the same :)

Comment 23

[Lucas Rocha (:lucasr)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22)
> Oh, comment 20. Lucas, please don't forget to clear checkin-needed when
> pushing, lest someone else waste time trying to do the same :)

Oopsie! Got it :-)

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/061c17bbab44

Comment 25

[u421692]

Still able to reproduce this issue by changing the device orientation, on latest Nightly 27.0a1 (2013-09-30). Reopening bug.

Comment 26

[Aaron Train [:aaronmt]]

Bug 922116 was filed about that. Closing this one.

Comment 27

[:Margaret Leibovic]

Note: We'll also need to uplift bug 922116 when we uplift this to Aurora. But we should probably let that bake on Nightly before doing the uplift.

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 810323 [details] [diff] [review]
V3 - Show no favicon until load fails, then show default favicon, or load result.

Approving for Aurora uplift, marked bug 922116 for tracking as a reminder to check back and make sure that also gets uplift (still needs an approval nomination).

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7964ac2c0dc6