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

[Riccardo Padovani]

> > 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.

Okay, so I leave it as it is now

> 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.

Yap, right

> 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.

Okay

> 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.

Right, it makes sense, thanks!

> 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.

Oh, right, thanks

> 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).

Okay, I'll try to avoid 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).

Done, hope it's ok
-- 
https://code.launchpad.net/~rpadovani/webbrowser-app/newTabRefactoring/+merge/247498
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.