neil@parkwaycc.co.uk Reporter
	Description • 9 years ago

The Findbar._onKeyPress handler in browser-content.js wants to pass its keypress message to the find bar chrome. It does this by shallow copying the event to an object, excluding the 7 properties which are of object and 8 of function type, however there are 199 properties that duplicate properties on the KeyboardEvent constructor (of which 8 are also on the Event constructor) and only 29 properties that are actually interesting.

Uninteresting properties: NONE CAPTURING_PHASE AT_TARGET BUBBLING_PHASE ALT_MASK CONTROL_MASK SHIFT_MASK META_MASK SCROLL_PAGE_DOWN SCROLL_PAGE_UP DOM_KEY_LOCATION_STANDARD DOM_KEY_LOCATION_LEFT DOM_KEY_LOCATION_RIGHT DOM_KEY_LOCATION_NUMPAD DOM_VK_*

Interesting properties: isTrusted charCode keyCode altKey ctrlKey shiftKey metaKey location repeat isComposing key code detail layerX layerY pageX pageY which rangeOffset cancelBubble isChar type eventPhase bubbles cancelable defaultPrevented timeStamp multipleActionsPrevented isSynthesized

	:Gijs (he/him) Assignee
	Comment 1 • 9 years ago

Do you have a suggestion about usefully distinguishing what we want and what we don't want without whitelisting?

	neil@parkwaycc.co.uk Reporter
	Comment 2 • 9 years ago

Yes, I wouldn't bother sending properties of KeyboardEvent itself.

	:Gijs (he/him) Assignee
	Comment 3 • 9 years ago

(In reply to neil@parkwaycc.co.uk from comment #2)
> Yes, I wouldn't bother sending properties of KeyboardEvent itself.

Note that this can't be done through hasOwnProperty, which returns false for all of the uninteresting properties.

	:Gijs (he/him) Assignee
	Comment 4 • 9 years ago

Bug 1190382 - don't send useless KeyboardEvent properties, r?neil

	:Gijs (he/him) Assignee
	Comment 5 • 9 years ago

Comment on attachment 8694746 [details]
MozReview Request: Bug 1190382 - don't send useless KeyboardEvent properties, r?neil

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26863/diff/1-2/

	:Gijs (he/him) Assignee
	Comment 6 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #5)
> Comment on attachment 8694746 [details]
> MozReview Request: Bug 1190382 - don't send useless KeyboardEvent
> properties, r?neil
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/26863/diff/1-2/

Uh, ignore that link - you want  https://reviewboard.mozilla.org/r/26863/ .

	neil@parkwaycc.co.uk Reporter
	Comment 7 • 9 years ago

No, I'm pretty sure the link *I* want is https://reviewboard-hg.mozilla.org/gecko/rev/1bf62bfb0d9a

	neil@parkwaycc.co.uk Reporter
	Comment 8 • 9 years ago

(In reply to Gijs Kruitbosch from comment #3)
> Note that this can't be done through hasOwnProperty, which returns false for
> all of the uninteresting properties.
Huh, for some reason isTrusted is an own property of a keyboard event, but (as I expected) all of the other properties live on the prototype chain.

	neil@parkwaycc.co.uk Reporter
	Comment 9 • 9 years ago

Comment on attachment 8694746 [details]
MozReview Request: Bug 1190382 - don't send useless KeyboardEvent properties, r?neil

LGTM

	Pulsebot
	Comment 10 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/4a9a6f9a2cf4

	Pulsebot
	Comment 11 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/ced582e28099

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4a9a6f9a2cf4
https://hg.mozilla.org/mozilla-central/rev/ced582e28099