Last Comment Bug 1257550 - Awesomebar result providers should be able to add document fragments or iframes
: Awesomebar result providers should be able to add document fragments or iframes
Status: RESOLVED FIXED
[fxsearch]
:
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: Unspecified Unspecified
P3 normal with 1 vote (vote)
: Firefox 53
Assigned To: Drew Willcoxon :adw
:
: Marco Bonardo [::mak]
Mentors:
Depends on: 1257544
Blocks:
  Show dependency treegraph
 
Reported: 2016-03-17 08:09 PDT by Panos Astithas [:past]
Modified: 2016-12-23 14:34 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
Bug 1257550 - Awesomebar result providers should be able to add document fragments or iframes. (58 bytes, text/x-review-board-request)
2016-12-08 16:39 PST, Drew Willcoxon :adw
mak77: review+
Details | Review
eslint follow-up patch (3.49 KB, patch)
2016-12-22 11:52 PST, Drew Willcoxon :adw
no flags Details | Diff | Splinter Review

Description User image Panos Astithas [:past] 2016-03-17 08:09:31 PDT
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 User image Drew Willcoxon :adw 2016-12-08 16:39:50 PST Comment hidden (mozreview-request)
Comment 2 User image Marco Bonardo [::mak] 2016-12-21 07:37:27 PST
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 User image Marco Bonardo [::mak] 2016-12-21 07:39:07 PST
Ehm, the last 2 "ditto" are referred to "commented out code?". Looks like I reverse-commented.
Comment 4 User image Drew Willcoxon :adw 2016-12-21 17:48:23 PST
(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 User image Drew Willcoxon :adw 2016-12-21 18:00:38 PST
(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 User image Drew Willcoxon :adw 2016-12-21 19:15:15 PST Comment hidden (mozreview-request)
Comment 7 User image Drew Willcoxon :adw 2016-12-21 19:16:10 PST
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 User image Drew Willcoxon :adw 2016-12-21 19:17:05 PST
(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 User image Marco Bonardo [::mak] 2016-12-22 03:12:08 PST
(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 User image Marco Bonardo [::mak] 2016-12-22 05:34:02 PST
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 User image Drew Willcoxon :adw 2016-12-22 09:54:05 PST
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 User image Pulsebot 2016-12-22 11:31:19 PST
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7c26572bc257
Awesomebar result providers should be able to add document fragments or iframes. r=mak
Comment 13 User image Drew Willcoxon :adw 2016-12-22 11:52:13 PST
Created attachment 8821261 [details] [diff] [review]
eslint follow-up patch
Comment 14 User image Pulsebot 2016-12-22 11:54:35 PST
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/f3b40fff0e49
eslint follow-up fixes. r=me

Note You need to log in before you can comment on or make changes to this bug.