Open Bug 1401194 Opened 2 years ago Updated Last year

Land newTabSection API in mozilla-central

Categories

(WebExtensions :: Frontend, enhancement, P3)

enhancement

Tracking

(Not tracked)

People

(Reporter: bsilverberg, Assigned: k88hudson)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [newTabSection] triaged)

User Story

https://github.com/k88hudson/newTabSection-test

Attachments

(2 files)

This is the tracking bug to land the WebExtensions experiment for the newTabSection API [1] in Firefox as a first-class WebExtensions API.

[1] https://github.com/mozilla/newtab-webextensions
It was decided that we will land the current code, pending a design review, into mozilla-central, which will be tracked by this bug. More bugs will be opened as follow-ups to make additional changes to the API.
Keywords: meta
Depends on: 1407978
Ok, I've made some progress on this with the following changes:

- Removed the onAction API as discussed (it can be added in the future if needed)
- I've fixed some stuff relating to the Preferences Panel and disabled the collapsible property for now until a better implementation can be landed in the SectionManager (see https://github.com/mozilla/activity-stream/pull/3936/files)

Still needs tests and a fix to make the enabled property permanent. I wonder if I should disable that as well for the time being since it needs changes to the SectionManager API too?
Assignee: nobody → khudson
Depends on: 1430859
During the meeting today, we noted that having an event for when items are (1) clicked on as well as when items are (2) dismissed would be useful. We should implement a version of dismissing that does not affect history but removes the item in memory.
Also: change the schema to remove context menu options for cards. URL could be optional if a click event can be handled
Depends on: 1431506
Comment on attachment 8942975 [details]
Bug 1401194 - Add newtab section webextension api

https://reviewboard.mozilla.org/r/213240/#review222412

Thanks for the initial commit, Kate, nice work!

I added some comments that refer back to the design review document (which is at https://docs.google.com/document/d/19egnIQ3SSPPvQDY9SjAB1DnTIwumYRTUmliWi5wPu00/edit?usp=sharing), as well as some conversations we've had.

::: browser/components/extensions/ext-newtabSection.js:64
(Diff revision 2)
> +    const onUpdateOption = () =>
> +      SectionsManager.sections.has(id) &&
> +      SectionsManager.updateSection(id, options, true);
> +
> +    const newtabSection = {
> +      setTitle(title) {

As per the design review, you may want to consider combining all of these `setX` methods into a single `update` method which would accept an `options` object.

::: browser/components/extensions/ext-newtabSection.js:96
(Diff revision 2)
> +      // (but the section is still there and will be shown in the pref pane).
> +      // Here we use "enable" to mean "add the section to ActivityStream and
> +      // then set `enabled` to be true". The second step is a workaround until
> +      // ActivityStream is responsible for setting the `enabled` property based
> +      // on whether the user had hidden the extension in the previous session.
> +      enable() {

As per the design review, you may want to consider removing the `enable` and `disable` methods, and simply automatically "enabling" the section upon install (i.e., in `onManifestEntry`) and "disabling" it upon extension disable or uninstall.

::: browser/components/extensions/ext-newtabSection.js:110
(Diff revision 2)
> +      addCards(cards, shouldBroadcast = false) {
> +        if (SectionsManager.sections.has(id)) {
> +          SectionsManager.updateSection(id, {rows: cards}, shouldBroadcast);
> +        }
> +      },
> +

As per the design review, please consider adding an `isEnabled` method to the API to query whether the section in question is currently enabled (i.e., visible) by the user.

::: browser/components/extensions/ext-newtabSection.js:112
(Diff revision 2)
> +          SectionsManager.updateSection(id, {rows: cards}, shouldBroadcast);
> +        }
> +      },
> +
> +      // Fired when the section is enabled
> +      onInitialized: new EventManager(context, "newtabSection.onInitialized", fire => {

As per the design review, we were unsure when exactlty this fires and what the uses cases are for it. Could you elaborate on that?

::: browser/components/extensions/ext-newtabSection.js:139
(Diff revision 2)
> +          SectionsManager.off(SectionsManager.ENABLE_SECTION, enabledListener);
> +        };
> +      }).api(),
> +
> +      // Fired when the section is disabled
> +      onUninitialized: new EventManager(context, "newtabSection.onUninitialized", fire => {

As above, could you provide a bit more information about this? Does this get fired when a user "disables" a section by unchecking it from the preferences page, or is it only fired when the section is disabled as performed by the `disable` method above (i.e., when the section is removed entirely)?

::: browser/components/extensions/schemas/newtabSection.json:1
(Diff revision 2)
> +[

General note: descriptions should be added for most of the items in the file to explain what they are/do.

::: browser/components/extensions/schemas/newtabSection.json:60
(Diff revision 2)
> +          "title": {"type": "string"},
> +          "description": {"type": "string", "optional": true},
> +          "hostname": {"type": "string", "optional": true},
> +          "type": {"type": "string", "optional": true},
> +          "image": {"type": "string", "optional": true},
> +          "context_menu_options": {

I might be misremembering, but I thought we said we were going to remove this for now, and not allow an extension to control which context menu options are available for a card.

::: browser/components/extensions/schemas/newtabSection.json:157
(Diff revision 2)
> +            "name": "cards",
> +            "type": "array",
> +            "items": {"type": "object", "$ref": "Card"}
> +          },
> +          {
> +            "name": "shouldBroadcast",

What does `shouldBroadcast` do? The description property could be used to explain this.

::: browser/components/extensions/schemas/newtabSection.json:164
(Diff revision 2)
> +            "optional": "true"
> +          }
> +        ]
> +      }
> +    ],
> +    "events": [

Descriptions should be added for all the events to explain when they are triggered, and parameters should be added to explain what data is received by a listener.
Comment on attachment 8942975 [details]
Bug 1401194 - Add newtab section webextension api

https://reviewboard.mozilla.org/r/213240/#review223896

Thanks for the update, Kate.

::: browser/components/extensions/schemas/newtabSection.json:80
(Diff revisions 2 - 3)
> +        "description": "Updates any of the available manifest options",
>          "type": "function",
>          "parameters": [
>            {
> -            "name": "icon",
> -            "$ref": "IconPath"
> +            "name": "options",
> +            "type": "object",

You might consider providing a type for this at the top of the file (e.g., UpdateOptions), rather than just describing an object's properties in here.

::: browser/components/extensions/schemas/newtabSection.json:110
(Diff revisions 2 - 3)
>            }
>          ]
>        },
>        {
>          "name": "enable",
> +        "description": "Makes the section visible (this will override a user's preference if they have set it in Activity Stream preferences)",

Nit: For all descriptions, add a period at the end of the sentence.

::: browser/components/extensions/schemas/newtabSection.json:142
(Diff revisions 2 - 3)
>        }
>      ],
>      "events": [
>        {
>          "name": "onInitialized",
> +        "description": "Fired when the Section Manager has initialized the section and is ready to receive new cards / updates",

Nit: Change to `and it is ready`.
Comment on attachment 8942975 [details]
Bug 1401194 - Add newtab section webextension api

https://reviewboard.mozilla.org/r/213240/#review222412

> As per the design review, you may want to consider removing the `enable` and `disable` methods, and simply automatically "enabling" the section upon install (i.e., in `onManifestEntry`) and "disabling" it upon extension disable or uninstall.

I am trying to think of use cases where it would be necessary to override/change users' settings for enabling and disabling, and I can't really come up with anything very compelling. So, I think removing them probably makes sense

> As per the design review, we were unsure when exactlty this fires and what the uses cases are for it. Could you elaborate on that?

This is fired when the section is ready, or when it's re-initialized via the prefs menu by users. I still think it's probably worth

> As above, could you provide a bit more information about this? Does this get fired when a user "disables" a section by unchecking it from the preferences page, or is it only fired when the section is disabled as performed by the `disable` method above (i.e., when the section is removed entirely)?

Same here, it's when a user disables the section as well as when it's uninstalled
Ok, I've added some mochi tests – I've also removed the infoOption property (since it's not something that's likely to stick around in Activity Stream)
Comment on attachment 8942975 [details]
Bug 1401194 - Add newtab section webextension api

https://reviewboard.mozilla.org/r/213240/#review230910


Code analysis found 33 defects in this patch (only the first 30 are reported here):
 - 33 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/extensions/ext-newtabSection.js:8
(Diff revision 5)
> +ChromeUtils.import("resource://activity-stream/lib/SectionsManager.jsm");
> +ChromeUtils.import("resource://gre/modules/ExtensionCommon.jsm");
> +
> +const ACTIVITY_STREAM_ACTIONS = {
> +  WEBEXT_CLICK: "Click",
> +  WEBEXT_DISMISS: "Dismiss"

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/ext-newtabSection.js:16
(Diff revision 5)
> +this.newtabSection = class extends ExtensionAPI { // eslint-disable-line no-unused-vars
> +  constructor(extension) {
> +    super(extension);
> +
> +    const manifestOptions = Object.assign({},
> +      this.extension.manifest.new_tab_section_options);

Error: Expected indentation of 42 spaces but found 6. [eslint: indent]

::: browser/components/extensions/ext-newtabSection.js:32
(Diff revision 5)
> +      maxRows: 1,
> +      contextMenuOptions: [
> +        "OpenInNewWindow",
> +        "OpenInPrivateWindow",
> +        "Separator",
> +        "WebExtDismiss"

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/ext-newtabSection.js:74
(Diff revision 5)
> +      update(newOptions) {
> +        Object.assign(options, newOptions);
> +        return new Promise(resolve => {
> +          // If we dynamically update a section option and the section was installed,
> +          // propagate the change to Activity Stream
> +          if (SectionsManager.sections.has(id)) SectionsManager.updateSection(id, options, true);

Error: Expected { after 'if' condition. [eslint: curly]

::: browser/components/extensions/ext-newtabSection.js:79
(Diff revision 5)
> +          if (SectionsManager.sections.has(id)) SectionsManager.updateSection(id, options, true);
> +          resolve();
> +        });
> +
> +
> +      },

Error: Block must not be padded by blank lines. [eslint: padded-blocks]

::: browser/components/extensions/ext-newtabSection.js:93
(Diff revision 5)
> +          resolve();
> +        });
> +      },
> +
> +      // Fired when a user enables a section and when the section is first initialized
> +      onEnabled: new EventManager(context, "newtabSection.onEnabled", fire => {

Error: 'eventmanager' is not defined. [eslint: no-undef]

::: browser/components/extensions/ext-newtabSection.js:120
(Diff revision 5)
> +          SectionsManager.off(SectionsManager.ENABLE_SECTION, enabledListener);
> +        };
> +      }).api(),
> +
> +      // Fired when the section is disabled or when the app shuts down
> +      onDisabled: new EventManager(context, "newtabSection.onDisabled", fire => {

Error: 'eventmanager' is not defined. [eslint: no-undef]

::: browser/components/extensions/ext-newtabSection.js:134
(Diff revision 5)
> +
> +        return () => {
> +          SectionsManager.off(SectionsManager.UNINIT, uninitListener);
> +          SectionsManager.off(SectionsManager.DISABLE_SECTION, disabledListener);
> +        };
> +      }).api()

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/ext-newtabSection.js:140
(Diff revision 5)
> +    };
> +
> +    // Constructs an EventManager for a single action
> +    const ActionEventManagerFactory = action => {
> +      const key = `newtabSection.on${ACTIVITY_STREAM_ACTIONS[action]}`;
> +      return new EventManager(context, key, fire => {

Error: 'eventmanager' is not defined. [eslint: no-undef]

::: browser/components/extensions/ext-newtabSection.js:142
(Diff revision 5)
> +    // Constructs an EventManager for a single action
> +    const ActionEventManagerFactory = action => {
> +      const key = `newtabSection.on${ACTIVITY_STREAM_ACTIONS[action]}`;
> +      return new EventManager(context, key, fire => {
> +        const listener = (event, type, data) => {
> +          if (type === action && data.source === this.extension.id) fire.async(data);

Error: Expected { after 'if' condition. [eslint: curly]

::: browser/components/extensions/ext-newtabSection.js:143
(Diff revision 5)
> +    const ActionEventManagerFactory = action => {
> +      const key = `newtabSection.on${ACTIVITY_STREAM_ACTIONS[action]}`;
> +      return new EventManager(context, key, fire => {
> +        const listener = (event, type, data) => {
> +          if (type === action && data.source === this.extension.id) fire.async(data);
> +        }

Error: Missing semicolon. [eslint: semi]

::: browser/components/extensions/ext-newtabSection.js:160
(Diff revision 5)
> +    }
> +
> +
> +    return {newtabSection};
> +  }
> +}

Error: Missing semicolon. [eslint: semi]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:7
(Diff revision 5)
> +
> +const TEST_SECTION_OPTIONS = {
> +  "title": "Red Pandas",
> +  "maxRows": 3,
> +  "emptyState": {
> +    "message": "Empty 123"

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:8
(Diff revision 5)
> +const TEST_SECTION_OPTIONS = {
> +  "title": "Red Pandas",
> +  "maxRows": 3,
> +  "emptyState": {
> +    "message": "Empty 123"
> +  }

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:13
(Diff revision 5)
> +  }
> +};
> +const TEST_MANIFEST = {
> +  "permissions": ["newtabSection"],
> +  "background": {"scripts": ["background.js"]},
> +  "new_tab_section_options": TEST_SECTION_OPTIONS

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:32
(Diff revision 5)
> +  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "about:newtab", false);
> +  let browser = tab.linkedBrowser;
> +
> +  // Wait for React to render something
> +  await BrowserTestUtils.waitForCondition(() => ContentTask.spawn(browser, {},
> +    () => content.document.getElementById("root").children.length),

Error: Expected indentation of 66 spaces but found 4. [eslint: indent]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:33
(Diff revision 5)
> +  let browser = tab.linkedBrowser;
> +
> +  // Wait for React to render something
> +  await BrowserTestUtils.waitForCondition(() => ContentTask.spawn(browser, {},
> +    () => content.document.getElementById("root").children.length),
> +    "Should render activity stream content");

Error: Expected indentation of 42 spaces but found 4. [eslint: indent]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:54
(Diff revision 5)
> +    const {testdata} = args;
> +
> +    // Find all sections
> +    const [sectionEl] = [...content.document.querySelectorAll(".sections-list .section")]
> +      .filter(el => (el.querySelector(".section-title").textContent === testdata.title));
> +    ok(sectionEl,"should render a section for the extension with a title as specified in the manifest");

Error: A space is required after ','. [eslint: comma-spacing]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:66
(Diff revision 5)
> +add_task(async function test_newtabSection_update() {
> +  const extension = loadExtension({
> +    async js() {
> +      await browser.newtabSection.update({title: "Foo Section"});
> +      browser.test.sendMessage("updated");
> +    }

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:72
(Diff revision 5)
> +  });
> +
> +  await Promise.all([extension.startup(), extension.awaitMessage("updated")]);
> +
> +  await runInNewTab({testdata: TEST_SECTION_OPTIONS}, args => {
> +    const {testdata} = args;

Error: 'testdata' is assigned a value but never used. allowed unused vars must match /^exported_symbols$/. [eslint: no-unused-vars]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:90
(Diff revision 5)
> +  const extension = loadExtension({
> +    async js() {
> +      await browser.newtabSection.addCards([
> +        {url: "foo.com", title: "Foo", description: "This is a card"},
> +        {url: "bar.com", title: "Bar", description: "This is a card"},
> +        {url: "baz.com", title: "Baz", description: "This is a card"}

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:93
(Diff revision 5)
> +        {url: "foo.com", title: "Foo", description: "This is a card"},
> +        {url: "bar.com", title: "Bar", description: "This is a card"},
> +        {url: "baz.com", title: "Baz", description: "This is a card"}
> +      ]);
> +      browser.test.sendMessage("updated");
> +    }

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:127
(Diff revision 5)
> +// The section should send events and remove elements when Dismiss context option is clicked
> +add_task(async function test_newtabSection_dismiss() {
> +  const extension = loadExtension({
> +    async js() {
> +      await browser.newtabSection.addCards([
> +        {url: "about:blank", title: "Foo", description: "This is a card"}

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:131
(Diff revision 5)
> +      await browser.newtabSection.addCards([
> +        {url: "about:blank", title: "Foo", description: "This is a card"}
> +      ]);
> +      browser.test.sendMessage("updated");
> +      browser.newtabSection.onDismiss.addListener(data => browser.test.sendMessage("dismissed"));
> +    }

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:147
(Diff revision 5)
> +      .filter(el => (el.querySelector(".section-title").textContent === testdata.title));
> +    ok(sectionEl, "should render a section for the extension");
> +
> +    // Find the link and its dismiss button
> +    let link;
> +    await ContentTaskUtils.waitForCondition(() => link = sectionEl.querySelector(".card-outer:not(.placeholder)"))

Error: Arrow function should not return assignment. [eslint: no-return-assign]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:149
(Diff revision 5)
> +
> +    // Find the link and its dismiss button
> +    let link;
> +    await ContentTaskUtils.waitForCondition(() => link = sectionEl.querySelector(".card-outer:not(.placeholder)"))
> +      .catch(e => ok(link, "should be a link"));
> +    if (!link) return;

Error: Expected { after 'if' condition. [eslint: curly]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:172
(Diff revision 5)
> +// The emptyState message should appear when no items are left in the section
> +add_task(async function test_newtabSection_empty_state() {
> +  const extension = loadExtension({
> +    async js() {
> +      await browser.newtabSection.addCards([
> +        {url: "about:blank", title: "Foo", description: "This is a card"}

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:175
(Diff revision 5)
> +    async js() {
> +      await browser.newtabSection.addCards([
> +        {url: "about:blank", title: "Foo", description: "This is a card"}
> +      ]);
> +      browser.test.sendMessage("updated");
> +    }

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:186
(Diff revision 5)
> +
> +    // Find the matching section
> +    const [sectionEl] = [...content.document.querySelectorAll(".sections-list .section")]
> +      .filter(el => (el.querySelector(".section-title").textContent === testdata.title));
> +    ok(sectionEl, "should render a section for the extension");
> +    if (!sectionEl) return;

Error: Expected { after 'if' condition. [eslint: curly]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:190
(Diff revision 5)
> +    ok(sectionEl, "should render a section for the extension");
> +    if (!sectionEl) return;
> +
> +    // Find the link and its dismiss button
> +    let link;
> +    await ContentTaskUtils.waitForCondition(() => link = sectionEl.querySelector(".card-outer:not(.placeholder)"))

Error: Arrow function should not return assignment. [eslint: no-return-assign]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:192
(Diff revision 5)
> +
> +    // Find the link and its dismiss button
> +    let link;
> +    await ContentTaskUtils.waitForCondition(() => link = sectionEl.querySelector(".card-outer:not(.placeholder)"))
> +      .catch(e => ok(link, "should be a link"));
> +    if (!link) return;

Error: Expected { after 'if' condition. [eslint: curly]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:198
(Diff revision 5)
> +    const dismissBtn = link.querySelector(".context-menu-item:last-child a");
> +
> +    dismissBtn.click();
> +
> +    let emptyStateEl;
> +    await ContentTaskUtils.waitForCondition(() => emptyStateEl = sectionEl.querySelector(".empty-state-message"))

Error: Arrow function should not return assignment. [eslint: no-return-assign]

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:201
(Diff revision 5)
> +
> +    let emptyStateEl;
> +    await ContentTaskUtils.waitForCondition(() => emptyStateEl = sectionEl.querySelector(".empty-state-message"))
> +      .catch(e => ok(link, "should have an empty state message"));
> +
> +    is(emptyStateEl.textContent, "Empty 123", "should have the empty state text defined in the manifest")

Error: Missing semicolon. [eslint: semi]
NI? on me to get up to speed on this.
Flags: needinfo?(mstriemer)
Severity: normal → enhancement
Comment on attachment 8942975 [details]
Bug 1401194 - Add newtab section webextension api

https://reviewboard.mozilla.org/r/213240/#review235238

::: browser/components/extensions/ext-newtabSection.js:12
(Diff revision 6)
> +const ACTIVITY_STREAM_ACTIONS = {
> +  WEBEXT_CLICK: "Click",
> +  WEBEXT_DISMISS: "Dismiss",
> +};
> +
> +this.newtabSection = class extends ExtensionAPI { // eslint-disable-line no-unused-vars

I think this should be `newTabSection` to match `new_tab_section`.

::: browser/components/extensions/ext-newtabSection.js:16
(Diff revision 6)
> +
> +this.newtabSection = class extends ExtensionAPI { // eslint-disable-line no-unused-vars
> +  constructor(extension) {
> +    super(extension);
> +
> +    const manifestOptions = Object.assign({}, this.extension.manifest.new_tab_section_options);

Other APIs, like `browserAction`/`browser_action`, exclude the options bit, so this should be `new_tab_section`, in my opinion.

::: browser/components/extensions/ext-newtabSection.js:21
(Diff revision 6)
> +    const manifestOptions = Object.assign({}, this.extension.manifest.new_tab_section_options);
> +
> +    for (const prop in manifestOptions) {
> +      // Necessary to avoid overwriting default values with null if no option is
> +      // provided in the manifest
> +      if (manifestOptions[prop] === null) { delete manifestOptions[prop]; }

You might be able to avoid this by restricting these properties in the schema file. For example requiring strings to be at least 1 character long.

::: browser/components/extensions/ext-newtabSection.js:25
(Diff revision 6)
> +      // provided in the manifest
> +      if (manifestOptions[prop] === null) { delete manifestOptions[prop]; }
> +    }
> +
> +    this.sectionOptions = Object.assign({
> +      title: this.extension.id,

The add-on's name seems like a better default than the id.

::: browser/components/extensions/schemas/newtabSection.json:13
(Diff revision 6)
> +          "new_tab_section_options": {
> +            "type": "object",
> +            "additionalProperties": {"$ref": "UnrecognizedProperty"},
> +            "optional": true,
> +            "properties": {
> +              "title": {"type": "string", "optional": true, "pattern": ".*\\S.*"},

I think you might want to add `"localize": true` to strings that get displayed so they can be localised by the extension author.

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:15
(Diff revision 6)
> +  "background": {"scripts": ["background.js"]},
> +  "new_tab_section_options": TEST_SECTION_OPTIONS,
> +};
> +
> +function loadExtension(options = {}) {
> +  return ExtensionTestUtils.loadExtension({

You can use `registerCleanupFunction()` to unload the extension here, then you don't need to do it in each test.

If there is an error during a test, then the extension will still be removed as well. If you let the `waitForCondition()` calls throw then you'll want to cleanup the extension here.

```
let extension = ExtensionTestUtils.loadExtension({
  manifest: options.manifest || TEST_MANIFEST,
  useAddonManager: "temporary",
  files: {"background.js": options.js || function() {
    browser.newtabSection.addCards([]);
  }},
});
registerCleanupFunction(() => extension.unload());
return extension;
```

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:50
(Diff revision 6)
> +
> +  await runInNewTab({testdata: TEST_SECTION_OPTIONS}, args => {
> +    const {testdata} = args;
> +
> +    // Find all sections
> +    const [sectionEl] = [...content.document.querySelectorAll(".sections-list .section")]

I'm not sure if you want to mess with activity stream but normally we'd use this `makeWidgetId` [1] function (which admittedly has its drawbacks) to set an id that can be queried directly.

Perhaps you could add a `data-extension-id={extension.id}` part to activity stream to make this simpler?

[1] https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/browser/components/extensions/ext-browser.js#101

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:105
(Diff revision 6)
> +    ok(sectionEl, "should render a section for the extension");
> +
> +    // Find the corresponding links
> +    let links;
> +    await ContentTaskUtils.waitForCondition(() => {
> +      links = [...sectionEl.querySelectorAll(".card-outer > a")];

nit: you don't need to cast to an array here.

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:107
(Diff revision 6)
> +    // Find the corresponding links
> +    let links;
> +    await ContentTaskUtils.waitForCondition(() => {
> +      links = [...sectionEl.querySelectorAll(".card-outer > a")];
> +      return links.length === 3;
> +    }).catch(e => is(links.length, 3, "should be 3 links"));

You shouldn't need to catch this, then you can remove the if on the next line.

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:143
(Diff revision 6)
> +      .filter(el => (el.querySelector(".section-title").textContent === testdata.title));
> +    ok(sectionEl, "should render a section for the extension");
> +
> +    // Find the link and its dismiss button
> +    let link;
> +    await ContentTaskUtils.waitForCondition(() => { link = sectionEl.querySelector(".card-outer:not(.placeholder)"); })

This is going to timeout and hit the catch every time since the link isn't returned. It would be better to avoid the catch and let it throw if the element doesn't appear.


```
let link;
await ContentTaskUtils.waitForCondition(() => {
  link = sectionEl.querySelector(".card-outer:not(.placeholder)");
  return link;
});
// If link isn't set then this won't execute.
link.querySelectory(".context-menu-item:last-child a").click();
```

::: browser/components/extensions/test/browser/browser_ext_newtabSection.js:190
(Diff revision 6)
> +      return;
> +    }
> +
> +    // Find the link and its dismiss button
> +    let link;
> +    await ContentTaskUtils.waitForCondition(() => { link = sectionEl.querySelector(".card-outer:not(.placeholder)"); })

These `waitForCondition()` calls should return something, as above.

::: browser/components/extensions/test/xpcshell/xpcshell.ini:23
(Diff revision 6)
>  
>  [test_ext_geckoProfiler_schema.js]
>  [test_ext_manifest_commands.js]
>  [test_ext_manifest_omnibox.js]
>  [test_ext_manifest_permissions.js]
> +[test_ext_newtab_section.js]

Looks like this file can be removed.
Comment on attachment 8942975 [details]
Bug 1401194 - Add newtab section webextension api

https://reviewboard.mozilla.org/r/213240/#review235342

Looks good. I made an extension to play around with the API and it was quite easy to get some content on the page.

A few comments/nits, you probably don't need to make all of these changes but I tried to share what seems common in our code/tests.

One thing I noticed while testing: if you add some cards, then hide and unhide the section, it ends up with 3 empty cards and there are some react errors in the browser console.
Flags: needinfo?(mstriemer)
Blocks: 1444980
No longer blocks: old-bugzy-epic
Iteration: --- → 61.3 - Apr 23
Iteration: 61.3 - Apr 23 → 61.4 - May 7
Iteration: 61.4 - May 7 → 62.1 - May 21
Blocks: 1457567
Commits pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/6fe9b9f9b33b02222e574266463b63cfb6510411
Bug 1401194 - Add data-section-id to sections for web extension tests
This is to support tests that will be added in Bug 1401194 in order for
web extension tests to identify the correct section more easily.

https://github.com/mozilla/activity-stream/commit/517b56c675858ffe004ed25c79974cc6cb326083
Merge pull request #4144 from k88hudson/add-section-data-uri

Bug 1401194 - Add data-section-id to sections for web extension tests
User Story: (updated)
Ok, this last batch of updates includes the following (some of which landed in Activity Stream):
* tests target data-section-id 
* section menu has a "Mange Extension" option which links to the extension settings detail page
* onEnabled onDisabled were removed and replaced with an async flow in which .addCards and other methods await on sectionmanager being ready, which simplifies the user code a lot

I also uploaded an example extension at https://github.com/k88hudson/newTabSection-test if it helps with manual testing. Thanks!
Priority: P3 → P1
Flags: needinfo?(mstriemer)
Comment on attachment 8942975 [details]
Bug 1401194 - Add newtab section webextension api

https://reviewboard.mozilla.org/r/213240/#review251876

Just some small comments. Looks like we're pretty much there!

I'll try out the test extension before our meeting this afternoon.

::: browser/components/extensions/parent/ext-newTabSection.js:34
(Diff revision 12)
> +        "OpenInNewWindow",
> +        "OpenInPrivateWindow",
> +        "Separator",
> +        "WebExtDismiss",
> +      ],
> +      emptyState: {message: "Loading"},

This looks like it should be localised.

::: browser/components/extensions/parent/ext-newTabSection.js:115
(Diff revision 12)
> +      }).api();
> +    };
> +
> +    // Single action events
> +    for (const action of Object.keys(ACTIVITY_STREAM_ACTIONS)) {
> +      const key = `on${ACTIVITY_STREAM_ACTIONS[action]}`;

It looks like using "onClick" as the value in `ACTIVITY_STREAM_ACTIONS` would avoid having to add it here and on line 99. I don't see "Click" being used on its own.

::: browser/components/extensions/schemas/newTabSection.json:77
(Diff revision 12)
> +      }
> +    ],
> +    "functions": [
> +      {
> +        "name": "update",
> +        "description": "Updates any of the available manifest options and returns a promise when updates have been applied. ",

nit: There's a trailing space here.

::: browser/components/extensions/test/browser/browser_ext_newTabSection.js:74
(Diff revision 12)
> +  await Promise.all([extension.startup(), extension.awaitMessage("updated")]);
> +
> +  await runInNewTab({id: extension.id}, args => {
> +    const {id} = args;
> +    const sectionEl = content.document.querySelector(".section[data-section-id=\"" + id + "\"]");
> +    ok(sectionEl, "should render the new title after it has been updated via browser.newTabSection.update");

It looks like this isn't checking the title.

::: browser/components/extensions/test/browser/browser_ext_newTabSection.js:205
(Diff revision 12)
> +
> +    // Open context menu
> +    link.click();
> +  });
> +
> +  ok(await clickPromise, "#foo");

Should this be an `is` instead?

::: browser/locales/en-US/chrome/browser/browser.properties:114
(Diff revision 12)
>  webextPerms.description.history=Access browsing history
>  webextPerms.description.management=Monitor extension usage and manage themes
>  # LOCALIZATION NOTE (webextPerms.description.nativeMessaging)
>  # %S will be replaced with the name of the application
>  webextPerms.description.nativeMessaging=Exchange messages with programs other than %S
> +webextPerms.description.newTabSection=Add a custom section to the newtab

Have you run this copy past anyone? I think it should be more like "Add a custom section to your New Tab page" but that's just a guess.
Attachment #8942975 - Flags: review?(mstriemer)
Iteration: 62.1 - May 21 → 62.2 - Jun 4
Flags: needinfo?(mstriemer)
Iteration: 62.2 - Jun 4 → 62.3 - Jun 18
Product: Toolkit → WebExtensions
Iteration: 62.3 - Jun 18 → 63.1 - July 9
Hey, would you be able to take a look at this as a second reviewer? Happy to discuss the context on IRC/vidyo!
Flags: needinfo?(mixedpuppy)
Comment on attachment 8942975 [details]
Bug 1401194 - Add newtab section webextension api

https://reviewboard.mozilla.org/r/213240/#review259322

I didn't read into SectionsManager code at all, so a bunch of my comments may be due to that.

::: browser/components/extensions/parent/ext-newTabSection.js:16
(Diff revision 13)
> +    const manifestOptions = Object.assign({}, this.extension.manifest.new_tab_section);
> +
> +    for (const prop in manifestOptions) {
> +      // Necessary to avoid overwriting default values with null if no option is
> +      // provided in the manifest
> +      if (manifestOptions[prop] === null) { delete manifestOptions[prop]; }
> +    }
> +
> +    this.sectionOptions = Object.assign({

I'm a bit ambivalent with my suggestion here.

Since it's only a couple options, I'd drop the assigns and just do (below).  If you're expecting to grow into a bunch of options, then I'd keep it as is.

let opts = this.extension.manifest.new_tab_section
this.sectionOptions = {
title: opts.title || this.extension.manifest.name,
icon: opts.icon && extension.baseURI.resolve(opts.icon),
maxRows: opts.maxRows,
emptyState: opts.emptyState,
...other stuff...
}

Also see the notes on defaults in the schema later in review, they help avoid checking/settting defaults in code.

Also be aware, IconPath can be an object with multiple IconPaths (size as key), so really the above resolve is not what you want, but I think my resolve example handles the wrapExtensionUrl stuff below.  I'm not sure if you need to resolve icons to a specific size, but you could look at ext-browser/pageAction to see what we did with icons there.

If you really only want one icon, just use ExtensionUrl instead.

::: browser/components/extensions/parent/ext-newTabSection.js:42
(Diff revision 13)
> +      // Don't show in about:preferences
> +      shouldHidePref: true,
> +      enabled: true,
> +    }, manifestOptions);
> +
> +    this.wrapExtensionUrl(this.sectionOptions, "icon");

I think you can just do: "icon = extension.baseURI.resolve(icon);" and get rid of wrapExtensionUrl

Is icon actually optional as the schema claims?

::: browser/components/extensions/parent/ext-newTabSection.js:46
(Diff revision 13)
> +
> +    this.wrapExtensionUrl(this.sectionOptions, "icon");
> +
> +    this.sectionReady = new Promise(resolve => {
> +      // Add the section
> +      SectionsManager.addSection(this.extension.id, this.sectionOptions);

All of the manifest keys are optional, is it really ok to add a section if none of the keys are present?

::: browser/components/extensions/parent/ext-newTabSection.js:98
(Diff revision 13)
> +        SectionsManager.updateSection(id, {rows: cards}, shouldBroadcast);
> +      },
> +    };
> +
> +    // Constructs an EventManager for a single action
> +    const ActionEventManagerFactory = action => {

move this out to a global similar to makeWebRequestEvent in ext-webRequest.js

::: browser/components/extensions/parent/ext-newTabSection.js:107
(Diff revision 13)
> +          if (type === action && data.source === this.extension.id) {
> +            fire.async(data);
> +          }
> +        };
> +
> +        SectionsManager.on(SectionsManager.ACTION_DISPATCHED, listener);

It seems like SectionsManager will emit different kinds of events, if so, it's possible we could have one global SectionsManager listener for all extensions and actions, rather than two per extension.  I'm not sure it matters, it seems unlikely to have lots of extensions installed that would be doing this.

::: browser/components/extensions/parent/ext-newTabSection.js:114
(Diff revision 13)
> +          SectionsManager.off(SectionsManager.ACTION_DISPATCHED, listener);
> +      }).api();
> +    };
> +
> +    // Single action events
> +    for (const action of Object.keys(ACTIVITY_STREAM_ACTIONS)) {

There are only two, I would drop this and use the pattern I suggested from ext-webRequest.

::: browser/components/extensions/schemas/newTabSection.json:8
(Diff revision 13)
> +          "new_tab_section": {
> +            "type": "object",

Seems like this could be a ref to UpdatedOptions below (in which case I'd call that SectionOptions)

::: browser/components/extensions/schemas/newTabSection.json:15
(Diff revision 13)
> +            "additionalProperties": {"$ref": "UnrecognizedProperty"},
> +            "optional": true,
> +            "properties": {
> +              "title": {"type": "string", "optional": true, "pattern": ".*\\S.*", "localize": true},
> +              "icon": {"$ref": "IconPath", "optional": true},
> +              "maxRows": {"type": "integer", "optional": true, "minimum": 1},

add "default": 1

::: browser/components/extensions/schemas/newTabSection.json:16
(Diff revision 13)
> +            "optional": true,
> +            "properties": {
> +              "title": {"type": "string", "optional": true, "pattern": ".*\\S.*", "localize": true},
> +              "icon": {"$ref": "IconPath", "optional": true},
> +              "maxRows": {"type": "integer", "optional": true, "minimum": 1},
> +              "emptyState": {

emptyState feels like implementation detail

::: browser/components/extensions/schemas/newTabSection.json:20
(Diff revision 13)
> +              "maxRows": {"type": "integer", "optional": true, "minimum": 1},
> +              "emptyState": {
> +                "type": "object",
> +                "optional": true,
> +                "properties": {
> +                  "message": {"type": "string", "localize": true}

add "default": "Loading", and probably make it localizable

::: browser/components/extensions/schemas/newTabSection.json:32
(Diff revision 13)
> +    ]
> +  },
> +  {
> +    "namespace": "newTabSection",
> +    "permissions": ["manifest:new_tab_section"],
> +    "types": [

The types & properties should have documentation (e.g. my question on hostname)

::: browser/components/extensions/schemas/newTabSection.json:37
(Diff revision 13)
> +    "types": [
> +      {
> +        "id": "Card",
> +        "type": "object",
> +        "properties": {
> +          "url": {"type": "string", "optional": true},

add "format": "url"

::: browser/components/extensions/schemas/newTabSection.json:40
(Diff revision 13)
> +        "type": "object",
> +        "properties": {
> +          "url": {"type": "string", "optional": true},
> +          "title": {"type": "string"},
> +          "description": {"type": "string", "optional": true},
> +          "hostname": {"type": "string", "optional": true},

Is this related to url above?  If so, why the duplication?

::: browser/components/extensions/schemas/newTabSection.json:41
(Diff revision 13)
> +        "properties": {
> +          "url": {"type": "string", "optional": true},
> +          "title": {"type": "string"},
> +          "description": {"type": "string", "optional": true},
> +          "hostname": {"type": "string", "optional": true},
> +          "type": {"type": "string", "optional": true},

should this be an enum?  Is there a default?

::: browser/components/extensions/schemas/newTabSection.json:42
(Diff revision 13)
> +          "url": {"type": "string", "optional": true},
> +          "title": {"type": "string"},
> +          "description": {"type": "string", "optional": true},
> +          "hostname": {"type": "string", "optional": true},
> +          "type": {"type": "string", "optional": true},
> +          "image": {"type": "string", "optional": true}

Is this image data?  A Url to an image?  I'm not sure you want type string here, try either format: url with it, or look at ImageData types.

::: browser/components/extensions/schemas/newTabSection.json:46
(Diff revision 13)
> +          "type": {"type": "string", "optional": true},
> +          "image": {"type": "string", "optional": true}
> +        }
> +      },
> +      {
> +        "id": "UpdateOptions",

all the propertie here are optional, shouldn't at least one be required?  But setting defaults (and please do where apropriate) as mentioned earlier would probably handle that anyway.

::: browser/components/extensions/schemas/newTabSection.json:79
(Diff revision 13)
> +    "functions": [
> +      {
> +        "name": "update",
> +        "description": "Updates any of the available manifest options and returns a promise when updates have been applied. ",
> +        "type": "function",
> +        "async": "callback",

no callbacks on new APIs, use "async": true and remove the callback functions (ditto later)

::: browser/components/extensions/schemas/newTabSection.json:94
(Diff revision 13)
> +          }
> +        ]
> +      },
> +      {
> +        "name": "addCards",
> +        "description": "Adds an array of cards to the section and returns a promise when all cards have been added.",

many of our apis have a one-or-many structure, so you could call addCard(card) or addCard(arrayOfCards).  not sure if that makes sense for this case.

::: browser/components/extensions/schemas/newTabSection.json:104
(Diff revision 13)
> +            "name": "shouldBroadcast",
> +            "description": "Should the updates be pushed immediately to all visible tabs? Defaults to false (cards will be updated in the background for the next tab only).",
> +            "type": "boolean",
> +            "optional": "true"

Add default: false
(In reply to Kate Hudson :k88hudson from comment #30)
> Hey, would you be able to take a look at this as a second reviewer? Happy to
> discuss the context on IRC/vidyo!

You might have got more than you bargained for.  Feel free to r? me.
Flags: needinfo?(mixedpuppy)
Iteration: 63.1 - July 9 → 63.2 - July 23
Iteration: 63.2 - July 23 → 63.3 - Aug 6
Iteration: 63.3 - Aug 6 → ---
Priority: P1 → P3
You need to log in before you can comment on or make changes to this bug.