1398350 - Add loadReplace option to tabs.update on Android

Status: NEW

(WebExtensions :: Android, enhancement, P3)

Description

[Bob Silverberg [:bsilverberg]]

This is a follow-up to bug 1397383. The code to add the feature to Android is very simple, and pretty much the same as what was done for desktop, but the test is going to be tricky as we do not have access to SessionStore to do the assertions in Android. This will be picked up post 57.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

I took a deeper look to how we can make the Android version of the test included with Bug 1397383.

The following are the only blockers for this bug (all related to the test case for the Firefox for Android implementation):

- we can't use the history API (because it is not currently available on Android)
- we don't have the SessionStore.jsm (which is implemented a browser level: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm)

Nevertheless, we should be able to retrieve the needed history data for a Firefox for Android tab by importing and using directly the SessionHistory.jsm. 

e.g. in a new mochitest-chrome test (mobile/android/components/extensions/test/mochitest/chrome.ini) 
we can do something like the following:

    Cu.import("resource://gre/modules/Services.jsm");
    Cu.import("resource://gre/modules/sessionstore/SessionHistory.jsm");

    add_task(async function test_tabs_update_url_loadReplace() {
      // A background page which register a listener to browser.test.onMessage,
      // and on a test case message (e.g. one named "tab-update"):
      // - calls browser.tabs.update with the passed options
      // - wait the tab to be fully loaded (e.g. using a browser.tabs.onUpdated listener)
      // - message back to the test case (e.g. using a message named "tab-update-done" 
      function background() {
        ...
      }

      let extension = ExtensionTestUtils.loadExtension({
        manifest: {
          "permissions": ["tabs"],
        },
        background,
      });

      // Retrieve the Firefox for Android's BrowserApp (which is a privileged object
      // which we can use to open or close Firefox for Android tabs). 
      const {BrowserApp} = Services.wm.getMostRecentWindow("navigator:browser");

      // Create and select a new tab.
      const tab1 = BrowserApp.addTab("about:blank", {selected: true});

      // Check that the session history for the tab is initially empty.
      let tabHistoryData = SessionHistory.collect(BrowserApp.selectedBrowser.docShell);
      is(tabHistoryData.entries.length, 0, "The tab history entries should be initially empty");

      await extension.startup();

      // Ask the background page to call tabs.update without the loadReplace option.
      extension.sendMessage("tabs-update", tab1.id, {url: "http://example.net"});
      
      // Wait that the background page send the "tabs-update-done" test message 
      // once the tab updated url has been loaded. 
      await extension.awaitMessage("tabs-update-done");

      // Retrieve the updated session history for the tab and assert that we have a new
      // history entry.
      tabHistoryData = SessionHistory.collect(BrowserApp.selectedBrowser.docShell);
      is(tabHistoryData.entries.length, 1, "...");
      
      // Assert on the entry url.
      ...

      // Ask the background page to call tabs.update with the loadReplace option.
      extension.sendMessage("tabs-update", tab1.id, {url: "http://mochi.test:8888", loadReplace: true});
      
      // Retrieve the updated session history and assert that there is only one entry
      ...

      // Assert on the entry url (which should be the one from last tabs.update with loadReplace).
      ...

      // Unload the extension and close the tab before exiting the test case.
    })

From the implementation point of view, we just have to apply the same changes included in the patch from Bug 1397383:

- changes to the metadata in the Firefox for Android API schemas for the tabs API: 
    mobile/android/components/extensions/schemas/tabs.json

- changes to the tabs.update API implementation for Firefox for Android:
    mobile/android/components/extensions/ext-tabs.js

Comment 2

[Jan Henning [:JanH]]

Session store data on Android is currently stored under tab.browser.__SS_data if it's of any help

Comment 3

[Melissa Auclaire]

Here is what I have so far. I would like to make sure that I'm testing the right thing.

<!DOCTYPE HTML>
<html>
<head>
  <title>Tabs create Test</title>
  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
  <script type="text/javascript" src="/tests/SimpleTest/SpawnTask.js"></script>
  <script type="text/javascript" src="/tests/SimpleTest/ExtensionTestUtils.js"></script>
  <script type="text/javascript" src="head.js"></script>
  <link rel="stylesheet" href="/tests/SimpleTest/test.css"/>
</head>
<body>

<script type="text/javascript">
  "use strict";

  Cu.import("resource://gre/modules/Services.jsm");
  Cu.import("resource://gre/modules/sessionstore/SessionHistory.jsm");

    add_task(async function test_tabs_update_url_loadReplace() {

      function background() {
        // A background page which register a listener to browser.test.onMessage,
        browser.test.onMessage.addListener(async (msg, options) => {
          let bgPage = browser.extension.getBackgroundPage();
          bgPage.addListener(listener);
        });

        // and on a test case message (e.g. one named "tab-update"):
        browser.test.sendMessage("tab-update");

        // - calls browser.tabs.update with the passed options
        browser.test.onMessage.addListener(async (msg, data) => {
          if (msg === "tab-update") {
            await browser.tabs.update(data, {active: true}); //(tabid, updateProperties)
            browser.test.sendMessage("tab-update");
          }
        });

        // - wait the tab to be fully loaded (e.g. using a browser.tabs.onUpdated listener)
        let updatedPromise = new Promise(resolve => {
          let onUpdated = (changedTabId, changed) => {
            if (changed.url) {
              browser.tabs.onUpdated.removeListener(onUpdated);
              resolve({tabId: changedTabId, url: changed.url});
            }
          };
          browser.tabs.onUpdated.addListener(onUpdated);
        });

        // - message back to the test case (e.g. using a message named "tab-update-done"
        browser.test.sendMessage("tab-update-done", browser.runtime.getURL("tab.html"));
      }

      let extension = ExtensionTestUtils.loadExtension({
        manifest: {
          "permissions": ["tabs"],
        },
        background,
      });

      // Retrieve the Firefox for Android's BrowserApp (which is a privileged object
      // which we can use to open or close Firefox for Android tabs).
      const {BrowserApp} = Services.wm.getMostRecentWindow("navigator:browser");

      // Create and select a new tab.
      const tab1 = BrowserApp.addTab("about:blank", {selected: true});

      // Check that the session history for the tab is initially empty.
      let tabHistoryData = SessionHistory.collect(BrowserApp.selectedBrowser.docShell);
      is(tabHistoryData.entries.length, 0, "The tab history entries should be initially empty");

      await extension.startup();

      // Ask the background page to call tabs.update without the loadReplace option.
      extension.sendMessage("tabs-update", tab1.id, {url: "http://example.net"});

      // Wait that the background page send the "tabs-update-done" test message
      // once the tab updated url has been loaded.
      await extension.awaitMessage("tabs-update-done");

      // Retrieve the updated session history for the tab and assert that we have a new
      // history entry.
      tabHistoryData = SessionHistory.collect(BrowserApp.selectedBrowser.docShell);
      is(tabHistoryData.entries.length, 1, "...");

      // Assert on the entry url.
      browser.test.assertTrue(entryUrl, `tabs.update with URL ${tabsUpdateURL} should not be rejected`);


      // Ask the background page to call tabs.update with the loadReplace option.
      extension.sendMessage("tabs-update", tab1.id, {url: "http://mochi.test:8888", loadReplace: true});


      // Retrieve the updated session history and assert that there is only one entry
      tabHistoryData = SessionHistory.collect(BrowserApp.selectedBrowser.docShell);
      browser.test.assertTrue(tabHistoryData.entries.length, 1);


      // Assert on the entry url (which should be the one from last tabs.update with loadReplace).
      browser.test.assertTrue(entryUrl, {url: "http://mochi.test:8888", loadReplace: true});

      // Unload the extension and close the tab before exiting the test case.
  });
  yield extension.startup();
  yield extension.awaitFinish("tabs.create");
  yield extension.unload();

</script>

</body>
</html>

Comment 4

[Melissa Auclaire]

Attachment: bug1398350.zip - test and ini files for onloadreplace fix

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Melissa,
to make it easier to review a proposed set of changes, these changes should be attached in the form of a patch (e.g. the attached zip contains the files you changes but not where they are in the mozilla-central working tree, this time it is easier enough to figure out where they should be, but it is not always that easy).

Here is some MDN doc pages related to how a patch should be created:

- https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
- https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Reviewer_Checklist
- https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

If you are using mercurial (hg), here is a sequence of common steps to create a patch:

```
### ensure that you are on mozilla-central tip
 
$ hg update -r central

### start a bookmark (a mercurial concept that is similar to a git feature branch)

$ hg bookmark bug-1398350-loadReplace-android

### apply your changes (create new file, change existent files)

...

### review your changes using hg status ('M' means modified, '?' means that the files has never been
### added to mercurial yet)

$ hg status

M mobile/android/components/extensions/test/mochitest/chrome.ini
? mobile/android/components/extensions/test/mochitest/test_ext_tabs_addloadreplace.html

### add new files to mercurial (e.g. the new test file)

$ hg add mobile/android/components/extensions/test/mochitest/test_ext_tabs_addloadreplace.html

### review your pending changes again ('A' means that a new file is going to be added)

$ hg status

M mobile/android/components/extensions/test/mochitest/chrome.ini
A mobile/android/components/extensions/test/mochitest/test_ext_tabs_addloadreplace.html

### save the changes in a new 'pending' commit in the mercurial bookmark

$ hg commit -m "Bug 1398350 - Support tabs.update loadReplace option on Firefox for Android."

### review the list of the draft (not yet merged) patches

$ hg log --stat -r 'ancestors(.)&draft()'

changeset:   438952:505a3a3c55e6
bookmark:    bug-1398350-loadReplace-android
tag:         tip
user:        Your Name <your@email>
date:        ...
summary:     Bug 1398350 - Support tabs.update loadReplace option on Firefox for Android.

 mobile/android/components/extensions/test/mochitest/chrome.ini                        |    2 +-
 mobile/android/components/extensions/test/mochitest/test_ext_tabs_addloadreplace.html |  111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+), 1 deletions(-)

### export a single patch in a file using the changeset listed in the previous 'hg log' command

$ hg export -g -r 438952 -o bug-1398350.patch
```

Now you have a file named "bug-1398350.patch" which contains the changes you have applied locally,
in a format that can be used to apply the same patch on a different mozilla-central mercurial clone,
and you can attach it to the bugzilla issue.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8923769 [details]
bug1398350.zip - test and ini files for onloadreplace fix

Besides the above comment (related to how to create a patch file), here is an initial set of comments related to the files in the attached zip:

- You can add a test file to the mochitest.ini (and it will be a mochitest-plain test) or to the chrome.ini (and it will be a mochitest-chrome), but you should not add it to both

- A mochitest plain and a mochitest chrome looks similar but there are difference, e.g. the html preamble uses different urls:

  - In a mochitest plain: http://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/mobile/android/components/extensions/test/mochitest/test_ext_tabs_create.html#5-9

  - In a mochitest chrome: http://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/mobile/android/components/extensions/test/mochitest/test_ext_options_ui.html#5-9

For this tests we will need to access to some of the privileged Firefox for Android internals and a mochitest-chrome can make it simpler, and so you can just remove the file from the mochitest.ini and add it only to the chrome.ini, and then fix the html preamble to use the urls used in the other mochitest chrome (e.g. the one linked above).

- In the chrome.ini file from the attached zip, the line:

```
skip-if = os == 'android' # bug 1373170
```

  this is part of the `[test_ext_pageAction_getPopup_setPopup.html]` section and you should add the section of the new test after that line (or the test will be skipped on android).
  Some tests have a skip-if in their own section (e.g. in this case the test file has been blacklisted for intermittent failures).

- the changes to the ext-tabs.js API implementation looks missing from the zip file

Some review comments related to the test implementation:

- When the test is implemented as an async function, you have to use `await` instead of `yield`, and the last three yield from the test file are actually outside of the async test function (they should be removed)

- Every test should call `await extension.unload()` near the end of the test

- The test should be indented as the other test files

- Cu is a common shortcut, but it is not defined by default in a mochitest-chrome, and it have to be defined explicitly using: `const {classes: Cc, interfaces: Ci, utils: Cu} = Components;`

- At line 24 and 25 there is a browser.extension.getBackgroundPage API call from the background page itself, and then bgPage.addListener(listener) is called, listener is not defined anywhere, but more important: it is not clear to me what is its role/goal in the test

- At line 29 it looks that the background page is sending a “tab-update” message, but that test message is supposed to be received from the background page and not sent

Here is a background script that should be near enough to the one needed for this test:

```
function background() {
  browser.test.onMessage.addListener(async (msg, tabId, updateOptions) => {
    if (msg !== "tabs-update") {
      return;
    }

    browser.tabs.onUpdated.addListener(function listener(updatedTabId, updateInfo) {
      if (updatedTabId !== tabId) {
        return;
      }

      if (updateInfo.status !== "complete" &&
          updateInfo.url !== updateOptions.url) {
        return;
      }

      browser.tabs.onUpdated.removeListener(listener);

      browser.test.sendMessage("tabs-update-done");
    });

    await browser.tabs.update(tabId, updateOptions);
  });
}
```

This background page subscribe a “test message” listener (which it doesn’t need to remove, because it will be automatically removed when the test extension is unloaded), and when it receive a “tabs-update” message (e.g. from the `extension.sendMessage("tabs-update", tab1.id, {url: "http://example.net"});` in your test file), it will subscribe a listener to tabs.onUpdated (which will wait that the tab updated is completely loaded, then it removes itself and send a tabs-update-done test message to signal that the test can go to its next step) and then it calls tabs.update on the given tabId and the given updateOptions (both coming from the extension.sendMessage calls).