Closed Bug 1254755 Opened 4 years ago Closed 4 years ago

Clean up WidgetKeyboardEvent

Categories

(Core :: Widget, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- affected
firefox49 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

WidgetKeyboardEvent has a lot of members. And some of them don't have "m" prefix, the order of them wastes memory due to bad memory alignment.

Now, we should clean up it.
Assignee: masayuki → nobody
Mentor: masayuki
Status: ASSIGNED → NEW
Whiteboard: [good first bug][for Gecko Inside #7 in Mozilla Japan]
hi masayuki I want to work on this one...can you guide me ?
Flags: needinfo?(masayuki)
(In reply to agore1 from comment #1)
> hi masayuki I want to work on this one...can you guide me ?

Ah, sorry, nobody should work on this now because patches for some urgent bugs will be broken by the changes for this bug. Do you want to work on this later? Or do you look for another one?
Flags: needinfo?(masayuki)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 4/29-5/8) from comment #2)
> (In reply to agore1 from comment #1)
> > hi masayuki I want to work on this one...can you guide me ?
> 
> Ah, sorry, nobody should work on this now because patches for some urgent
> bugs will be broken by the changes for this bug. Do you want to work on this
> later? Or do you look for another one?

I am looking more bugs to work on.
Sorry for that (removing [good first bug] until urgent bugs are fixed).
Mentor: masayuki
Whiteboard: [good first bug][for Gecko Inside #7 in Mozilla Japan]
Now, it is the change to fix this bug. I'll fix this bug quickly.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
And also WidgetKeyboardEvent::mKeyCode should be compared with NS_VK_* rather than nsIDOMKeyEvent::DOM_VK_*.

Review commit: https://reviewboard.mozilla.org/r/52720/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52720/
Attachment #8752698 - Flags: review?(bugs)
Attachment #8752699 - Flags: review?(bugs)
Attachment #8752700 - Flags: review?(bugs)
Attachment #8752701 - Flags: review?(bugs)
Attachment #8752702 - Flags: review?(bugs)
Attachment #8752703 - Flags: review?(bugs)
And mCharCode shouldn't be compared with NS_VK_*, nsIDOMKeyEvent::DOM_VK_*. Additionally, when it's compared with a character constant, cast isn't necessary.

Review commit: https://reviewboard.mozilla.org/r/52722/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52722/
For reducing the instance size of WidgetKeyboardEvent, this patch also explicitly defines the type of KeyNameIndex and CodeNameIndex.

Review commit: https://reviewboard.mozilla.org/r/52730/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52730/
Comment on attachment 8752698 [details]
MozReview Request: Bug 1254755 part.1 Rename WidgetKeyboardEvent::keyCode to WidgetKeyboardEvent::mKeyCode r?smaug

https://reviewboard.mozilla.org/r/52720/#review49680

::: editor/libeditor/nsHTMLEditor.cpp:632
(Diff revision 1)
> -    case nsIDOMKeyEvent::DOM_VK_META:
> -    case nsIDOMKeyEvent::DOM_VK_WIN:
> -    case nsIDOMKeyEvent::DOM_VK_SHIFT:
> -    case nsIDOMKeyEvent::DOM_VK_CONTROL:
> -    case nsIDOMKeyEvent::DOM_VK_ALT:
> -    case nsIDOMKeyEvent::DOM_VK_BACK_SPACE:
> +    case NS_VK_META:
> +    case NS_VK_WIN:
> +    case NS_VK_SHIFT:
> +    case NS_VK_CONTROL:
> +    case NS_VK_ALT:
> +    case NS_VK_BACK:

Not about this bug, but it is tiny bit odd that we have DOM_VK_BACK_SPACE, but NS_VK_BACK. And there is also KEY_NAME_INDEX_Backspace. Why the NS_ variant misses "space"
Attachment #8752698 - Flags: review?(bugs) → review+
Comment on attachment 8752699 [details]
MozReview Request: Bug 1254755 part.2 Rename WidgetKeyboardEvent::charCode to WidgetKeyboardEvent::mCharCode r?smaug

https://reviewboard.mozilla.org/r/52722/#review49682
Attachment #8752699 - Flags: review?(bugs) → review+
Attachment #8752700 - Flags: review?(bugs) → review+
Comment on attachment 8752700 [details]
MozReview Request: Bug 1254755 part.3 Rename WidgetKeyboardEvent::alternativeCharCodes to WidgetKeyboardEvent::mAlternativeCharCodes r?smaug

https://reviewboard.mozilla.org/r/52724/#review49684
Attachment #8752701 - Flags: review?(bugs) → review+
Comment on attachment 8752701 [details]
MozReview Request: Bug 1254755 part.4 Rename WidgetKeyboardEvent::location to WidgetKeyboardEvent::mLocation r?smaug

https://reviewboard.mozilla.org/r/52726/#review49686
Attachment #8752702 - Flags: review?(bugs) → review+
Comment on attachment 8752702 [details]
MozReview Request: Bug 1254755 part.5 Rename WidgetKeyboardEvent::isChar to WidgetKeyboardEvent::mIsChar r?smaug

https://reviewboard.mozilla.org/r/52728/#review49688
Attachment #8752703 - Flags: review?(bugs) → review+
Comment on attachment 8752703 [details]
MozReview Request: Bug 1254755 part.6 Reorder the members of WidgetKeyboardEvent for reducing its instance size r?smaug

https://reviewboard.mozilla.org/r/52730/#review49690
You need to log in before you can comment on or make changes to this bug.