[Usage][Refactoring] ViewManager.loadPanel should cleanup panel's DOM once scripts/links are activated

RESOLVED FIXED

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: azasypkin, Assigned: azasypkin)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

To activate panel's nested scripts and links in ViewManager.loadPanel we create new nodes with the same src/href links and append it to the document head. In this bug we should try to _move_ nested resource nodes directly to the head instead or at least remove it from panel's DOM once they're activated.

See bug 1100915 comment 4 for details.
> try to _move_ nested resource nodes directly to the head instead

As per :bz it won't work because script node cloned from one that was inserted via innerHTML is still "marked" as not-to-be-loaded-or-executed. So we'll need to create new script nodes anyway.
Patch is ready, waiting for bug 1100915 to be landed before asking for review.
Comment on attachment 8526790 [details] [review]
GitHub pull request URL

Hey Salvador,

Could you please share your feedback on this patch? I have some questions that I left on Github.

Thanks!
Attachment #8526790 - Flags: feedback?(salva)
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
Comment on attachment 8526790 [details] [review]
GitHub pull request URL

My comments on GitHub, nothing remarkable. Good work!
Attachment #8526790 - Flags: feedback?(salva) → feedback+
Comment on attachment 8526790 [details] [review]
GitHub pull request URL

(In reply to Salvador de la Puente González [:salva] from comment #4)
> Comment on attachment 8526790 [details] [review]
> GitHub pull request URL
> 
> My comments on GitHub, nothing remarkable. Good work!

Thanks for feedback and replies! Per feedback, no code changes requested yet, so asking for review :)

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

No problems with this implementation but before landing, can you confirm if `cloneNode` could work instead of regenerating the node from scratch? Thank you Oleg.
Attachment #8526790 - Flags: review?(salva) → review+
(In reply to Salvador de la Puente González [:salva] from comment #6)
> Comment on attachment 8526790 [details] [review]
> GitHub pull request URL
> 
> No problems with this implementation but before landing, can you confirm if
> `cloneNode` could work instead of regenerating the node from scratch? Thank
> you Oleg.

Nope, unfortunately cloneNode copies these script "hidden state" flags as well.

Thanks!
Treeherder is green, landed.

Master: https://github.com/mozilla-b2g/gaia/commit/ee6443a912f75267b8fb51fd67909882ca2b5f4b

Thanks!
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.