Create a loader for MarionetteJS Actions

RESOLVED FIXED

Status

Firefox OS
Gaia::System
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kgrandon, Assigned: kgrandon)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
Created attachment 8533447 [details] [review]
[PullReq] KevinGrandon:bug_1108874_marionette_actions to mozilla-b2g:master
(Assignee)

Comment 2

4 years ago
Comment on attachment 8533447 [details] [review]
[PullReq] KevinGrandon:bug_1108874_marionette_actions to mozilla-b2g:master

Etienne - Marionette actions is such a common thing that we use that I think we should probably streamline the inclusion of it, and follow what we're now doing with app libraries.

Let me know what you think, thanks!
Attachment #8533447 - Flags: review?(etienne)
Comment on attachment 8533447 [details] [review]
[PullReq] KevinGrandon:bug_1108874_marionette_actions to mozilla-b2g:master

Makes sense.
I do feel that we should flag each app peer for review, even just as a heads up, to make sure that it's used moving forward.
Attachment #8533447 - Flags: review?(etienne) → review+
(Assignee)

Comment 4

4 years ago
Comment on attachment 8533447 [details] [review]
[PullReq] KevinGrandon:bug_1108874_marionette_actions to mozilla-b2g:master

(In reply to Etienne Segonzac (:etienne) from comment #3)
> I do feel that we should flag each app peer for review, even just as a heads
> up, to make sure that it's used moving forward.

Sounds good. I'm more happy spamming people now that we have a few people on board with this idea.

I think I'm flagging sufficient peers here, and to re-iterate comment 2, marionette actions is such a common thing that we use that I think we should probably streamline the inclusion of it. For other app libraries we've started using a loader plugin which will allow us to load library content in a more consistent way. I do think that the actions library should probably be a marionette plugin, but this is a stepping step to get there.

Thank you all for taking a look.
Attachment #8533447 - Flags: review?(timdream)
Attachment #8533447 - Flags: review?(m)
Attachment #8533447 - Flags: review?(gweng)
Attachment #8533447 - Flags: review?(francisco)
Attachment #8533447 - Flags: review?(arthur.chen)
Comment on attachment 8533447 [details] [review]
[PullReq] KevinGrandon:bug_1108874_marionette_actions to mozilla-b2g:master

From the parts I touched I found that

    this.Actions

actually means the 'constructor', not the instance.
So this would fail:

    + this.Actions = client.loader.getActions();

Either change it as `this.actions` or we remove the line if it's not used in the test files. I've commented on the PR page.
Attachment #8533447 - Flags: review?(gweng)
Comment hidden (obsolete)
(Assignee)

Comment 7

4 years ago
Comment on attachment 8533447 [details] [review]
[PullReq] KevinGrandon:bug_1108874_marionette_actions to mozilla-b2g:master

Greg - Ok, I guess the concern was mostly about the unused parts - I've addressed those now. Thanks for the quick feedback, please take another look when you get a chance.
Attachment #8533447 - Flags: review?(gweng)
Attachment #8533447 - Flags: review?(arthur.chen) → review+
Comment on attachment 8533447 [details] [review]
[PullReq] KevinGrandon:bug_1108874_marionette_actions to mozilla-b2g:master

Agreed with that Greg said and the new patch looks good.
Attachment #8533447 - Flags: review?(timdream) → review+
Hi Kevin: I've left one comment on the PR. If you fix that I think the patch is OK to me.
Flags: needinfo?(kgrandon)
(Assignee)

Comment 10

4 years ago
Thanks Greg. I've changed it to hold the instance at ._actions because the class already holds an .actions object which is a modified actions instance. Please let me know if this is acceptable.
Flags: needinfo?(kgrandon)
Comment on attachment 8533447 [details] [review]
[PullReq] KevinGrandon:bug_1108874_marionette_actions to mozilla-b2g:master

IMO if you can provide the instance, it's no need to keep the constructor. So we don't need name it with '_' prefix. Anyway, it fixed the issue, thanks.
Attachment #8533447 - Flags: review?(gweng) → review+
Attachment #8533447 - Flags: review?(m) → review+
Comment on attachment 8533447 [details] [review]
[PullReq] KevinGrandon:bug_1108874_marionette_actions to mozilla-b2g:master

Thanks Kevin
Attachment #8533447 - Flags: review?(francisco) → review+
(Assignee)

Comment 13

4 years ago
Thanks all for the reviews. In master: https://github.com/mozilla-b2g/gaia/commit/59555eb2364bd23a08c811fa9575a7dffbf4aecd
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.