[Merge] lp:~rpadovani/webbrowser-app/newTabRefactoring into lp:webbrowser-app

[Olivier Tilloy]

Review: Needs Fixing

> I fixed that, so I hope it fixes also the one you found.

It seems to fix the issue I was seeing indeed.


> Are you sure? Having a gray line at bottom is ugly, and the effect
> is very strange.

It shouldn’t be a gray line, just some additional empty space, IMHO. Anyway, I’m fine with keeping it like that for now, changing that in the future is easy if we want to.


Some additional remarks:

The item that contains the "You haven't visited any site yet" text doesn’t need to be rectangle, it could simply be a plain Item. This will avoid an ugly white background. Actually it doesn’t even need a container, the Text could be made larger, and its text vertically centered.

The (bookmarksModel.count !== undefined) and (historyModel.count !== undefined) checks are unnecessary: as long as bookmarksModel and historyModel are not null, it is safe to expect them to have a count property. If they don’t then something much bigger is broken anyway.

In onNumberOfBookmarksChanged, please check if (numberOfBookmarks <= 4) rather than for equality. If for some reason two bookmarks are removed in a batch, that number might go from 5 to 3 without stepping at 4, and the condition wouldn’t be verified.
Also, in the same if() block, you don’t need to check the value of seeMoreBookmarksView, you can inconditionally set it to false. If it was already false, nothing will happen.

Please do not add semi-colons at the end of lines for pure QML code (javascript is OK, although throughout the code base we tend not to use them).

Finally, one suggestion for performance optimization: I don’t know why we have one new tab view per webview in the first place, but it seems to me that one instance for all webviews would be enough. We would avoid lots of potential issues with updating one view and not the others, and of course it would perform better as we would instantiate fewer objects. Do you think you can do that change? In Browser.qml, around line 176, we would need an additional Loader, similar to the two loaders above (one is for the ErrorSheet and the other one for the InvalidCertificateErrorSheet).
-- 
https://code.launchpad.net/~rpadovani/webbrowser-app/newTabRefactoring/+merge/247498
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.