1159660 - [System][WindowManager] check activeElement before focus or blur a mozbrowser iframe

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[John Hu [:johnhu][:johu][:醬糊小弟]]

As we known, call focus() on a mozbrowser iframe consumes a lot of performance. We should prevent that. Because changing it, we should also check if call blur() on a mozbrowser iframe costs a lot.

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

I had done two different test cases:

1. Calculate elapsed time while we call focus() and blur() synchronized and calculate the average time used by focus() and blur()[1].
    The average time of 100 times call focus() and blur() synchronized is 8.7488ms

2. Calculate elapsed time between focus() => focus event and blur() => blur event[2].
    The average time of 100 times from focus() to focus event is 3.2067 ms.
    The average time of 100 times from blur() to blur event is 0.6719 ms.

[1] https://github.com/huchengtw-moz/gaia/commit/2e9e6ac77c1f6918cf3bbdd2648e7bd8e24f1410
[2] https://github.com/huchengtw-moz/gaia/commit/aab51de6904956824ec984c7415fbfd021b68abd

Comment 2

[John Hu [:johnhu][:johu][:醬糊小弟]]

If I commented out [1] which means we try to call focus() multiple times without call blur(), it seems that gecko had already checked if a mozbrowser iframe is activeElement. The average time of it is 0.186 ms.


[1] https://github.com/huchengtw-moz/gaia/commit/2e9e6ac77c1f6918cf3bbdd2648e7bd8e24f1410#diff-8e3ee5eaa7effd00d972282afc13f1c8R14

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

The implementation of HTMLElement.focus() of Gecko is at [1]. It uses focus manager's setFocus() and which calls SetFocusInner() to focus it. Theoretically, comment 2 is wrong. But I had tried to add event listener of focus and blur at the test case of comment 2. It doesn't fire extra focus or blur events.


[1] https://dxr.mozilla.org/mozilla-central/source/dom/html/nsGenericHTMLElement.cpp#2583-2589

Comment 4

[John Hu [:johnhu][:johu][:醬糊小弟]]

More information about comment 3:

According to the code [1][2], gecko doesn't check if an element is activeElement. So, I feel comment 2 is wrong.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/html/nsGenericHTMLElement.cpp#2583-2589
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1161

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] huchengtw-moz:bug-1159660-check-before-focus > mozilla-b2g:master

Comment 6

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8600836 [details] [review]
[gaia] huchengtw-moz:bug-1159660-check-before-focus > mozilla-b2g:master

Currently, I don't know how to measure it. The patch is ready. But I will discuss this with others about why comment 2 and comment 3/4 is conflict.

Comment 7

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8600836 [details] [review]
[gaia] huchengtw-moz:bug-1159660-check-before-focus > mozilla-b2g:master

As per comment 1, we still need to check activeElement before changing focus. Please review this patch.

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8600836 [details] [review]
[gaia] huchengtw-moz:bug-1159660-check-before-focus > mozilla-b2g:master

only nit: put a comment to explain why do this

Comment 9

[John Hu [:johnhu][:johu][:醬糊小弟]]

merged to master:
https://github.com/mozilla-b2g/gaia/commit/eb3f82f3b947f1ed52fa4e9246bed0f0b3e49785

tree herder is all green:
https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=0ab56c16152011b5d4ef8d3f75730a92dd2feb34

Comment 10

[John Hu [:johnhu][:johu][:醬糊小弟]]

Thanks for the finding from Sean Lin. He found that gecko will check if an element is focused before focus it[1]. But we still need this patch.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1187-1191