Code review comment for lp:~artmello/webbrowser-app/webbrowser-app-bookmarks_view

> Thanks a lot for reviewing this. Comments bellow:
>
> > - I can not get any favicon to load, even for sites that I just visited and
> > have a favicon. The "globe" icon is always shown instead
>
> I am not able to reproduce it, it seems to be working with old bookmarks and
> with new ones. How are you reproducing it? Some specific url domain?

It must have been my fault. It seems to be working OK now

The style of fonts and other visuals in the bookmarks window is now matching the style of fonts in the new tab view. Thanks for fixing this.

> > - In non-widescreen mode:
> > - The bookmarks view has an header, the history doesn't. I would add one
> > to the history.
>
> Since that would be a change on history view, what do you think of me doing
> this on a different MR?

It is OK to do it here, since it is a change with very little impact other than visuals. But I am happy to have it done in another MR if you prefer (though I won't be around to review it next week).

> > - The folders in the bookmarks view should probably start all collapsed
> > instead of all open, unless no folders exist.
>
> Done

Please add unit tests to ensure this works properly, and make sure this change did not break any AP or unit test.

> Today we have URlDelegate and UrlDelegateWide with a lot of duplicate code,
> specially the one related with the url/title font. I didnt look further but
> would make sense to mix both those components on the same one? What do you
> think?

I am happy to wait and do this in a separate MR. There is enough in this one already.

Other than this, there are lots of AP tests failing, and they seem to be related to bookmarks, so I suspect they are caused by your changes. Please check and make sure everything is passing (the infrastructure is OK at the moment, because most other branches pass all AP tests, so all AP errors are most likely caused by your changes).