Merge into trunk : newTabRefactoring : Code : webbrowser-app

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Needs Fixing on 2015-05-25
Olivier Tilloy		2015-01-24	Approve on 2015-05-25
Review via email: mp+247498@code.launchpad.net

PS Jenkins bot (ps-jenkins) wrote on 2015-01-24:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1388/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-01-25:

#

FAILED: Continuous integration, rev:878
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1391/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/976
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-vivid/457
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/149
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/149
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/149/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/149/console
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/865
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/974
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/974/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17427
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-vivid/374
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/549
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/549/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1391/rebuild

review: Needs Fixing (continuous-integration)

Olivier Tilloy (osomon) wrote on 2015-01-26:

#

That looks closer to the visual spec, but needs tweaking still:
- the More/Less button should have a strokeColor of "#5d5d5d"
- it should be taller
- there should be an orange star to the left of the "bookmarks" section header
- when expanding the bookmarks section, the "popular sites" one should be hidden (just like the current implementation does, which your patch seems to change)

review: Needs Fixing

Riccardo Padovani (rpadovani) wrote on 2015-01-26:

#

> That looks closer to the visual spec, but needs tweaking still:
> - the More/Less button should have a strokeColor of "#5d5d5d"
> - it should be taller

Fixed

> - there should be an orange star to the left of the "bookmarks" section
> header

Added, but I had to rewrite all the header, let me know if it's still good

> - when expanding the bookmarks section, the "popular sites" one should be
> hidden (just like the current implementation does, which your patch seems to
> change)

I thought it was because when you click on "more" in the old implementation it shows all the bookmarks, and now it's 5 by 5. Anyway, I've restored it!

Thanks :-)

PS Jenkins bot (ps-jenkins) wrote on 2015-01-26:

#

FAILED: Continuous integration, rev:879
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/1394/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/983
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-vivid/463
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/152
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/152
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/152/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/152/console
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/871
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/981
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/981/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17445
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-vivid/376
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/556
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/556/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1394/rebuild

review: Needs Fixing (continuous-integration)

Francis Ginther (fginther) wrote on 2015-01-26:

#

The jenkins node for the i386 build failed, I've restarted a new ci run.

PS Jenkins bot (ps-jenkins) wrote on 2015-01-27:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1400/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-02-25:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1489/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-03-02:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1502/rebuild

review: Needs Fixing (continuous-integration)

Olivier Tilloy (osomon) wrote on 2015-03-09:

#

This looks better, however I’m seeing some glitches, see the screenshots at http://people.canonical.com/~osomon/newTabRefactoring/ (taken in a vivid desktop VM).

- capture.png shows how the new page looks like when there are no bookmarks
- capture2.png shows how it looks like after adding a bookmark if it was initially created with no bookmarks

review: Needs Fixing

Riccardo Padovani (rpadovani) wrote on 2015-03-13:

#

As note, this could be related: https://bugreports.qt.io/browse/QTBUG-40271

Riccardo Padovani (rpadovani) wrote on 2015-03-14:

#

Ok, now should be ok.

Let me do a summary on this branch and on the new tab view, maybe there are better solutions of the one I thought.

So, designers want the behavior of the new tav view is like a listview with two sections, bookmarks and topsites.

The problem we have the model of bookmarks and of topsites are two different models.
A possible solution is to create a new model that is a merge of the others two, but could be have some problems, so I preferred to avoid this.

The solution already implemnented is to create a lisview with two delegate, a bookmark and a topsite.

Every delegate has another listview (or repeater) inside, with the related model.

The solution works well, but miss a the adding/removing of the section header when the last element is added/removed (as bug #1389605 reports).

My first thought was to use 'visible' element in the section header, but for some reason it doesn't work (at least on vivid). You can try just adding 'visible: false' in the section header.

So I choose to set height of the section header to 0 when there is nothing in the delegate, but I hit the upstream bug I reported in the comment above.

In the last commit I changed the approach: I add/remove section in the main listview when there aren't elements anymore.

I think this worsk well, but it doesn't scale well (if in a future we want to add a third section it's quite a mess) and the code is a bit ugly, but I haven't figured any better solution.

What do you think?

PS Jenkins bot (ps-jenkins) wrote on 2015-03-14:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1508/rebuild

review: Needs Fixing (continuous-integration)

Olivier Tilloy (osomon) wrote on 2015-03-16:

#

Mmm… that does look ugly indeed, and is not scalable, but I understand the problem as you clearly expressed it.

In any case with this approach, there’s one thing that needs fixing:

    } else if (numberOfBookmarks == 1 &&
    […]
    } else if (numberOfTopSites == 1 &&

you need to check for numberOf… > 0, as we cannot assume that only one bookmark or one topsite was added

Another approach we could take is to not use section delegates, i.e. implement the entire new tab view as a column inside a flickable. That would remove much of the complexity of the code, I think.

review: Needs Fixing

Riccardo Padovani (rpadovani) wrote on 2015-03-23:

#

We discussed about this on IRC, but I left a comment here as future reference.

I found a couple of bugs with limitedlistview:

- it doesn't emit a signal when the number of bookmarks change so if you have 5 bookmarks, open a new page, bookmark it, and then return to the new tab view, there isn't the button 'show more'. But there is if you open a *new* new tab

- If I place the limitedlistview in a column, how we discussed, there is a strange bug when you remove an element: let's say the model has 6 elements and the limit is 5: column contentHeight is limitedlistview.delegate * 5, as it should be. When I remove an element, the column contentHeight becomes limitedlistview.delegate * 4, while it should remain * 5. Since the limitedlistview isn't the only element in the column, forcing the contentHeight of the column is difficult and require a lot of ugly code

Riccardo Padovani (rpadovani) wrote on 2015-04-13:

#

Okay, I changed approach, what do you think? It doesn't hide anymore sections header. I think I removed all unused code, but could be there is some useless code.

PS Jenkins bot (ps-jenkins) wrote on 2015-04-13:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1630/rebuild

review: Needs Fixing (continuous-integration)

Riccardo Padovani (rpadovani) wrote on 2015-04-13:

#

Actually, I also found a workaround for the LimitProxyModel bug (it doesn't emit the signal when a delegate is removed), so now it should works well in any case (add more than 5 elements, the show more button appears, fix bug #1442190).

Now we can do whatever we want with sections, we can also hide headers when we want.
And I think code is cleaner.

A thing I can do is adding a count property to BookmarksModel class to avoid to use LimitProxyModel to count number of object (but we still need it for the top sites section).

I propose an implementation I like, but we can discuss it now there isn't any of the bug I reported in previous comments.

What do you think?

PS Jenkins bot (ps-jenkins) wrote on 2015-04-13:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1631/rebuild

review: Approve (continuous-integration)

Olivier Tilloy (osomon) wrote on 2015-04-14:

#

Let’s put this on hold for a week or two: Arthur is currently working on updating the design of the top sites section, and that will likely conflict with your changes (sorry I didn’t see that coming). I think most (if not all) of your changes will still apply, but there’s likely going to be a few conflicts to resolve.

I’d be curious to see if your changes fix bug #1444023 (which I just reported), by the way.

Riccardo Padovani (rpadovani) wrote on 2015-04-14:

#

> Let’s put this on hold for a week or two: Arthur is currently working on
> updating the design of the top sites section, and that will likely conflict
> with your changes (sorry I didn’t see that coming). I think most (if not all)
> of your changes will still apply, but there’s likely going to be a few
> conflicts to resolve.

No problem at all, I'll wait for the update :-)

> I’d be curious to see if your changes fix bug #1444023 (which I just
> reported), by the way.

Mhhh maybe, I need to test that

Riccardo Padovani (rpadovani) wrote on 2015-04-30:

#

Ok, I updated the MR, now is definitely cleaner and seems to solve in an elegant way all our bugs :-)

PS Jenkins bot (ps-jenkins) wrote on 2015-04-30:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1716/rebuild

review: Needs Fixing (continuous-integration)

Olivier Tilloy (osomon) wrote on 2015-04-30:

#

I’m seeing a number of minor discrepancies with the visual spec (https://docs.google.com/presentation/d/1woHjO8K4iqyVZZlfQ4BXL0DhYbwkEmZ7wvcUhYzHDRk/edit?pli=1#slide=id.g34608d763_0170):

- the star icon for the bookmarks section header is too big
- the font size for the section headers should be small
- the spacing above and below the section headers should be increased (there should be 1GU above and 1GU below the More/Less button)
- the color of the horizontal line is not correct
- there should be much more spacing between the bottom of the bookmarks section and the top of the top sites section

Also, a functional issue:
- when I press the More button to expand the bookmarks section, I can scroll the top of the list view out of sight, even though the listview doesn’t completely fill the screen

review: Needs Fixing

Olivier Tilloy (osomon) wrote on 2015-05-12:

#

424 + Component.onCompleted: {
425 + if (model.hidden !== undefined && model.hidden) {
426 + height = 0;
427 + limit++;
428 + }
429 + }

The above is not correct. Use the TopSitesModel that filters out hidden entries instead.

review: Needs Fixing

Riccardo Padovani (rpadovani) wrote on 2015-05-12:

#

Hi Olivier,
thanks for your review!

I addressed all your comments about UI.

But I've troubles with the TopSitesModel component - I'm sure I'm missing something very easy, but I'm not able to figure what. At the moment no sites are displayed in the top sites area, but no error are printed on the console.

Could you take a look, please?

PS Jenkins bot (ps-jenkins) wrote on 2015-05-12:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1746/rebuild

review: Needs Fixing (continuous-integration)

Olivier Tilloy (osomon) wrote on 2015-05-12:

#

This is getting there, nice job! And it now fixes 5 bugs (just added one to the list), you’re going to set a new record here :)

A few comments to address still:

If I have a number of bookmarks (more than 5), then open a new tab and start removing all bookmarks by swiping them to the right and confirming, they are correctly removed (and following bookmarks in the list show up), but the height of the bookmarks section is not updated, it doesn’t shrink when the total number of bookmarks gets < 5. Eventually, when only one bookmark is left, there is a big, almost empty bookmarks section, which looks akward. If I then open another new tab, the section’s height is correct.

There should be some spacing above the message that says that the user hasn’t visited any sites yet, it’s currently stuck to the section separator, which looks ugly. And the text color probably shouldn’t be plain black.

numberOfBookmarks and numberOfTopSites should check that bookmarksModel and historyModel are not null before trying to read their 'count' properties. This is because those models are instantiated asynchronously at startup. If a blank tab is restored at startup and for some reason the models take a long time to load, you will see (harmless) warnings in the console. Those are easy enough to fix that it’s worth doing it.

There should be some spacing between the bottom of the top sites list and the bottom of the window, to account for the bottom edge hint on mobile (I guess it’s ok to have that spacing unconditionally, even on desktop where it’s not strictly needed).

In the UrlList for top sites, why do you increase the limit in the onUrlRemoved handler?

Can you update the unit tests for the bookmarks model to check that the rowCountChanged signal is correctly emitted when expected, and that the value of the count is always as expected?

review: Needs Fixing

PS Jenkins bot (ps-jenkins) wrote on 2015-05-12:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1749/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-05-16:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1774/rebuild

review: Needs Fixing (continuous-integration)

Riccardo Padovani (rpadovani) wrote on 2015-05-16:

#

> If I have a number of bookmarks (more than 5), then open a new tab and start
> removing all bookmarks by swiping them to the right and confirming, they are
> correctly removed (and following bookmarks in the list show up), but the
> height of the bookmarks section is not updated, it doesn’t shrink when the
> total number of bookmarks gets < 5. Eventually, when only one bookmark is
> left, there is a big, almost empty bookmarks section, which looks akward. If I
> then open another new tab, the section’s height is correct.

Actually, I wasn't able to reproduce this. Are you on vivid?

But I reproduced a similar issue: if I've 2 new tab view open, and I remove bookmarks in the 2nd one, the height in the 1st isn't updated. I fixed that, so I hope it fixes also the one you found.
Also, I found a similar issue with 'See more' - expand in one tab, remove bookmarks in the other one, go back - but I think I fixed also this one

> There should be some spacing above the message that says that the user hasn’t
> visited any sites yet, it’s currently stuck to the section separator, which
> looks ugly. And the text color probably shouldn’t be plain black.

Done, and now the text it's of the same color of 'See more' button

> numberOfBookmarks and numberOfTopSites should check that bookmarksModel and
> historyModel are not null before trying to read their 'count' properties. This
> is because those models are instantiated asynchronously at startup. If a blank
> tab is restored at startup and for some reason the models take a long time to
> load, you will see (harmless) warnings in the console. Those are easy enough
> to fix that it’s worth doing it.

Done

> There should be some spacing between the bottom of the top sites list and the
> bottom of the window, to account for the bottom edge hint on mobile (I guess
> it’s ok to have that spacing unconditionally, even on desktop where it’s not
> strictly needed).

Are you sure? Having a gray line at bottom is ugly, and the effect is very strange.
Take a look to Dekko or Reminders, they don't have any space at bottom, and looks good anyway

> In the UrlList for top sites, why do you increase the limit in the
> onUrlRemoved handler?

Legacy code from the first implementation, thanks, removed

> Can you update the unit tests for the bookmarks model to check that the
> rowCountChanged signal is correctly emitted when expected, and that the value
> of the count is always as expected?

Done

Olivier Tilloy (osomon) wrote on 2015-05-18:

#

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

review: Needs Fixing

Riccardo Padovani (rpadovani) wrote on 2015-05-18:

#

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

PS Jenkins bot (ps-jenkins) wrote on 2015-05-18:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1777/rebuild

review: Needs Fixing (continuous-integration)

Olivier Tilloy (osomon) wrote on 2015-05-18:

#

In NewTabView.qml, please s/Text/Label/, to ensure consistent theming.

newTabViewLoader is well placed, but there are a few issues with it:
- it shouldn’t fill the parent, its anchors should be the same as the two other Loaders above it
- please set its 'asynchronous' property to true, to ensure it doesn’t block rendering at startup
- the NewTabView component should be loaded only when needed, not always (like it is now)

review: Needs Fixing

Riccardo Padovani (rpadovani) wrote on 2015-05-18:

#

Fixed all, I think

PS Jenkins bot (ps-jenkins) wrote on 2015-05-18:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1779/rebuild

review: Needs Fixing (continuous-integration)

Olivier Tilloy (osomon) wrote on 2015-05-18:

#

When launching the app, previously open tabs are restored (default behaviour), and the webview for the current tab loads, but for a brief moment I can see the new tab view displayed. It shouldn’t be loaded at all in this case.

Conversely, when I open a new tab, the new tab view is not displayed.

Also, I’m seeing the following warning in the console at startup:

QQmlExpression: Expression file:///home/osomon/dev/phablet/browser/webbrowser-app/src/app/webbrowser/Browser.qml:194:30 depends on non-NOTIFYable properties:
WebViewImpl_QMLTYPE_85_QML_146::restoreState

To address those issues, I think you need to do the following:
- sourceComponent should not be set (no property binding)
- have a Connections object that monitors when currentWebview changes and that does what was done prior to your changes in the Loader’s Component.onCompleted handler

One thing we’ll need to verify is that when switching from one empty tab to another empty tab, the new tab view isn’t deleted and re-created. We could easily write an autopilot test for that, but given that there are zero autopilot tests for this feature so far, let’s add some in a separate MR (I’ll do it).

Another minor remark: I think "anchors.fill: parent" inside the NewTabView instance is unneeded, since by default a component will be anchored to fill its parent loader.

review: Needs Fixing

PS Jenkins bot (ps-jenkins) wrote on 2015-05-20:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1802/rebuild

review: Needs Fixing (continuous-integration)

Olivier Tilloy (osomon) wrote on 2015-05-21:

#

I’m still seeing the new tab view being briefly instantiated and destroyed right away at startup, when restoring a tab that isn’t empty. This needs fixing, as it adds unnecessarily to the startup time (and the view can actually be seen for a split second, which is confusing).

Otherwise I verified that when switching from one new tab to another new tab the view isn’t destroyed and re-created, that’s good.

review: Needs Fixing

Olivier Tilloy (osomon) wrote on 2015-05-21:

#

After some deep investigation, I finally nailed where the issue is: the problem is that oxide’s WebView.restoreState will always return an empty string (see http://bazaar.launchpad.net/~oxide-developers/oxide/oxide.trunk/view/head:/qt/quick/api/oxideqquickwebview.cc#L1606). So !currentWebview.restoreState will always be true. Instead we need to check the value of restoreState on the tab object.

Also one note that will simplify the code: instead of setting sourceComponent conditionally on the Loader, we can set it once, and then toggle the 'active' property of the loader.

I produced a patch that does those two things and should apply cleanly on top of your branch: http://pastebin.ubuntu.com/11261520/.

PS Jenkins bot (ps-jenkins) wrote on 2015-05-21:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1806/rebuild

review: Needs Fixing (continuous-integration)

Olivier Tilloy (osomon) wrote on 2015-05-21:

#

That looks great now, thanks!

Note: there will be conflicts in Browser.qml and NewTabView.qml when merging into trunk after https://code.launchpad.net/~artmello/webbrowser-app/webbrowser-app-private_browsing/+merge/259388 lands (which I expect to happen today), so they will need to be resolved.

review: Approve

Olivier Tilloy (osomon) wrote on 2015-05-22:

#

The private browsing branch landed in trunk, and this MR now conflicts.

Riccardo Padovani (rpadovani) wrote on 2015-05-23:

#

Fixed (I hope, I don't have oxide updated on this pc and I'm not able to test it)

Olivier Tilloy (osomon) wrote on 2015-05-25:

#

8 -import Ubuntu.Components.Popups 1.0

Please do not remove that line, it is needed.

44 + sourceComponent: webviewimpl.incognito ? newPrivateTabViewComponent : newTabViewComponent

This should be browser.incognito, not webviewimpl.incognito.

151 - function resetFocus() {

This function is still being used, it shouldn’t be removed.

168 - var tab = tabComponent.createObject(tabContainer, {"initialUrl": url, 'incognito': browser.incognito})
169 + var tab = tabComponent.createObject(tabContainer, {"initialUrl": url})

This change should be reverted.

177 - for (var i = 0; i < publicTabsModel.count; ++i) {
178 - var tab = publicTabsModel.get(i)
179 + for (var i = 0; i < tabsModel.count; ++i) {
180 + var tab = tabsModel.get(i)

This change should be reverted.

review: Needs Fixing

lp:~rpadovani/webbrowser-app/newTabRefactoring updated on 2015-05-25

900. By Riccardo Padovani on 2015-05-25: Fix wronge merge request

Riccardo Padovani (rpadovani) wrote on 2015-05-25:

#

Sorry for the wrong merge, I should have fixed all now

Olivier Tilloy (osomon) wrote on 2015-05-25:

#

Thanks, that’s better! Can you remove the anchors in the NewPrivateTabView instance in Browser.qml? They are not needed since the parent loader already defines its anchors (the loaded item will fill its parent loader by default).

lp:~rpadovani/webbrowser-app/newTabRefactoring updated on 2015-05-25

901. By Riccardo Padovani on 2015-05-25: Remove useless anchors

Riccardo Padovani (rpadovani) wrote on 2015-05-25:

#

Done :-)

Olivier Tilloy (osomon) wrote on 2015-05-25:

#

This looks good again, thanks!

review: Approve

PS Jenkins bot (ps-jenkins) wrote on 2015-05-25:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1808/rebuild

review: Needs Fixing (continuous-integration)

webbrowser-app

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

Commit Message

Description of the Change

Preview Diff

Subscribers

 === modified file 'src/app/webbrowser/Browser.qml'
 --- src/app/webbrowser/Browser.qml	2015-05-22 19:21:10 +0000
 +++ src/app/webbrowser/Browser.qml	2015-05-25 07:08:15 +0000
@@ -185,6 +185,64 @@
              asynchronous: true
+         }
++        Loader {
++            id: newTabViewLoader
++            anchors {
++                fill: tabContainer
++                topMargin: (chrome.state == "shown") ? chrome.height : 0
++            }
++
++            // Avoid loading the new tab view if the webview is about to load
++            // content. Since WebView.restoreState is not a notifyable property,
++            // this can’t be achieved with a simple property binding.
++            Connections {
++                target: currentWebview
++                onUrlChanged: {
++                    newTabViewLoader.active = false
++                }
++            }
++            active: false
++
++            Connections {
++                target: browser
++                onCurrentWebviewChanged: {
++                    if (currentWebview) {
++                        var tab = tabsModel.currentTab
++                        newTabViewLoader.active = !tab.url.toString() && !tab.restoreState
++                    }
++                }
++            }
++
++            sourceComponent: browser.incognito ? newPrivateTabViewComponent : newTabViewComponent
++
++            Component {
++                id: newTabViewComponent
++
++                NewTabView {
++                    historyModel: browser.historyModel
++                    bookmarksModel: browser.bookmarksModel
++                    onBookmarkClicked: {
++                        chrome.requestedUrl = url
++                        currentWebview.url = url
++                        currentWebview.forceActiveFocus()
++                    }
++                    onBookmarkRemoved: browser.bookmarksModel.remove(url)
++                    onHistoryEntryClicked: {
++                        chrome.requestedUrl = url
++                        currentWebview.url = url
++                        currentWebview.forceActiveFocus()
++                    }
++                }
++            }
++
++            Component {
++                id: newPrivateTabViewComponent
++
++                NewPrivateTabView { }
++            }
++            asynchronous: true
++        }
++
          SearchEngine {
              id: currentSearchEngine
              searchPaths: searchEnginesSearchPaths
@@ -827,64 +885,6 @@
                          Component.onDestruction: bottomEdgeHint.forceShow = false
+                     }
+                 }
--
--                Loader {
--                    id: newTabViewLoader
--                    anchors.fill: parent
--
--                    // Avoid loading the new tab view if the webview is about to load
--                    // content. Since WebView.restoreState is not a notifyable property,
--                    // this can’t be achieved with a simple property binding.
--                    Component.onCompleted: {
--                        if (!parent.url.toString() && !parent.restoreState) {
--                            if (!webviewimpl.incognito) {
--                                sourceComponent = newTabViewComponent
--                            } else {
--                                sourceComponent = newPrivateTabViewComponent
--                            }
--                        }
--                    }
--                    Connections {
--                        target: newTabViewLoader.parent
--                        onUrlChanged: newTabViewLoader.sourceComponent = null
--                    }
--
--                    Component {
--                        id: newTabViewComponent
--
--                        NewTabView {
--                            anchors {
--                                fill: parent
--                                topMargin: (chrome.state == "shown") ? chrome.height : 0
--                            }
--
--                            historyModel: browser.historyModel
--                            bookmarksModel: browser.bookmarksModel
--                            onBookmarkClicked: {
--                                chrome.requestedUrl = url
--                                currentWebview.url = url
--                                currentWebview.forceActiveFocus()
--                            }
--                            onBookmarkRemoved: browser.bookmarksModel.remove(url)
--                            onHistoryEntryClicked: {
--                                chrome.requestedUrl = url
--                                currentWebview.url = url
--                                currentWebview.forceActiveFocus()
--                            }
--                        }
--                    }
--
--                    Component {
--                        id: newPrivateTabViewComponent
--
--                        NewPrivateTabView {
--                            anchors {
--                                fill: parent
--                                topMargin: (chrome.state == "shown") ? chrome.height : 0
--                            }
--                        }
--                    }
--                }
+             }
+         }
+     }
@@ -1100,7 +1100,7 @@
      Component {
          id: leavePrivateModeDialog
--
++
          LeavePrivateModeDialog {
              id: dialogue
              objectName: "leavePrivateModeDialog"
 === modified file 'src/app/webbrowser/NewTabView.qml'
 --- src/app/webbrowser/NewTabView.qml	2015-05-19 14:53:12 +0000
 +++ src/app/webbrowser/NewTabView.qml	2015-05-25 07:08:15 +0000
@@ -1,5 +1,5 @@
  /*
-- * Copyright 2014 Canonical Ltd.
++ * Copyright 2014-2015 Canonical Ltd.
+  *
   * This file is part of webbrowser-app.
+  *
@@ -26,149 +26,223 @@
      id: newTabView
      property QtObject bookmarksModel
--    property QtObject historyModel
++    property alias historyModel: historyTimeframeModel.sourceModel
      signal bookmarkClicked(url url)
      signal bookmarkRemoved(url url)
      signal historyEntryClicked(url url)
++    TopSitesModel {
++        id: topSitesModel
++        sourceModel: HistoryTimeframeModel {
++            id: historyTimeframeModel
++        }
++    }
++
      QtObject {
          id: internal
--        property int bookmarksCountLimit: 5
--        property bool seeMoreBookmarksView: false
--    }
--
--    ListModel {
--        id: sectionsModel
--
--        Component.onCompleted: {
--            if (bookmarksListModel && bookmarksListModel.count !== 0)
--                sectionsModel.append({ section: "bookmarks" });
--            if (historyListModel && historyListModel.count !== 0 && !internal.seeMoreBookmarksView )
--                sectionsModel.append({ section: "topsites" });
--        }
--    }
--
--    LimitProxyModel {
--        id: bookmarksListModel
--
--        sourceModel: newTabView.bookmarksModel
--
--        limit: internal.seeMoreBookmarksView ? -1 : internal.bookmarksCountLimit
--    }
--
--    LimitProxyModel {
--        id: historyListModel
--
--        sourceModel: TopSitesModel {
--            sourceModel: HistoryTimeframeModel {
--                sourceModel: newTabView.historyModel
--                // We only show sites visited on the last 60 days
--                start: {
--                    var date = new Date()
--                    date.setDate(date.getDate() - 60)
--                    return date
--                }
++        property bool seeMoreBookmarksView: bookmarksCountLimit > 4
++        property int bookmarksCountLimit: Math.min(4, numberOfBookmarks)
++        property int numberOfBookmarks: bookmarksModel ? bookmarksModel.count : 0
++        property int numberOfTopSites: historyModel ? historyModel.count : 0
++
++        // Force the topsites section to reappear when remove a bookmark while
++        // the bookmarks list is expanded and there aren't anymore > 5
++        // bookmarks
++        onNumberOfBookmarksChanged: {
++            if (numberOfBookmarks <= 4) {
++                seeMoreBookmarksView = false
+             }
+         }
--
--        limit: 10
+     }
      Rectangle {
--        id: newTabBackground
          anchors.fill: parent
          color: "#f6f6f6"
+     }
--    ListView {
--        id: newTabListView
++    Flickable {
          anchors.fill: parent
--
--        model: sectionsModel
--
--        delegate: Loader {
--            anchors {
--                left: parent.left
--                right: parent.right
--                margins: units.gu(2)
--            }
--
--            width: parent.width
--            height: children.height
--
--            sourceComponent: modelData == "bookmarks" ? bookmarksComponent : topSitesComponent
--        }
--
--        section.property: "section"
--        section.delegate: Rectangle {
--            anchors {
--                left: parent.left
--                right: parent.right
--            }
--
--            height: sectionHeader.height + units.gu(1)
--
--            opacity: section == "topsites" && internal.seeMoreBookmarksView ? 0.0 : 1.0
--
--            color: newTabBackground.color
--
--            Behavior on opacity { UbuntuNumberAnimation {} }
--
--            ListItem.Header {
--                id: sectionHeader
--
--                text: {
--                    if (section == "bookmarks") {
--                        return i18n.tr("Bookmarks")
--                    } else if (section == "topsites") {
--                        return i18n.tr("Top sites")
--                    }
--                }
--            }
--        }
--
--        section.labelPositioning: ViewSection.InlineLabels | ViewSection.CurrentLabelAtStart
--    }
--
--    Component {
--        id: bookmarksComponent
--
--        UrlsList {
--            id: bookmarksList
--
--            width: parent.width
--
--            model: bookmarksListModel
--
--            footerLabelText: internal.seeMoreBookmarksView ? i18n.tr("see less") : i18n.tr("see more")
--            footerLabelVisible: bookmarksListModel.unlimitedCount > internal.bookmarksCountLimit
--
--            onUrlClicked: newTabView.bookmarkClicked(url)
--            onUrlRemoved: newTabView.bookmarkRemoved(url)
--            onFooterLabelClicked: internal.seeMoreBookmarksView = !internal.seeMoreBookmarksView
--        }
--    }
--
--    Component {
--        id: topSitesComponent
--
--        UrlsList {
--            objectName: "topSitesList"
--
--            width: parent.width
--            opacity: internal.seeMoreBookmarksView ? 0.0 : 1.0
--
--            height: opacity == 0.0 ? 0 : childrenRect.height
--            model: historyListModel
--
--            footerLabelVisible: false
--
--            onUrlClicked: newTabView.historyEntryClicked(url)
--            onUrlRemoved: newTabView.historyModel.hide(url)
--
--            Behavior on opacity { UbuntuNumberAnimation {} }
++        contentHeight: internal.seeMoreBookmarksView ?
++                                          bookmarksColumn.height + units.gu(6) :
++                                          contentColumn.height
++
++        Column {
++            id: contentColumn
++            anchors {
++                left: parent.left
++                leftMargin: units.gu(1.5)
++                right: parent.right
++                rightMargin: units.gu(1.5)
++            }
++            height: childrenRect.height
++
++            Row {
++                height: units.gu(6)
++                anchors { left: parent.left; right: parent.right }
++                spacing: units.gu(1.5)
++
++                Icon {
++                    id: starredIcon
++                    color: "#dd4814"
++                    name: "starred"
++
++                    height: units.gu(2)
++                    width: height
++
++                    anchors {
++                        leftMargin: units.gu(1)
++                        topMargin: units.gu(1)
++                        verticalCenter: moreButton.verticalCenter
++                    }
++                }
++
++                Label {
++                    width: parent.width - starredIcon.width - moreButton.width - units.gu(3)
++                    anchors.verticalCenter: moreButton.verticalCenter
++
++                    text: i18n.tr("Bookmarks")
++                    fontSize: "small"
++                }
++
++                Button {
++                    id: moreButton
++                    height: parent.height - units.gu(2)
++
++                    anchors { top: parent.top; topMargin: units.gu(1) }
++
++                    strokeColor: "#5d5d5d"
++
++                    visible: internal.numberOfBookmarks > 4
++
++                    text: internal.bookmarksCountLimit >= internal.numberOfBookmarks
++                    ? i18n.tr("Less") : i18n.tr("More")
++
++                    onClicked: {
++                        internal.numberOfBookmarks > internal.bookmarksCountLimit ?
++                        internal.bookmarksCountLimit += 5:
++                        internal.bookmarksCountLimit = 4
++                    }
++                }
++            }
++
++            Rectangle {
++                height: units.gu(0.1)
++                anchors { left: parent.left; right: parent.right }
++                color: "#d3d3d3"
++            }
++
++            Column {
++                id: bookmarksColumn
++                anchors {
++                    left: parent.left
++                    leftMargin: units.gu(-1.5)
++                    right: parent.right
++                }
++
++                // Force the height to be updated when bookmarks are removed
++                // in another new tab
++                height: units.gu(5) * (Math.min(internal.bookmarksCountLimit, internal.numberOfBookmarks) + 1)
++                spacing: 0
++
++                UrlDelegate {
++                    id: homepageBookmark
++                    anchors {
++                        left: parent.left
++                        right: parent.right
++                    }
++                    height: units.gu(5)
++
++                    title: i18n.tr('Homepage')
++
++                    url: settings.homepage
++                    onItemClicked: newTabView.bookmarkClicked(url)
++                }
++
++                UrlsList {
++                    id: bookmarksList
++                    anchors {
++                        left: parent.left
++                        right: parent.right
++                    }
++
++                    spacing: 0
++                    limit: internal.bookmarksCountLimit
++
++                    model: newTabView.bookmarksModel
++
++                    onUrlClicked: newTabView.bookmarkClicked(url)
++                    onUrlRemoved: newTabView.bookmarkRemoved(url)
++                }
++            }
++
++            Rectangle {
++                height: units.gu(6)
++                anchors {
++                    left: parent.left
++                    right: parent.right
++                }
++                color: "#f6f6f6"
++
++                Label {
++                    anchors {
++                        left: parent.left
++                        right: parent.right
++                        bottom: parent.bottom
++                        bottomMargin: units.gu(1)
++                    }
++
++                    opacity: internal.seeMoreBookmarksView ? 0.0 : 1.0
++                    Behavior on opacity { UbuntuNumberAnimation {} }
++
++                    text: i18n.tr("Top sites")
++                    fontSize: "small"
++                }
++            }
++
++            Rectangle {
++                height: units.gu(0.1)
++                anchors { left: parent.left; right: parent.right }
++                color: "#d3d3d3"
++
++                opacity: internal.seeMoreBookmarksView ? 0.0 : 1.0
++                Behavior on opacity { UbuntuNumberAnimation {} }
++            }
++
++            Label {
++                height: units.gu(11)
++                anchors {
++                    left: parent.left
++                    right: parent.right
++                }
++                visible: internal.numberOfTopSites === 0
++
++                horizontalAlignment: Text.AlignHCenter
++                verticalAlignment: Text.AlignVCenter
++
++                text: i18n.tr("You haven't visited any site yet")
++                color: "#5d5d5d"
++            }
++
++            UrlsList {
++                anchors {
++                    left: parent.left
++                    leftMargin: units.gu(-1.5)
++                    right: parent.right
++                }
++
++                opacity: internal.seeMoreBookmarksView ? 0.0 : 1.0
++                Behavior on opacity { UbuntuNumberAnimation {} }
++
++                limit: 10
++                spacing: 0
++
++                model: topSitesModel
++
++                onUrlClicked: newTabView.historyEntryClicked(url)
++                onUrlRemoved: newTabView.historyModel.hide(url)
++            }
+         }
+     }
+ }
 === modified file 'src/app/webbrowser/UrlsList.qml'
 --- src/app/webbrowser/UrlsList.qml	2015-04-10 15:41:57 +0000
 +++ src/app/webbrowser/UrlsList.qml	2015-05-25 07:08:15 +0000
@@ -1,5 +1,5 @@
  /*
-- * Copyright 2014 Canonical Ltd.
++ * Copyright 2014-2015 Canonical Ltd.
+  *
   * This file is part of webbrowser-app.
+  *
@@ -22,12 +22,10 @@
  Column {
      id: urlsList
      property alias model: urlsListRepeater.model
--    property alias footerLabelText: footerLabel.text
--    property alias footerLabelVisible: footerLabel.visible
++    property int limit
      signal urlClicked(url url)
      signal urlRemoved(url url)
--    signal footerLabelClicked()
      spacing: units.gu(1)
@@ -37,70 +35,76 @@
          id: urlsListRepeater
          property var _currentSwipedItem: null
--        delegate: UrlDelegate{
--            id: urlDelegate
--            width: urlsList.width
--            height: units.gu(5)
--
--            icon: model.icon
--            title: model.title ? model.title : model.url
--            url: model.url
--
--            onItemClicked: urlClicked(model.url)
--
--            property var removalAnimation
--            function remove() {
--                removalAnimation.start()
--            }
--
--            onSwippingChanged: {
--                urlsListRepeater._updateSwipeState(urlDelegate)
--            }
--
--            onSwipeStateChanged: {
--                urlsListRepeater._updateSwipeState(urlDelegate)
--            }
--
--            leftSideAction: Action {
--                iconName: "delete"
--                text: i18n.tr("Delete")
--                onTriggered: {
--                    urlDelegate.remove()
--                }
--            }
--
--            ListView.onRemove: ScriptAction {
--                script: {
--                    if (urlsListRepeater._currentSwipedItem === urlDelegate) {
--                        urlsListRepeater._currentSwipedItem = null
--                    }
--                }
--            }
--
--            removalAnimation: SequentialAnimation {
--                alwaysRunToEnd: true
--
--                PropertyAction {
--                    target: urlDelegate
--                    property: "ListView.delayRemove"
--                    value: true
--                }
--
--                UbuntuNumberAnimation {
--                    target: urlDelegate
--                    property: "height"
--                    to: 0
--                }
--
--                PropertyAction {
--                    target: urlDelegate
--                    property: "ListView.delayRemove"
--                    value: false
--                }
--
--                ScriptAction {
--                    script: {
--                        urlRemoved(model.url)
++        delegate: Loader {
++            sourceComponent: (index < limit) ? realDelegate : undefined
++            Component {
++                id: realDelegate
++                UrlDelegate{
++                    id: urlDelegate
++                    width: urlsList.width
++                    height: units.gu(5)
++
++                    icon: model.icon
++                    title: model.title ? model.title : model.url
++                    url: model.url
++
++                    onItemClicked: urlClicked(model.url)
++
++                    property var removalAnimation
++                    function remove() {
++                        removalAnimation.start()
++                    }
++
++                    onSwippingChanged: {
++                        urlsListRepeater._updateSwipeState(urlDelegate)
++                    }
++
++                    onSwipeStateChanged: {
++                        urlsListRepeater._updateSwipeState(urlDelegate)
++                    }
++
++                    leftSideAction: Action {
++                        iconName: "delete"
++                        text: i18n.tr("Delete")
++                        onTriggered: {
++                            urlDelegate.remove()
++                        }
++                    }
++
++                    ListView.onRemove: ScriptAction {
++                        script: {
++                            if (urlsListRepeater._currentSwipedItem === urlDelegate) {
++                                urlsListRepeater._currentSwipedItem = null
++                            }
++                        }
++                    }
++
++                    removalAnimation: SequentialAnimation {
++                        alwaysRunToEnd: true
++
++                        PropertyAction {
++                            target: urlDelegate
++                            property: "ListView.delayRemove"
++                            value: true
++                        }
++
++                        UbuntuNumberAnimation {
++                            target: urlDelegate
++                            property: "height"
++                            to: 0
++                        }
++
++                        PropertyAction {
++                            target: urlDelegate
++                            property: "ListView.delayRemove"
++                            value: false
++                        }
++
++                        ScriptAction {
++                            script: {
++                                urlRemoved(model.url)
++                            }
++                        }
+                     }
+                 }
+             }
@@ -124,29 +128,4 @@
+             }
+         }
+     }
--
--    Item {
--        width: parent.width
--        height: footerLabel.visible ? footerLabel.height + units.gu(6) : units.gu(3)
--
--        MouseArea {
--            anchors.centerIn: footerLabel
--
--            width: footerLabel.width + units.gu(4)
--            height: footerLabel.height + units.gu(4)
--
--            enabled: footerLabel.visible
--
--            onClicked: footerLabelClicked()
--        }
--
--        Label {
--            id: footerLabel
--            anchors.centerIn: parent
--
--            visible: true
--
--            font.bold: true
--        }
--    }
+ }
 === modified file 'src/app/webbrowser/bookmarks-model.cpp'
 --- src/app/webbrowser/bookmarks-model.cpp	2014-07-30 15:52:11 +0000
 +++ src/app/webbrowser/bookmarks-model.cpp	2015-05-25 07:08:15 +0000
@@ -63,6 +63,7 @@
      createOrAlterDatabaseSchema();
      endResetModel();
      populateFromDatabase();
++    Q_EMIT rowCountChanged();
+ }
  void BookmarksModel::createOrAlterDatabaseSchema()
@@ -203,6 +204,7 @@
          endInsertRows();
          Q_EMIT added(url);
          insertNewEntryInDatabase(entry);
++        Q_EMIT rowCountChanged();
+     }
+ }
@@ -236,7 +238,7 @@
                  endRemoveRows();
                  Q_EMIT removed(url);
                  removeExistingEntryFromDatabase(url);
--
++                Q_EMIT rowCountChanged();
                  return;
              } else {
                  index++;
 === modified file 'src/app/webbrowser/bookmarks-model.h'
 --- src/app/webbrowser/bookmarks-model.h	2014-07-30 15:52:11 +0000
 +++ src/app/webbrowser/bookmarks-model.h	2015-05-25 07:08:15 +0000
@@ -33,6 +33,7 @@
      Q_OBJECT
      Q_PROPERTY(QString databasePath READ databasePath WRITE setDatabasePath NOTIFY databasePathChanged)
++    Q_PROPERTY(int count READ rowCount NOTIFY rowCountChanged)
      Q_ENUMS(Roles)
@@ -63,6 +64,7 @@
      void databasePathChanged() const;
      void added(const QUrl& url) const;
      void removed(const QUrl& url) const;
++    void rowCountChanged();
  private:
      QSqlDatabase m_database;
 === modified file 'tests/unittests/bookmarks-model/tst_BookmarksModelTests.cpp'
 --- tests/unittests/bookmarks-model/tst_BookmarksModelTests.cpp	2014-07-30 15:52:11 +0000
 +++ tests/unittests/bookmarks-model/tst_BookmarksModelTests.cpp	2015-05-25 07:08:15 +0000
@@ -179,6 +179,21 @@
          model->setDatabasePath(fileName);
          QCOMPARE(model->rowCount(), 2);
+     }
++
++    void shouldCountNumberOfEntries()
++    {
++        QSignalSpy spyCount(model, SIGNAL(rowCountChanged()));
++        QCOMPARE(model->property("count").toInt(), 0);
++        model->add(QUrl("http://example.org/"), "Example Domain", QUrl());
++        QCOMPARE(model->property("count").toInt(), 1);
++        QCOMPARE(spyCount.count(), 1);
++        model->add(QUrl("http://example.com/"), "Example Domain", QUrl());
++        QCOMPARE(model->property("count").toInt(), 2);
++        QCOMPARE(spyCount.count(), 2);
++        model->remove(QUrl("http://example.com/"));
++        QCOMPARE(model->property("count").toInt(), 1);
++        QCOMPARE(spyCount.count(), 3);
++    }
  };
  QTEST_MAIN(BookmarksModelTests)