[Cost Control] Consider having DOM nodes imported from standalone files.

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kgrandon, Assigned: kgrandon)

Tracking

({perf})

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, b2g-v1.2 fixed)

Details

(Whiteboard: [c= p=2 s=2013.10.25 u=])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
The app currently lazy loads the DOM via comments inside of HTML files. It should be possible to use our new import polyfill to have a cleaner dom.
(Assignee)

Updated

5 years ago
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
Whiteboard: [c= p= s= u=] → [c= p=2 s= u=]
(Assignee)

Comment 1

5 years ago
Created attachment 801000 [details]
Github Pull Request

Opening and tracking the PR. Need to test this further with a SIM first.
(Assignee)

Comment 2

5 years ago
Comment on attachment 801000 [details]
Github Pull Request

Hi Salva - 

Currently working on migrating apps away from commented out nodes in favor of using our HTML import polyfill. Because Usage app already uses the LazyLoader it should be as simple as moving HTML out into separate files. If this is done, there will be no performance impact. We will simply leverage HTML imports and the build system to gain syntax highlighting and abstraction of HTML components. 

I need to do some more testing because my sim card is giving me problems, but I'd like to ask your feedback on this. Thanks!
Attachment #801000 - Flags: feedback?(salva)
Hello Kevin

I do not see any change in the JS and I don't get how this will work. Can you explain me before I give you the f+?

Thank you very much.
Flags: needinfo?(kgrandon)
(Assignee)

Comment 4

5 years ago
(In reply to Salvador de la Puente González [:salva] from comment #3)
> I do not see any change in the JS and I don't get how this will work. Can
> you explain me before I give you the f+?

Hi Salva,

Because Cost Control leverages the lazy loader - it automatically has access to the HTML import polyfill which has recently landed. This basically means we can manage HTML in separate files with syntax highlighting and IDE features and it will be *real* HTML (not comments).

There is actually no impact on performance, because we make an optimization for PRODUCTION builds. For DEBUG builds, we follow this path: https://github.com/mozilla-b2g/gaia/blob/master/shared/js/lazy_loader.js#L44

You can see where we did this with settings: https://github.com/mozilla-b2g/gaia/commit/9beddce50107268582af4618654ffb4852ac42be

It's a bit tricky to see how it all fits together, so if you need any more information, please let me know!
Flags: needinfo?(kgrandon)
Kevin, sorry for the delay, it looks good buy I'm going to test it a little bit.

Can you solve me a couple of doubts, when are these parts loaded into the DOM? Now they are being loaded when the ViewManager component in view_manager.js tries to access these views then it calls `loadPanel()` [1] & [2] but I don't see any of this logic changed.

Another thing, how does this know the import content should be inside the container? Should I assume this is given by the extend attribute? Do you have some specification about this?

Once I finish the testing and you answer me, I will answer you feedback request.

Thank you very much.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/costcontrol/js/view_manager.js#L37
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/costcontrol/js/view_manager.js#L53
Flags: needinfo?(kgrandon)
The testing shows there is no regression at a glance so if we don't call loadPanel() (or it do nothing) this shows the HTML is already loaded in the DOM. As we did not rely on LazyLoad to 'uncomment' the former nodes. Am I wrong?
(Assignee)

Comment 7

5 years ago
(In reply to Salvador de la Puente González [:salva] from comment #6)
> The testing shows there is no regression at a glance so if we don't call
> loadPanel() (or it do nothing) this shows the HTML is already loaded in the
> DOM. As we did not rely on LazyLoad to 'uncomment' the former nodes. Am I
> wrong?

Salva - sorry, my comment above was mistaken. I assumed that the app was using lazy loader, when in fact it is doing it's own HTML lazy loading. It's going through this path here: https://github.com/mozilla-b2g/gaia/blob/master/apps/costcontrol/js/view_manager.js#L111

Basically the new syntax is simply a way of importing the HTML at build time into comments. Giving you syntax highlighting/etc.
Flags: needinfo?(kgrandon)
So, correct me if I'm wrong. The build process insert the external files into the HTML as comments? So my lazy loader is acting like always but, in return, I have syntax highlight and other benefits. Is not it?
Flags: needinfo?(kgrandon)
(Assignee)

Comment 9

5 years ago
(In reply to Salvador de la Puente González [:salva] from comment #8)
> So, correct me if I'm wrong. The build process insert the external files
> into the HTML as comments? So my lazy loader is acting like always but, in
> return, I have syntax highlight and other benefits. Is not it?

Exactly. There is no performance gain or loss - it is only extracting the HTML into separate files, and the build process will inline them.
Flags: needinfo?(kgrandon)
Attachment #801000 - Flags: feedback?(salva) → feedback+
Ok, in view there is no regressions and I understand how it works, you have my f+.
(Assignee)

Comment 11

5 years ago
Comment on attachment 801000 [details]
Github Pull Request

Hi Salva - 

I did as much testing as I was able to do with my current sim situation, and I haven't noticed any regressions. If you like this patch and could put your official review stamp on this we can get it landed. Thanks!
Attachment #801000 - Flags: review?(salva)
Comment on attachment 801000 [details]
Github Pull Request

You have some comments on GitHub, not blocking on them but if you feel the same, please fix them.

Thank you Kevin, I think it is a smart solution.
Attachment #801000 - Flags: review?(salva) → review+
(Assignee)

Comment 13

5 years ago
(In reply to Salvador de la Puente González [:salva] from comment #12)
> Comment on attachment 801000 [details]
> Github Pull Request
> 
> You have some comments on GitHub, not blocking on them but if you feel the
> same, please fix them.

Great catch! I actually used xpc shell to script the migration which must've changed these. Updating as requested, thanks!
(Assignee)

Comment 14

5 years ago
Landed in master: https://github.com/mozilla-b2g/gaia/commit/9ae10e566e7696761e253fc9f6c3f4dc6da1af96
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [c= p=2 s= u=] → [c= p=2 s=2013.10.25 u=]
Blocking the blocker bug 912510. Should we mark it as koi+? Are these enhancements available in v1.2 platform?
blocking-b2g: --- → koi?
Flags: needinfo?(kgrandon)
(Assignee)

Comment 16

5 years ago
If a blocker depends on this change, I would say that we should uplift it - it's been stable on master for quite some time. If we don't get koi+ - I'm more than happy to rewrite it for a one-off, but that's a waste of time.
Flags: needinfo?(kgrandon)
koi+ perf issue
blocking-b2g: koi? → koi+
v1.2: a6472bbf10f7b8ea991ef0b3330c932c65ad50f7

Uplifted as it is a koi+ blocking a blocker.
status-b2g-v1.2: --- → fixed
(In reply to Salvador de la Puente González [:salva] from comment #15)
> Blocking the blocker bug 912510. Should we mark it as koi+? Are these
> enhancements available in v1.2 platform?

https://bugzilla.mozilla.org/show_bug.cgi?id=912510 is already marked as fixed, unclear why this would be needed in that case ? Can you please help with more information as to how/why this should be a 1.2 blocker

Updated

5 years ago
blocking-b2g: koi? → koi+
(In reply to bhavana bajaj [:bajaj] from comment #19)
> (In reply to Salvador de la Puente González [:salva] from comment #15)
> > Blocking the blocker bug 912510. Should we mark it as koi+? Are these
> > enhancements available in v1.2 platform?
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=912510 is already marked as
> fixed, unclear why this would be needed in that case ? Can you please help
> with more information as to how/why this should be a 1.2 blocker

Because this was in master but not in v1.2 so the bug 912510 was solved in master (and therefore marked as FIXED) but it did not merge well in v1.2 as this dependency was not in that branch.

Note I say "did" and "was" because I uplifted this once it was marked as koi+.
You need to log in before you can comment on or make changes to this bug.