Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Favicons of imported bookmarks on the toolbar are missing #10503

Closed
luixxiul opened this issue Aug 15, 2017 · 10 comments
Closed

Favicons of imported bookmarks on the toolbar are missing #10503

luixxiul opened this issue Aug 15, 2017 · 10 comments
Assignees
@NejcZdovc
Labels
0.20.x issue first seen in 0.20.x bug feature/bookmarks feature/importer QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified release/blocking release-notes/exclude
Milestone
0.20.x (Release C...

Comments

@luixxiul
Copy link
Contributor

luixxiul commented Aug 15, 2017 •
edited
Loading

Describe the issue you encountered:
Favicons of imported bookmarks on the toolbar are missing. So are the title and the URL.

Brave: 0.20.0
V8: 6.0.286.52
rev: 46b16c5
Muon: 4.4.7
OS Release: 16.7.0
Update Channel: dev
OS Architecture: x64
OS Platform: macOS
Node.js: 7.9.0
Brave Sync: v1.3.5
libchromiumcontent: 60.0.3112.90

  • Steps to reproduce 1:

    1. Import bookmarks from Safari
    2. Open about:bookmarks
    3. Move icloud.com to Bookmarks Toolbar
    4. Open about:preferences
    5. Change the favicon setting to Text and Favicons
    6. Click iCloud
    7. The favicon is not displayed. The bookmark's URL and title are not populated on the dialog either
      screenshot 2017-08-15 20 36 05
    8. Input something on the empty fields
    9. Click Done
    10. The bookmark is duplicated
    11. Click the dupe
    12. The UI hangs with this error:
    bookmarkFoldersState.js:39 Uncaught TypeError: Cannot read property 'toString' of undefined
    at Object.getFolder (bookmarkFoldersState.js:39)
    at Object.findBookmark (bookmarksState.js:56)
    at mergeProps (bookmarkToolbarButton.js:179)
    at buildPropsImpl (reduxComponent.js:14)
    at ReduxComponent.buildProps (reduxComponent.js:88)
    at ReduxComponent.componentWillReceiveProps (reduxComponent.js:74)
    at ReactCompositeComponent.js?d2b3:610
    at measureLifeCyclePerf (ReactCompositeComponent.js?d2b3:75)
    at ReactCompositeComponentWrapper.updateComponent (ReactCompositeComponent.js?d2b3:609)
    at ReactCompositeComponentWrapper.receiveComponent (ReactCompositeComponent.js?d2b3:546)
assert.js?9281:84 Uncaught AssertionError {name: "AssertionError", message: "state must contain an Immutable.Map of historySites", actual: false, expected: true, operator: "==", …}

  • Any related issues:
@luixxiul luixxiul added this to the 0.18.x Hotfix milestone Aug 15, 2017
@luixxiul
Copy link
Contributor Author

Also the browser cannot be restarted due to the error.

@NejcZdovc
Copy link
Contributor

this happens in Brave: 0.20.0?

@luixxiul luixxiul removed this from the 0.18.x Hotfix milestone Aug 15, 2017
@luixxiul
Copy link
Contributor Author

on the latest master

@NejcZdovc NejcZdovc added this to the 0.21.x (Nightly Channel) milestone Aug 15, 2017
@NejcZdovc
Copy link
Contributor

NejcZdovc commented Aug 15, 2017 •
edited
Loading

will add this to 0.21 where we introduce this change (split sites) where you see this error

@luixxiul
Copy link
Contributor Author

i confirmed that cannot be repro'd on 0.18.22

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Aug 20, 2017
@NejcZdovc
Follow up for sites splits PR brave#10296
8bdd858 
Resolves brave#10296
Resolves brave#10348
Resolves brave#10503

Auditors:

Test Plan:
@NejcZdovc NejcZdovc mentioned this issue Aug 20, 2017
Merged
8 tasks
@ghost ghost added the sprint/1 label Sep 13, 2017
@bbondy bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017
@cezaraugusto
Copy link
Contributor

ok I followed the same STR being the only change I imported from Chrome instead of Safari. While the main issue persists (no favicon for imported BM), the dialog was properly populated w/o errors. So apart from no favicon, I'm not able to reproduce other errors.

tested in:

@cezaraugusto
Copy link
Contributor

@luixxiul mind confirming the above pls? If the issue is only w/ favicons I think we can remove the relase-blocking label.

@cezaraugusto cezaraugusto added the needs-info Another team member needs information from the PR/issue opener. label Oct 26, 2017
@bbondy bbondy removed the sprint/1 label Oct 27, 2017
@luixxiul luixxiul added release/blocking 0.20.x issue first seen in 0.20.x labels Oct 30, 2017
@luixxiul
Copy link
Contributor Author

luixxiul commented Nov 6, 2017

It's been fixed 👍

@luixxiul luixxiul closed this as completed Nov 6, 2017
@luixxiul luixxiul added QA/checked-Linux QA/test-plan-specified release-notes/exclude and removed needs-info Another team member needs information from the PR/issue opener. labels Nov 6, 2017
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Nov 7, 2017
@NejcZdovc
Follow up for sites splits PR brave#10296
f1a68a8 
Resolves brave#10296
Resolves brave#10348
Resolves brave#10503

Auditors:

Test Plan:
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Nov 24, 2017
@NejcZdovc
Follow up for sites splits PR brave#10296
f2b1d2a 
Resolves brave#10296
Resolves brave#10348
Resolves brave#10503

Auditors:

Test Plan:
This was referenced Dec 20, 2017
Closed
Closed
@srirambv
Copy link
Collaborator

Verified on Windows. No favicon for imported BM but bookmark hanger is populated

@LaurenWags
Copy link
Member

Good on MacOS:
screen shot 2017-12-21 at 10 07 18 am

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@NejcZdovc NejcZdovc

Labels
0.20.x issue first seen in 0.20.x bug feature/bookmarks feature/importer QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified release/blocking release-notes/exclude
Projects
None yet
Milestone
0.20.x (Release Channel)
Development

No branches or pull requests
6 participants
@bbondy @luixxiul @cezaraugusto @NejcZdovc @srirambv @LaurenWags