Closed Bug 1109770 Opened 5 years ago Closed 5 years ago

[gaia-header] trigger the reformat only once even if several mutations happen, for the same heading

Categories

(Firefox OS Graveyard :: Gaia::Components, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S3 (9jan)

People

(Reporter: julienw, Assigned: julienw)

References

Details

(Whiteboard: [sms-sprint-2.2S3])

Attachments

(2 files)

54 bytes, text/x-github-pull-request
gmarty
: review+
gmarty
: feedback+
Details | Review
46 bytes, text/x-github-pull-request
julienw
: review+
Details | Review
This happens when there is a nested markup in a gaia-header and we're changing several elements at once.
Attached file gaia-header github PR
I still need to test properly the change.

This happens especially in the SMS app, in the new message panel, when entering recipients. One part of the issue is bug 1109801, and I also think that l10n.js has the same issue with its MutationObserver.
Assignee: nobody → felash
Summary: [gaia-header] trigger the reformat only once even if several mutations happen → [gaia-header] trigger the reformat only once even if several mutations happen, for the same heading
Bug 1109805 filed for l10n.js.
Comment on attachment 8534524 [details] [review]
gaia-header github PR

Hey Guillaume,

I don't have a good idea to do a unit test for this. Especially I couldn't reproduce the issue using a more synthetic testcase in jsbin... (see http://jsbin.com/jujuvavura/1/edit?html,js,console) so I don't know how to reproduce in a test :/
Attachment #8534524 - Flags: feedback?(gmarty)
Oh, funnily, I reproduce in this jsbin when I just load it and it autoruns, but not when I click on "run JS". I don't know why.
Julien, I don't think you can reliably use jsbin for that. The reason why you see 2 vs 1 mutations is because jsbin is adding/removing scripts. To see that, just do "console.log(mutations[i]);" in your loop and you'll see added/removed nodes.

Can you create a minimized testcase that works in a browser? The one from jsbin does not reproduce for me.
Yep I'll try; on the device and in the SMS app, I admit it happened on places I didn't expect it to happen, but didn't on the places I did expect :) So likely I don't understand how this is working ;)
Comment on attachment 8534524 [details] [review]
gaia-header github PR

Julien, try to replace the `/edit?blabla` in the URL by `/quiet` in jsbin and you'll just get the HTML and JS loaded as a regular HTML page.

Apart from that, using Set to dedupe the elements looks good to me, so f+. Waiting on tests before the review.
Attachment #8534524 - Flags: feedback?(gmarty) → feedback+
Comment on attachment 8534524 [details] [review]
gaia-header github PR

Hey Guillaume,

I managed to write a test that's failing with master and working with my code. Good thing: it also exercizes the whole mutation handling :)

My previous issues were indeed directly caused by jsbin. Testing using a plain HTML file showed me it was working as I expected.
Attachment #8534524 - Flags: review?(gmarty)
We should consider making it a feature blocker if this improves launch time.
feature-b2g: --- → 2.2?
Comment on attachment 8534524 [details] [review]
gaia-header github PR

Looks all good to me! Thanks Julien.
Attachment #8534524 - Flags: review?(gmarty) → review+
gaia-header master: 0a6d93154c2cac8356f1abf9779f2fd016a8d26b
Since it's landed to master, +'ed this one.
feature-b2g: 2.2? → 2.2+
note that it's landed to gaia-header master only, not gaia master yet :)
Attached file gaia github PR
Carrying over r=gmarty
Waiting for a try run.
Attachment #8543930 - Flags: review+
Attachment #8534524 - Attachment description: github PR → gaia-header github PR
gaia master: 8f8995f24ea9a0f5568b786f2c0668d6415de222
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [sms-sprint-2.2S3]
Hi Julien, do you think we can take this patch to improve sms perfromance on 2.1? I found gaia-header version has big gap in between, would that be a problem if we uplift this?
Flags: needinfo?(felash)
Hey Sherman,

I'd suggest to wait for bug 1089920 and then uplift all of this.
Flags: needinfo?(felash)
Cool, thank you Juilen :)
feature-b2g: 2.2+ → ---
Target Milestone: --- → 2.2 S3 (9jan)
You need to log in before you can comment on or make changes to this bug.