[Merge] lp:~uriboni/webbrowser-app/topsite-previews into lp:webbrowser-app

Olivier Tilloy olivier.tilloy at canonical.com
Tue Oct 6 11:05:50 UTC 2015


A first incomplete round of (mostly) functional review, with comments in no particular order:

In UrlPreviewDelegate.hide_from_history(), the comment mentions bug #1205144, which has been fixed in the UITK, so the hack can now be cleaned up. Same in test_new_tab_view.py.

When launching the app, I’m seeing the following error in the console:
  src/app/webbrowser/Browser.qml:377:30: Unable to assign [undefined] to bool

When opening a new tab, I’m seeing the following error in the console:
  src/app/webbrowser/Browser.qml:518: ReferenceError: UrlUtils is not defined

I don’t think this is a bug introduced by your changes, rather an issue that’s always been there, but it is now more visible: initially, the captures for my top sites grid are blank (expected). If from there I click on one top site to open it, wait for the page to load, then close the tab, and open a new tab again, there is no capture for the page that I just viewed. This is because a capture is not made when closing a tab (only when switching tabs). Not sure how much work this would require to implement, but worth taking a look anyway.

On a related note, can captures be removed when closing tabs? I thought this was implemented already, but it doesn’t seem to work.

The captures in the grid view look very blurry, is this because of downscaling, and if so can we do something to improve it?

In narrow mode, when dragging the grid view upwards, it overlaps with the list of bookmarks above. It needs to be clipped.

The context menu that appears on grid items when long-pressing/right-clicking can be dismissed when clicked outside, but the click is propagated to whatever’s underneath, so if I click anywhere to dismiss the menu I might activate a top site.

Not a big deal as (AFAIK) keyboard navigation was never implemented in narrow views, but since the grid view handles it correctly, can the narrow new tab view be fixed to fully support keyboard nav? If it would require too many changes, I’m fine with leaving it for another branch.
-- 
https://code.launchpad.net/~uriboni/webbrowser-app/topsite-previews/+merge/269771
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.



More information about the Ubuntu-reviews mailing list