[Usage][Refactoring] Use LazyLoader to load views

RESOLVED FIXED

Status

Firefox OS
Gaia::Cost Control
--
minor
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: azasypkin, Assigned: azasypkin)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

If we use LazyLoader to load panels we'll have the following benefits:

* it allows us to remove some duplicated code (html activation) that's already performed by LazyLoader;

* LazyLoader correctly embeds HTML imports for debug _desktop_ builds that don't use webapp-optimize.js and based on original sources. This should help us to run Usage/CostControl app inside Mulet (see bug 1096898).
(Assignee)

Comment 1

3 years ago
Created attachment 8524646 [details] [review]
GitHub pull request URL

Hey Salvador,

Here is extraction from patch for bug 1096898 + I've tried to add few unit tests for ViewManager.loadPanel.

Please, tell me what you think.

Thanks!
Attachment #8524646 - Flags: review?(salva)
Comment on attachment 8524646 [details] [review]
GitHub pull request URL

Looks great but address the comments on GitHub and see if we could get rid of the activation process. Ask for my review when you're done. Thank you very much for the effort and please, share your thoughts on GitHub.
Attachment #8524646 - Flags: review?(salva)
(Assignee)

Comment 3

3 years ago
(In reply to Salvador de la Puente González [:salva] from comment #2)
> Comment on attachment 8524646 [details] [review]
> GitHub pull request URL
> 
> Looks great but address the comments on GitHub and see if we could get rid
> of the activation process. Ask for my review when you're done. Thank you
> very much for the effort and please, share your thoughts on GitHub.

Thanks for review! I've replied on GitHub (+ one question that I'd like to clarify before asking for review again). 

Regarding to activation: we still need it because as per HTML 5 spec script that is inserted via innerHTML isn't executed [1].

[1] http://dev.w3.org/html5/spec-LC/scripting-1.html#the-script-element
Thanks for the replies! I noticed two bugs in the load activation process. One is what you detected about the callback not being called is the resources are already activated. Let's file a separated bug, write a regression test catching the bug and solve it. The other is a matter of polish: I think we could simply move the nodes for activation or, at least, remove the original node once we've attached the node to the head. What do you think?
Flags: needinfo?(azasypkin)
(Assignee)

Comment 5

3 years ago
(In reply to Salvador de la Puente González [:salva] from comment #4)
> Thanks for the replies! I noticed two bugs in the load activation process.
> One is what you detected about the callback not being called is the
> resources are already activated. Let's file a separated bug, write a
> regression test catching the bug and solve it.

Sure, will file a bug + unit test (then I won't check for duplicated resources in unit tests for this bug, since it won't pass without fix :))

>The other is a matter of polish: I think we could simply move the nodes for activation or, at least,
> remove the original node once we've attached the node to the head. What do
> you think?

Yeah, I think we should definitely try to do this, but I think it's a good candidate for the separate bug, don't you mind?
Flags: needinfo?(azasypkin) → needinfo?(salva)
Yes, of course. Two bugs -> to **different** ones. Go with it if you want. Thank you.
Flags: needinfo?(salva)
(Assignee)

Updated

3 years ago
Blocks: 1102168
(Assignee)

Updated

3 years ago
Blocks: 1102171
(Assignee)

Comment 7

3 years ago
Comment on attachment 8524646 [details] [review]
GitHub pull request URL

Okay, I believe it's ready for the second round of review, I added changes to a separate commit to easier track what has changed.

I also filed follow-ups: bug 1102168 and bug 1102171. I'll take care of those a bit later too. For now my main priority is bug 1096898 :)

Thanks!
Attachment #8524646 - Flags: review?(salva)
Comment on attachment 8524646 [details] [review]
GitHub pull request URL

LGTM, just a couple of nits on GitHub. Thank you!
Attachment #8524646 - Flags: review?(salva) → review+
(Assignee)

Comment 9

3 years ago
(In reply to Salvador de la Puente González [:salva] from comment #8)
> Comment on attachment 8524646 [details] [review]
> GitHub pull request URL
> 
> LGTM, just a couple of nits on GitHub. Thank you!

Thanks for review! Nits fixed and Treeherder is green.

Master: https://github.com/mozilla-b2g/gaia/commit/aa1cf32a3e1e68703826d07a963de37082a40501
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.