Merge lp:~fboucault/webbrowser-app/background_open_tabs_adjacent into lp:webbrowser-app/staging

[Florian Boucault]

Make new tabs opened in the background to be placed next to the tab opening requesting them instead of at the end of the list of tabs.

Make new tabs opened in the background to be placed next to the tab opening requesting them instead of at the end of the list of tabs.

[Olivier Tilloy (osomon)]

Can you please re-target the merge request to lp:webbrowser-app/staging?

The "Open link in new tab" and "Open link in new background tab" context menu entries should also open the new tab next to the caller (and there should be autopilot tests for those use cases).

Also, see one minor comment inline.

review: Needs Fixing

[lp:~fboucault/webbrowser-app/background_open_tabs_adjacent]

1602. By Florian Boucault on 2017-02-02

Also apply opening new tab adjacent to current tab when using the following contextual actions:
- opening a link in a new tab
- opening a link in a new background tab
- opening an image in a new tab
- opening a video in a new tab

[Olivier Tilloy (osomon)]

I am getting 17 failures out of 20 tests when running the test_contextmenu autopilot tests in narrow mode on desktop (gotta modify the initial window width to 50 GU in src/app/BrowserWindow.qml). This is a regression.

review: Needs Fixing

[lp:~fboucault/webbrowser-app/background_open_tabs_adjacent]

1603. By Florian Boucault on 2017-02-02

TabsModel unit test: deallocate memory properly

1604. By Florian Boucault on 2017-02-02

Fix AP tests in narrow mode

[Olivier Tilloy (osomon)]

I just observed that in chromium on desktop, when closing a tab that had been opened from another tab, the parent tab becomes current again, whereas in webbrowser-app the tab to the right of the one being closed becomes current. Maybe not something we want to address in this branch, but it would be a nice improvement to build on top of this branch.

[Olivier Tilloy (osomon)]

webbrowser_app.tests.test_tabs.TestTabsManagement.test_ctrl_click_open_link_in_next_tab appears to be reliably failing in narrow mode.

review: Needs Fixing

[Olivier Tilloy (osomon)]

> Maybe not something we want to address in this branch, but it would be
> a nice improvement to build on top of this branch.

Actually, I think it would make sense to implement this in this branch. I guess we need to add a property to BrowserTab so that it remembers its "parent" tab, and restores focus on it when closed.

[Florian Boucault (fboucault)]

> > Maybe not something we want to address in this branch, but it would be
> > a nice improvement to build on top of this branch.
>
> Actually, I think it would make sense to implement this in this branch. I
> guess we need to add a property to BrowserTab so that it remembers its
> "parent" tab, and restores focus on it when closed.

I think this behaviour could have been implemented before this branch. Therefore I would say it is very independent and think we should implement it separately.

[lp:~fboucault/webbrowser-app/background_open_tabs_adjacent]

1605. By Florian Boucault on 2017-02-15

Merged staging

1606. By Florian Boucault on 2017-02-15

Fixed AP test test_ctrl_click_open_link_in_next_tab in narrow mode

[Olivier Tilloy (osomon)]

LGTM, thanks.

review: Approve

[Olivier Tilloy (osomon)]

> I think this behaviour could have been implemented before this branch.
> Therefore I would say it is very independent and think we should
> implement it separately.

Filed bug #1664990 to track its implementation.

=== modified file 'src/app/webbrowser/TabComponent.qml'
--- src/app/webbrowser/TabComponent.qml	2016-11-25 15:49:50 +0000
+++ src/app/webbrowser/TabComponent.qml	2017-02-15 08:26:53 +0000
@@ -33,6 +33,7 @@
     id: tabComponent
 
     BrowserTab {
+        id: browserTab
         anchors.fill: parent
         incognito: browser ? browser.incognito : false
         current: browser ? browser.tabsModel && browser.tabsModel.currentTab === this : false
@@ -84,12 +85,14 @@
                 Actions.OpenLinkInNewTab {
                     objectName: "OpenLinkInNewTabContextualAction"
                     enabled: contextModel && contextModel.linkUrl.toString()
-                    onTriggered: internal.openUrlInNewTab(contextModel.linkUrl, true)
+                    onTriggered: internal.openUrlInNewTab(contextModel.linkUrl, true,
+                                                          true, tabsModel.indexOf(browserTab) + 1)
                 }
                 Actions.OpenLinkInNewBackgroundTab {
                     objectName: "OpenLinkInNewBackgroundTabContextualAction"
                     enabled: contextModel && contextModel.linkUrl.toString()
-                    onTriggered: internal.openUrlInNewTab(contextModel.linkUrl, false)
+                    onTriggered: internal.openUrlInNewTab(contextModel.linkUrl, false,
+                                                          true, tabsModel.indexOf(browserTab) + 1)
                 }
                 Actions.OpenLinkInNewWindow {
                     objectName: "OpenLinkInNewWindowContextualAction"
@@ -142,7 +145,8 @@
                     enabled: contextModel &&
                              (contextModel.mediaType === Oxide.WebView.MediaTypeImage) &&
                              contextModel.srcUrl.toString()
-                    onTriggered: internal.openUrlInNewTab(contextModel.srcUrl, true)
+                    onTriggered: internal.openUrlInNewTab(contextModel.srcUrl, true,
+                                                          true, tabsModel.indexOf(browserTab) + 1)
                 }
                 Actions.CopyImage {
                     objectName: "CopyImageContextualAction"
@@ -165,7 +169,8 @@
                     enabled: contextModel &&
                              (contextModel.mediaType === Oxide.WebView.MediaTypeVideo) &&
                              contextModel.srcUrl.toString()
-                    onTriggered: internal.openUrlInNewTab(contextModel.srcUrl, true)
+                    onTriggered: internal.openUrlInNewTab(contextModel.srcUrl, true,
+                                                          true, tabsModel.indexOf(browserTab) + 1)
                 }
                 Actions.SaveVideo {
                     objectName: "SaveVideoContextualAction"
@@ -253,9 +258,9 @@
             contextMenu: browser && browser.wide ? contextMenuWideComponent : contextMenuNarrowComponent
 
             onNewViewRequested: {
-                var tab = browser.createTab({"request": request})
+                var newTab = browser.createTab({"request": request})
                 var setCurrent = (request.disposition == Oxide.NewViewRequest.DispositionNewForegroundTab)
-                internal.addTab(tab, setCurrent)
+                internal.addTab(newTab, setCurrent, tabsModel.indexOf(browserTab) + 1)
                 if (setCurrent) tabContainer.forceActiveFocus()
             }
 
@@ -263,11 +268,9 @@
             onPrepareToCloseResponse: {
                 if (proceed) {
                     if (tab) {
-                        for (var i = 0; i < tabsModel.count; ++i) {
-                            if (tabsModel.get(i) === tab) {
-                                tabsModel.remove(i)
-                                break
-                            }
+                        var i = tabsModel.indexOf(tab);
+                        if (i != -1) {
+                            tabsModel.remove(i);
                         }
 
                         // tab.close() destroys the context so add new tab before destroy if required

=== modified file 'src/app/webbrowser/tabs-model.cpp'
--- src/app/webbrowser/tabs-model.cpp	2015-12-03 10:47:22 +0000
+++ src/app/webbrowser/tabs-model.cpp	2017-02-15 08:26:53 +0000
@@ -203,6 +203,15 @@
     return m_tabs.at(index);
 }
 
+/*!
+    Returns the index position of the first occurrence of tab in the model.
+    Returns -1 if no item matched.
+*/
+int TabsModel::indexOf(QObject* tab) const
+{
+    return m_tabs.indexOf(tab);
+}
+
 void TabsModel::move(int from, int to)
 {
     if ((from == to) || !checkValidTabIndex(from) || !checkValidTabIndex(to)) {

=== modified file 'src/app/webbrowser/tabs-model.h'
--- src/app/webbrowser/tabs-model.h	2015-09-02 16:37:20 +0000
+++ src/app/webbrowser/tabs-model.h	2017-02-15 08:26:53 +0000
@@ -60,6 +60,7 @@
     Q_INVOKABLE int insert(QObject* tab, int index);
     Q_INVOKABLE QObject* remove(int index);
     Q_INVOKABLE QObject* get(int index) const;
+    Q_INVOKABLE int indexOf(QObject* tab) const;
     Q_INVOKABLE void move(int from, int to);
 
 Q_SIGNALS:

=== modified file 'tests/autopilot/webbrowser_app/tests/__init__.py'
--- tests/autopilot/webbrowser_app/tests/__init__.py	2016-11-08 15:03:01 +0000
+++ tests/autopilot/webbrowser_app/tests/__init__.py	2017-02-15 08:26:53 +0000
@@ -176,6 +176,15 @@
 
         return new_tab_view
 
+    def switch_to_tab(self, index):
+        if self.main_window.wide:
+            tab = self.main_window.chrome.get_tabs_bar().get_tab(index)
+            self.pointing_device.click_object(tab)
+        else:
+            tabs_view = self.open_tabs_view()
+            tabs_view.get_previews()[index].select()
+            tabs_view.visible.wait_for(False)
+
     def open_new_window(self):
         windows = self.app.get_windows(incognito=False)
         chrome = self.main_window.chrome

=== modified file 'tests/autopilot/webbrowser_app/tests/test_contextmenu.py'
--- tests/autopilot/webbrowser_app/tests/test_contextmenu.py	2016-09-21 07:33:26 +0000
+++ tests/autopilot/webbrowser_app/tests/test_contextmenu.py	2017-02-15 08:26:53 +0000
@@ -28,6 +28,8 @@
 
     def setUp(self, path):
         super(TestContextMenuBase, self).setUp(path)
+        self.open_new_tab(open_tabs_view=not self.main_window.wide)
+        self.switch_to_tab(0 if self.main_window.wide else 1)
         self.menu = self.main_window.open_context_menu()
         # The context menu should never steal active focus from the webview,
         # but because it’s currently implemented as a dialog on narrow screens,
@@ -36,8 +38,16 @@
             webview = self.main_window.get_current_webview()
             self.assertThat(webview.activeFocus, Equals(True))
 
-    def verify_link_opened_in_a_new_tab(self):
-        self.assert_number_webviews_eventually(2)
+    def verify_link_opened_in_a_new_tab(self, background=False):
+        self.assert_number_webviews_eventually(3)
+        # make sure the new tab is adjacent to the tab that created it,
+        # in other words that its index is 1 in wide mode, 0 in narrow mode
+        # http://pad.lv/1499780
+        if not self.main_window.wide and not background:
+            new_tab_index = 0
+        else:
+            new_tab_index = 1
+        self.switch_to_tab(new_tab_index)
         webview = self.main_window.get_current_webview()
         new_url = self.base_url + "/test1"
         self.assertThat(webview.url, Eventually(Equals(new_url)))
@@ -48,8 +58,16 @@
         self.main_window.wait_until_page_loaded(url)
         self.main_window.chrome.bookmarked.wait_for(True)
 
-    def verify_image_opened_in_a_new_tab(self):
-        self.assert_number_webviews_eventually(2)
+    def verify_image_opened_in_a_new_tab(self, background=False):
+        self.assert_number_webviews_eventually(3)
+        # make sure the new tab is adjacent to the tab that created it,
+        # in other words that its index is 1 in wide mode, 0 in narrow mode
+        # http://pad.lv/1499780
+        if not self.main_window.wide and not background:
+            new_tab_index = 0
+        else:
+            new_tab_index = 1
+        self.switch_to_tab(new_tab_index)
         webview = self.main_window.get_current_webview()
         self.assertThat(webview.url,
                         Eventually(StartsWith(self.data_uri_prefix)))
@@ -79,6 +97,10 @@
         self.menu.click_action("OpenLinkInNewTabContextualAction")
         self.verify_link_opened_in_a_new_tab()
 
+    def test_open_link_in_new_background_tab(self):
+        self.menu.click_action("OpenLinkInNewBackgroundTabContextualAction")
+        self.verify_link_opened_in_a_new_tab(background=True)
+
     def test_open_link_in_new_window(self):
         self.menu.click_action("OpenLinkInNewWindowContextualAction")
         self.verify_link_opened_in_a_new_window(False)
@@ -132,6 +154,10 @@
         self.menu.click_action("OpenLinkInNewTabContextualAction")
         self.verify_link_opened_in_a_new_tab()
 
+    def test_open_link_in_new_background_tab(self):
+        self.menu.click_action("OpenLinkInNewBackgroundTabContextualAction")
+        self.verify_link_opened_in_a_new_tab(background=True)
+
     def test_open_link_in_new_window(self):
         self.menu.click_action("OpenLinkInNewWindowContextualAction")
         self.verify_link_opened_in_a_new_window(False)

=== modified file 'tests/autopilot/webbrowser_app/tests/test_tabs.py'
--- tests/autopilot/webbrowser_app/tests/test_tabs.py	2016-08-10 15:43:04 +0000
+++ tests/autopilot/webbrowser_app/tests/test_tabs.py	2017-02-15 08:26:53 +0000
@@ -222,6 +222,26 @@
         self.assertThat(len(views), Equals(1))
         self.check_current_tab(url)
 
+    # http://pad.lv/1499780
+    @testtools.skipIf(model() != "Desktop", "on desktop only")
+    def test_ctrl_click_open_link_in_next_tab(self):
+        self.open_new_tab(open_tabs_view=not self.main_window.wide)
+        self.switch_to_tab(0 if self.main_window.wide else 1)
+
+        url = self.base_url + "/link"
+        self.main_window.go_to_url(url)
+        self.main_window.wait_until_page_loaded(url)
+        webview = self.main_window.get_current_webview()
+
+        self.keyboard.press('Ctrl')
+        self.pointing_device.click_object(webview)
+        self.keyboard.release('Ctrl')
+
+        # Eventually we should have three webviews
+        self.assert_number_webviews_eventually(3)
+        self.switch_to_tab(1)
+        self.check_current_tab(self.url)
+
     def test_open_iframe_target_blank_in_new_tab(self):
         url = self.base_url + "/fulliframewithblanktargetlink"
         self.main_window.go_to_url(url)

=== modified file 'tests/unittests/tabs-model/tst_TabsModelTests.cpp'
--- tests/unittests/tabs-model/tst_TabsModelTests.cpp	2016-04-04 10:17:33 +0000
+++ tests/unittests/tabs-model/tst_TabsModelTests.cpp	2017-02-15 08:26:53 +0000
@@ -486,6 +486,19 @@
         QCOMPARE(model->get(3), (QObject*) nullptr);
     }
 
+    void shouldReturnTabIndex()
+    {
+        QQuickItem* tab1 = createTab();
+        model->add(tab1);
+        QQuickItem* tab2 = createTab();
+        model->add(tab2);
+        QQuickItem* nonAddedTab = createTab();
+        QCOMPARE(model->indexOf(tab1), 0);
+        QCOMPARE(model->indexOf(tab2), 1);
+        QCOMPARE(model->indexOf(nonAddedTab), -1);
+        delete nonAddedTab;
+    }
+
 private:
     void moveTabs(int from, int to, bool moved, bool indexChanged, int newIndex)
     {