1100915 - [Usage][Refactoring] Use LazyLoader to load views

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Cost Control, defect)

Description

[Oleg Zasypkin [:azasypkin]]

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).

Comment 1

[Oleg Zasypkin [:azasypkin]]

Attachment: 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!

Comment 2

[Salvador de la Puente González [: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.

Comment 3

[Oleg Zasypkin [:azasypkin]]

(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

Comment 4

[Salvador de la Puente González [:salva]]

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?

Comment 5

[Oleg Zasypkin [:azasypkin]]

(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?

Comment 6

[Salvador de la Puente González [:salva]]

Yes, of course. Two bugs -> to **different** ones. Go with it if you want. Thank you.

Comment 7

[Oleg Zasypkin [:azasypkin]]

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!

Comment 8

[Salvador de la Puente González [:salva]]

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

LGTM, just a couple of nits on GitHub. Thank you!

Comment 9

[Oleg Zasypkin [:azasypkin]]

(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