511117 - when page is loading and you bookmark via star, popup dialog doesn't show

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Bookmarks, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

the new bookmark popup dialog is supposed to show when you click on the star to bookmark a URL.  During a page load (tested on my maemo device) if you click on the star (the star turns yellow), you will not see the bookmark popup.

Comment 1

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

Attachment: patch

I removed the code to load the favicon in the places favicon service. This stops the bug (popup is displayed) and the favicon is still shown in the awesomebar and in the bookmark list.

I guess we didn't need the code. Less is more!

Comment 2

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

Background:

Error happens here:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser-ui.js#711

It seems that this._favicon.src is "", set in updateToolbar when loading. i suppose we could do a check and skip the favicon service

Comment 3

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

On IRC Gavin asked if removing the code was wise, as Firefox does the same thing when bookmarking a page.

To test this I started a new profile (no history or visits or favicons) and loaded several pages. I bookmarked the pages before they finished loading, although waiting until they were loaded would have made no difference since I removed the code to set the favicon in the favicon service.

After bookmarking, I loaded the bookmark list and the favicon for each bookmark was present.

It seems that we don't need the favicons.setAndLoadFaviconForPage called when bookmarking.

Comment 4

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

... and the reason is probably because we call favicons.setAndLoadFaviconForPage when we finish loading any page:

http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser-ui.js#168

Comment 5

[:Gavin Sharp [email: [e-mail]]]

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

Oh, that's what Firefox does too!

Comment 6

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

pushed: https://hg.mozilla.org/mobile-browser/rev/a6da11ffd9b8

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

verified with 20090827 winmo trunk nightly