Sergi Mansilla [:sergi] (Telenor) [PTO] Assignee
	Description • 11 years ago

      No description provided.

	Sergi Mansilla [:sergi] (Telenor) [PTO] Assignee
	Comment 1 • 11 years ago

This patch takes care of sanitising innerHTML as per issue https://bugzilla.mozilla.org/show_bug.cgi?id=837740. It makes use of textContent and DocumentFragment in some cases, which also improves the speed of these operations. Some other small fixes in code and comments have been made.

	David Flanagan [:djf]
	Comment 2 • 11 years ago

Comment on attachment 727663 [details]
Github pull request

Thanks for doing this. The patch looks mostly good.  r- because of linter errors (see the Travis log on the github page starting around line 60).  And also because I think I found one or two errors in your patch.  See github for specific comments.

	Sergi Mansilla [:sergi] (Telenor) [PTO] Assignee
	Comment 3 • 11 years ago

Moved the review to https://github.com/mozilla-b2g/gaia/pull/8791.

	David Flanagan [:djf]
	Comment 5 • 11 years ago

Comment on attachment 728895 [details]
New Pull Request URL

r- this time because I don't want want dom.js to go in the shared directory with only one function in it.  I think a small library of shared dom utilities would be worth having.  But not until it has more utility than this.

Also, see the discussion that has continued in https://github.com/mozilla-b2g/gaia/pull/8718 about the fastest method of clearing a DOM element.  I think that Rick's test had an error. In my versions, setting innerHTML to the empty string is actually the fastest technique.

It would be fine with me to just use innerHTML='' in render.js

	David Flanagan [:djf]
	Comment 7 • 11 years ago

Comment on attachment 732243 [details]
New Pull Request URL

Sergi,

The code looks good, except for one linter error and one indentation error that should be trivial to fix.

Unfortunately, in testing this out, it appears that it breaks the keyboard layout switcher menu.  

To try this out, go to Settings->Keyboard and enable multiple layouts (like "other latin").  Then, when you use the keyboard, you'll see a globe icon in the lower left.  Pressing and holding it brings up a menu of available layouts.

On master, I can move my finger up the menu to select the layout I want. With your patch applied, the menu disappears when I move my finger off the globe button.

	Sergi Mansilla [:sergi] (Telenor) [PTO] Assignee
	Comment 9 • 11 years ago

David,

Good catch. I found the problem and it is solved now. I added the newline and gjslint seems happy. I don't believe there is any indentation error left, but let me know if you find any.

Sergi

	David Flanagan [:djf]
	Comment 10 • 11 years ago

Comment on attachment 732748 [details]
Pull Request URL

Sergi,

I don't understand why your cloneNode() code didn't work the way the existing code did.  By going back to the old code, you've got innerHTML in there again, but this is just a safe use of it to clone, so I'm okay with that.  I'm pretty sure that this layout switching menu is going to be replaced anyway with a full-screen system menu.

There is still an indentation issue at render.js:130, at least according to they way emacs indents my JavaScript, but that's not worth holding this up for.

r+, but squash your commits before landing anything.  Let me know (by email, irc, or needinfo?) if you'd like me to land it.

	Sergi Mansilla [:sergi] (Telenor) [PTO] Assignee
	Comment 11 • 11 years ago

David,

Yeah, it shouldn't be a problem since innerHTML is set to empty string. I fixed the indentation issue and squashed commits together.

Also, what would be necessary for me to be able to land commits (of course not my own)?

Sergi

	Sergi Mansilla [:sergi] (Telenor) [PTO] Assignee
	Comment 12 • 11 years ago

Daivid, please land it if it's ok for you.

	David Flanagan [:djf]
	Comment 13 • 11 years ago

Landed on master: https://github.com/mozilla-b2g/gaia/commit/f4e802d142148106263a56b2b3c9a2b98562c7ef

	David Flanagan [:djf]
	Comment 14 • 11 years ago

(In reply to Sergi Mansilla from comment #11)
> 
> Also, what would be necessary for me to be able to land commits (of course
> not my own)?
> 

I actually don't know what are policy is about that.  Maybe ask on dev-gaia?

Sergi, do you feel that there is a noticeable performance difference with this patch applied? If so, we should consider uplifting it to v1-train.  (And uplifting to v1-train will make it easier to uplift keyboard auto-correction to v1-train, since that will also modify the render.js file.)

	Sergi Mansilla [:sergi] (Telenor) [PTO] Assignee
	Comment 15 • 11 years ago

David,

It feels a bit faster, but it might as well be placebo effect. I didn't make any benchmark test, but I might do that when I have time. I think it is safe enough to go to v1, but it is obviously not my call.

	David Flanagan [:djf]
	Comment 16 • 11 years ago

If this ever gets uplifted, we also need to uplift 859159 which fixes a regression it caused.