Merge lp:~artmello/webbrowser-app/webbrowser-app-changes_on_new_tab into lp:webbrowser-app

[Arthur Mello]

Use favicons in place of blank thumbnails in the new tab view.

Use favicons in place of blank thumbnails
Show an empty Bookmarks section if no bookmarks exist

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:616
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/935/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/1807/console
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-utopic/1521
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-amd64-ci/134
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-armhf-ci/134
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-armhf-ci/134/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-i386-ci/134
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-mako/2068/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2889
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2889/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/9626
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-utopic/1274
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/1708
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/1708/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[Olivier Tilloy (osomon)]

I’m getting the following error when trying to launch the application:

    file:///home/osomon/dev/phablet/browser/webbrowser-app/src/app/webbrowser/PageDelegate.qml:29 Duplicate property name

(hint: merge the latest changes from trunk into your branch)

review: Needs Fixing

[Olivier Tilloy (osomon)]

Once the above error is fixed (just remove "property url icon" from PageDelegate.qml), a couple of remarks on the visual aspect:

 - I wouldn’t embed the icon in an UbuntuShape, that makes a lot of nested shapes, and it’s visually loaded up

 - Favicons tend to be low-resolution images that don’t scale up well, so they should have a fixed size. Have a look at what I did in UrlDelegate.qml, you probably want to factor that out in a common Favicon component that can be used by PageDelegate and UrlDelegate.

[Olivier Tilloy (osomon)]

Why the change to NewTabView.qml ? I’m not sure it makes sense to display an empty bookmarks section, does it?

[Arthur Mello (artmello)]

> Why the change to NewTabView.qml ? I’m not sure it makes sense to display an
> empty bookmarks section, does it?

It was requested on the Browser UX Freeze Plan doc. I think the issue was that when the user has no Bookmarks he does not know that bookmarks can be displayed there.

[Olivier Tilloy (osomon)]

> > Why the change to NewTabView.qml ? I’m not sure it makes sense to display an
> > empty bookmarks section, does it?
>
> It was requested on the Browser UX Freeze Plan doc. I think the issue was that
> when the user has no Bookmarks he does not know that bookmarks can be
> displayed there.

In that case there should be a message stating that there are no bookmarks, otherwise I think it’s even more confusing, people will think it’s a bug or something.

[lp:~artmello/webbrowser-app/webbrowser-app-changes_on_new_tab]

617. By Arthur Mello on 2014-07-11

Merge with trunk

618. By Arthur Mello on 2014-07-11

Create a Favicon component
Reduce size of the Favicon on the PageDelegate
Remove UbuntuShape from the Favicon on the PageDelegate

619. By Arthur Mello on 2014-07-11

Do not show empty sections on NewTabView

[Arthur Mello (artmello)]

> Once the above error is fixed (just remove "property url icon" from
> PageDelegate.qml), a couple of remarks on the visual aspect:
>
> - I wouldn’t embed the icon in an UbuntuShape, that makes a lot of nested
> shapes, and it’s visually loaded up
>
> - Favicons tend to be low-resolution images that don’t scale up well, so they
> should have a fixed size. Have a look at what I did in UrlDelegate.qml, you
> probably want to factor that out in a common Favicon component that can be
> used by PageDelegate and UrlDelegate.

Fixed on rev618

[Arthur Mello (artmello)]

> > > Why the change to NewTabView.qml ? I’m not sure it makes sense to display
> an
> > > empty bookmarks section, does it?
> >
> > It was requested on the Browser UX Freeze Plan doc. I think the issue was
> that
> > when the user has no Bookmarks he does not know that bookmarks can be
> > displayed there.
>
> In that case there should be a message stating that there are no bookmarks,
> otherwise I think it’s even more confusing, people will think it’s a bug or
> something.

I'm not sure if I like the section with an "empty bookmarks" message. That would use some space that could be better used to display history if there is no bookmarks. Anyway I removed the change on r619 and added a comment to the Browser UX Freeze Plan doc

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:619
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/936/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/1855
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-utopic/1553
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-amd64-ci/135
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-armhf-ci/135
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-armhf-ci/135/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-i386-ci/135
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-mako/2116
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2951
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2951/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/9691
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-utopic/1301
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/1743
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/1743/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Olivier Tilloy (osomon)]

In Favicon.qml:

 - the id is unused, it can be removed

 - you need to add an import for "Ubuntu.Components 0.1", as you’re referring to units.dp(), which is part of the Ubuntu UITK

review: Needs Fixing

[lp:~artmello/webbrowser-app/webbrowser-app-changes_on_new_tab]

620. By Arthur Mello on 2014-07-11

Minor changes on Favicon

[Arthur Mello (artmello)]

> In Favicon.qml:
>
> - the id is unused, it can be removed
>
> - you need to add an import for "Ubuntu.Components 0.1", as you’re referring
> to units.dp(), which is part of the Ubuntu UITK

Fixed on r620

[Olivier Tilloy (osomon)]

LGTM.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:620
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/939/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/1882
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-utopic/1572
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-amd64-ci/138
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-armhf-ci/138
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-armhf-ci/138/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-i386-ci/138
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-mako/2137
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2980
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2980/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/9714
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-utopic/1316
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/1762
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/1762/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

=== added file 'src/app/webbrowser/Favicon.qml'
--- src/app/webbrowser/Favicon.qml	1970-01-01 00:00:00 +0000
+++ src/app/webbrowser/Favicon.qml	2014-07-11 16:17:35 +0000
@@ -0,0 +1,25 @@
+/*
+ * Copyright 2014 Canonical Ltd.
+ *
+ * This file is part of webbrowser-app.
+ *
+ * webbrowser-app is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * webbrowser-app is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+import QtQuick 2.0
+import Ubuntu.Components 0.1
+
+Image {
+    width: units.dp(16)
+    height: units.dp(16)
+}

=== modified file 'src/app/webbrowser/PageDelegate.qml'
--- src/app/webbrowser/PageDelegate.qml	2014-07-09 10:11:10 +0000
+++ src/app/webbrowser/PageDelegate.qml	2014-07-11 16:17:35 +0000
@@ -24,8 +24,8 @@
 
     property url url
     property alias thumbnail: thumbnail.source
+    property alias icon: icon.source
     property alias label: label.text
-    property url icon
     property bool canClose: false
     property bool canBookmark: false
     property bool bookmarked
@@ -54,6 +54,14 @@
             image: Image {
                 id: thumbnail
             }
+
+            // Show the favicon if no thumbnail defined for the url
+            Favicon {
+                id: icon
+                anchors.centerIn: parent
+
+                visible: !thumbnail.source.toString()
+            }
         }
 
         MouseArea {

=== modified file 'src/app/webbrowser/UrlDelegate.qml'
--- src/app/webbrowser/UrlDelegate.qml	2014-07-09 10:35:00 +0000
+++ src/app/webbrowser/UrlDelegate.qml	2014-07-11 16:17:35 +0000
@@ -42,11 +42,9 @@
             height: parent.height
             width: parent.height
 
-            Image {
+            Favicon {
                 id: icon
                 anchors.centerIn: parent
-                width: units.dp(16)
-                height: units.dp(16)
             }
         }