393582 - Improve the nsXULPopupManager.cpp nsNavigationDirection code

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

From https://bugzilla.mozilla.org/show_bug.cgi?id=279703#c41

Improve the NavigationDirection code

Comment 1

[karuna (kay)]

what kind of improvements are expected here?? On a coarse view, HandleKeyboardNavigationInPopup and HandleKeyboardNavigation code seems to be fine.

Comment 2

[Neil Deakin]

The changes described in https://bugzilla.mozilla.org/show_bug.cgi?id=279703#c41

Comment 3

[karuna (kay)]

Attachment: Improve the nsXULPopupManager.cpp nsNavigationDirection code

Comment 4

[Neil Deakin]

(In reply to comment #3)
> Created an attachment (id=296295) [details]
> Improve the nsXULPopupManager.cpp nsNavigationDirection code
> 

Doesn't this bring back the warnings that were removed in bug 388356?

Comment 5

[karuna (kay)]

Attachment: Improve the nsXULPopupManager.cpp nsNavigationDirection code

Comment 6

[Neil Deakin]

Comment on attachment 296928 [details] [diff] [review]
Improve the nsXULPopupManager.cpp nsNavigationDirection code

Looks OK, but keep all the 'broken ordering' assertions somewhere, probably where you moved the 'illegal key code' assertion

Comment 7

[Neil Deakin]

Attachment: updated patch

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 354309 [details] [diff] [review]
updated patch

>+++ b/layout/xul/base/public/nsXULPopupManager.h
>+#if NS_STYLE_DIRECTION_LTR != 0 || NS_STYLE_DIRECTION_RTL != 1
>+  #error "Can't deal with this language"
>+#endif

Now that we have static asserts, this is probably simpler to write as:

PR_STATIC_ASSERT(NS_STYLE_DIRECTION_LTR == 0 && NS_STYLE_DIRECTION_RTL == 1);

>-/**
>- * DirectionFromKeyCode_rl_tb: an array that maps keycodes to values of
>- * nsNavigationDirection for right-to-left and top-to-bottom flow orientation
>- * This is defined in nsXULPopupManager.cpp.
>- */

Some variant of this comment should likely stay.

>+++ b/layout/xul/base/src/nsXULPopupManager.cpp
>+  NS_ASSERTION((NS_VK_HOME == NS_VK_END + 1) &&
>+               (NS_VK_LEFT == NS_VK_END + 2) &&
>+               (NS_VK_UP == NS_VK_END + 3) &&
>+               (NS_VK_RIGHT == NS_VK_END + 4) &&
>+               (NS_VK_DOWN == NS_VK_END + 5), "Broken ordering");

This could also be a PR_STATIC_ASSERT in the header, right?

sr=bzbarsky with those nits picked.