Date:  17 Jan 2006 00:34:35 Hong Kong Time

superreview granted: [Bug 323230] Need FindAttrValueIn : [Attachment 208621] fix


Boris Zbarsky  has granted Robert O'Callahan (Novell)
's request for superreview:
Bug 323230: Need FindAttrValueIn

Attachment 208621: fix

------- Additional Comments from Boris Zbarsky 
>Index: content/base/public/nsIContent.h

>+   * @return 

Say "described above" or something, I guess?

>Index: content/events/src/nsEventStateManager.cpp
>@@ -1144,27 +1144,28 @@ nsEventStateManager::CreateClickHoldTime
>+    static nsIAtom* const* const strings[] = {&nsXULAtoms::_empty, NULL};


>@@ -1254,37 +1255,37 @@ nsEventStateManager::FireContextClick()
>+	    static nsIAtom* const* const strings[] = {&nsXULAtoms::_empty,
>+	    PRInt32 index =
>+		nsXULAtoms::container, strings, eCaseMatters);
>+	    if (index == nsIContent::ATTR_VALUE_NO_MATCH) {
>	      allowedToDispatch = PR_FALSE;
>+	    } else {
>+	      // If the toolbar button has an open menu, don't attempt to open
>+	      // a second menu
>+	      if (mGestureDownContent->AttrValueIs(kNameSpaceID_None,
>+						   nsXULAtoms::_true,
eCaseMatters)) {
>+		allowedToDispatch = PR_FALSE;

It seems that this might (to me at least) be more readable as:

  static nsIAtom* const* const strings[] = {&nsXULAtoms::_empty, nsnull};
  if (mGestureDownContent->FindAttrValueIn(kNameSpaceID_None,
		nsXULAtoms::container, strings, 
		eCaseMatters) == nsIContent::ATTR_VALUE_NO_MATCH ||
      mGestureDownContent->AttrValueIs(kNameSpaceID_None, nsXULAtoms::open,
				       nsXULAtoms::_true, eCaseMatters)) {
    allowedToDispatch = PR_FALSE;

or something.  That is, instead of nested ifs use a single if with ||.	Either
way, though.  It's hard to tell how readable stuff is in this bugzilla
textfield.  ;)

>@@ -4906,27 +4907,26 @@ nsEventStateManager::MoveFocusToCaret(PR
>	*aIsSelectionWithFocus = testContent->HasAttr(kNameSpaceID_XLink,
>	if (*aIsSelectionWithFocus) {
>-	  nsAutoString xlinkType;
>-	  testContent->GetAttr(kNameSpaceID_XLink, nsHTMLAtoms::type,
>-	  if (!xlinkType.EqualsLiteral("simple")) {
>+	  if (!testContent->AttrValueIs(kNameSpaceID_XLink, nsHTMLAtoms::type,
>+					nsHTMLAtoms::simple, eCaseMatters)) {
>	    *aIsSelectionWithFocus = PR_FALSE;	// Xlink must be type="simple"

I think should should definitely be:

  *aIsSelectionWithFocus = 
    testContent->HasAttr(kNameSpaceID_XLink, nsHTMLAtoms::href) &&
    testContent->AttrValueIs(kNameSpaceID_XLink, nsHTMLAtoms::type,
			     nsHTMLAtoms::simple, eCaseMatters);

Makes it easier to translate the code into English.  ;)

>Index: content/xul/content/src/nsXULAtomList.h

I assume what you did here is alphabetized the list and added _empty?  If not,
what other changes?  This part is pretty much impossible to actually review...

For what it's worth, I thought there was some benefit in having the atoms
grouped by what actually used them; is there a reason we really prefer

>Index: content/xul/content/src/nsXULElement.cpp
>Index: layout/xul/base/src/nsBoxFrame.cpp
> nsBoxFrame::GetInitialHAlignment(nsBoxFrame::Halignment& aHalign)

>+  static const Halignment values[] =
>+    {hAlign_Left, hAlign_Left, hAlign_Center, hAlign_Right};

Comment that the first entry is unused?  Or use [index-1] to index into this
array and just remove that first entry?

> nsBoxFrame::GetInitialVAlignment(nsBoxFrame::Valignment& aValign)

This code screams to me of attribute mapping.  :(  We should really get that
set up for XUL...

>+  static nsIAtom* const* const strings[] =
>+    {&nsXULAtoms::_empty, &nsXULAtoms::start, &nsXULAtoms::center,
>+     &nsXULAtoms::baseline, &nsXULAtoms::end, NULL};

Same comment as for GetInitialHAlignment.

sr=bzbarsky with those nits.