857661 - Topsites tab is showing an entry with blank content

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Tony Chung [:tchung]]

Attachment: screenshot

See screenshot.   Tapping that empty entry goes to about:home.  Not sure why it came up blank.

Repro:
1) install 4/2 nightly build
2) launch homescreen and tap about:home
3) launch awesomescreen, and verify blank entry in the Topsites.

Expected:
- about:home should have content listed

Actual:
- white row

Comment 1

[Aaron Train [:aaronmt]]

Long-tap hold and hit 'edit'; does it have a title?

Comment 2

[:Margaret Leibovic]

You can reproduce this by pinning an empty site. What you're seeing here is a fake pinned bookmark with no data in it. I think we shouldn't let users pin empty urls.

Comment 3

[:Margaret Leibovic]

Attachment: patch

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 737743 [details] [diff] [review]
patch

We should show an error the user (probably a toast?). Or alternatively, we could take this is an "unpin this" command. Also, I like brackets in java code.

Comment 5

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #4)
> Comment on attachment 737743 [details] [diff] [review]
> patch
> 
> We should show an error the user (probably a toast?). Or alternatively, we
> could take this is an "unpin this" command.

I wonder what a user is actually trying to do when submitting an emtpy serach/url. I interpreted it as cancelling, which is why I didn't add an error message or do anything to change the pinned site. Unpinning seems like the wrong thing to do if the user does think this is going to cancel their changes. 

Ian, what do you think we should do?

> Also, I like brackets in java code.

I was just copying the bracing style right above this, which was written by you :P

Comment 6

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #5)
> (In reply to Wesley Johnston (:wesj) from comment #4)
> > Comment on attachment 737743 [details] [diff] [review]
> > patch
> > 
> > We should show an error the user (probably a toast?). Or alternatively, we
> > could take this is an "unpin this" command.
> 
> I wonder what a user is actually trying to do when submitting an emtpy
> serach/url. I interpreted it as cancelling, which is why I didn't add an
> error message or do anything to change the pinned site. Unpinning seems like
> the wrong thing to do if the user does think this is going to cancel their
> changes. 
> 
> Ian, what do you think we should do?

I agree with your comment 3, that we shouldn't even be letting people pin empty URLs to begin with.

Comment 7

[:Margaret Leibovic]

(In reply to Ian Barlow (:ibarlow) from comment #6)
> (In reply to :Margaret Leibovic from comment #5)
> > (In reply to Wesley Johnston (:wesj) from comment #4)
> > > Comment on attachment 737743 [details] [diff] [review]
> > > patch
> > > 
> > > We should show an error the user (probably a toast?). Or alternatively, we
> > > could take this is an "unpin this" command.
> > 
> > I wonder what a user is actually trying to do when submitting an emtpy
> > serach/url. I interpreted it as cancelling, which is why I didn't add an
> > error message or do anything to change the pinned site. Unpinning seems like
> > the wrong thing to do if the user does think this is going to cancel their
> > changes. 
> > 
> > Ian, what do you think we should do?
> 
> I agree with your comment 3, that we shouldn't even be letting people pin
> empty URLs to begin with.

Yes, that's what my patch does, but Wes gave in an r- because he thinks that we should be showing an error message, and I was wondering what you think about that.

Comment 8

[:Margaret Leibovic]

Comment on attachment 737743 [details] [diff] [review]
patch

Re-requesting review, since ibarlow said he approved of this approach.

Comment 9

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/812da2a6d2f8

Comment 10

[(Out Sep 17-23) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/812da2a6d2f8

Comment 11

[Aaron Train [:aaronmt]]

Uplift?