Closed Bug 1072757 Opened 5 years ago Closed 5 years ago

[System2] Implement SettingsCore

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S6 (10oct)

People

(Reporter: alive, Assigned: alive)

References

Details

Attachments

(1 file, 2 obsolete files)

The SettingsCore as an API wrapper of mozSettingsAPI will provide the following service after it is started.
* SettingsCore#addObserver(NAME, ObserverObject); if the ObserverObject has a "observe" method, it will be invoked every time the value of NAME is changed.
* SettingsCore#read(NAME); read the value of the key given from settings db
* SettingsCore#write(SettingsObject); write the settings to settings db and return a promise.
Assignee: nobody → alive
Target Milestone: --- → 2.1 S6 (10oct)
Attached file v5
* Introduce sandbox System with abilities to register/unregister services.
* Introduce BaseModule
* Introduce Core loader
* Introduce SettingsCore for observe/read/write settings
Attachment #8495214 - Flags: feedback?(etienne)
Just curious, why do we need another API on top of the current Settings API? I'm missing the context for implementing this.
Flags: needinfo?(alive)
(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #2)
> Just curious, why do we need another API on top of the current Settings API?
> I'm missing the context for implementing this.

We had already a library to help us to access mozSettings for a long time.
Please grep SettingsListener in code base.
But I feel it's still insufficient so I improve it by using Promise and provide an observer interface.
Flags: needinfo?(alive)
So why do we still need that library? Why not just remove it and use the settings API? I'm not seeing anything really special that it does.
Flags: needinfo?(alive)
(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #4)
> So why do we still need that library? Why not just remove it and use the
> settings API? I'm not seeing anything really special that it does.

Please read comment 3. It's hard to use in my case, and exactly we have more than one libraries to deal with settings API: SettingsHelper, https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard/settings.js#L7, SettingsListener.

I don't understand why this is a problem.
Flags: needinfo?(alive)
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5)
> (In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment
> #4)
> > So why do we still need that library? Why not just remove it and use the
> > settings API? I'm not seeing anything really special that it does.
> 
> Please read comment 3. It's hard to use in my case, and exactly we have more
> than one libraries to deal with settings API: SettingsHelper,
> https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard/
> settings.js#L7, SettingsListener.
> 
> I don't understand why this is a problem.

The library was created when we were missing certain features (observers) in the API. What use-cases are missing in the API? If we can, we should fix the existing API and avoid building an API on top of an API.
(In reply to Gregor Wagner [:gwagner] from comment #6)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5)
> > (In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment
> > #4)
> > > So why do we still need that library? Why not just remove it and use the
> > > settings API? I'm not seeing anything really special that it does.
> > 
> > Please read comment 3. It's hard to use in my case, and exactly we have more
> > than one libraries to deal with settings API: SettingsHelper,
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard/
> > settings.js#L7, SettingsListener.
> > 
> > I don't understand why this is a problem.
> 
> The library was created when we were missing certain features (observers) in
> the API. What use-cases are missing in the API? If we can, we should fix the
> existing API and avoid building an API on top of an API.

* We need promise instead of DOM request.
* I want something like

mozSettings2.addObserver('lockscreen.enabled', this);
this.observe = function(name, value) {
}


BTW I am really surprised. I proposed many API change stuff to dev-webapi and nearly none of them is taken care but now see what happens :)
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #7)
> (In reply to Gregor Wagner [:gwagner] from comment #6)
> > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5)
> > > (In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment
> > > #4)
> > > > So why do we still need that library? Why not just remove it and use the
> > > > settings API? I'm not seeing anything really special that it does.
> > > 
> > > Please read comment 3. It's hard to use in my case, and exactly we have more
> > > than one libraries to deal with settings API: SettingsHelper,
> > > https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard/
> > > settings.js#L7, SettingsListener.
> > > 
> > > I don't understand why this is a problem.
> > 
> > The library was created when we were missing certain features (observers) in
> > the API. What use-cases are missing in the API? If we can, we should fix the
> > existing API and avoid building an API on top of an API.
> 
> * We need promise instead of DOM request.
> * I want something like
> 
> mozSettings2.addObserver('lockscreen.enabled', this);
> this.observe = function(name, value) {
> }
> 
> 
> BTW I am really surprised. I proposed many API change stuff to dev-webapi
> and nearly none of them is taken care but now see what happens :)

To clarify, I didn't mean we should not make API better; but we are having lots of use cases that we want to provide a more semantic interface on top of API.

* window.SIMSlotManager.isMultiSIM() means there are more than one navigator.mozMobileConnection instances. Is it possible to make API provide this 'semantic sugar'? If so, and if you want to implement it for me, I am glad to use it and deprecate it.
* window.applications.getAppByManifest will seek the cache of the mozApps objects by the manifestURL given. If we didn't cache it and seek the mozApps.mgmt.getAll() each time we want to know, the performance will be slowed down much more than now. Is API possible to cache it and give me a synchronous callback?
* We want to get the same settings lock each time we access mozSettings. Now it doesn't always give us the lock it had created but we need to hold the lock on our own https://github.com/mozilla-b2g/gaia/blob/master/shared/js/settings_listener.js#L28
* window.AppWindow is totally a mozBrowser API wrapper. Is it possible not to create such a helper class but |document.create('iframe'); // ignoring 2000 lines....| every time we want to create an app window? (and supports modal dialog, authentication dialog, browser context menu, and all native mozbrowser events)

IMO API is something will/should provide basic usage to certain specific stuff/topic; I don't see there is a rule to forbid frontend dev to create any library upon current API. Is there?
Alive: I'm sorry if we've missed suggestions for API improvements from you. Input from developers like yourself is what should drive the design of our APIs. That said, sometimes there's disagreements about what makes a good API.

However in this case I agree that the API in comment 0 is better than what we currently have. In part because Promises are clearly better than DOMRequest, in part because I think that the transactional aspect of the Settings API was most likely a mistake.

But I don't think this bug is the place to discuss changes to the APIs. Maybe we should start a thread on dev-b2g?
(In reply to Jonas Sicking (:sicking) from comment #9)
> Alive: I'm sorry if we've missed suggestions for API improvements from you.
> Input from developers like yourself is what should drive the design of our
> APIs. That said, sometimes there's disagreements about what makes a good API.
> 
> However in this case I agree that the API in comment 0 is better than what
> we currently have. In part because Promises are clearly better than
> DOMRequest, in part because I think that the transactional aspect of the
> Settings API was most likely a mistake.
> 
> But I don't think this bug is the place to discuss changes to the APIs.
> Maybe we should start a thread on dev-b2g?

Ya, I am also curious why I am challenged here in a gaia bug.
I will propose it to dev-webapi if everyone thinks we should, but that won't block my work to cleanup system app!
Comment on attachment 8495214 [details]
v5

Change to review request since basic unit tests are added.
Attachment #8495214 - Flags: feedback?(etienne) → review?(etienne)
Comment on attachment 8495214 [details]
v5

The scope of this patch is really good, it was a huge help in order to reason about it as a whole.
The changes I'm suggesting might sound "big" but it's mainly moving/renaming code. My goal is to make sure we discuss this thoroughly while it's easy to change.

* * *

First and foremost the BaseModule content. I think it's telling that our very first BaseModule-based module doesn't actually use much of the BaseModule code. My suggestion would be to extract the SETTINGS part and the EVENTS part out in 2 different mixins optionally added to a BaseModule-based module. And keep the BaseModule content as: sub-module supports, lifetime management and service registration.

This opens up the possibility to move the SettingsCore logic inside the SETTINGS mixin, saving us a module. But I understand if we want to keep the SettingsCore for queuing/startup reasons (although I don't think the API availability check would be an argument since we surely won't ship devices without mozSettings).

* * *

About tests: the coverage is good but I want to rework the structure. From our experience with the WindowManager I know that if the test file is not granular enough we just add extra cases in the middle of an existing test when fixing a bug and it gets out of hand quickly. This is why I'd like to split them in sub-suites and have clear names for what's being tested. For example the SETTINGS mixin test suite should have a "reading" a "writing" and a "listening" sub-suite with multiple tests inside.

* * *

The relationship between bootstrap.js, core.js and system.js is getting fuzzier and fuzzier. I wouldn't know easily which file I should touch when working on a bug. I think it's mainly a naming issue. I like the semantic of "core" but keeping it in window.systemApp looks kind of weird (maybe just window.core). And I think system.js should be rename to something to the like of service.js now that it's basically our service exchange where modules meet.

* * *

Not sure what all the use cases are for the _pre/_post handlers so maybe I'm missing something. But 
- I'm not sure we need them for the SETTINGS mixin 
- I think we might want a way to "prevent" the invocation of the handler from _pre_handleEvent (maybe with a return value or a flag passed to each function).

I suspect we might also have good use cases for a |listToNext(evtName, cb)| method adding a listener and removing it once we got the event.

* * *

This is really the fundamental brick for the next system refactoring phase. This is why I'd rather "over-discuss" now since everything we'll emerge from here.

On that note also flagging Kevin for feedback to get an extra pair of eyes on the design and Arthur to comment on the Settings part since (iirc) he wrote the original SettingsHelper.
Attachment #8495214 - Flags: review?(etienne)
Attachment #8495214 - Flags: feedback?(kgrandon)
Attachment #8495214 - Flags: feedback?(arthur.chen)
Comment on attachment 8495214 [details]
v5

I left a few notes on github, I'm sure there's more things that I could comment on. If this were up to me, I would opt to transpile the system JS and use harmony modules. I think that this would help future-proof the system app, prevent additional refactoring down the road (e.g., systemV3), and setup a nice convention for us to use in regards to declaring dependencies and interfaces.

I would urge you to think about leveraging harmony modules via transpiling, and at least prototyping it before deciding on a direction. If you have a good reason not to though, I will understand and can continue to provide feedback here if desired.
Attachment #8495214 - Flags: feedback?(kgrandon)
(In reply to Kevin Grandon :kgrandon from comment #13)
> Comment on attachment 8495214 [details]
> v1, incomplete tests.
> 
> I left a few notes on github, I'm sure there's more things that I could
> comment on. If this were up to me, I would opt to transpile the system JS
> and use harmony modules. I think that this would help future-proof the
> system app, prevent additional refactoring down the road (e.g., systemV3),
> and setup a nice convention for us to use in regards to declaring
> dependencies and interfaces.
> 
> I would urge you to think about leveraging harmony modules via transpiling,
> and at least prototyping it before deciding on a direction. If you have a
> good reason not to though, I will understand and can continue to provide
> feedback here if desired.

The reason is simple:
* Harmony has limited feature ready now - see https://bugzilla.mozilla.org/show_bug.cgi?id=568953
* "Compiling" is something interesting - it may hide some bugs from us between the code we write and the code we generate. I think that's why we don't use CoffeeScript/LiveScript.

I could start to study the way to compile, but this should not block us from moving forward.
Why do you feel that transpiling would hide bugs?

I too think that using ES6 modules would bring a lot of benefits. It's a module system that people has spent a ton of time designing, and it's where the web platform is heading so it's a format that a lot of other code will be using. That both makes it easier to on-board new developers, and makes it easier to reuse existing externally written code.
(In reply to Etienne Segonzac (:etienne) from comment #12)
> Comment on attachment 8495214 [details]
> v1, incomplete tests.
> 
> The scope of this patch is really good, it was a huge help in order to
> reason about it as a whole.
> The changes I'm suggesting might sound "big" but it's mainly moving/renaming
> code. My goal is to make sure we discuss this thoroughly while it's easy to
> change.
> 
> * * *
> 
> First and foremost the BaseModule content. I think it's telling that our
> very first BaseModule-based module doesn't actually use much of the
> BaseModule code. My suggestion would be to extract the SETTINGS part and the
> EVENTS part out in 2 different mixins optionally added to a BaseModule-based
> module. And keep the BaseModule content as: sub-module supports, lifetime
> management and service registration.
> 
> This opens up the possibility to move the SettingsCore logic inside the
> SETTINGS mixin, saving us a module. But I understand if we want to keep the
> SettingsCore for queuing/startup reasons (although I don't think the API
> availability check would be an argument since we surely won't ship devices
> without mozSettings).

The main reason of baking a Settings API Helper is the short supply of the mozSettings API.
If we have the desired Settings Observer API, we don't need it.
I agree the mixin part change. But put SettingsCore implementation inside a mixin seems weird - especially it's coming from the baseModule too. But I know what we currently have doesn't be better.

> 
> * * *
> 
> About tests: the coverage is good but I want to rework the structure. From
> our experience with the WindowManager I know that if the test file is not
> granular enough we just add extra cases in the middle of an existing test
> when fixing a bug and it gets out of hand quickly. This is why I'd like to
> split them in sub-suites and have clear names for what's being tested. For
> example the SETTINGS mixin test suite should have a "reading" a "writing"
> and a "listening" sub-suite with multiple tests inside.

I try to understand what you want but seem not easy for me. But yes, now writing a test for classes inherited from BaseModule may be not intuitive. I need to think more but appreciate if you have hints, code snippets for this part.

> 
> * * *
> 
> The relationship between bootstrap.js, core.js and system.js is getting
> fuzzier and fuzzier. I wouldn't know easily which file I should touch when
> working on a bug. I think it's mainly a naming issue. I like the semantic of
> "core" but keeping it in window.systemApp looks kind of weird (maybe just
> window.core). 

Works for me, don't really care how it is named as an instance because core instance should not be accessed by anymore. (Maybe we will not attach it to window but this will increase the difficulty to debug.)

And I think system.js should be rename to something to the
> like of service.js now that it's basically our service exchange where
> modules meet.

Renaming works for me. But we need to be careful because change this name means change everything in the system app because it's accessed by globalObject.

One of the option I didn't implement is always providing a sandbox reference while instantiating a new module via baseModule:
var a = new MyModule(parentModuleInstance, ..... , serviceSandbox);

And we should not use System.create but BaseModule.create in this case.

> 
> * * *
> 
> Not sure what all the use cases are for the _pre/_post handlers so maybe I'm
> missing something. But 

The reason is, in the progress of refactoring everything,
I found there are several handlers in a module doing the same thing but different from evt.type.
In this case I tend to think a global handler is more suitable without writing _handle_xxxx 10 times.

> - I'm not sure we need them for the SETTINGS mixin 
> - I think we might want a way to "prevent" the invocation of the handler
> from _pre_handleEvent (maybe with a return value or a flag passed to each
> function).

// How about this?
if (!the._pre_handleEvent(evt)) {
  return;
}

> 
> I suspect we might also have good use cases for a |listToNext(evtName, cb)|
> method adding a listener and removing it once we got the event.

Does BaseModule.ONCE = [...] sound good?
or to provide BaseModule.prototype.once also works for me.

> 
> * * *
> 
> This is really the fundamental brick for the next system refactoring phase.
> This is why I'd rather "over-discuss" now since everything we'll emerge from
> here.

Good catch and nice feedback!

> 
> On that note also flagging Kevin for feedback to get an extra pair of eyes
> on the design and Arthur to comment on the Settings part since (iirc) he
> wrote the original SettingsHelper.
(In reply to Jonas Sicking (:sicking) from comment #15)
> Why do you feel that transpiling would hide bugs?
> 
> I too think that using ES6 modules would bring a lot of benefits. It's a
> module system that people has spent a ton of time designing, and it's where
> the web platform is heading so it's a format that a lot of other code will
> be using. That both makes it easier to on-board new developers, and makes it
> easier to reuse existing externally written code.

I love Javascript because what I write is what I get. I am not sure
* Is there a mozilla trusted tool to transpile?
* Is there no side effect? For example, extra generated codes?

I agree your second paragraph. I watched the harmony module bug from 3 years ago. I hope it is ready now, but it seems not. I don't have the knowledge about transpiling tools now so I cannot consult, but what I am worry about is I don't way to write *another script* without knowing what the tool does for me.
Comment on attachment 8495214 [details]
v5

The patch seems trying to replace `SettingsListener` instead of `SettingsHelper`. I'm not the author of `SettingsListener` but IIUIC one of the purpose of it is to serve the scenario where you would like the callback to be called upon the registration of it. For example, a function that updates an UI element is called when we initialize the element and when the corresponding value changes. I'm fine with the removal the use case as we can simply called the function manually.

The `addObserver` function only accepts an object with a `observe` method and which seems too restricted. It would be better to add the support of using a function as it seems a general use case.

The rest of the part of SettingsCore looks good to me.
Attachment #8495214 - Flags: feedback?(arthur.chen) → feedback+
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #14)
> I could start to study the way to compile, but this should not block us from
> moving forward.

Because this is such an important piece, I only suggest doing the due diligence in determining if a modules build solution would be the right one. In order to do this I would recommend prototyping a simple system library, transpiled into harmony modules. I would recommend prototyping modules use before landing this, in order to ensure that this is the right solution, and something that we won't have to revisit. I'm more than happy to help out in prototyping this and will begin on working on something in parallel to your efforts here. I also don't want you to be blocked on this though, so feel free to get reviews and land this if you think it's the right thing.

I am also not too familiar with harmony modules, but I think we will need a few things:

1 - A few simple modules which call import {} and export{} on libraries.
2 - A build step to transpile these modules into javascript, possibly using traceur [1] or similar.
3 - Some module loader polyfill[2].

[1] https://www.npmjs.org/package/traceur
[2] https://github.com/ModuleLoader/es6-module-loader
Attached file Harmony Modules Example - Template App (obsolete) —
Here is a very basic example of the template app leverage transpiled harmony modules.
Here is a very basic example of the system app leveraging transpiled harmony modules to pull in a dependency of the statusbar.

This also lets us use very clean class syntax like: https://github.com/KevinGrandon/gaia/blob/bug_1072757_harmony_module_polyfill_system_app/apps/system/js/clock.js#L24

Alive - I would just like to get your feedback here on this very basic implementation. This does not have any of the necessary build steps yet to transpile inside of build_stage, but it should be rather trivial. I'm not too worried about the compiled nature of things as we already do it for l10n and concatenation purposes. Please let me know what you think. If you're not a fan, I'll just close these pull requests and go away.
Attachment #8497306 - Flags: feedback?(alive)
Comment on attachment 8497306 [details] [review]
Harmony Modules Example - [System] Statusbar/clock implementation

For sure I prefer the clock.js "look".
* How do we test a harmony-like javascript file in unit test?
* How do we debug the generated javascript?

Again, if we are having a trusted library / build flag for traceur, I could try to use it later in followup, but this takes some time and should be parallel to this bug.

Could you open another bug for introduce build support for traceur/es6-module-loader in system?
Attachment #8497306 - Flags: feedback?(alive)
Comment on attachment 8495214 [details]
v5

Updated:
* Rename _servers/clients in System to providers/consumers
* Use BaseModule.create instead of System.create
* Have SettingMixin/EventMixin and only inject them if there is SETTINGS/EVENTS defined in constructor.
* rename window.systemApp to window.core

To do:
* Rename lowerCapital (minor issue)

To confirm:
* Do we really need to rename the whole System to Service? Is this still necessary if we don't have window.systemApp?
* Do we really need to deprecate exports in this bug? (See github comments)
* The test issues raised by etienne.
Attachment #8495214 - Attachment description: v1, incomplete tests. → v2
Attachment #8495214 - Flags: feedback?(kgrandon)
Attachment #8495214 - Flags: feedback?(etienne)
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #22)
> Comment on attachment 8497306 [details] [review]
> Harmony Modules Example - [System] Statusbar/clock implementation
> 
> Could you open another bug for introduce build support for
> traceur/es6-module-loader in system?

My only concern is that landing this sets us down a path that makes it harder to introduce harmony modules. I'll obsolete the attachments, and move them to another bug for discussion though.
Attachment #8497304 - Attachment is obsolete: true
Attachment #8497306 - Attachment is obsolete: true
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #16)
> > About tests: the coverage is good but I want to rework the structure. From
> > our experience with the WindowManager I know that if the test file is not
> > granular enough we just add extra cases in the middle of an existing test
> > when fixing a bug and it gets out of hand quickly. This is why I'd like to
> > split them in sub-suites and have clear names for what's being tested. For
> > example the SETTINGS mixin test suite should have a "reading" a "writing"
> > and a "listening" sub-suite with multiple tests inside.
> 
> I try to understand what you want but seem not easy for me. But yes, now
> writing a test for classes inherited from BaseModule may be not intuitive. I
> need to think more but appreciate if you have hints, code snippets for this
> part.

I was actually talking about the |base_module_test.js| itself.
To illustrate let's take the |test('Event handler', ...| if it breaks it wont tell me much except that the event handling part is broken.
And if I fix an edge case I'll probably add it randomly inside the test. This test will not age well.

Now if we had something like
* suite('Centralized event handler'...
  + should trigger _pre_handler if available
  + should trigger handler if available
  + should trigger _post_handler if available
  + should not trigger handler if _pre_handler returned false...
  + should not trigger anything for an event not included in EVENTS...

This will age much better :)

> And I think system.js should be rename to something to the
> > like of service.js now that it's basically our service exchange where
> > modules meet.
> 
> Renaming works for me. But we need to be careful because change this name
> means change everything in the system app because it's accessed by
> globalObject.

Yeah definitely postponable to a follow-up :)

> > - I'm not sure we need them for the SETTINGS mixin 

What about this part? I don't think we need _pre_observer/_post_observe

> > - I think we might want a way to "prevent" the invocation of the handler
> > from _pre_handleEvent (maybe with a return value or a flag passed to each
> > function).
> 
> // How about this?
> if (!the._pre_handleEvent(evt)) {
>   return;
> }

Yes that was my idea.

> 
> > 
> > I suspect we might also have good use cases for a |listToNext(evtName, cb)|
> > method adding a listener and removing it once we got the event.
> 
> Does BaseModule.ONCE = [...] sound good?
> or to provide BaseModule.prototype.once also works for me.

The use-cases I have in mind are more of the |BaseModule.prototype.once| type, but let's keep that for later when we're actually converting one of those modules.
 > > I try to understand what you want but seem not easy for me. But yes, now
> > writing a test for classes inherited from BaseModule may be not intuitive. I
> > need to think more but appreciate if you have hints, code snippets for this
> > part.
> 
> I was actually talking about the |base_module_test.js| itself.
> To illustrate let's take the |test('Event handler', ...| if it breaks it
> wont tell me much except that the event handling part is broken.
> And if I fix an edge case I'll probably add it randomly inside the test.
> This test will not age well.
> 
> Now if we had something like
> * suite('Centralized event handler'...
>   + should trigger _pre_handler if available
>   + should trigger handler if available
>   + should trigger _post_handler if available
>   + should not trigger handler if _pre_handler returned false...
>   + should not trigger anything for an event not included in EVENTS...
> 
> This will age much better :)

The |system_test.js|' "Services" suite that you just wrote is also a good example of what I'd like to see for |base_module_test.js| :)
Comment on attachment 8495214 [details]
v5

f+ since most of my open issues are about tests

Don't have a strong opinion on the harmony part of the debate but unifying the shape of our system module (and their interactions) today should help us switch to harmony tomorrow right? Or are we actively making the future transition worse with this patch?
I think the "service" part of Alive's patch will be needed for *a while* because we have tons of edge-cases with APIs that might be there or not, might be initialized or not, might be ready or not etc... I don't think @imports alone will get us there. But I'm open to being proved wrong :)
Attachment #8495214 - Flags: feedback?(etienne) → feedback+
Comment on attachment 8495214 [details]
v5

You have two feedback+ here, so I don't think mine is really needed. My thoughts are still that this will make it more work to switch over to ES6 modules, and that's the direction we should be going. I don't want to block you though, so I'll get down from my ES6 module soapbox and let you do your thing :)
Attachment #8495214 - Flags: feedback?(kgrandon)
(In reply to Kevin Grandon :kgrandon from comment #24)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #22)
> > Comment on attachment 8497306 [details] [review]
> > Harmony Modules Example - [System] Statusbar/clock implementation
> > 
> > Could you open another bug for introduce build support for
> > traceur/es6-module-loader in system?
> 
> My only concern is that landing this sets us down a path that makes it
> harder to introduce harmony modules. I'll obsolete the attachments, and move
> them to another bug for discussion though.

It's even harder to refactor "current system app" to harmony like.

Lemme fight for my spirit again:
* What tool we use does not matter what kind of architecture we are going to have.
* Tools/language/library features are just something "help", but the something decides what we do.
  I could even use es6 feature to write a bunch of dirty codes:
  1. non-clear module responsibility and division
  2. No clear loading strategy
  3. 2000 line in one function

What I hope to see is each module inside system app is having a clear scope and simple functionality and share functionalities as most as possible and be standalone operable; this should not be restricted by any language feature. If harmony makes us - makes all prototype-based javascript in the world - hard to transfer, why is this an issue? Could you prove these thoughts is in the opposite direction of harmony language?

For new developer facing, if I am a new comer, even I rewrites all current javascript files inside system into harmony style,
there is still nothing changed. The code looks nice but the architecture is still non-architecture. Still coupling responsibility between modules, still 2000 line statusbar.js/window_manager.js, still unknown loading managements.

Again, language feature is pretty good; if you are able to work out a trustable and simple-use transpiling tools I am willing to transfer all modules which are based on baseModule to be harmony stuff, I promise. But saying this work violates harmony spirit is something too heavy.
(In reply to Etienne Segonzac (:etienne) from comment #25)
> 
> > And I think system.js should be rename to something to the
> > > like of service.js now that it's basically our service exchange where
> > > modules meet.
> > 
> > Renaming works for me. But we need to be careful because change this name
> > means change everything in the system app because it's accessed by
> > globalObject.
> 
> Yeah definitely postponable to a follow-up :)
> 
> > > - I'm not sure we need them for the SETTINGS mixin 
> 
> What about this part? I don't think we need _pre_observer/_post_observe

My debate is there in comment 16.

"The reason is, in the progress of refactoring everything,
I found there are several handlers in a module doing the same thing but different from evt.type.
In this case I tend to think a global handler is more suitable without writing _handle_xxxx 10 times."

Since there is nobody using it, I could remove them if you think this is a bad idea.
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #30)
> (In reply to Etienne Segonzac (:etienne) from comment #25)
> > 
> > > And I think system.js should be rename to something to the
> > > > like of service.js now that it's basically our service exchange where
> > > > modules meet.
> > > 
> > > Renaming works for me. But we need to be careful because change this name
> > > means change everything in the system app because it's accessed by
> > > globalObject.
> > 
> > Yeah definitely postponable to a follow-up :)
> > 
> > > > - I'm not sure we need them for the SETTINGS mixin 
> > 
> > What about this part? I don't think we need _pre_observer/_post_observe
> 
> My debate is there in comment 16.
> 
> "The reason is, in the progress of refactoring everything,
> I found there are several handlers in a module doing the same thing but
> different from evt.type.
> In this case I tend to think a global handler is more suitable without
> writing _handle_xxxx 10 times."

I mean in some modules, _observe_xxxx(value) is doing the same thing for several settings value by settings name. Does _pre_observe sound not the same as _pre_handleEvent and we should be consistent?

> 
> Since there is nobody using it, I could remove them if you think this is a
> bad idea.
Comment on attachment 8495214 [details]
v5

* Remove pre/post observer
* Add canceling mechanism in preHandleEvent
* Test cases more
* Remove the lwm hack since we have regular request now.
Attachment #8495214 - Attachment description: v2 → v3
Attachment #8495214 - Flags: review?(etienne)
(In reply to Kevin Grandon :kgrandon from comment #28)
> My thoughts are still that this will make it more work to switch over to ES6 modules

I'd really like to understand more about this and how it can be mitigated without bringing shims and transpilers into a patch that we worked so hard to keep small and manageable :)
(In reply to Etienne Segonzac (:etienne) from comment #33)
> I'd really like to understand more about this and how it can be mitigated
> without bringing shims and transpilers into a patch that we worked so hard
> to keep small and manageable :)

It's mainly the module implementation in the pull request that we would have to update to use harmony modules. Our own homegrown registration flow and way that we're lazy loading probably wouldn't work too well with harmony modules. It's nothing we couldn't fix, but it's certainly more work if harmony modules is a thing that people want to use in the future.

But Alive is right, it would be a lot more work than what's here to properly implement harmony modules in the system app today.

The settings work here is interesting. I would prefer to find a way to iterate on settings_listener.js in shared/ so we can benefit all apps and try to have some consistency across gaia. It's much more work to do this in a way to use cross-app though I suppose.
Comment on attachment 8495214 [details]
v5

Comments on github (many but simple ones :)), also flagging Greg for review for the lockscreen part (which doesn't come with tests :/)
Attachment #8495214 - Flags: review?(etienne) → review?(gweng)
Comment on attachment 8495214 [details]
v5

Yes, no test is the major problem, and I notice there are some LockScreen Gij failures on Gaia-Try (thus I cancel the review, like what the other reviewers would do when my patch failed at the similar cases).

Other parts are good as Alive and me discussed and Etienne commented. I think we can add a simple test to ensure the System.register get called, and while the current '-request-unlock' event has test without touch 'responseUnlock/unlock', we may be able to test this. But I don't know for most of reviewers, to make unit test coupled with implementation is good or not.

And please note that I only review the LocKScreen part as the comment above. Since the major reviewer is not me and the patch has been reviewed.

By the way, if you need my help to solve the Gij failures, please NI me, thanks.
Attachment #8495214 - Flags: review?(gweng)
Comment on attachment 8495214 [details]
v5

Etienne: tried to address as most as your comments, please take a look.
Greg: I am sure |System.request('unlock', { forcibly: true });| does work on real device so don't know why timeout still happen, please help out.
Attachment #8495214 - Flags: review?(gweng)
Attachment #8495214 - Flags: review?(etienne)
Comment on attachment 8495214 [details]
v5

Looks like the gij failure is due to the active app window is not brought to foreground while |System.request('unlock');| is executed. Investigating.
Attachment #8495214 - Flags: review?(gweng)
Attachment #8495214 - Flags: review?(etienne)
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #38)
> Comment on attachment 8495214 [details]
> v3
> 
> Looks like the gij failure is due to the active app window is not brought to
> foreground while |System.request('unlock');| is executed. Investigating.

The problem is coming from |lockscreen-request-unlock| is not fired when LockscreenWindowManager.prototype.unlock is directly invoked; hence VisibilityManager does not know it should tell AppWindowManager to show the active window.
Comment on attachment 8495214 [details]
v5

I think I'd fixed the gij failure.
Attachment #8495214 - Flags: review?(gweng)
Attachment #8495214 - Flags: review?(etienne)
Attachment #8495214 - Attachment description: v3 → v4
Comment on attachment 8495214 [details]
v5

For LockScreen parts it's review+ with nits. Thanks.
Attachment #8495214 - Flags: review?(gweng) → review+
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #17)
> (In reply to Jonas Sicking (:sicking) from comment #15)
> I love Javascript because what I write is what I get. I am not sure
> * Is there a mozilla trusted tool to transpile?

This is a good question. I don't think there are any "officially sanctioned" ones, but I suspect there are ones that are quite good and actively maintained. I can check with Dave Herman (responsible for the module spec) for recommendations.

> * Is there no side effect? For example, extra generated codes?

Good question. Probably depends on what transpiler that we use.

> I agree your second paragraph. I watched the harmony module bug from 3 years
> ago. I hope it is ready now, but it seems not. I don't have the knowledge
> about transpiling tools now so I cannot consult, but what I am worry about
> is I don't way to write *another script* without knowing what the tool does
> for me.

Makes sense. I see the downsides of using a transpiler. But it also seems like there are upshots to it. The question is if the downsides or the upsides are bigger.
Comment on attachment 8495214 [details]
v5

Updated:
* Move service registration functions into a mixin.
* Move submodule management functions into a mixin.
* More structured unit tests for baseModule.
Comment on attachment 8495214 [details]
v5

Hmm, canceling at first because another issue was found (settingsCore is not loaded now)
Attachment #8495214 - Flags: review?(etienne)
Comment on attachment 8495214 [details]
v5

* System.lazyLoad now returns promise as well.
* Core.startAPIHandler will not recursively load now.
* EventMixin/SettingMixin/ServiceMixin/SubmoduleMixin are departed from BaseModule now.
* Enhance tests
Attachment #8495214 - Attachment description: v4 → v5
Attachment #8495214 - Flags: review?(etienne)
Comment on attachment 8495214 [details]
v5

r=me with the 2 last comments on github addressed.
Attachment #8495214 - Flags: review?(etienne) → review+
Waiting tbpl, it seems stop working now.
https://github.com/mozilla-b2g/gaia/commit/0c158a964b40b88727ab6792a03e91bc6fdc105b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.