1257550 - Awesomebar result providers should be able to add document fragments or iframes

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Some upcoming prototypes need to display rich markup in the awesomebar results. Instead of forcing people to become intimately familiar with the awesomebar internals (XUL, XBL, oh my!) we could provide them with the ability to add a document fragment or even an iframe as a result.

This depends on bug 1257544, as otherwise every result item would be resized.

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1257550 - Awesomebar result providers should be able to add document fragments or iframes.

Review commit: https://reviewboard.mozilla.org/r/97766/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97766/

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8817521 [details]
Bug 1257550 - Awesomebar result providers should be able to add document fragments or iframes.

https://reviewboard.mozilla.org/r/97766/#review100666

Honestly, it's really hard to review this. It surely contains hours of brainstorming, and surely the consumers will have their own feedback.
My review doesn't have a huge value vs those two, we should rather collect feedback ASAP. As such, if this doesn't cause perf regressions or test failures, I see no reasons to land it sooner (with the following comments addressed/answered)

::: browser/base/content/test/urlbar/Panel.jsm:72
(Diff revision 1)
> +  },
> +
> +  _initIframeContent(win) {
> +    // Clone the urlbar API functions into the iframe window.
> +    win = XPCNativeWrapper.unwrap(win);
> +    let apiInstance = Cu.cloneInto(iframeAPIPrototype, win, {

can we somehow move this burden into our code?
We are trying to remove xpcom complexity, and this looks exactly the kind of thing we should remove.

::: browser/base/content/test/urlbar/Panel.jsm:184
(Diff revision 1)
> +    // entirely the reason for having a queue instead of simply dispatching
> +    // events as they're created, unfortunately.
> +    if (!this.iframeWindow) {
> +      if (!this._processEmitQueueTimer) {
> +        this._processEmitQueueTimer =
> +          Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

can this use Timer.jsm?

::: browser/base/content/test/urlbar/Panel.jsm:198
(Diff revision 1)
> +      this._processEmitQueueTimer.cancel();
> +      delete this._processEmitQueueTimer;
> +    }
> +
> +    let { name, detail } = this._emitQueue.shift();
> +    let win = XPCNativeWrapper.unwrap(this.iframeWindow);

can we have an helper in urlbar that returns an unwrapped iframe as part of the API? Or maybe even make one that returns the built event.
I'd like to avoid as much visible XPCOM as possible.

::: browser/base/content/test/urlbar/Panel.jsm:208
(Diff revision 1)
> +    this.iframeWindow.dispatchEvent(event);
> +
> +    // More events may be queued up, so recurse.  Do it after a turn of the
> +    // event loop to avoid growing the stack as big as the queue, and to let the
> +    // caller handle the returned event first.
> +    let recurseTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

Timer.jsm

::: browser/base/content/test/urlbar/browser_urlbarAddonIframe.js:127
(Diff revision 1)
> +    // promiseEvent sends an async message to the iframe, but synthesizeKey is
> +    // sync, so give the message time to reach the iframe first.  A timeout of
> +    // zero (i.e., a single turn of the event loop (or small number of turns))
> +    // sometimes isn't sufficient, so wait a sec to be safe.
> +    setTimeout(resolve, 1000);
> +  });

can we move to a polling strategy or somehow use waitForCondition? The same below.

::: browser/base/content/urlbarBindings.xml:1824
(Diff revision 1)
>          ]]></body>
>        </method>
>  
> +      <field name="_addonIframe">null</field>
> +      <field name="_addonIframeOwner">null</field>
> +      <field name="_addonIframeOverriddenFunctionsByName">{}</field>

reasons to not use a Map? does it make the add-on code much worse?

::: browser/base/content/urlbarBindings.xml:1826
(Diff revision 1)
>  
> +      <field name="_addonIframe">null</field>
> +      <field name="_addonIframeOwner">null</field>
> +      <field name="_addonIframeOverriddenFunctionsByName">{}</field>
> +      <field name="_addonIframeOverrideFunctionNames">[
> +        "_invalidate",

off-hand i was thinking Map.keys() makes a lot of sense... but there's this default "_invalidate" override... Is this something we could avoid and rather enforce the consumer to at least provide a valid _invalidate override (throw otherwise).

It's just looks like we have a lot of bookkeeping here, and maybe some could be removed.

::: browser/base/content/urlbarBindings.xml:1828
(Diff revision 1)
> +      <field name="_addonIframeOwner">null</field>
> +      <field name="_addonIframeOverriddenFunctionsByName">{}</field>
> +      <field name="_addonIframeOverrideFunctionNames">[
> +        "_invalidate",
> +      ]</field>
> +      <field name="_addonIframeHiddenAnonids">[

could you please add a comment explaining the usage and what should be put here in case of future changes?

::: browser/base/content/urlbarBindings.xml:1844
(Diff revision 1)
> +            // Another add-on has already requested the iframe.  Return null to
> +            // signal to the calling add-on that it should not take over the
> +            // popup.  First add-on wins for now.
> +            return null;
> +          }
> +          this._addonIframeOwner = owner;

should we make add some input checks on owner and overrides? just to be sure we are not wrongly invoked, and then maybe the partner spends time trying to figure out why he doesn't see an error, but it still doesn't work.

::: browser/base/content/urlbarBindings.xml:1861
(Diff revision 1)
> +
> +      <method name="destroyAddonIframe">
> +        <parameter name="owner"/>
> +        <body><![CDATA[
> +          if (this._addonIframeOwner != owner) {
> +            return;

nit: Imo, this should throw.
A consumer that fails init, should not try to destroy

::: browser/base/content/urlbarBindings.xml:1870
(Diff revision 1)
> +          this._addonIframe = null;
> +          for (let anonid of this._addonIframeHiddenAnonids) {
> +            let child = document.getAnonymousElementByAttribute(
> +              this, "anonid", anonid
> +            );
> +            child.style.display = "block";

do all of the pointed out use block? should we store the previous display along with the anonid and restore that, just in case?

::: toolkit/content/widgets/autocomplete.xml:1366
(Diff revision 1)
>              }
>              else {
>                // set the class at the end so we can use the attributes
>                // in the xbl constructor
>                item.className = "autocomplete-richlistitem";
> +              //item.className = "autocomplete-richlistitem-html";

ditto

::: toolkit/content/widgets/autocomplete.xml:1461
(Diff revision 1)
>        </handler>
>      </handlers>
>    </binding>
>  
> +
> +<!--

ditto

::: toolkit/content/xul.css:888
(Diff revision 1)
>    -moz-binding: url("chrome://global/content/bindings/autocomplete.xml#autocomplete-richlistitem");
>    -moz-box-orient: vertical;
>    overflow: -moz-hidden-unscrollable;
>  }
>  
> +/*

commented out code?

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Ehm, the last 2 "ditto" are referred to "commented out code?". Looks like I reverse-commented.

Comment 4

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [::mak] from comment #2)
> Honestly, it's really hard to review this. It surely contains hours of
> brainstorming, and surely the consumers will have their own feedback.
> My review doesn't have a huge value vs those two, we should rather collect
> feedback ASAP.

That's one reason I want the part we land in m-c to be small, at least initially, i.e., simply creating and tearing down the iframe.

> As such, if this doesn't cause perf regressions or test failures, I see
> no reasons to land it sooner (with the following comments
> addressed/answered)

Did you mean to say "no reasons *not* to land it sooner"?  (i.e., we should land this soon)

> ::: browser/base/content/test/urlbar/Panel.jsm:72
> (Diff revision 1)
> > +  },
> > +
> > +  _initIframeContent(win) {
> > +    // Clone the urlbar API functions into the iframe window.
> > +    win = XPCNativeWrapper.unwrap(win);
> > +    let apiInstance = Cu.cloneInto(iframeAPIPrototype, win, {
> 
> can we somehow move this burden into our code?
> We are trying to remove xpcom complexity, and this looks exactly the kind of
> thing we should remove.

This part is our code -- it's the API runtime implementation, which lives in my repo (right now) and which consumers will pull into their add-ons.  Sorry for not explaining well.

> ::: browser/base/content/test/urlbar/Panel.jsm:198
> (Diff revision 1)
> > +      this._processEmitQueueTimer.cancel();
> > +      delete this._processEmitQueueTimer;
> > +    }
> > +
> > +    let { name, detail } = this._emitQueue.shift();
> > +    let win = XPCNativeWrapper.unwrap(this.iframeWindow);
> 
> can we have an helper in urlbar that returns an unwrapped iframe as part of
> the API? Or maybe even make one that returns the built event.
> I'd like to avoid as much visible XPCOM as possible.

Same here

> ::: browser/base/content/urlbarBindings.xml:1826
> (Diff revision 1)
> >  
> > +      <field name="_addonIframe">null</field>
> > +      <field name="_addonIframeOwner">null</field>
> > +      <field name="_addonIframeOverriddenFunctionsByName">{}</field>
> > +      <field name="_addonIframeOverrideFunctionNames">[
> > +        "_invalidate",
> 
> off-hand i was thinking Map.keys() makes a lot of sense... but there's this
> default "_invalidate" override... Is this something we could avoid and
> rather enforce the consumer to at least provide a valid _invalidate override
> (throw otherwise).
> 
> It's just looks like we have a lot of bookkeeping here, and maybe some could
> be removed.

Yeah, this is one spot I wasn't so sure about.  I was thinking about a single version of the runtime being used with different versions of Firefox, or vice versa, and managing those different combinations in a robust and flexible way...  But I think you're right that this should throw if the runtime doesn't provide _invalidate, because if it doesn't, it will break the popup.

Comment 5

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #4)
> > ::: browser/base/content/urlbarBindings.xml:1826
> > (Diff revision 1)
> > >  
> > > +      <field name="_addonIframe">null</field>
> > > +      <field name="_addonIframeOwner">null</field>
> > > +      <field name="_addonIframeOverriddenFunctionsByName">{}</field>
> > > +      <field name="_addonIframeOverrideFunctionNames">[
> > > +        "_invalidate",
> > 
> > off-hand i was thinking Map.keys() makes a lot of sense... but there's this
> > default "_invalidate" override... Is this something we could avoid and
> > rather enforce the consumer to at least provide a valid _invalidate override
> > (throw otherwise).
> > 
> > It's just looks like we have a lot of bookkeeping here, and maybe some could
> > be removed.
> 
> Yeah, this is one spot I wasn't so sure about.  I was thinking about a
> single version of the runtime being used with different versions of Firefox,
> or vice versa, and managing those different combinations in a robust and
> flexible way...  But I think you're right that this should throw if the
> runtime doesn't provide _invalidate, because if it doesn't, it will break
> the popup.

Oh one reason for _addonIframeOverrideFunctionNames is so that it's clear which methods need to be overridden by the runtime.  Maybe that's not really worth a separate field though?  I'll remove it for now.  We can always add it back later if we decide that's a good idea.

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8817521 [details]
Bug 1257550 - Awesomebar result providers should be able to add document fragments or iframes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97766/diff/1-2/

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 8817521 [details]
Bug 1257550 - Awesomebar result providers should be able to add document fragments or iframes.

Re-requesting review to be safe than sorry since there's been some discussion.

Comment 8

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [::mak] from comment #2)
> ::: browser/base/content/test/urlbar/Panel.jsm:184
> (Diff revision 1)
> > +    // entirely the reason for having a queue instead of simply dispatching
> > +    // events as they're created, unfortunately.
> > +    if (!this.iframeWindow) {
> > +      if (!this._processEmitQueueTimer) {
> > +        this._processEmitQueueTimer =
> > +          Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> 
> can this use Timer.jsm?

Fixed

> ::: browser/base/content/test/urlbar/Panel.jsm:208
> (Diff revision 1)
> > +    this.iframeWindow.dispatchEvent(event);
> > +
> > +    // More events may be queued up, so recurse.  Do it after a turn of the
> > +    // event loop to avoid growing the stack as big as the queue, and to let the
> > +    // caller handle the returned event first.
> > +    let recurseTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> 
> Timer.jsm

Fixed

> ::: browser/base/content/test/urlbar/browser_urlbarAddonIframe.js:127
> (Diff revision 1)
> > +    // promiseEvent sends an async message to the iframe, but synthesizeKey is
> > +    // sync, so give the message time to reach the iframe first.  A timeout of
> > +    // zero (i.e., a single turn of the event loop (or small number of turns))
> > +    // sometimes isn't sufficient, so wait a sec to be safe.
> > +    setTimeout(resolve, 1000);
> > +  });
> 
> can we move to a polling strategy or somehow use waitForCondition? The same
> below.

Fixed by having the content JS send two acks in this case: one when it adds its event listener, and then one as before when it receives the event.

> ::: browser/base/content/urlbarBindings.xml:1824
> (Diff revision 1)
> >          ]]></body>
> >        </method>
> >  
> > +      <field name="_addonIframe">null</field>
> > +      <field name="_addonIframeOwner">null</field>
> > +      <field name="_addonIframeOverriddenFunctionsByName">{}</field>
> 
> reasons to not use a Map? does it make the add-on code much worse?

IMO there's no reason to use a Map over a plain object here.  We're just collecting the XBL implementations of these method(s) so that they can be restored later when the iframe consumer is removed.  If think a Map is better though, I don't mind.

> ::: browser/base/content/urlbarBindings.xml:1826
> (Diff revision 1)
> >  
> > +      <field name="_addonIframe">null</field>
> > +      <field name="_addonIframeOwner">null</field>
> > +      <field name="_addonIframeOverriddenFunctionsByName">{}</field>
> > +      <field name="_addonIframeOverrideFunctionNames">[
> > +        "_invalidate",
> 
> off-hand i was thinking Map.keys() makes a lot of sense... but there's this
> default "_invalidate" override... Is this something we could avoid and
> rather enforce the consumer to at least provide a valid _invalidate override
> (throw otherwise).
> 
> It's just looks like we have a lot of bookkeeping here, and maybe some could
> be removed.

Actually despite my earlier comments, I think it makes sense to keep _addonIframeOverrideFunctionNames.  In addition to being useful to call out the methods that need to be overridden, it's also easy for this code here to iterate over the names and throw when an override is not provided.

> ::: browser/base/content/urlbarBindings.xml:1828
> (Diff revision 1)
> > +      <field name="_addonIframeOwner">null</field>
> > +      <field name="_addonIframeOverriddenFunctionsByName">{}</field>
> > +      <field name="_addonIframeOverrideFunctionNames">[
> > +        "_invalidate",
> > +      ]</field>
> > +      <field name="_addonIframeHiddenAnonids">[
> 
> could you please add a comment explaining the usage and what should be put
> here in case of future changes?

Done

> 
> ::: browser/base/content/urlbarBindings.xml:1844
> (Diff revision 1)
> > +            // Another add-on has already requested the iframe.  Return null to
> > +            // signal to the calling add-on that it should not take over the
> > +            // popup.  First add-on wins for now.
> > +            return null;
> > +          }
> > +          this._addonIframeOwner = owner;
> 
> should we make add some input checks on owner and overrides? just to be sure
> we are not wrongly invoked, and then maybe the partner spends time trying to
> figure out why he doesn't see an error, but it still doesn't work.

Fixed by throwing when an override isn't provided, like you suggested.  I'm not sure what checks should be performed on owner.  Really it's just an opaque object as far as this code is concerned.  If we add requirements on owner in the future, then we can do checks then.

> ::: browser/base/content/urlbarBindings.xml:1861
> (Diff revision 1)
> > +
> > +      <method name="destroyAddonIframe">
> > +        <parameter name="owner"/>
> > +        <body><![CDATA[
> > +          if (this._addonIframeOwner != owner) {
> > +            return;
> 
> nit: Imo, this should throw.
> A consumer that fails init, should not try to destroy

Done

> ::: browser/base/content/urlbarBindings.xml:1870
> (Diff revision 1)
> > +          this._addonIframe = null;
> > +          for (let anonid of this._addonIframeHiddenAnonids) {
> > +            let child = document.getAnonymousElementByAttribute(
> > +              this, "anonid", anonid
> > +            );
> > +            child.style.display = "block";
> 
> do all of the pointed out use block? should we store the previous display
> along with the anonid and restore that, just in case?

Good point, fixed

> ::: toolkit/content/widgets/autocomplete.xml:1366
> (Diff revision 1)
> >              }
> >              else {
> >                // set the class at the end so we can use the attributes
> >                // in the xbl constructor
> >                item.className = "autocomplete-richlistitem";
> > +              //item.className = "autocomplete-richlistitem-html";
> 
> ditto
> ::: toolkit/content/widgets/autocomplete.xml:1461
> (Diff revision 1)
> >        </handler>
> >      </handlers>
> >    </binding>
> >  
> > +
> > +<!--
> 
> ditto
> 
> ::: toolkit/content/xul.css:888
> (Diff revision 1)
> >    -moz-binding: url("chrome://global/content/bindings/autocomplete.xml#autocomplete-richlistitem");
> >    -moz-box-orient: vertical;
> >    overflow: -moz-hidden-unscrollable;
> >  }
> >  
> > +/*
> 
> commented out code?

Fixed

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Drew Willcoxon :adw from comment #4)
> Did you mean to say "no reasons *not* to land it sooner"?  (i.e., we should
> land this soon)

Ehr, yes.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8817521 [details]
Bug 1257550 - Awesomebar result providers should be able to add document fragments or iframes.

my point about XPCunwrap and such, was that I thought in Panel.jsm (being something that tge consumer imports) you didn't want XPCOM concepts cause the consumer was supposed to modify Panel.jsm to his needs, and that may still require to understand those pieces.

Comment 11

[Drew Willcoxon :adw]

The consumer should not have to modify or understand Panel.jsm or anything else in the runtime.  If they do, then it's our failure and it's something for us to fix.  But since the runtime code is packaged in the add-on, the consumer can if they want to.  It's possible.  Which I think may come in handy as people actually start using the API and find problems with it.  Someone like Jared might be able to actually fix a problem and give us a patch, or propose a new function by writing a patch, and he'd be able to test his changes in his add-on immediately.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c26572bc257
Awesomebar result providers should be able to add document fragments or iframes. r=mak

Comment 13

[Drew Willcoxon :adw]

Attachment: eslint follow-up patch

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3b40fff0e49
eslint follow-up fixes. r=me

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7c26572bc257
https://hg.mozilla.org/mozilla-central/rev/f3b40fff0e49

Comment 16

[Andrei Vaida [:avaida]]

Setting qe-verify- since this seems to have (at least some) automated coverage.

Drew, if you think manual QA should instead be looking at this, please flip the flag. ni?'ing you just to make sure we're not overlooking this for Beta 53.

Comment 17

[Drew Willcoxon :adw]

Thanks for asking.  This doesn't need any manual QA since it's not a user-facing change.