1042194 - Provide a small framework for adding widgets to error pages

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Attachment: Patch v1

It would be nifty if we could offer some helpful widgets on error pages. Toggle wifi on-off. Search for this term. Suggested urls. etc.

To make that modular and doable, I'm building a little framework for creating and adding widgets. The api looks like:

NetErrorHelper.registerHandler({
  onErrorPageShown: function(NetErrorHelper helper) {
    // if you should be shown on this error page
      // build a node. helper may have some functions to make this easier.
      // add callbacks to the node using NetErrorHelper (see below)
      // insert it into the page
  },
});

Callbacks are added via:
helper.registerCallback(myNode, {
  click: function() { },
  input: function() { },
  pagehide: function() { },
})

I'll file separate bugs for the default widgets.

Comment 1

[Wesley Johnston (:wesj)]

Comment on attachment 8460372 [details] [diff] [review]
Patch v1

Might as well start reviewing these things.

Comment 2

[:Margaret Leibovic]

Comment on attachment 8460372 [details] [diff] [review]
Patch v1

Review of attachment 8460372 [details] [diff] [review]:
-----------------------------------------------------------------

I'm having a hard time understanding how this will work in practice, but I have some drive-by feedback.

::: mobile/android/modules/NetErrorHelper.jsm
@@ +22,5 @@
> +        input: function() { },
> +      })
> + * Some helper functions are also provided to make it easier to create common widgets
> + * i.e.
> + *    helper.createSwitch({label: "", on: "", off: "", callback: foo});

I don't see an implementation for this anywhere.

@@ +40,5 @@
> +      handler.onErrorPageShown(this);
> +    });
> +
> +    this.browser.addEventListener("click", this, true);
> +    this.browser.addEventListener("input", this, true);

Should we only add these event listeners if there are callbacks registered to handle these?

@@ +72,5 @@
> +  unregisterCallbacks: function(node) {
> +    this._callbacks.delete(node);
> +  },
> +
> +  _createNode: function(attrs) {

Should we hide some of these helper methods from the exported scope? Or if this is an exported helper method, it's confusing that it has a _ prefix.

@@ +92,5 @@
> +      }
> +    }
> +
> +    return elt;
> +  },

It looks like you have a syntax error here.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8460372 [details] [diff] [review]
Patch v1

I like this approach, but I think we should scale it back a little. Wes and I talked about how we could simplify parts of this approach. We also talked about using simple form elements where possible.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Much simplified. This doesn't have registration methods. We just hardcode them in a dictionary. When an error page is shown we still notify the handlers. That gives them a chance to add dynamic widgets if they want. Otherwise, the widgets can be hardcoded into the dtd/markup.

When the user clicks on something in an error page, we loop through all the listeners and look for any who's ID matches the ID of the clicked element (walking up the DOM Tree). If we find one, we send it the event to handle.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8466512 [details] [diff] [review]
Patch v2

Review of attachment 8466512 [details] [diff] [review]:
-----------------------------------------------------------------

I think my questions here will probably be answered by follow-up patches, since it doesn't look like this really does anything without more patches. Is there another patch I can look at?

::: mobile/android/chrome/content/browser.js
@@ +3766,5 @@
>          // pages and other similar page. This lets us fix bugs like 401575 which
>          // require error page UI to do privileged things, without letting error
>          // pages have any privilege themselves.
> +        if (docURI.startsWith("about:neterror")) {
> +          let helper = new NetErrorHelper(this.browser);

Why is this a local variable?

::: mobile/android/modules/NetErrorHelper.jsm
@@ +14,5 @@
> +function NetErrorHelper(browser) {
> +  browser.addEventListener("click", this, true);
> +
> +  let listener = () => {
> +    browser.removeEventListener("click", this, true);

Perhaps instead of making the whole object the handler, you could specify a method like this.handleClick, to make it clear what event is being handled. Or maybe this module should be renamed NetErrorClickHelper, if its main job is to intercept clicks.

@@ +21,5 @@
> +
> +  browser.addEventListener("pagehide", listener, true);
> +
> +  // Handlers may want to customize the page
> +  for (let id in handlers) {

Since the handlers object is empty, could we just get rid of this for now and add it when we actually start adding handlers?

I'm also a bit confused about how this will work, since it doesn't look like NetErrorHelper consumers would be able to specify handlers. I also think you should add some documentation explaining what the structure of the handlers object is supposed to look like and what it is used for.

@@ +23,5 @@
> +
> +  // Handlers may want to customize the page
> +  for (let id in handlers) {
> +    if (handlers[id].onPageShown) {
> +      handlers[id].onPageShown(brower);

Typo: s/browser/browser.

@@ +37,5 @@
> +    while(node) {
> +      for (let id in handlers) {
> +        if (id === node.id) {
> +          callback = handlers[id];
> +          node = null;

If the point of this function is just to find the appropriate callback to call, I think it would be clearer to just call that callback here and return.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch v3

(In reply to :Margaret Leibovic from comment #5)
> Is there another patch I can look at?
bug 1042196 is probably your best bet right now.
 
> > +          let helper = new NetErrorHelper(this.browser);
> Why is this a local variable?
No reason. I just can't quite figure out the right way to do this. These objects are "per-tab" since you could have multiple error pages up. I changed this to:

NetErrorHelper.attachToBrowser(browser)

in this patch. It still returns an object, but you can ignore it if you want (it keeps itself alive by attaching listeners to the browser).

> Or maybe this module should be renamed NetErrorClickHelper, if its main job
Hmm.. Its also going to handle page loads, but mostly I don't want to pigeon hole this into just handling clicks so early. I made a dedicated method for the click handling.

> Since the handlers object is empty, could we just get rid of this for now
> and add it when we actually start adding handlers?
Heh. The joys of micro-patching. I left this in because without it this class does nothing. We either put it in here or have a "mega" patch later that puts it in when its used, I like having it here better.

> NetErrorHelper consumers would be able to specify handlers.
Yeah. mark didn't quite want the "registration" bit left in. The handlers will be hardcoded in this file for now.

> If the point of this function is just to find the appropriate callback to
> call, I think it would be clearer to just call that callback here and return.
Good idea. Legacy cruft. Removed.

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 8468760 [details] [diff] [review]
Patch v3

Sorry. Need to make some changes to this.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Found a bug in that last patch. This one's better tested :)

Comment 9

[:Margaret Leibovic]

Comment on attachment 8468898 [details] [diff] [review]
Patch v3

Review of attachment 8468898 [details] [diff] [review]:
-----------------------------------------------------------------

Looking much better! I just have a few pieces of feedback I'd like you to consider before giving this an r+.

::: mobile/android/modules/NetErrorHelper.jsm
@@ +18,5 @@
> + *   handleEvent: function(event) { },
> + * }
> + *
> + * The key that you register yourself with should match the ID of the elementyou want to
> + * watch for click events on.

Nice documentation :)

(Typo: s/elementyou/element you)

@@ +47,5 @@
> +
> +NetErrorHelper.prototype = {
> +  handleClick: function(event) {
> +    let node = event.target;
> +    let callback = null;

Unused variable.

@@ +53,5 @@
> +    while(node) {
> +      for (let id in handlers) {
> +        if (id === node.id) {
> +          if (handlers[id] && handlers[id].handleEvent) {
> +            handlers[id].handleEvent(event);

If we're only registering for click events, I think that the handler API should also be handleClick (instead of handleEvent).

Also, instead of iterating through the handlers like this, could you just check for the node id directly? What about something like this:

// Walk up the tree looking for a node with a registered click handler.
while (node) {
  if (node.id in handlers && handlers[node.id].handleClick) {
    handlers[node.id].handleClick(event);
    return;
  }
  node = node.parentNode;
}

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch v4

I'm not sure why i was looping through the handlers there either. Copy-paste shenanigans I guess.

Comment 11

[:Margaret Leibovic]

Comment on attachment 8469329 [details] [diff] [review]
Patch v4

Review of attachment 8469329 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, looks good.

Comment 12

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/4d0872d38274

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4d0872d38274