Closed Bug 393582 Opened 12 years ago Closed 11 years ago

Improve the nsXULPopupManager.cpp nsNavigationDirection code

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: enndeakin, Assigned: karunasagark)

Details

(Keywords: helpwanted, Whiteboard: [good first bug])

Attachments

(2 files, 1 obsolete file)

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

Improve the NavigationDirection code
Keywords: helpwanted
Whiteboard: [good first bug]
what kind of improvements are expected here?? On a coarse view, HandleKeyboardNavigationInPopup and HandleKeyboardNavigation code seems to be fine.
The changes described in https://bugzilla.mozilla.org/show_bug.cgi?id=279703#c41
Attachment #296295 - Flags: review?(enndeakin)
(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?
Attachment #296295 - Attachment is obsolete: true
Attachment #296928 - Flags: review?(enndeakin)
Attachment #296295 - Flags: review?(enndeakin)
Assignee: nobody → karunasagark
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: PC → All
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
Attachment #296928 - Flags: review?(enndeakin) → review+
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
Attached patch updated patchSplinter Review
Attachment #354309 - Flags: superreview?(bzbarsky)
Attachment #354309 - Flags: superreview?(bzbarsky) → superreview+
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.