980936 - Lockscreen Gadgets

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::System::Lockscreen, enhancement)

Description

[Zoran Jovanovic]

Attachment: Gadgets-on-Lockscreen-framework.patch

Provide hooks in Lockscreen for gadget framework to add, remove, rearrange and interact with gadgets in Lockscreen app.

The initial drop attached (NOTE: work in progress!) is intended as UX proof of concept, comes with some obvious shortcomings, and should serve as proposal for the way forward.

The initial drop provides the following:
    - method to add and remove gadgets in Lockscreen
      Any type of application can provide a gadget. Application's manifest file defines a gadget, path to its source and placement. UI in Lockscreen is added to add/remove gadgets - long tap will open a context menu with gadgets actions.
    - method to interact with gadget and its parent application
      Message handler defines the messages propagated. Gadget can trigger a mozActivity or start the parent application.
    - method to unlock the Lockscreen
    - demo application

The initial drop experiments with providing process sandboxing with some obvious shortcomings:
    - attempt to start the app unlocks the device but does not start the actual app
    - layouting issues (unwanted artifacts)

Comment 1

[Zoran Jovanovic]

Attachment: gadget_demo.tar

Comment 2

[dynamis (Tomoya ASAI)]

Greg is now working for new architecture of Lockscreen in bug 965105.
I believe this bug depends on the bug.

Please check the refactoring of lockscreen.js too (bug 960381).

Comment 3

[Zoran Jovanovic]

Thanks, we are very aware of the ongoing work in lockscreen and are looking to find connecting points. Hopefully we could collaborate on this.

Comment 4

[Zoran Jovanovic]

(In reply to dynamis (Tomoya ASAI) from comment #2)
> Greg is now working for new architecture of Lockscreen in bug 965105.
> I believe this bug depends on the bug.
> 
> Please check the refactoring of lockscreen.js too (bug 960381).

Comment 5

[Olle Klang]

Attachment: Gadgets-on-Lockscreen-framework.patch

Comment 6

[Olle Klang]

(In reply to Olle Klang from comment #5)
> Created attachment 8392818 [details] [diff] [review]
> Gadgets-on-Lockscreen-framework.patch

Added a patch for the framework to work with recently changes in ActionMenu.

Comment 7

[Zoran Jovanovic]

Comment on attachment 8387612 [details] [diff] [review]
Gadgets-on-Lockscreen-framework.patch

Obsoleted in favor of Olle Klang's patch from 2013-03-18.

Comment 8

[Olle Klang]

Attachment: Bug-980936-Lockscreen-Gadgets.patch

This patch:
- Fixes parent app launch and triggering MozActivity.
- Restores gadget state after reboot.
- Removes preloading of empty iframe to save on resources.
- Limits gadgets on lockscreen to 6 gadgets.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Just to let you know that this patch is not lost in space and I'm currently reviewing it. I'm hoping to have finished the review tomorrow.

I also want to try that on a device in order to see how it looks. Thanks for the patch and the demo app.

Comment 10

[Olle Klang]

Attachment: gadget-demo.tar

Added the viewport meta tag in the demo app. Now that the gadgets is loaded OOP (in iframes with the 'remote' attribute) it can not be omitted.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8395577 [details] [diff] [review]
Bug-980936-Lockscreen-Gadgets.patch

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

That's a good start and it raises many questions. Notably about the UX, the way to register widgets, and some APIs. Let's continue the work and needinfo's some of the relevant folks here.

::: apps/system/js/gadget_manager.js
@@ +11,5 @@
> + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
> + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
> + * License for the specific language governing permissions and limitations under
> + * the License.
> + */

Gaia is already under an Apache Licence (https://github.com/mozilla-b2g/gaia/blob/master/LICENSE) do you really need to add that here ?

@@ +41,5 @@
> +  SCREEN_DIVIDER: 6,
> +
> +  init: function gm_init() {
> +
> +    //Adds gadget container

nit: for most of those comments in this file, in many places in Gaia there is a space between the // and the comment. Let's stay consistent.

@@ +42,5 @@
> +
> +  init: function gm_init() {
> +
> +    //Adds gadget container
> +    this.container = document.createElement('div');

var container = this.container = document.createElement('div');

And use |container| instead of |this.container| for the rest of the code.

@@ +43,5 @@
> +  init: function gm_init() {
> +
> +    //Adds gadget container
> +    this.container = document.createElement('div');
> +    this.container.setAttribute('id', 'lockscreen-gadgets');

I would have preferred this container and the inner elements to be explicitly declared into the lockscreen html code.

Also there is probably some UX changes that needs to be done. For instance the current code move widgets on the first screen and it collapses with the clock and the notifications on a 480x320 screen (clock is above the widget and notifications appears on top of the widget too).
I wonder if the clock + notifications can be wrapped into a container and considered as a widget in terms of panning.

Needinfo'ing some UX folks for that.

@@ +55,5 @@
> +    this.container.appendChild(removeBtn);
> +    parentNode.insertBefore(this.container, beforeChild);
> +    this.getPlacedGadgets();
> +    this.listInstalledGadgets();
> +    this.scrollLimit = window.innerWidth / this.SCREEN_DIVIDER;

nit: You can probably declared that next to SCREEN_DIVIDER directly. You can even |scrollLimit: window.innerWidth / 6|; and explain a bit why 6 in a comment.

@@ +61,5 @@
> +    //add listeners for installation/uninstallation
> +    window.addEventListener('applicationinstall', this);
> +    window.addEventListener('applicationuninstall', this);
> +
> +    this.container.addEventListener('tap', this);

'tap' does not seems to be used.

@@ +65,5 @@
> +    this.container.addEventListener('tap', this);
> +    this.container.addEventListener('touchmove', this);
> +    this.container.addEventListener('touchstart', this);
> +    this.container.addEventListener('touchend', this);
> +    document.body.addEventListener('contextmenu', this);

You probably want to listen the contextmenu event on the lockscreen, and not the whole document.body.

@@ +66,5 @@
> +    this.container.addEventListener('touchmove', this);
> +    this.container.addEventListener('touchstart', this);
> +    this.container.addEventListener('touchend', this);
> +    document.body.addEventListener('contextmenu', this);
> +    window.addEventListener('mozbrowserlocationchange', this, false);

The |mozbrowserlocationchange| mechanism used as a communication mechanism between the widget and the lockscreen should be avoided. I know that this is something that we have done in the past for the keyboard, but this was not intended to be promoted as something that third party developers should do.

In fact it's not clear to me why you need to forward the WebActivity to the lockscreen (probably because of the z-index of apps that prevent the app to be rendered on top of the lockscreen ?).

About opening the parent the widget should have a way to do that from the content side. So mozapp.launch() from a widget should be understood by the platform as a request to open the full featured application.

@@ +77,5 @@
> +    var request = navigator.mozApps.mgmt.getAll();
> +    request.onsuccess = function(evt) {
> +      var apps = evt.target.result;
> +      apps.forEach(function eachApp(app) {
> +        if (app.manifest.gadgets) {

I'm not against a |gadgets| field in the manifest but we probably need to support it at the mozApps level too to make sure that it can be localized (override for some locales) and sanitized.

We also need to see if other vendors are interested by that.

Needinfo'ing Marcos.

Marcos please see in the demo application the manifest. There is a new |gadgets| field. What do you think ?

We may also consider the example where we want to have a widget in the notification tray, for example the cost control app, or the music player.

@@ +89,5 @@
> +
> +    }.bind(this);
> +    request.onerror = function(e) {
> +      alert('Error calling listInstalledGadgets: ' +
> +        e.target.error.name);

You can't use |alert| in the system scope. It is not supported on the device as |alert| is a modal dialog and that may block all the system app from doing anything :)

@@ +100,5 @@
> +   */
> +  showMenu: function gm_showMenu() {
> +    if (!lockScreen.enabled) {
> +      return;
> +    }

If you listen the contextmenu on the lockscreen this is unlikely to happen and that can probably be removed.

@@ +105,5 @@
> +    var callback = function(index) {
> +      this.placeGadget(index);
> +      this.setEditMode(false);
> +    }.bind(this);
> +    var menu = new ActionMenu(this.listItems(), 'Add Gadget', callback);

'Add Gadget' needs to be localized using the navigator.mozL10n API.

@@ +134,5 @@
> +        break;
> +      }
> +    }
> +    this.gadgets.splice(insertPos, 0, gadget);
> +    this.gadgetsByOrigin[gadget.origin] = app;

It's a bit strange that the naming of this.gadgets and this.gadgetsByOrigin are so close while the first store custom gadgets object and the second stores |app| objects.

@@ +150,5 @@
> +        value: i
> +      };
> +      gadgets.push(gadget);
> +    }
> +    return gadgets;

nit: for the sake or reading the code you may consider renaming |var gadgets| to |var items|. That may avoid some confusion between the object you're iterating on and the object you return.

@@ +175,5 @@
> +    var relY = Math.round(pageY - elemY);
> +
> +    elem.sendMouseEvent('mouseup', relX * window.devicePixelRatio ,
> +                         relY * window.devicePixelRatio , 0, 1);
> +  },

It's likely that app authors will expect a complete sequence of mouse events. It also makes me a bit sad that this is limited to mouse events too in a touch world. I would have preferred to used something like https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/touch_forwarder.js

For example I understand that the horizontal swipe is used for panning across widgets but I can imagine some widgets that would like to use the vertical space for scrolling. Like a facebook widgets for example.

@@ +206,5 @@
> +          }
> +          break;
> +      }
> +    }
> +  },

As I explained above in next to the |mozbrowserlocationchange| handler I hope all this code can be removed and that we can find different ways of achieving the same results from the content space.

@@ +243,5 @@
> +        var right = this.touchStartX < this.touchPos;
> +
> +        this.changeCurrent(right);
> +      }
> +    }

I'm not reviewing this part as the panning code would likely be simpler by using the native scroll. And the event dispatching for tap should use the helper in the system app.

@@ +265,5 @@
> +        break;
> +
> +      case 'contextmenu':
> +        if (this.currentGadget) {
> +          this.setEditMode(true);

For some reasons I can't see the |remove| widget button on my device. Sounds like there is something wrong here.

@@ +302,5 @@
> +          } else if (!right && this.rightGadget) {
> +            this.rightGadget.style.transform =
> +              'translateX('  + (window.innerWidth + delta) + 'px)';
> +          }
> +        }

Instead of using transforms to moves gadgets on screen, can we use a big div that contains the remote iframes and use native scrolling instead ?

For the moment this is not the fastest thing in the world, but as soon bug 950934 is fixed it will be as fast as we can pan.

@@ +330,5 @@
> +            if (this.gadgets[i].origin === origin) {
> +              this.gadgets.splice(i, 1);
> +            }
> +          }
> +        }

Is it possible to pass |app| as the parameter of removeGadget and do all the work there ?

That would be more consistent with the addGadget method.

@@ +347,5 @@
> +            console.warn('Activity error:', activity.error.name);
> +          };
> +        } else {
> +          this.pendingAppLaunch.launch();
> +        }

Let's remove the above code and let's explore what we can do from the app content side to do that.

@@ +348,5 @@
> +          };
> +        } else {
> +          this.pendingAppLaunch.launch();
> +        }
> +        this.resetUnlock();

At some point you need to call iframe.setVisible(false) for the widgets. If you don't do that their priority and OOM score won't be adjusted correctly.

@@ +386,5 @@
> +      this.rightGadget.classList.remove('right');
> +      this.currentGadget = this.rightGadget;
> +      this.currentGadget.classList.add('current');
> +      this.rightGadget = this.currentGadget.nextElementSibling;
> +    }

Left/Right gadget would be as easy as previousElementSibling/nextElementSibling with the native scroll approach.

@@ +408,5 @@
> +    }
> +    this.container.insertBefore(iframe, this.rightGadget);
> +    this.addedGadgets++;
> +    this.currentGadget = iframe;
> +    this.saveState();

You can probably have a addElement method similar to the removeElement one. That would make the code more symetric.

@@ +420,5 @@
> +   */
> +  fetchGadget: function gm_fetchGadget(origin, manifest) {
> +    var url = origin + manifest.gadgets.lockscreen;
> +    var iframe = document.createElement('iframe');
> +    iframe.setAttribute('mozbrowser', true);

Seems like you also need to have the 'mozapp' attribute if you want to access the app permissions.

@@ +422,5 @@
> +    var url = origin + manifest.gadgets.lockscreen;
> +    var iframe = document.createElement('iframe');
> +    iframe.setAttribute('mozbrowser', true);
> +    iframe.setAttribute('remote', true);
> +    iframe.setAttribute('src', url);

nit: Add a line break here.

@@ +425,5 @@
> +    iframe.setAttribute('remote', true);
> +    iframe.setAttribute('src', url);
> +    iframe.addEventListener('mozbrowserloadend', function(evt) {
> +      evt.target.parentNode.classList.remove('loading-gadget');
> +      evt.target.style.pointerEvents = '';

Maybe this can be achieve directly with CSS since there is a loading-gadget class on the parent.

@@ +428,5 @@
> +      evt.target.parentNode.classList.remove('loading-gadget');
> +      evt.target.style.pointerEvents = '';
> +      this.addingGadget = false;
> +      window.dispatchEvent(new CustomEvent('gadget-loaded'));
> +    }.bind(this));

nit: add a line break here.

@@ +430,5 @@
> +      this.addingGadget = false;
> +      window.dispatchEvent(new CustomEvent('gadget-loaded'));
> +    }.bind(this));
> +    iframe.addEventListener('mozbrowsererror', function(evt) {
> +      evt.target.style.pointerEvents = 'auto';

Don't you need to remove the loading-gadget class as well ?

@@ +433,5 @@
> +    iframe.addEventListener('mozbrowsererror', function(evt) {
> +      evt.target.style.pointerEvents = 'auto';
> +      this.addingGadget = false;
> +      window.dispatchEvent(new CustomEvent('gadget-error'));
> +    }.bind(this));

nit: add a line break here.

@@ +436,5 @@
> +      window.dispatchEvent(new CustomEvent('gadget-error'));
> +    }.bind(this));
> +    iframe.classList.add('current');
> +    iframe.classList.add('gadget');
> +    this.addingGadget = true;

I wonder if this guard should not be on a per-widget basis. What happens if you add 2 widgets at the same time and one finishes to load before the other ?

@@ +469,5 @@
> +        window.addEventListener('will-unlock', this);
> +        window.addEventListener('lockpanelchange', this);
> +        this.unlockLockscreen();
> +      }.bind(this));
> +    }.bind(this);

I hope that this code can be removed.

@@ +550,5 @@
> +  /**
> +   * Toggles editMode on/off.
> +   * @param {Boolean} editMode
> +   */
> +  setEditMode: function gm_setEditMode(editMode) {

this.editMode = editMode and remove the 2 places where there are updated after.

@@ +556,5 @@
> +      this.editMode = true;
> +      document.body.removeEventListener('contextmenu', this);
> +      this.container.removeEventListener('touchmove', this);
> +      this.container.classList.add('edit-mode');
> +      if (this.addedGadgets == this.MAX_GADGETS){

Nit: add a white space between ) and {

@@ +557,5 @@
> +      document.body.removeEventListener('contextmenu', this);
> +      this.container.removeEventListener('touchmove', this);
> +      this.container.classList.add('edit-mode');
> +      if (this.addedGadgets == this.MAX_GADGETS){
> +        this.container.classList.add('delete-mode');

Oh maybe that explains why I can't access the delete mode. I feel like people should be able to remove a widget even if there is less than MAX_GADGETS.

@@ +576,5 @@
> +  saveState: function gm_saveState() {
> +    var placedGadgets = [];
> +    var frames = this.container.getElementsByClassName('gadget');
> +    for (var i = 0; i < frames.length; i++) {
> +        placedGadgets.push(frames[i].getAttribute('src'));

nit: too many white spaces.

@@ +578,5 @@
> +    var frames = this.container.getElementsByClassName('gadget');
> +    for (var i = 0; i < frames.length; i++) {
> +        placedGadgets.push(frames[i].getAttribute('src'));
> +    }
> +    localStorage.setItem('placedGadgets', JSON.stringify(placedGadgets));

Please use asyncStorage. We try to avoid to use localStorage as it synchronously read on the disk.

(see https://github.com/mozilla-b2g/gaia/blob/master/shared/js/async_storage.js)

@@ +585,5 @@
> +  /**
> +   * Retrieves a list of which gadgets should be placed on lockscreen
> +   */
> +  getPlacedGadgets: function gm_getPlacedGadgets(){
> +    var placedGadgets = JSON.parse(localStorage.getItem('placedGadgets'));

Same comment as above.

@@ +595,5 @@
> +  /**
> +   * Replaces running gadgets on lockscreen
> +   */
> +  addPlacedGadgets: function gm_addPlacedGadgets() {
> +    if (this.gadgetsOnStartup.length <= 0) {

if (!this.gadgetsOnStartup.lenght) or if (this.gadgetsOnStartup.length === 0) as it can't be lower than 0 :)

@@ +600,5 @@
> +      return;
> +    }
> +
> +    var origin = this.getOrigin(this.gadgetsOnStartup[0]);
> +    this.gadgetsOnStartup.splice(0,1);

Array.shift ?

@@ +602,5 @@
> +
> +    var origin = this.getOrigin(this.gadgetsOnStartup[0]);
> +    this.gadgetsOnStartup.splice(0,1);
> +    var gadget = this.gadgetsByOrigin[origin];
> +    if (gadget){

Nit: add a space between ) and {

@@ +609,5 @@
> +      window.addEventListener('gadget-error', this);
> +    }
> +  }
> +};
> +GadgetManager.init();

I wonder if this file can not be splitted into 2 files. One that manage the widgets on the lockscreen, and one that manages the database of widgets for the lockscreen and lives in shared/js/.

My assumption is that the one that manage the database of widgets may be useful for the homescreen at some point.

::: apps/system/style/gadgets/gadgets.css
@@ +67,5 @@
> +}
> +
> +#lockscreen-gadgets iframe.right {
> +  transform: translateX(100%);
> +}

I would be happy if we can get rid of those with the native scrolling change.

@@ +71,5 @@
> +}
> +
> +#lockscreen-gadgets iframe.nextFrame {
> +  display: none;
> +}

This rule seems unused.

@@ +101,5 @@
> +.loading-gadget iframe {
> +  display: none
> +}
> +
> +#gadgetmenu-dialog {

Does all the code related to #gadgetmenu-dialog is related to the lockscreen or is it a homescreen leftover ?

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Marcos, Jonas, Ehsan, please comment on how to register a widget for an application.

The current patch uses a new field in the manifest.

'gadgets': {
  'lockscreen': 'foo.html',
  'homescreen': 'foo2.html'
}

I think we also need to consider the case of the notification tray here. I don't know if we really want to have a list of places where a widget can live however.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Josh, Jaime, can you please comment on the use of widgets and is there any UX resources that can help here ?

The current patch let users add a widget on the lockscreen, with a button to add them on a long press and a menu to pick which widgets.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I forgot to say during my review that this will obviously need some tests too :)

See https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

We also probably need to decide the naming here. widgets or gadgets, I keep using both. I don't mind which name is used at the end, I just feel like I'm making more typos with 'gadgets' than 'widgets' due to the 2 'g', which is really not a good argument when it come to naming :)

Comment 16

[André Jaenisch]

Well, we could discuss about 'gizmo' and 'applet' here as well.

Looking into Oxford:

 * Widget: http://www.oxforddictionaries.com/definition/english/widget
 * Gadget: http://www.oxforddictionaries.com/definition/english/gadget
 * Applet: http://www.oxforddictionaries.com/definition/english/applet
 * Gizmo: http://www.oxforddictionaries.com/definition/english/gizmo

When I consider the definition, I'd go for Widget rather than Gadget. But Applet suits best here. Gizmo is somewhat nerdy :-)

Comment 17

[Zoran Jovanovic]

naming: 
As stated in meta-bug (https://bugzilla.mozilla.org/show_bug.cgi?id=980929) "The name 'gadget' is chosen to disambiguate from other smart phone widgets and especially 'widgets' application packaging format standard proposal."

1. W3C widgets proposal: http://www.w3.org/TR/widgets/ - not really good for name reuse.
2. As pretty much all other mobile OSs use term widget in this context, we feel FFOS has potential to do something different/better.

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Zoran Jovanovic from comment #17)
> naming: 
> As stated in meta-bug (https://bugzilla.mozilla.org/show_bug.cgi?id=980929)
> "The name 'gadget' is chosen to disambiguate from other smart phone widgets
> and especially 'widgets' application packaging format standard proposal."
> 
> 1. W3C widgets proposal: http://www.w3.org/TR/widgets/ - not really good for
> name reuse.
> 2. As pretty much all other mobile OSs use term widget in this context, we
> feel FFOS has potential to do something different/better.

Sounds good enough for me then as I have no opinion on that. If someone wants to discuss that let's move the discussion on the meta bug in order to not pollute to much this one with naming discussion.

Comment 19

[(no longer active)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #12)
> Marcos, Jonas, Ehsan, please comment on how to register a widget for an
> application.
> 
> The current patch uses a new field in the manifest.
> 
> 'gadgets': {
>   'lockscreen': 'foo.html',
>   'homescreen': 'foo2.html'
> }

I'm not entirely clear on the requirements of what we're trying to implement here, but let me call out a few implications of the above:

1. The application can register up to but no more than one widget for each of the lockscreen and homescreen.
2. The application can unregister a widget that it had registered before or register a new widget by updating its manifest, and the apps which embed widgets should deal with these widgets appearing and disappearing.
3. The application's widgets are all executed with the same permissions as the app registering them.
4. The application's widgets do not have any size or appearance restrictions, i.e. they will be unable to enforce a size, so they need to be prepared to render themselves depending on how the embedder chooses to position them on the screen.

None of these seem problematic to me per se, but at least some of them are UX issues.  If we all agree on these limitations, then this syntax looks good to me.

(At the risk of opening up a bikeshedding discussion here, I really dislike the name "gadget".  I think "widget" is a better name, and not sure how much we should worry about a name clash with http://www.w3.org/TR/widgets/, but I'll let Marcos confirm that.)

> I think we also need to consider the case of the notification tray here. I
> don't know if we really want to have a list of places where a widget can
> live however.

Do you mean placing widgets in the notification area?  I'm not sure if that's something that we want to do because killing those widgets because of the system running low on memory for example can result in potentially surprising UX, but I'm not really sure what the use cases there are...

Comment 20

[(no longer active)]

Also, I'd appreciate if someone could please explain the approach uses in this patch to implement embedding of the widgets.  Does this patch use nested OOP iframes that are being worked on in bug 879475?

Comment 21

[Marcos Caceres [:marcosc]]

I was mainly going to echo @ehsan's comments, and I was also unclear about how the sizing is specified by the developer (i.e., how does the developer say that this is a 50x50px floating clock?). About the naming "widget", it's all just marketing so please call it whatever you feel works best. 

Also, will widgets and apps be able to use cross-document messaging to send messages to each other? 

About the notification tray, it would be good to work with @annevk if you need some enhancements to the Web Notifications API. Hopefully the API provides sufficient functionality to meet the use cases.

Comment 22

[(no longer active)]

(In reply to comment #21)
> I was mainly going to echo @ehsan's comments, and I was also unclear about how
> the sizing is specified by the developer (i.e., how does the developer say that
> this is a 50x50px floating clock?).

FWIW I think it's actually better to not restrict the sizes of widgets to pre-determined sizes, because a) Web content should be possible to render at arbitrary sizes, and while things such as scrolling UIs are probably not great for widgets, it would be nice to allow them to build those if they want to (for example, a weather widget could let you "scroll" it horizontally to see more detailed whether info) the widgets can discover their window's size and resize themselves accordingly, and b) it makes it easier to make the homescreen apps innovate on how to embed widgets to not restrict them to the sizes that we come up with here.

Comment 23

[Olle Klang]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #20)
> Also, I'd appreciate if someone could please explain the approach uses in
> this patch to implement embedding of the widgets.  Does this patch use
> nested OOP iframes that are being worked on in bug 879475?

The gadgets on the lockscreen are imbedded using iframes with the remote attribute in the same way the browser app loads different webpages in tabs today. As the lockscreen is part of the system app the patch is not dependent on the suggested solution in bug 879475. 

To have OOP gadgets on the homescreen however would not be possible without bug 879475 beeing fixed.

Comment 24

[(no longer active)]

(In reply to comment #23)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #20)
> > Also, I'd appreciate if someone could please explain the approach uses in
> > this patch to implement embedding of the widgets.  Does this patch use
> > nested OOP iframes that are being worked on in bug 879475?
> 
> The gadgets on the lockscreen are imbedded using iframes with the remote
> attribute in the same way the browser app loads different webpages in tabs
> today. As the lockscreen is part of the system app the patch is not dependent
> on the suggested solution in bug 879475. 
> 
> To have OOP gadgets on the homescreen however would not be possible without bug
> 879475 beeing fixed.

I see, that makes a lot of sense.  Thanks!

Comment 25

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #19)
> > I think we also need to consider the case of the notification tray here. I
> > don't know if we really want to have a list of places where a widget can
> > live however.
> 
> Do you mean placing widgets in the notification area?  I'm not sure if
> that's something that we want to do because killing those widgets because of
> the system running low on memory for example can result in potentially
> surprising UX, but I'm not really sure what the use cases there are...

There is already the use case of the Cost Control application which is displaying a kind of 'widget' into the notification bar. I think the Notifications API is way too limited when it comes to the flexibility of the UI to achieve what this 'widget' does as of today.

Also I can imagine that a music controller would like to live in the notification tray instead instead of only beeing available from the lockscreen, so you can control the music without having to leave the context of your current application.

Comment 26

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Marcos Caceres [:marcosc] from comment #21)
> I was mainly going to echo @ehsan's comments, and I was also unclear about
> how the sizing is specified by the developer (i.e., how does the developer
> say that this is a 50x50px floating clock?). About the naming "widget", it's
> all just marketing so please call it whatever you feel works best. 
> 
> Also, will widgets and apps be able to use cross-document messaging to send
> messages to each other? 
> 

You can imagine multiple ways of doing that I believe. The widget and the app can use a shared worker as a communication mechanism (but that's seems a bit of a hack).

Otherwise if the widget lives into a named window, the app should be able to retrieve reference to the widget using window.open('foo.html', '_widget') and start to communicate via postMessage.

Ehsan any comments on that ?

> About the notification tray, it would be good to work with @annevk if you
> need some enhancements to the Web Notifications API. Hopefully the API
> provides sufficient functionality to meet the use cases.

Comment 27

[Zoran Jovanovic]

Comment on attachment 8387625 [details]
gadget_demo.tar

Obsoleted in favor of gadget-demo.tar, the newer version uploaded by Olle Klang on 2014-03-27 06:53 PDT.

Comment 28

[Olle Klang]

Attachment: gadget-demo.tar

Comment 29

[Olle Klang]

Attachment: delete_icons.tar

Adding missing delete icons, should be added in 'apps/system/style/gadgets/'.

Comment 30

[Olle Klang]

Attachment: Bug-980936-Lockscreen-Gadgets_WIP.patch

Comment 31

[Olle Klang]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #11)

Thank you Vivien for your review and input. I've uploaded an new patch which adresses most issues. It's a work in progress and I'll continue with the unadressed issues below:

> @@ +609,5 @@
> > +      window.addEventListener('gadget-error', this);
> > +    }
> > +  }
> > +};
> > +GadgetManager.init();
> 
> I wonder if this file can not be splitted into 2 files. One that manage the
> widgets on the lockscreen, and one that manages the database of widgets for
> the lockscreen and lives in shared/js/.
> 
> My assumption is that the one that manage the database of widgets may be
> useful for the homescreen at some point.

I agree that we should try to share as much code as possible with homescreen gadgets and I'll look into that.
   
> @@ +175,5 @@
> > +    var relY = Math.round(pageY - elemY);
> > +
> > +    elem.sendMouseEvent('mouseup', relX * window.devicePixelRatio ,
> > +                         relY * window.devicePixelRatio , 0, 1);
> > +  },
> 
> It's likely that app authors will expect a complete sequence of mouse
> events. It also makes me a bit sad that this is limited to mouse events too
> in a touch world. I would have preferred to used something like
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> touch_forwarder.js

This is in pipeline.

> 
> @@ +302,5 @@
> > +          } else if (!right && this.rightGadget) {
> > +            this.rightGadget.style.transform =
> > +              'translateX('  + (window.innerWidth + delta) + 'px)';
> > +          }
> > +        }
> 
> Instead of using transforms to moves gadgets on screen, can we use a big div
> that contains the remote iframes and use native scrolling instead ?
> 
> For the moment this is not the fastest thing in the world, but as soon bug
> 950934 is fixed it will be as fast as we can pan.

I'm not sure why scrolling would be better than using transforms in this case. Do you know of any performance issues with transforms? I don't think it's a bad idea to use css transforms for transitions between gadgets instead of writing custom easing methods in javascript. It is used today in both homescreen to change between pages and in cards view to move between open apps. This also opens up for more creative ways to animate gadget switches (zoom in/out etc).

Comment 32

[Olle Klang]

Needinfoing Vivien, please see comment above.

Comment 33

[(no longer active)]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #25)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #19)
> > > I think we also need to consider the case of the notification tray here. I
> > > don't know if we really want to have a list of places where a widget can
> > > live however.
> > 
> > Do you mean placing widgets in the notification area?  I'm not sure if
> > that's something that we want to do because killing those widgets because of
> > the system running low on memory for example can result in potentially
> > surprising UX, but I'm not really sure what the use cases there are...
> 
> There is already the use case of the Cost Control application which is
> displaying a kind of 'widget' into the notification bar. I think the
> Notifications API is way too limited when it comes to the flexibility of the
> UI to achieve what this 'widget' does as of today.
> 
> Also I can imagine that a music controller would like to live in the
> notification tray instead instead of only beeing available from the
> lockscreen, so you can control the music without having to leave the context
> of your current application.

OK, so I'm not sure if these kinds of widgets are really "notifications" at least in the sense of the Notifications API.  On Android for example, apps can place widgets in the notification area but they're treated differently in the UI (for example, you can't remove them like regular notifications by swiping to the left or right) and they're also placed somewhat differently.  Do we have a UX spec for what we want to do here?

Comment 34

[(no longer active)]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #26)
> (In reply to Marcos Caceres [:marcosc] from comment #21)
> > I was mainly going to echo @ehsan's comments, and I was also unclear about
> > how the sizing is specified by the developer (i.e., how does the developer
> > say that this is a 50x50px floating clock?). About the naming "widget", it's
> > all just marketing so please call it whatever you feel works best. 
> > 
> > Also, will widgets and apps be able to use cross-document messaging to send
> > messages to each other? 
> > 
> 
> You can imagine multiple ways of doing that I believe. The widget and the
> app can use a shared worker as a communication mechanism (but that's seems a
> bit of a hack).
> 
> Otherwise if the widget lives into a named window, the app should be able to
> retrieve reference to the widget using window.open('foo.html', '_widget')
> and start to communicate via postMessage.
> 
> Ehsan any comments on that ?

window.open would not do what you want, I'm afraid, since it's designed for opening windows, not finding already open ones.  I think you can build what you need on top of SharedWorker though.

Comment 35

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Olle Klang from comment #31)
> > 
> > @@ +302,5 @@
> > > +          } else if (!right && this.rightGadget) {
> > > +            this.rightGadget.style.transform =
> > > +              'translateX('  + (window.innerWidth + delta) + 'px)';
> > > +          }
> > > +        }
> > 
> > Instead of using transforms to moves gadgets on screen, can we use a big div
> > that contains the remote iframes and use native scrolling instead ?
> > 
> > For the moment this is not the fastest thing in the world, but as soon bug
> > 950934 is fixed it will be as fast as we can pan.
> 
> I'm not sure why scrolling would be better than using transforms in this
> case. Do you know of any performance issues with transforms? 

I know at least 2 performances issues. The first one is related to the layerisation of a div when you start to do a transform on it from JS. It make take a hundred ms on some devices. It can be avoid using |will-change: transform| but you need to always keep the side widgets visible somewhere (even 0.0001px of the widget).

The second is because of a layout bug, that forces too much CSS recomputations when a element.style rule is updated. The cost varies based on the number of childrens of the container that is transitioned. So I don't expect it to be expensive for widgets since those does not have any direct child as the content is remote. But if notifications is moved into a container that can be transitioned then it may be very expensive depending how many notifications you have.

> I don't think
> it's a bad idea to use css transforms for transitions between gadgets
> instead of writing custom easing methods in javascript. It is used today in
> both homescreen to change between pages and in cards view to move between
> open apps. This also opens up for more creative ways to animate gadget
> switches (zoom in/out etc).

I do not disagree 100% with you. But I would like this patch to stay as simple as possible since it already brings it own set of questions and issues.
Once everything as been figured out and this patch has landed, someone can open a followup to change the panning algorithm if needed, and that's fine. Does it makes sense ?

For what it worth we are also going to change the homescreen panning algorithm to use the native APZ scrolling. And our current POC shows that it is much much smoother :)

Comment 36

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #34)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails,
> needinfo? please) from comment #26)
> > (In reply to Marcos Caceres [:marcosc] from comment #21)
> > > I was mainly going to echo @ehsan's comments, and I was also unclear about
> > > how the sizing is specified by the developer (i.e., how does the developer
> > > say that this is a 50x50px floating clock?). About the naming "widget", it's
> > > all just marketing so please call it whatever you feel works best. 
> > > 
> > > Also, will widgets and apps be able to use cross-document messaging to send
> > > messages to each other? 
> > > 
> > 
> > You can imagine multiple ways of doing that I believe. The widget and the
> > app can use a shared worker as a communication mechanism (but that's seems a
> > bit of a hack).
> > 
> > Otherwise if the widget lives into a named window, the app should be able to
> > retrieve reference to the widget using window.open('foo.html', '_widget')
> > and start to communicate via postMessage.
> > 
> > Ehsan any comments on that ?
> 
> window.open would not do what you want, I'm afraid, since it's designed for
> opening windows, not finding already open ones.  I think you can build what
> you need on top of SharedWorker though.

I'm referring to the paragraph of https://developer.mozilla.org/en-US/docs/Web/API/Window.open:

If a window with the name strWindowName already exists, then strUrl is loaded into the existing window. In this case the return value of the method is the existing window and strWindowFeatures is ignored. Providing an empty string for strUrl is a way to get a reference to an open window by its name without changing the window's location. To open a new window on every call of window.open(), use the special value _blank for strWindowName.

Comment 37

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #33)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails,
> needinfo? please) from comment #25)
> > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > #19)
> > > > I think we also need to consider the case of the notification tray here. I
> > > > don't know if we really want to have a list of places where a widget can
> > > > live however.
> > > 
> > > Do you mean placing widgets in the notification area?  I'm not sure if
> > > that's something that we want to do because killing those widgets because of
> > > the system running low on memory for example can result in potentially
> > > surprising UX, but I'm not really sure what the use cases there are...
> > 
> > There is already the use case of the Cost Control application which is
> > displaying a kind of 'widget' into the notification bar. I think the
> > Notifications API is way too limited when it comes to the flexibility of the
> > UI to achieve what this 'widget' does as of today.
> > 
> > Also I can imagine that a music controller would like to live in the
> > notification tray instead instead of only beeing available from the
> > lockscreen, so you can control the music without having to leave the context
> > of your current application.
> 
> OK, so I'm not sure if these kinds of widgets are really "notifications" at
> least in the sense of the Notifications API.  On Android for example, apps
> can place widgets in the notification area but they're treated differently
> in the UI (for example, you can't remove them like regular notifications by
> swiping to the left or right) and they're also placed somewhat differently. 
> Do we have a UX spec for what we want to do here?

I think there is one somewhere. Gregor should know where the Settings Drawer specs lives (needinfo'ing him).

Comment 38

[(no longer active)]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #36)
> > > Otherwise if the widget lives into a named window, the app should be able to
> > > retrieve reference to the widget using window.open('foo.html', '_widget')
> > > and start to communicate via postMessage.
> > > 
> > > Ehsan any comments on that ?
> > 
> > window.open would not do what you want, I'm afraid, since it's designed for
> > opening windows, not finding already open ones.  I think you can build what
> > you need on top of SharedWorker though.
> 
> I'm referring to the paragraph of
> https://developer.mozilla.org/en-US/docs/Web/API/Window.open:
> 
> If a window with the name strWindowName already exists, then strUrl is
> loaded into the existing window. In this case the return value of the method
> is the existing window and strWindowFeatures is ignored. Providing an empty
> string for strUrl is a way to get a reference to an open window by its name
> without changing the window's location. To open a new window on every call
> of window.open(), use the special value _blank for strWindowName.

So first of all AFAIK we have nothing to check for the names of windows across processes.  Also, even in the same process, the window.open call will cause foo.html to be loaded inside the window named "_widget", navigating away the page that is currently loaded in it.  That's not desirable here right?

Comment 39

[Gregor Wagner [:gwagner]]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #37)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #33)
> > (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails,
> > needinfo? please) from comment #25)
> > > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > > #19)
> > > > > I think we also need to consider the case of the notification tray here. I
> > > > > don't know if we really want to have a list of places where a widget can
> > > > > live however.
> > > > 
> > > > Do you mean placing widgets in the notification area?  I'm not sure if
> > > > that's something that we want to do because killing those widgets because of
> > > > the system running low on memory for example can result in potentially
> > > > surprising UX, but I'm not really sure what the use cases there are...
> > > 
> > > There is already the use case of the Cost Control application which is
> > > displaying a kind of 'widget' into the notification bar. I think the
> > > Notifications API is way too limited when it comes to the flexibility of the
> > > UI to achieve what this 'widget' does as of today.
> > > 
> > > Also I can imagine that a music controller would like to live in the
> > > notification tray instead instead of only beeing available from the
> > > lockscreen, so you can control the music without having to leave the context
> > > of your current application.
> > 
> > OK, so I'm not sure if these kinds of widgets are really "notifications" at
> > least in the sense of the Notifications API.  On Android for example, apps
> > can place widgets in the notification area but they're treated differently
> > in the UI (for example, you can't remove them like regular notifications by
> > swiping to the left or right) and they're also placed somewhat differently. 
> > Do we have a UX spec for what we want to do here?
> 
> I think there is one somewhere. Gregor should know where the Settings Drawer
> specs lives (needinfo'ing him).

The new settings drawer UX can be found here https://mozilla.app.box.com/s/pd93ts6r1qt0npf5uome and 959722 is the meta bug for the new notification tray.

Comment 40

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #38)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails,
> needinfo? please) from comment #36)
> > > > Otherwise if the widget lives into a named window, the app should be able to
> > > > retrieve reference to the widget using window.open('foo.html', '_widget')
> > > > and start to communicate via postMessage.
> > > > 
> > > > Ehsan any comments on that ?
> > > 
> > > window.open would not do what you want, I'm afraid, since it's designed for
> > > opening windows, not finding already open ones.  I think you can build what
> > > you need on top of SharedWorker though.
> > 
> > I'm referring to the paragraph of
> > https://developer.mozilla.org/en-US/docs/Web/API/Window.open:
> > 
> > If a window with the name strWindowName already exists, then strUrl is
> > loaded into the existing window. In this case the return value of the method
> > is the existing window and strWindowFeatures is ignored. Providing an empty
> > string for strUrl is a way to get a reference to an open window by its name
> > without changing the window's location. To open a new window on every call
> > of window.open(), use the special value _blank for strWindowName.
> 
> So first of all AFAIK we have nothing to check for the names of windows
> across processes.  Also, even in the same process, the window.open call will
> cause foo.html to be loaded inside the window named "_widget", navigating
> away the page that is currently loaded in it.  That's not desirable here
> right?

I don't think we need anything that is cross-process here, as the music widget will lived in the same process as the music app.
In the same process, the window.open call would not change the location of the named window if strURL == ''.

Comment 41

[(no longer active)]

(In reply to comment #40)
> > So first of all AFAIK we have nothing to check for the names of windows
> > across processes.  Also, even in the same process, the window.open call will
> > cause foo.html to be loaded inside the window named "_widget", navigating
> > away the page that is currently loaded in it.  That's not desirable here
> > right?
> 
> I don't think we need anything that is cross-process here, as the music widget
> will lived in the same process as the music app.
> In the same process, the window.open call would not change the location of the
> named window if strURL == ''.

I guess I don't really understand the architecture being implemented here then.  Here is what I was thinking of: each widget would be an OOP iframe to a specific URL in the respective app, created (ultimately after bug 879475 lands) independently of the process for the app itself.  This means that for example we would be able to load a widget even if the app itself is not running, and getting an app itself killed will not mean that its widgets are also killed with it (and vice versa).  I thought this is what comment 24 suggested, but your comment here makes me think that I'm misunderstanding large parts of this picture.

For window.open to work the way you suggest, the architecture would probably be very different.  For one thing, we wouldn't have an iframe for the widgets, instead we would need to launch the music app, get it to call window.open with a specific window name, somehow magically convert that window to something that sits in the DOM for the lockscreen app and renders the widget running in the music app's process.  Then, if you called window.open in the music app with an empty string as a URL, you would get the existing window object back.  Is this what the architecture here looks like?  And are we doing this just to get the window.open trick to work?

Comment 42

[(no longer active)]

(In reply to comment #39)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo?
> please) from comment #37)
> > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > #33)
> > > (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails,
> > > needinfo? please) from comment #25)
> > > > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > > > #19)
> > > > > > I think we also need to consider the case of the notification tray here. I
> > > > > > don't know if we really want to have a list of places where a widget can
> > > > > > live however.
> > > > > 
> > > > > Do you mean placing widgets in the notification area?  I'm not sure if
> > > > > that's something that we want to do because killing those widgets because of
> > > > > the system running low on memory for example can result in potentially
> > > > > surprising UX, but I'm not really sure what the use cases there are...
> > > > 
> > > > There is already the use case of the Cost Control application which is
> > > > displaying a kind of 'widget' into the notification bar. I think the
> > > > Notifications API is way too limited when it comes to the flexibility of the
> > > > UI to achieve what this 'widget' does as of today.
> > > > 
> > > > Also I can imagine that a music controller would like to live in the
> > > > notification tray instead instead of only beeing available from the
> > > > lockscreen, so you can control the music without having to leave the context
> > > > of your current application.
> > > 
> > > OK, so I'm not sure if these kinds of widgets are really "notifications" at
> > > least in the sense of the Notifications API.  On Android for example, apps
> > > can place widgets in the notification area but they're treated differently
> > > in the UI (for example, you can't remove them like regular notifications by
> > > swiping to the left or right) and they're also placed somewhat differently. 
> > > Do we have a UX spec for what we want to do here?
> > 
> > I think there is one somewhere. Gregor should know where the Settings Drawer
> > specs lives (needinfo'ing him).
> 
> The new settings drawer UX can be found here
> https://mozilla.app.box.com/s/pd93ts6r1qt0npf5uome and 959722 is the meta bug
> for the new notification tray.

Based on this, I don't see why we would want to manipulate those kinds of widgets through the notification API.  It seems like the UX spec here specifies something very similar to the way Android does this, and these widgets are not even conceptually notifications.

Comment 43

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hi all,

We had made a similar discussions on TV widget development before. And I collect them as a list for evaluating each widget design. Here is the list and the evaluation result of lockscreen patch, this one, and homescreen patch, bug 980935. Before the list, please note that lots decisions must be made here. The item marked as not-supported doesn't mean we need to make it as supported.

1. the entry point of widget?
 > let's add a new item to manifest

2. widget <=> app communication?
 > not defined but it may be shared worker

3. may we operate the widget by tapping, swipe it?
 > only mouse-up is bridged

4. may we have multiple instances of the same widget?
 > yes

5. may each instance of the same widget has its owned configuration? (Different timezone shown at different clock widget instance)
 > no

6. may we enlarge the widget's size and it becomes the app mode?
 > no, we use entry point to separate them.

7. 3-parties homescreen/lockscreen support?
 > with the current implementation, yes. But for long-term, we may need to open some permission to privileged app, like open-remote-window, etc.

8. widget creation is handled by?
 > system app handles lockscreen case and homescreen app handles homescreen case.

9. widget layout is determined by?
 > system app handles lockscreen case and homescreen app handles homescreen case.

10. app update/removed is handled by?
 > only removed is handled. And system app handles lockscreen case and homescreen app handles homescreen case.

11. widget error is handled by (mozbrowsererror)?
 > system app handles lockscreen case and homescreen app handles homescreen case.

12. other mozbrowser events is handed by?
 > system app handles lots events in lockscreen case but no one handles homescereen case.

13. is input field allowed to be used in widget?
 > no, only mouse up is bridged.

Comment 44

[John Hu [:johnhu][:johu][:醬糊小弟]]

BTW, item 12, other mozbrowser events, includes mozbrowseropenwindow, mozbrowsershowmodalprompt, mozbrowserusernameandpasswordrequired, etc.

Comment 45

[John Hu [:johnhu][:johu][:醬糊小弟]]

For some testing, I had wrote few testing widgets to test widget operations, like touch event, mouse event, open window, keyboard, etc. The patch is in my repository:
https://github.com/huchengtw-moz/gaia/commit/7ca08871304b6d2c75e40ba638674defc54b2f25

Comment 46

[John Hu [:johnhu][:johu][:醬糊小弟]]

add 898348 because it breaks the lockscreen and system app when we use "crash it" widget with this implementation. After lockscreen as an app, the crashed lockscreen will be restored by system app.

Comment 47

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #41)
> (In reply to comment #40)
> > > So first of all AFAIK we have nothing to check for the names of windows
> > > across processes.  Also, even in the same process, the window.open call will
> > > cause foo.html to be loaded inside the window named "_widget", navigating
> > > away the page that is currently loaded in it.  That's not desirable here
> > > right?
> > 
> > I don't think we need anything that is cross-process here, as the music widget
> > will lived in the same process as the music app.
> > In the same process, the window.open call would not change the location of the
> > named window if strURL == ''.
> 
> I guess I don't really understand the architecture being implemented here
> then.  Here is what I was thinking of: each widget would be an OOP iframe to
> a specific URL in the respective app, created (ultimately after bug 879475
> lands) independently of the process for the app itself.  This means that for
> example we would be able to load a widget even if the app itself is not
> running, and getting an app itself killed will not mean that its widgets are
> also killed with it (and vice versa).  I thought this is what comment 24
> suggested, but your comment here makes me think that I'm misunderstanding
> large parts of this picture.
> 
> For window.open to work the way you suggest, the architecture would probably
> be very different.  For one thing, we wouldn't have an iframe for the
> widgets, instead we would need to launch the music app, get it to call
> window.open with a specific window name, somehow magically convert that
> window to something that sits in the DOM for the lockscreen app and renders
> the widget running in the music app's process.  Then, if you called
> window.open in the music app with an empty string as a URL, you would get
> the existing window object back.  Is this what the architecture here looks
> like?  And are we doing this just to get the window.open trick to work?

After a video-conf discussion with Ehsan, it seems like the way to address the communications mechanism between multiple browsing contexts (in this case the widget, and the app) would be bug 966439.

Comment 48

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #43)
> 2. widget <=> app communication?
>  > not defined but it may be shared worker

Shared Worker is nice, but is sometimes a bit of a hammer for simple data exchanges. Bug 966439 should make it lighter. 


> 3. may we operate the widget by tapping, swipe it?
>  > only mouse-up is bridged

As I said in the first review of the patch on this bug, app authors will expect the full sequence of events. So one solution would be to simply wrap all the widgets into a scrollable div (either vertically or horizontally) and use the scrollgrab attribute (see https://bugzilla.mozilla.org/show_bug.cgi?id=912666#c0) 

As a result it means that in a vertical scrollable frame, the scroll action will first be given to the container. As a result child widgets likely needs to implement any panning action into the opposite axis if they want to fit nicely. 


> 7. 3-parties homescreen/lockscreen support?
>  > with the current implementation, yes. But for long-term, we may need to
> open some permission to privileged app, like open-remote-window, etc.

One thing that has been discussed a bit (maybe offline) would be to create a variant of the embed-apps permission. Currently the embed-apps permission let an application (like the system app) read data from an other application, and it also let it renders other apps inside <iframe mozapp>.

A variant of this permission could be to only authorize the second thing. A homescreen/lockscreen with this permission will not be allowed to read data from an other app, but will be allowed to render it inside a <iframe mozapp>.

> 8. widget creation is handled by?
>  > system app handles lockscreen case and homescreen app handles homescreen
> case.

With the lockscreen code moving as a separate application, the lockscreen app would handle lockscreen widgets.

> 9. widget layout is determined by?
>  > system app handles lockscreen case and homescreen app handles homescreen
> case.

Same answer as above. Let's consider the fact that the lockscreen belongs to the system app as a bug for now. (bug 898348)


> 10. app update/removed is handled by?
>  > only removed is handled. And system app handles lockscreen case and
> homescreen app handles homescreen case.
> 

Same answer as above.

> 11. widget error is handled by (mozbrowsererror)?
>  > system app handles lockscreen case and homescreen app handles homescreen
> case.
> 

Same answer as above.

> 12. other mozbrowser events is handed by?
>  > system app handles lots events in lockscreen case but no one handles
> homescereen case.

This is tricky. And that's one of the thing we need to figured out. For example I would expect the widgets embedders to handle widgets loading errors. But a widget sometimes needs to open a new window. For instance if you have an email widget that display the list of your last received email, you may expect that clicking on one of this widget will open the app on the email details view.

navigator.mozApps.getSelf().launch() is not really helpful here as it lack the supports for arguments, but we can imagine that the widget is trying to do a call to window.open('app_email_detail_view_url.html?custom_parameters=...').

There is an assumption as of today that most of the window.open calls are handled by the system app. If the widget lives into an other app, the system app won't receive the mozbrowseropenwindow call and the responsibility of handling such a call is delegated to the widget embedders.

IMO we will need to handle that at the system app level, if the app does not want to handle this kind of messages. For this we need to figure out the righ list of messages that should bubble if the embedder does not handle it. It seems to me that all the messages that may result into a popup/prompts needs to bubble to be handle by the system app, such as window.open, window.alert, window.confirm, window.prompt, and possibly others I may forgot. 

> 13. is input field allowed to be used in widget?
>  > no, only mouse up is bridged.

As I said above it seems to me that app authors will expect the full sequence of events for a normal app. I understand that it may be difficult to handle the keyboard popping up with a widget that is part of the internal layout of an application but that's an issue on the platform side that needs an answer.

Maybe bug 970093 may help for the keyboard case.

Comment 49

[Olle Klang]

Attachment: Bug-980936-Lockscreen-Gadgets_WIP.patch

Updated patch to use native scrolling in favor of css transforms.

Comment 50

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #12)
> 'gadgets': {
>   'lockscreen': 'foo.html',
>   'homescreen': 'foo2.html'
> }

I think I'd rather invert that to something like this:

"gadgets": [
 {
   "title": "album titles",
   "url": "foo.html",
   "locations": ["homescreen"]
 },
 {
   "title": "audio controls",
   "url": "bar.html",
   "locations": ["homescreen", "lockscreen"]
 }
]

The main problem with this is that the localization format wouldn't be able to target the titles here. It's something that we keep running into with our localization format so maybe something we should fix. Alternatively the Array could be turned into some object with arbitrarily named keys.

Comment 51

[Olle Klang]

Attachment: Bug-980936-Lockscreen-Gadgets_v6.patch

With bug 1006495 out of the way, touch forwarding works as it should.

Comment 52

[Olle Klang]

Attachment: gadget-demo_v4.tar

Updates the demo gadget to comply with framework changes:
- now works with touchforwarding
- vertical scrolling can be displayed

Comment 53

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8439200 [details] [diff] [review]
Bug-980936-Lockscreen-Gadgets_v6.patch

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

Let's do an other iteration. Thanks for splitting the files, the code is easier to understand this way.

I didn't reviewed all the touch events related code yet but there is enough comments to start on a new version. I would like to see some tests as well.

::: apps/system/js/activities.js
@@ +87,3 @@
>     /**
>      * Displays the activity menu if needed.
>      * If there is only one option, the activity is automatically launched.

I understand why you add this code to activities.js and app_window_factory.js instead of the lockscreen directly - especially since the lockscreen can be a separated app.

But it makes me a little sad to have this code duplicated between those 2 files and I wonder if there is anything better we can do in terms of code.

Alive do you have any opinions here ?

::: apps/system/js/gadget_manager.js
@@ +38,5 @@
> +  init: function gm_init() {
> +    window.addEventListener('lockscreen-appcreated', this);
> +  },
> +
> +  start: function gm_start() {

There is probably plenty of work you can can in |init|. Having both |init| and |start| is confusing. So making |start| smaller with a different name would be helpful :)

@@ +43,5 @@
> +    window.addEventListener('gadgets-installed', this);
> +    this.gdgInstMngr = new GadgetInstallManager('lockscreen');
> +    var container = this.container =
> +      document.getElementById('lockscreen-gadget-container');
> +    container.addEventListener('gadgets-retrieved', this);

Let's add the other container.addEventListeners call for touch events right here.

@@ +44,5 @@
> +    this.gdgInstMngr = new GadgetInstallManager('lockscreen');
> +    var container = this.container =
> +      document.getElementById('lockscreen-gadget-container');
> +    container.addEventListener('gadgets-retrieved', this);
> +    this.getPlacedGadgets();

Let's move this call somewhere else in the function. It's hard to see it otherwise (I looked for it!).

@@ +48,5 @@
> +    this.getPlacedGadgets();
> +    this.scroller = document.getElementById('lockscreen-gadget-scroller');
> +    this.addedGadgets = document.getElementById('lockscreen-gadgets');
> +
> +    window.addEventListener('gadget-uninstalled', this);

Let's declare the GadgetManager instance just above this line, to make the relationship clearer.
Let's also move the |window.addEventListener('gadgets-installed', this);| declaration here, as it also comes from the helper.

@@ +55,5 @@
> +    container.addEventListener('touchmove', this);
> +    container.addEventListener('touchend', this);
> +
> +    this.lockscreen = document.getElementById('lockscreen');
> +    this.lockscreen.addEventListener('contextmenu', this);

nit: add a line break here.

@@ +67,5 @@
> +   * Shows the gadgetmenu
> +   */
> +  showMenu: function gm_showMenu() {
> +    var callback = function(index) {
> +      this.setEditMode(false);

Don't you want to exit the edit mode as well, if the menu is cancelled ?

@@ +180,5 @@
> +   * @param {Object} evt event object.
> +   */
> +  handleEvent: function gm_handleEvent(evt) {
> +    switch (evt.type) {
> +      case 'gadgets-installed':

s/gadgets-installed/gadgetsready

@@ +181,5 @@
> +   */
> +  handleEvent: function gm_handleEvent(evt) {
> +    switch (evt.type) {
> +      case 'gadgets-installed':
> +        this.container.removeEventListener('gadgets-installed', this);

s/'gadgets-installed./evt.type

@@ +183,5 @@
> +    switch (evt.type) {
> +      case 'gadgets-installed':
> +        this.container.removeEventListener('gadgets-installed', this);
> +        this.gadgetsInstalled = true;
> +        if (this.gadgetsRetrieved){

Add a space between ) and {

@@ +198,5 @@
> +        this.start();
> +        break;
> +
> +      case 'gadgets-retrieved':
> +        this.container.removeEventListener('gadgets-retrieved', this);

s/'gadgets-retrieved'/evt.type

@@ +200,5 @@
> +
> +      case 'gadgets-retrieved':
> +        this.container.removeEventListener('gadgets-retrieved', this);
> +        this.gadgetsRetrieved = true;
> +        if (this.gadgetsInstalled){

Add a space between ) and {

@@ +203,5 @@
> +        this.gadgetsRetrieved = true;
> +        if (this.gadgetsInstalled){
> +          this.addPlacedGadgets();
> +        }
> +        break;

Let's move gadgets-installed and gadgets-retrieved closer, so it will be easier to follow that the addPlacedGadgets is done only when both have fired.

@@ +223,5 @@
> +          this.touchForwarder.destination = this.currentGadget.firstChild;
> +          this.touchStartEvt = evt;
> +        }
> +        this.touchStartX = evt.targetTouches[0].pageX;
> +        this.touchStartY = evt.targetTouches[0].pageY;

Can you add a onTouchStart method ? If will be easier to read than having everything into handleEvent. I used to do that in the past and it ends up into a very long method :)

@@ +248,5 @@
> +        } else {
> +          var newPos = this.currentIndex * window.innerWidth +
> +            this.touchStartX - this.touchPos.pageX;
> +          this.scroller.scrollLeft = newPos;
> +        }

Same comment than touchstart.

@@ +329,5 @@
> +    iframe.setAttribute('mozapp', manifestURL);
> +    iframe.setAttribute('remote', true);
> +    iframe.setAttribute('src', url);
> +
> +    iframe.addEventListener('mozbrowserloadend', function(evt) {

You can probably remove the listener once this has fired.

@@ +331,5 @@
> +    iframe.setAttribute('src', url);
> +
> +    iframe.addEventListener('mozbrowserloadend', function(evt) {
> +      evt.target.parentNode.classList.remove('loading-gadget');
> +      evt.target.style.pointerEvents = '';

Because of the loading-gadget class can't we just do that in the CSS file ?

@@ +335,5 @@
> +      evt.target.style.pointerEvents = '';
> +      window.dispatchEvent(new CustomEvent('gadget-loaded'));
> +    }.bind(this));
> +
> +    iframe.addEventListener('mozbrowsererror', function(evt) {

You can probably remove the listener once this has fired.

@@ +337,5 @@
> +    }.bind(this));
> +
> +    iframe.addEventListener('mozbrowsererror', function(evt) {
> +      evt.target.parentNode.classList.remove('loading-gadget');
> +      evt.target.style.pointerEvents = 'auto';

Because of the loading-gadget class can't we just do that in the CSS file ?

@@ +370,5 @@
> +   * Sets the gadget container width.
> +   */
> +  setWidth: function gm_setWidth(){
> +    var width = window.innerWidth * this.addedGadgets.children.length + 'px';
> +    this.addedGadgets.style.width = width;

I don't really understand why we need to set the width ourself. Does it not auto-resize by itself ?

@@ +381,5 @@
> +    var elementToRemove = this.currentGadget;
> +    this.currentGadget = elementToRemove.nextSibling;
> +    if (!this.currentGadget) {
> +      this.currentGadget = elementToRemove.previousSibling;
> +    }

var current = this.currentGadget;
this.currentGadget = current.nextSibling ||
                     current.previousSibling;

@@ +383,5 @@
> +    if (!this.currentGadget) {
> +      this.currentGadget = elementToRemove.previousSibling;
> +    }
> +    this.removeElement(elementToRemove);
> +    this.setEditMode(false);

Do you really want to call setEditMode here ? Sounds like the responsibility of the caller (otherwise the name of the function lies compare to what it does exactly).

@@ +394,5 @@
> +  removeGadget: function gm_removeGadget(gadget) {
> +    var origin = gadget.origin;
> +    var src = origin + gadget.url;
> +
> +    var wrappers = this.addedGadgets.children;

You can probably use a querySelector to get the exact list of widgets to iterate on.

@@ +395,5 @@
> +    var origin = gadget.origin;
> +    var src = origin + gadget.url;
> +
> +    var wrappers = this.addedGadgets.children;
> +    for (var i = 0; i < wrappers.length; i++) {

In general iterating in this direction (i++) is not good when you remove elements on the fly. Because wrappers[i] will not be what you expect. When removing elements from the DOM it is safer to iterate backward.

@@ +418,5 @@
> +      this.container.removeEventListener('touchmove', this);
> +      this.container.classList.add('edit-mode');
> +      if (this.addedGadgets.children.length == this.MAX_GADGETS) {
> +        this.container.classList.add('delete-mode');
> +      }

I still don't understand why we only allow people to uninstall a widget if they have already install MAX_GADGETS of them on the lockscreen ?

As a side note, if we allow that then we may end up into a situation where people delete a widget while the others are still loading, corrupting the widget state saved in the database if they reboot the phone right after.

This race can also probably happens if someone start the lockscreen, and try to add a widget while the lockscreen is loading its sequence of widgets.

@@ +425,5 @@
> +      this.container.addEventListener('touchmove', this);
> +      this.container.classList.remove('edit-mode');
> +      this.container.classList.remove('delete-mode');
> +    }
> +  },

nit: Let's have 2 methods too. enterEditMode and exitExitMode. Boolean parameters usually wants to results into 2 methods.

@@ +433,5 @@
> +   * so that they can be added upon reboot.
> +   */
> +  saveState: function gm_saveState() {
> +    var placedGadgets = {
> +      'current': '',

'' -> this.currentIndex

@@ +439,5 @@
> +    };
> +
> +    var frames = this.addedGadgets.getElementsByTagName('iframe');
> +    for (var i = 0; i < frames.length; i++) {
> +      placedGadgets.gadgets.push(frames[i].getAttribute('src'));

OK. Now it explains why you use the origin. Still feel like manifestURL would be better.

@@ +445,5 @@
> +
> +    placedGadgets.current = this.currentIndex;
> +    asyncStorage.setItem('lockscreen-gadgets', placedGadgets, function() {
> +      // added mainly for testing purposes
> +      this.container.dispatchEvent(new CustomEvent('state-saved'));

Speaking of tests it would be nice to add some to this patch using our Unit Testing framework :)

@@ +450,5 @@
> +    }.bind(this));
> +  },
> +
> +  /**
> +   * Retrieves a list of which gadgets should be placed on lockscreen

nit: Let's be consistent in the comments. Add '.' at the end of each of them.

@@ +460,5 @@
> +        this.gadgetsOnStartup = placedGadgets;
> +      }
> +      this.container.dispatchEvent(new CustomEvent('gadgets-retrieved'));
> +    }.bind(this));
> +  },

I do wonder if having a

GadgetManager.database = {
  add: function(gadget) {
  },

  remove: function(gadget) {
  },

  restore: function(callback) {

    ...
    callback(gadgets);
  }
}

Would not be clearer than saveState/getPlacedGadgets (which dispatch an event).

It would also make it easier for this database object to maintain a copy of the gadget list and make atomic updates in order to avoid the possible corruption I mentioned a few times.

@@ +466,5 @@
> +  /**
> +   * Replaces running gadgets on lockscreen, starting with the last
> +   * shown gadget.
> +   */
> +  addPlacedGadgets: function gm_addPlacedGadgets() {

var gadgets = this.gadgetsOnStartup.gadgets;
if (!gadgets) {
  return;
}

@@ +474,5 @@
> +
> +    var gadgets = this.gadgetsOnStartup.gadgets;
> +    // if we are done adding gadgets
> +    if (gadgets.length === 0) {
> +      if (this.currentGadget){

Add a space between ) and {

@@ +478,5 @@
> +      if (this.currentGadget){
> +        this.saveState();
> +      }
> +      delete this.addingLeft;
> +      delete this.leftGadgets;

The addingLeft/leftGadgets usage in this method is confusing. Can't we just really on the content of |gadgets| in this method ?

@@ +483,5 @@
> +      return;
> +    }
> +
> +    var index = 0;
> +    if (!this.currentGadget){

nit: add a space between ) and {

@@ +505,5 @@
> +      }
> +      window.addEventListener('gadget-loaded', this);
> +      window.addEventListener('gadget-error', this);
> +      this.leftGadgets--;
> +    }

If you can't retrieve the gadget in the GadgetInstallManager, maybe you need to remove it from the database ? Otherwise it will corrupt your database in such a way that you will never load the following gadgets if I correctly read the code. (this situation probably can't happen now but will happen once the lockscreen will be a separated app who can be killed).

@@ +519,5 @@
> +      frames[i].setVisible(visible);
> +    }
> +  }
> +};
> +GadgetManager.init();

I didn't looked yet at setCurrent, the touch events handling or ease in place. But let's iterate on them on the next review.

::: apps/system/locales/system.en-US.properties
@@ +254,4 @@
>  # LOCALIZATION NOTE(panel-passcode.ariaLabel): The following string is spoken by
>  # screen readers and not shown on the screen.
>  panel-passcode.ariaLabel = Passcode panel
> +contextmenu-add-gadget=Add Gadget

Let's create a new category Gadget here.

# Gadgets
contextmenu-add-gadget = Add Gadget

::: apps/system/style/gadgets/gadgets.css
@@ +3,5 @@
> +  bottom: 10rem;
> +  left: 0;
> +  width: 100%;
> +  height: 40vh;
> +  z-index: 1;

Any reason for |z-index: 1| ?

@@ +6,5 @@
> +  height: 40vh;
> +  z-index: 1;
> +}
> +
> +#lockscreen-gadget-scroller{

nit: add a space between scroller and {

@@ +17,5 @@
> +}
> +
> +#lockscreen-gadget-container.delete-mode div#add-gadget {
> +  display: none;
> +}

Is there a chance that we can play with visibility instead of display here ? display will trigger some reflows, so if we have a chance to avoid them :)

@@ +55,5 @@
> +  margin: 0;
> +  position: relative;
> +}
> +
> +.gadget iframe{

nit: add a space between iframe and {

@@ +59,5 @@
> +.gadget iframe{
> +  width: 100vw;
> +  height: 40vh;
> +  border: none;
> +  pointer-events: none;

Do you really want |pointer-events: none;| here ? It means that you won't be able to interact with the gadget directly.

@@ +91,5 @@
> +  }
> +  to {
> +    transform: rotate(360deg);
> +  }
> +}

Any chance we can benefit from the code in shared/style/progress_activity.css ?

@@ +94,5 @@
> +  }
> +}
> +
> +.loading-gadget iframe {
> +  pointer-events: auto;

I would have expect |pointer-events: none;| here fwiw.

::: shared/js/gadget_install_manager.js
@@ +1,1 @@
> +'use strict';

nit: let's move 'use strict'; inside the (function(exports) { })(), at the top.

@@ +1,3 @@
> +'use strict';
> +(function(exports) {
> +var GadgetInstallManager = function GadgetInstallManager(location) {

I wonder if s/GadgetInstallManager is not a bit lying as a name, since this listens for a bit more things.

@@ +8,5 @@
> +  this.init();
> +};
> +
> +GadgetInstallManager.prototype.init = function gim_init() {
> +  this.listInstalledGadgets();

Instead of using your own method, let's add a listener on applicationready, and look at the evt.detail.applications object to have the list of apps.
The reason for this is that a race condition may happens if you try to access mozApps.mgmt too early. By re-using the Applications object you're safe.

@@ +26,5 @@
> +      if (app.manifest.gadgets) {
> +        this.installGadget(app);
> +      }
> +    }.bind(this));
> +    window.dispatchEvent(new CustomEvent('gadgets-installed'));

Once you have iterated over the list of apps, and get all your gadgets, can you fire a gadgetsready event instead of this one ? I would like to keep those events names similar to the one fired from Applications (so gadgetinstall and gadgetuninstall would be good to).

If it has already fired there is a |ready| property on Applications.

@@ +44,5 @@
> +      break;
> +
> +    case 'applicationuninstall':
> +      var origin = this.getOrigin(evt.detail.application.manifestURL);
> +      var gadget = this.gadgetsByOrigin[origin];

It's not clear to me why you don't use the manifestURL directly.

@@ +46,5 @@
> +    case 'applicationuninstall':
> +      var origin = this.getOrigin(evt.detail.application.manifestURL);
> +      var gadget = this.gadgetsByOrigin[origin];
> +      if (gadget) {
> +        this.uninstallGadget(gadget);

Can you directly pass |app| to the this.uninstallGadget method ? That would make installGadget/uninstallGadget more symetric.

@@ +71,5 @@
> +    return;
> +  }
> +
> +  // only pick the first occurring gadget with matching location
> +  var gadget = null;

Let's create a |var gadgets = app.manifest.gadgets;| at the beginning of the method and use that everywhere you use app.manifest.gadgets.

@@ +73,5 @@
> +
> +  // only pick the first occurring gadget with matching location
> +  var gadget = null;
> +  for (var i = 0; i < app.manifest.gadgets.length; i++){
> +    if (!gadget &&

Instead of checking |if (!gadget &&| all the time, you can just call |break;| in the loop once you have found the desired gadget.

@@ +75,5 @@
> +  var gadget = null;
> +  for (var i = 0; i < app.manifest.gadgets.length; i++){
> +    if (!gadget &&
> +        app.manifest.gadgets[i].locations &&
> +        app.manifest.gadgets[i].locations.indexOf(this.location) >= 0){

var locations = gadgets[i].locations;
if (!locations || locations.indexOf(this.location) == -1) {
 continue;
}

gadget = {
  ...
};
break;

Btw can we make a exact match for the locations.indexOf(this.location) ? I would like to avoid people making typos like lockscreennnn to have it working.

@@ +85,5 @@
> +      };
> +    }
> +  }
> +
> +  if (!gadget){

nit: |if (!gadget) {| - notice the additional space.

@@ +112,5 @@
> +
> +  for (var i = 0; i < this.gadgets.length; i++) {
> +    if (this.gadgets[i].origin == origin) {
> +      this.gadgets.splice(i, 1);
> +      window.dispatchEvent(new CustomEvent('gadget-uninstalled', {

s/gadget-uninstalled/gadgetuninstall

@@ +115,5 @@
> +      this.gadgets.splice(i, 1);
> +      window.dispatchEvent(new CustomEvent('gadget-uninstalled', {
> +        detail: gadget
> +      }));
> +    }

You can probably call |break;| once the gadget has been found since from what I understood you can have only one gadget per origin for now.

@@ +132,5 @@
> +      value: i
> +    };
> +    items.push(gadget);
> +  }
> +  return items;

It seems like this method formats the content of the gadgets just to make gadgets_manager.js happy in terms of formatting. I would prefer gadgets_managers to create the necessary array itself.

@@ +153,5 @@
> + */
> +GadgetInstallManager.prototype.getGadgetByIndex =
> +  function gim_getGadgetByIndex(index) {
> +  return this.gadgets[index];
> +};

If the formatting of the array to from getAll if made by the caller, we may not need this method anymore, as the index will depends on gadgets_manager.js.

@@ +166,5 @@
> +    var urlArr = url.split('/');
> +    url = urlArr[0] + '//' + urlArr[2];
> +  }
> +  return url;
> +};

Why can't you use directly the manifestURL here ?

@@ +170,5 @@
> +};
> +
> +  exports.GadgetInstallManager = GadgetInstallManager;
> +
> +}(window));
\ No newline at end of file

The biggest question I have from this file is why do you use a custom origin instead of manifestURL ?

The file also deserves some unit tests (see https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests)

Comment 54

[Olle Klang]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #53)

Thank you for your input Vivien. I just want to clarify some parts of the code:

> ::: apps/system/js/gadget_manager.js    
> @@ +337,5 @@
> > +    }.bind(this));
> > +
> > +    iframe.addEventListener('mozbrowsererror', function(evt) {
> > +      evt.target.parentNode.classList.remove('loading-gadget');
> > +      evt.target.style.pointerEvents = 'auto';
> 
> Because of the loading-gadget class can't we just do that in the CSS file ?

There is the state when you're offline and try to load a hosted apps gadget that is not cashed. This will show an unable to connect screen within the iframe with the button 'tap to check settings'. Removing the 'loading-gadget' will set |pointer-events: none;| from css. This is not what we want as the user will not be able to push the button.
 
> @@ +370,5 @@
> > +   * Sets the gadget container width.
> > +   */
> > +  setWidth: function gm_setWidth(){
> > +    var width = window.innerWidth * this.addedGadgets.children.length + 'px';
> > +    this.addedGadgets.style.width = width;
> 
> I don't really understand why we need to set the width ourself. Does it not
> auto-resize by itself ?

It resizes to the available space but the containing element must extend outside the maximum width of the window. If we don't set the width the gadgets are placed underneath each other. Maybe there some easier way I've missed?

> @@ +418,5 @@
> > +      this.container.removeEventListener('touchmove', this);
> > +      this.container.classList.add('edit-mode');
> > +      if (this.addedGadgets.children.length == this.MAX_GADGETS) {
> > +        this.container.classList.add('delete-mode');
> > +      }
> 
> I still don't understand why we only allow people to uninstall a widget if
> they have already install MAX_GADGETS of them on the lockscreen ?

I admit it's not crystal clear but 'edit-mode' contains both the add and remove button so you can always remove a gadget. Whereas 'delete-mode' only displays the delete button so that you can not add more gadgets. Maybe 'delete-only-mode' would be more precise. 

> @@ +59,5 @@
> > +.gadget iframe{
> > +  width: 100vw;
> > +  height: 40vh;
> > +  border: none;
> > +  pointer-events: none;
> 
> Do you really want |pointer-events: none;| here ? It means that you won't be
> able to interact with the gadget directly.

If we allow direct interaction we cannot scroll between gadgets. This is also the reason why we need the touchforwarder.


I won't be able to spend time on this feature for the time being, but I'll try to get back on it whenever possible.

Comment 55

[jachen]

FYI Gordon

Comment 56

[BMO Automation]

Firefox OS is not being worked on