Closed Bug 333896 Opened 16 years ago Closed 16 years ago

Convert GetAttr calls to AttrValueIs and FindAttrValueIn

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: alfred.peng, Assigned: alfred.peng)

References

Details

Attachments

(7 files, 13 obsolete files)

1.15 KB, patch
roc
: review+
Details | Diff | Splinter Review
115.88 KB, patch
roc
: review+
Details | Diff | Splinter Review
72.16 KB, patch
roc
: review+
mark
: review+
Details | Diff | Splinter Review
78.86 KB, patch
mark
: review+
roc
: review+
Details | Diff | Splinter Review
26.93 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
26.83 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
14.95 KB, patch
Details | Diff | Splinter Review
The new methods nsIContent::AttrValueIs and nsIContent::FindAttrValueIn can be used in many many places where nsIContent::GetAttr is currently used. The new methods reduce code size and are faster. We need to convert GetAttr calls to use the new methods.
Attached patch Patch v1 (obsolete) — Splinter Review
First version of the patch.

Roc, is this the right direction? Don't know whether I should change the atomlist files.
Attachment #218325 - Flags: review?(roc)
Attached patch Patch v1 for Mac (obsolete) — Splinter Review
I've also made some changes to the mac specific code. Don't know who I should ask for review from.
I would comment that the result of GetAttr is an empty string both when an attribute is set to an empty string and when an attribute isn't set, and AttrValueIs returns false when an attribute isn't set.  Therefore, I think you're changing behavior in a number of cases where attributes are unset.

Also, it would be good to say how exactly you came up with these places in particular that needed to be changed.  We should probably be going through the GetAttr calls in some systematic way.
(In reply to comment #3)
> I would comment that the result of GetAttr is an empty string both when an
> attribute is set to an empty string and when an attribute isn't set, and
> AttrValueIs returns false when an attribute isn't set.  Therefore, I think
> you're changing behavior in a number of cases where attributes are unset.
> 

Yes, I miss this issue. For IsEmpty call, there isn't proper way to replace GetAttr with AttrValueIs. I'll update the patch.

> Also, it would be good to say how exactly you came up with these places in
> particular that needed to be changed.  We should probably be going through the
> GetAttr calls in some systematic way.
> 

Not all the places using GetAttr need to be changed. I've searched through the codebase and found out the places showed in my patch. If there is systematic way to do that, that will be better.
Attached patch Patch v2 (obsolete) — Splinter Review
The update patch address the IsEmpty issue.

Following is the rules I adopted to make this patch. It's based on how the caller function make use of the aResult after returning from GetAttr.
1. Evaluate IsEmpty for the aResult => Don't change.
2. aResult is used in the following function calls in the caller function, such as ToInteger, SetAttr, Substitutetext... => Don't change.
3. The caller function passes one of its own parameter as an out parameter to GetAttr => Don't change.
4. aResult will be compared to a literal string => Replace with AttrValueIs.
5. The caller function chooses different path by different aResult => Don't change, no performance improvement in this case.
6. aResult will be compared to a nsAString => Replace with AttrValueIs.
7. Evaluate !IsEmpty of aResult and compare it to a PRUnichar * value => Replace with AttrValueIs.
8. aResult will be compared to a bunch of literal strings => Replace with FindAttrValueIn.
9. Some changes will influence the code readability => Don't change.
Attachment #218325 - Attachment is obsolete: true
Attachment #218429 - Flags: review?(roc)
Attachment #218325 - Flags: review?(roc)
I'll also update the patch for Mac later.
Code like
  GetAttr(..., aValue);
  if (aValue.IsEmpty()) {

can be converted to use nsContentUtils::HasNonEmptyAttr, which is based on FindAttrValueIn.
> 5. The caller function chooses different path by different aResult => Don't
> change, no performance improvement in this case.

What's an example of this?
Take the code snippet from nsXMLElement.cpp as an example:
GetAttr(kNameSpaceID_XLink, nsLayoutAtoms::show, value);
if (value.EqualsLiteral("new")) {
  verb = eLinkVerb_New;
} else if (value.EqualsLiteral("replace")) {
  verb = eLinkVerb_Replace;
} else if (value.EqualsLiteral("embed")) {
  break;
}

If we change the GetAttr function with AttrValueIs, the performance will be decreased I think.
(In reply to comment #10)
> Use FindAttrValueIn for that.
> 

So we need to use FindAttrValueIn several times in this case, which will in turn call nsAttrAndChildArray::GetAttr several times. That will influence the performance I think. Roc, do we really need this change?
Duplicated header in nsAccessible.cpp.
Attachment #218670 - Flags: review?(roc)
Rule update:
1. Evaluate IsEmpty for the aResult => Replace with nsContentUtils::HasNonEmptyAttr

Rule added:
10. aResult is ignored in the following code => Don't change.
(In reply to comment #11)
> So we need to use FindAttrValueIn several times in this case, which will in
> turn call nsAttrAndChildArray::GetAttr several times. That will influence the
> performance I think. Roc, do we really need this change?

Am I missing something?
static nsIContent::AttrValuesArray strings[] =
  {&nsGkAtoms::new, &nsGkAtoms::replace, &nsGkAtoms::embed, nsnull};
PRBool stop = PR_FALSE;
switch (FindAttrValueIn(kNameSpaceID_XLink, nsLayoutAtoms::show, strings, eCaseMatters)) {
  case 0: verb = eLinkVerb_New; break;
  case 1: verb = eLinkVerb_Replace; break;
  case 2: stop = PR_TRUE; break;
}
if (stop)
  break;
(In reply to comment #13)
> Rule added:
> 10. aResult is ignored in the following code => Don't change.

If aResult is ignored, can't we just remove the GetAttr?
(In reply to comment #14)
> Am I missing something?
> static nsIContent::AttrValuesArray strings[] =

Yes, you're right. I thought it the wrong way. Will address it in the next patch

(In reply to comment #15)
> If aResult is ignored, can't we just remove the GetAttr?

For all the occurrences of the cases I found, the bool return values are used in an if conditional statement. I think they can be replaced by HasAttr.
Attached patch Patch v3 (obsolete) — Splinter Review
Updated patch based on Rules 1, 5 and 10 listed above.
Attachment #218429 - Attachment is obsolete: true
Attachment #218853 - Flags: review?(roc)
Attachment #218429 - Flags: review?(roc)
   nsAutoString rel;
-  nsAutoString rev;
   GetAttr(kNameSpaceID_None, nsHTMLAtoms::rel, rel);

I think you can use FindAttrValueIn here to avoid getting 'rel'.

-          verb = eLinkVerb_Replace;
-        } else if (value.EqualsLiteral("embed")) {
+          case 1: verb = eLinkVerb_Replace; break;
           // XXX TODO
-          break;
+          case 2: break;

I think this is incorrect. The "break" used to break out of the do loop. Now, in case 2 you're only breaking out of the switch, not the do loop. The clearest thing to do is probably to set a flag and break out of the do loop when the flag is set, as I suggested before.

+          static nsIContent::AttrValuesArray strings[] =
+            {&nsLayoutAtoms::_new, &nsLayoutAtoms::replace,
+             &nsLayoutAtoms::embed, nsnull};

You could share this array with the other one which has the same values, just make it global.

-                nsAutoString ref;
-                child->GetAttr(kNameSpaceID_None, nsXULAtoms::ref, ref);
-                if (!ref.IsEmpty() && ref.Equals(colID)) {
+                if (child->AttrValueIs(kNameSpaceID_None, nsXULAtoms::ref,
+                                       nsDependentString(colID),
+                                       eCaseMatters)) {

Check that colID[0] != 0, otherwise you'd be changing behaviour when ref and colID are both empty.

                   webbrwsr \
                   windowwatcher \
                   pref \
+                  js \
                   $(NULL)

Why do you need this?

-  nsString disabled;
-  aContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::disabled, disabled);
-  if (disabled.EqualsLiteral("true"))
+  if (aContent->AttrValueIs(kNameSpaceID_None, nsHTMLAtoms::disabled,
+                            nsHTMLAtoms::_true, eCaseMatters))
     return PR_TRUE;
   return PR_FALSE;

Just "return aContent->AttrValueIs(...);"
-  nsString disabled;
-  aContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::disabled, disabled);
-  if (disabled.EqualsLiteral("true"))
+  if (aContent->AttrValueIs(kNameSpaceID_None, nsHTMLAtoms::disabled,
+                            nsHTMLAtoms::_true, eCaseMatters))
     return PR_TRUE;
   return PR_FALSE;

There's another one of these...

-      nsAutoString ref;
-      cell->GetAttr(kNameSpaceID_None, nsXULAtoms::ref, ref);
-      if (!ref.IsEmpty() && ref.Equals(colID)) {
+      if (cell->AttrValueIs(kNameSpaceID_None, nsXULAtoms::ref,
+                            nsDependentString(colID), eCaseMatters)) {

Check here that colID[0] != 0.
Attached patch Patch v4Splinter Review
Patch v4 addresses Roc's comments.
Attachment #218853 - Attachment is obsolete: true
Attachment #219054 - Flags: review?(roc)
Attachment #218853 - Flags: review?(roc)
Comment on attachment 219054 [details] [diff] [review]
Patch v4

Please tell me why you added the "js" dependency.
Attachment #219054 - Flags: superreview+
Attachment #219054 - Flags: review?(roc)
Attachment #219054 - Flags: review+
(In reply to comment #18)
>    nsAutoString rel;
> -  nsAutoString rev;
>    GetAttr(kNameSpaceID_None, nsHTMLAtoms::rel, rel);
> 
> I think you can use FindAttrValueIn here to avoid getting 'rel'.
> 

Replace with FindAttrValueIn here, and four more places are changed to use it also.

>                    webbrwsr \
>                    windowwatcher \
>                    pref \
> +                  js \
>                    $(NULL)
> 
> Why do you need this?

Add header file nsContentUtils.h to nsMathMLmactionFrame.cpp and replace IsEmpty function with nsContentUtils::HasNonEmptyAttr. The file nsContentUtils.h includes another header jspubtd.h, which depends on js.

> 
> -  nsString disabled;
> -  aContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::disabled, disabled);
> -  if (disabled.EqualsLiteral("true"))
> +  if (aContent->AttrValueIs(kNameSpaceID_None, nsHTMLAtoms::disabled,
> +                            nsHTMLAtoms::_true, eCaseMatters))
>      return PR_TRUE;
>    return PR_FALSE;
> 
> Just "return aContent->AttrValueIs(...);"
> 

Respect to this case, three places are found and changed.

For nsTypeAheadFind.cpp in extension and toolkits directory, if I pass nsLayoutAtoms::simple to AttrValueIs, the build process is ok. But when I run seamonkey, there is a runtime error shows that: symbol __1cJnsGkAtomsGsimple_: referenced symbol not found. Maybe I need to change Makefile.in in this case also. Currently I change back to pass NS_LITERAL_STRING("simple") to AttrValueIs.
Yeah, you can't use nsGkAtoms or nsContentUtils outside of content/, layout/, view/ and dom/.

That's all fine, go ahead and check in.
Roc, could you help to check in the patch?

I'll also provide the mac specific one. Don't know who I should get the review from?
To figure out who to ask for review, you should generally look at http://www.mozilla.org/owners.html and ask for review from a module owner or peer for the relevant component.  If you end up asking the wrong person, the person you ask will generally correct it.
Attached patch Patch v2 for MAC (obsolete) — Splinter Review
MAC specific patch for this bug.
Attachment #218326 - Attachment is obsolete: true
Attachment #219148 - Flags: review?(mark)
Patch v4 checked in.
this patch makes the build fail if SVG is disabled.
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Thunderbird
I've checked in a patch that should hopefully fix the redness.

When checking in a patch as big as this one, make sure that all tinderboxes cycle fine before leaving the computer.
Blocks: 335212
Jonas, thanks. 
Comment on attachment 219148 [details] [diff] [review]
Patch v2 for MAC

This looks good, but I'd like to test it because I get the feeling that nobody's tested it on the Mac yet.  (If someone else has tested it, then I don't need to.)  Unfortunately, it's bit-rotten (my fault).  Alfred, could you post an updated patch?

We'll need changes in widget/src/cocoa too.
Attached patch Patch v3 for MAC (obsolete) — Splinter Review
I've not tested the patch here due to the lack of the platform. Appreciate if someone can help.

Later on the cocoa patch.
Attachment #219148 - Attachment is obsolete: true
Attachment #219722 - Flags: review?(mark)
Attachment #219148 - Flags: review?(mark)
Roc, how about the duplicated header patch?
(In reply to comment #33)
> Roc, how about the duplicated header patch?

What is that?
roc, nsAccessible.cpp includes nsIDOMHTMLDocument.h twice. It's just a typo I think.
Attached patch Patch v4 for MAC (obsolete) — Splinter Review
Some typo for the previous patch. Update it.
Attachment #219722 - Attachment is obsolete: true
Attachment #219730 - Flags: review?(mark)
Attachment #219722 - Flags: review?(mark)
Attached patch Patch v1 for Cocoa (obsolete) — Splinter Review
Here is the patch for Cocoa.

Since I don't know much about the cocoa environment, it hasn't been tested.
Attachment #219734 - Flags: review?(mark)
Comment on attachment 219730 [details] [diff] [review]
Patch v4 for MAC

Doesn't build, errors like:

/lizard/trunk/mozilla/widget/src/mac/nsMenuX.cpp: In member function 'virtual nsresult nsMenuX::Create(nsISupports*, const nsAString_internal&, const nsAString_internal&, nsIChangeManager*, nsIDocShell*, nsIContent*)':
/lizard/trunk/mozilla/widget/src/mac/nsMenuX.cpp:169: error: '_true' is not a member of 'nsWidgetAtoms'
/lizard/trunk/mozilla/widget/src/mac/nsMenuX.cpp:171: error: '_true' is not a member of 'nsWidgetAtoms'
/lizard/trunk/mozilla/widget/src/mac/nsMenuX.cpp: In member function 'void nsMenuX::LoadMenuItem(nsIMenu*, nsIContent*)':
/lizard/trunk/mozilla/widget/src/mac/nsMenuX.cpp:820: error: '_true' is not a member of 'nsWidgetAtoms'
/lizard/trunk/mozilla/widget/src/mac/nsMenuX.cpp:840: error: '_true' is not a member of 'nsWidgetAtoms'
/lizard/trunk/mozilla/widget/src/mac/nsMenuX.cpp:843: error: 'checkbox' is not a member of 'nsWidgetAtoms'

Did you mean to refer to some class other than nsWidgetAtoms?
Attachment #219730 - Flags: review?(mark) → review-
Attached patch Patch v5 for MAC and cocoa (obsolete) — Splinter Review
The patch contains the previous mac and cocoa patches. And add some MACROes to the nsWidgetAtomList.h.

I notice that the items in nsWidgetAtomList.h are disordered. Maybe we should make it in alphabet order.
Attachment #219730 - Attachment is obsolete: true
Attachment #219734 - Attachment is obsolete: true
Attachment #219834 - Flags: review?(mark)
Attachment #219734 - Flags: review?(mark)
Attached patch Patch v1 for xpwidgets (obsolete) — Splinter Review
Found another place where GetAttr can be replaced. Roc, is that an reasonable change?
Attachment #219858 - Flags: review?(roc)
Attached patch Patch v2 for xpwidgets (obsolete) — Splinter Review
One indent problem addressed.
Attachment #219858 - Attachment is obsolete: true
Attachment #219860 - Flags: review?(roc)
Attachment #219858 - Flags: review?(roc)
Attached patch Patch v6 for MAC and cocoa (obsolete) — Splinter Review
Sorry about the wrong patch v5 for MAC and cocoa. Here is the correct one.
Attachment #219834 - Attachment is obsolete: true
Attachment #219880 - Flags: review?(mark)
Attachment #219834 - Flags: review?(mark)
Comment on attachment 219860 [details] [diff] [review]
Patch v2 for xpwidgets

We don't want to use dynamic atoms like this, that's costly. The IsBottomTab change isn't really changing anything. The IsIndeterminateProgress is OK but it should really take an atom. I guess the problem is we have no atom collection in widget?
Attachment #219860 - Flags: review?(roc) → review-
(In reply to comment #43)
> We don't want to use dynamic atoms like this, that's costly. The IsBottomTab
> change isn't really changing anything. The IsIndeterminateProgress is OK but it
> should really take an atom. I guess the problem is we have no atom collection
> in widget?

My intent to change IsBottomTab is to get rid of the private method GetAttr in class nsNativeTheme.

The atom is used in lots of places in widget. And there is atom collection in public/nsWidgetAtomList.h. Do you suggest that we make use of it in the whole widget directory?
I guess we could add to nsWidgetAtomList. You could use #ifdefs to add some platform-specific atoms there.
Comment on attachment 219880 [details] [diff] [review]
Patch v6 for MAC and cocoa

>Index: widget/src/cocoa/nsNativeScrollbar.mm
>-    nsAutoString orient;
>-    mContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::orient, orient);
>-    if ( orient.Equals(NS_LITERAL_STRING("horizontal")) )
>+    if (mContent->->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::orient,

Oops!  ->->

>Index: widget/src/mac/nsMenuItemX.cpp
>-        nsAutoString value;
>-        mContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::autocheck, value);
>-        if (!value.EqualsLiteral("false"))
>+        if (!mContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::autocheck,
>+                                   nsWidgetAtoms::_false, eCaseMatters))

You didn't define nsWidgetAtoms::_false.
Attachment #219880 - Flags: review?(mark) → review-
Patch for the whole widget directory.

1. Reorganize the nsWidgetAtomList and add some atoms. Most of the atoms are used in the code for several different platforms. No #ifdef used.

2. Add include path to Makefile.in in gtk, gtk2, windows and xpwidgets directories. Don't know why mac doesn't need this.

3. The patch is built successfully on Windows and Solaris gtk2. For Mac, cocoa and gtk, don't have environment here.

4. roc, do I need to split the patch into several small ones according to different platform?
Attachment #219860 - Attachment is obsolete: true
Attachment #219880 - Attachment is obsolete: true
Attachment #220243 - Flags: review?(roc)
Comment on attachment 220243 [details] [diff] [review]
Patch v1 for widget

Mark, could you help me review the MAC and cocoa specifc parts of this patch? I can provide a separated one if you want.
Attachment #220243 - Flags: review?(mark)
Comment on attachment 220243 [details] [diff] [review]
Patch v1 for widget

r+ on the non-Mac chunks
Attachment #220243 - Flags: superreview+
Attachment #220243 - Flags: review?(roc)
Attachment #220243 - Flags: review+
Comment on attachment 220243 [details] [diff] [review]
Patch v1 for widget

Nice.  r=me in widget/src/mac and widget/src/cocoa.
Attachment #220243 - Flags: review?(mark) → review+
when stuff like this lands are there any performance tests done to see if we gain anything? Some ms here and some ms there all sums up.

Keep up the great job!
This turned balsa orange:

###!!! ASSERTION: Must have attr name: 'aName', file /builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/content/xul/content/src/nsXULElement.cpp, line 1298
  
  Program firefox-bin (pid = 1337) received signal 6.
  Stack:
  nsProfileLock::FatalSignalHandler(int)+0x0000014E [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libxul.so +0x0003B260]
  UNKNOWN [/lib/i686/libpthread.so.0 +0x000098D5]
  UNKNOWN [/lib/i686/libc.so.6 +0x0002E848]
  abort+0x0000015E [/lib/i686/libc.so.6 +0x0002FF82]
  PR_Assert+0x00000000 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libnspr4.so +0x00011737]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x001168E5]
  NS_DebugBreak_P+0x000002FE [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x001168B4]
  nsXULElement::AttrValueIs(int, nsIAtom*, nsAString_internal const&, nsCaseTreatment) const+0x0000004A [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x0085F5A8]
  nsNativeTheme::CheckBooleanAttr(nsIFrame*, nsIAtom*)+0x000000C7 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libwidget_gtk.so +0x000787A3]
  nsNativeTheme::IsDisabled(nsIFrame*)+0x00000030 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libwidget_gtk.so +0x0005DBD2]
  nsNativeThemeGTK::GetGtkWidgetAndState(unsigned char, nsIFrame*, GtkThemeWidgetType&, GtkWidgetState*, int*)+0x000001AB [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libwidget_gtk.so +0x0005CABF]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libwidget_gtk.so +0x0005CFEE]
  nsCSSRendering::PaintBackgroundWithSC(nsPresContext*, nsIRenderingContext&, nsIFrame*, nsRect const&, nsRect const&, nsStyleBackground const&, nsStyleBorder const&, nsStylePadding const&, int, nsRect*)+0x000001BA [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x002E6CB6]
  nsCSSRendering::PaintBackground(nsPresContext*, nsIRenderingContext&, nsIFrame*, nsRect const&, nsRect const&, nsStyleBorder const&, nsStylePadding const&, int, nsRect*)+0x0000010D [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x002E692F]
  nsDisplayBackground::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&)+0x000000F4 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x002F8D0E]
  nsDisplayList::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&) const+0x0000004D [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x002F805F]
  nsDisplayWrapList::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&)+0x00000035 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x002F94CD]
  nsDisplayClip::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&)+0x0000008C [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x002FA436]
  nsDisplayList::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&) const+0x0000004D [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x002F805F]
  nsLayoutUtils::PaintFrame(nsIRenderingContext*, nsIFrame*, nsRegion const&, unsigned int)+0x000002DF [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x00320B2F]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x003356BB]
  nsViewManager::RenderViews(nsView*, nsIRenderingContext&, nsRegion const&, nsIDrawingSurface*)+0x00000217 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x007AEA11]
  nsViewManager::Refresh(nsView*, nsIRenderingContext*, nsIRegion*, unsigned int)+0x000006E3 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x007AE1BB]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x007B074C]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x007A7029]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libwidget_gtk.so +0x00046D41]
  nsWidget::DispatchWindowEvent(nsGUIEvent*)+0x00000035 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libwidget_gtk.so +0x00046945]
  nsWindow::DoPaint(nsIRegion*)+0x000003A5 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libwidget_gtk.so +0x0004CFE3]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libwidget_gtk.so +0x0004D147]
  nsViewManager::UpdateWidgetsForView(nsView*)+0x00000073 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x007B29EB]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x007B38A1]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x007AF4CB]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x007B314D]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x007B32F7]
  nsEditor::EndUpdateViewBatch()+0x00000209 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libeditor.so +0x00134969]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libeditor.so +0x00129203]
  nsAutoPlaceHolderBatch::~nsAutoPlaceHolderBatch()+0x0000003B [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libeditor.so +0x000917DD]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libeditor.so +0x00112D6C]
  nsTextControlFrame::SetValue(nsAString_internal const&)+0x000005AF [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x0041EC0D]
  nsTextControlFrame::SetFormProperty(nsIAtom*, nsAString_internal const&)+0x00000078 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x0041B6CC]
  nsHTMLInputElement::SetValueInternal(nsAString_internal const&, nsITextControlFrame*)+0x000000F6 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x0066A0AA]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x00669E57]
  XPTC_InvokeByIndex+0x00000029 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x0011D8C5]
  XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)+0x000011C3 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libxpconnect.so +0x000A5607]
  XPCWrappedNative::SetAttribute(XPCCallContext&)+0x00000025 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libxpconnect.so +0x000B2889]
  XPC_WN_GetterSetter(JSContext*, JSObject*, unsigned int, long*, long*)+0x00000204 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libxpconnect.so +0x000B11B8]
  js_Invoke+0x00001189 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libmozjs.so +0x00059BE7]
  js_InternalInvoke+0x00000152 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libmozjs.so +0x0005A052]
  js_InternalGetOrSet+0x0000027B [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libmozjs.so +0x0005A397]
  js_SetProperty+0x000003EF [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libmozjs.so +0x000837B8]
  js_Interpret+0x0000CDD9 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libmozjs.so +0x0006892D]
  js_Invoke+0x0000121A [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libmozjs.so +0x00059C78]
  js_InternalInvoke+0x00000152 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libmozjs.so +0x0005A052]
  js_InternalGetOrSet+0x0000027B [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libmozjs.so +0x0005A397]
  js_SetProperty+0x000003EF [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libmozjs.so +0x000837B8]
  js_Interpret+0x0000CDD9 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libmozjs.so +0x0006892D]
  js_Invoke+0x0000121A [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libmozjs.so +0x00059C78]
  js_InternalInvoke+0x00000152 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libmozjs.so +0x0005A052]
  JS_CallFunctionValue+0x00000048 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libmozjs.so +0x000215F2]
  nsJSContext::CallEventHandler(JSObject*, JSObject*, unsigned int, long*, long*)+0x0000017E [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x007BEDE4]
  nsGlobalWindow::RunTimeout(nsTimeout*)+0x00000517 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x007DC999]
  nsGlobalWindow::TimerCallback(nsITimer*, void*)+0x00000037 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libgklayout.so +0x007DD59D]
  nsTimerImpl::Fire()+0x0000025B [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x0010B109]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x0010B35B]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x001051A5]
  NS_ProcessNextEvent_P(nsIThread*, int)+0x00000087 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libxpcom_core.so +0x0008C6C1]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libwidget_gtk.so +0x0005EA51]
  UNKNOWN [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/components/libtoolkitcomps.so +0x00023412]
  XRE_main+0x00002168 [/builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/obj/dist/bin/libxul.so +0x0002C56E]
  main+0x00000044 [firefox-bin +0x00000628]
  __libc_start_main+0x00000093 [/lib/i686/libc.so.6 +0x0001C507]
The orange might be relative to the Dep build. We need a clean build to test. Who can help me to do that on balsa?
Do you have a good reason to think it's a dependency problem?  Is there a particular file that didn't rebuild, but should have?
And whatever you do, please do NOT ask someone to clobber the entire tree; that takes forever and is probably unnecessary.
My guess is that the problem is that nothing calls nsWidgetAtoms::RegisterAtoms, and before this patch that was ok since nsWidgetAtoms were only used on mac.  Given that the atom memory is all uninitialized, I'm not sure how this patch appeared to work remotely correctly.
I backed out the checkin to fix the tinderbox orange; please add code to initialize nsWidgetAtoms correctly to the code for other platforms (other than mac and cocoa, where it is already initialized) before relanding.  I really don't see how in the world this would have worked correctly in testing.
This is the second time a patch was checked in from this bug turning tinderboxes red or orange and then just left, requiring action from other people to clean up the mess.

Again, when checking in big patches like this keep an eye on the tree until all tinderboxes has turned green!
(In reply to comment #54)
> Do you have a good reason to think it's a dependency problem?  Is there a
> particular file that didn't rebuild, but should have?
> 

Sorry for asking that without a reason.

I've built(gtk2) with the patch in my workspace. Without clobbering, the same ASSERT error shows. After rebuilding it, works well.

By changing "widget/src/xpwidgets/Makefile.in" to include nsWidgetAtoms.cpp to all the environments besides mac and cocoa, the nsWidgetAtoms can be successfully used.

It's really weird the nsWidgetAtoms can be used without initializing. I'll check on that.
(In reply to comment #58)
> This is the second time a patch was checked in from this bug turning
> tinderboxes red or orange and then just left, requiring action from other
> people to clean up the mess.
> 
> Again, when checking in big patches like this keep an eye on the tree until all
> tinderboxes has turned green!
> 

Thanks for pointing out that.

Next time I'll have somebody else back the patch out.
[ irc.mozilla.org, #developers ]
<Peter6> dbaron: should the backout of bug 333896 fix the problem with checkbox/radiobutton not displaying the checkmark (on Win32 nightly now) ?
<dbaron> Peter6, probably
<Peter6> dbaron: in that case, could we have a respin, so much checked in... it would be a pitty if the build would be "useless"
<dbaron> Peter6, ask on #build
Attached patch Patch v2 for widget (obsolete) — Splinter Review
Guys, sorry that the patch make so much trouble.

dbaron, thanks for pointing out the atom initialization problem. I've added the initialization in the gtk/gtk2/windows directory.

Reorganize the nsWidgetAtomList and add some atoms. Make use of "#if defined()" to classify the atoms by different widgets.
Attachment #222490 - Flags: review?(dbaron)
Sorry, didn't notice this review request while I was travelling.  That I backed out the previous patch doesn't make me the right reviewer for the new one; you may want to request review from someone else (roc?), although I'll probably get to it eventually.
Comment on attachment 222490 [details] [diff] [review]
Patch v2 for widget

Ask roc for review.

I've reorganized the Atoms list which I think is necessary and it will influence the mac part. So ask Mark to review it again.
Attachment #222490 - Flags: review?(roc)
Attachment #222490 - Flags: review?(mark)
Attachment #222490 - Flags: review?(dbaron)
Comment on attachment 222490 [details] [diff] [review]
Patch v2 for widget

Patch no longer applies after bug 290255.
Update the patch based on the new checkin for bug 290255.
Attachment #222490 - Attachment is obsolete: true
Attachment #224586 - Flags: review?(mark)
Attachment #222490 - Flags: review?(roc)
Attachment #222490 - Flags: review?(mark)
Attachment #224586 - Flags: review?(roc)
Comment on attachment 224586 [details] [diff] [review]
Patch v3 for widget

Mac portions look good.
Attachment #224586 - Flags: review?(mark) → review+
Thanks for the review. The patch has been checked in. I'll watch the tree carefully this time.
This patch still has a bunch of problems.  You added code in widget/src/xpwidgets/ that uses the atoms; therefore you should register the atoms on *all* platforms, not just gtk, gtk2, windows, mac, and cocoa.

Also, the patch has a bunch of makefile changes that add public directories to LOCAL_INCLUDES.  This should normally be unnecessary, since files in public are intended to be exported.  The problem here is that nsWidgetAtoms.h and nsWidgetAtomList.h are not exported.  They shouldn't be exported, since they can't be used outside the widget module.  Therefore, the better solution would be to move nsWidgetAtoms.h and nsWidgetAtomList.h to widget/src/xpwidgets/ and undo the changes to LOCAL_INCLUDES.  At this point, that should be a followup patch with separate review.
The patch addresses the problems David pointed out. Add the initializations to beos/os2/photon/qt/xlib.
Attachment #224981 - Flags: review?(dbaron)
Attachment #224981 - Flags: review?(dbaron) → review+
Attachment #224981 - Flags: superreview?(roc)
Is there any logic to the #ifdef grouping of these atoms? I've had to move some around to get my native theme work to compile again, as some that are in the Mac group are needed on Win. Are you trying to only have an atom defined if it is needed? Should I #ifdef the moved ones to be only OS X + Win or does it not matter?
Atoms shouldn't be ifdefed.  It's a recipe for build bustage.
It's hard to only define the atoms needed. I put the atoms into three categories: 1) MACOSX only; 2) GTK,GTK2 only; 3) Shared by all platforms. If the atom is needed by WIN and MACOSX, we'd better share it I think.

(In reply to comment #72)
> Atoms shouldn't be ifdefed.  It's a recipe for build bustage.
> 

David, you mean that we should share all the atoms instead of using #ifdef? I think it can help to reduce some memory space been used in some platforms.
Yes, please just define all the atoms on all platforms.
Reorganize the atoms list.
Attachment #225142 - Flags: superreview?(roc)
Attachment #225142 - Flags: review?(dbaron)
Attachment #224981 - Flags: superreview?(roc)
Comment on attachment 225142 [details] [diff] [review]
Followup patch v2 for widget

r=dbaron

It might be nice not to label this as "patch v5 for widget" since it's actually it's not a new version of the patch in version 3; it's a separate patch.
Attachment #225142 - Flags: review?(dbaron) → review+
Attachment #224981 - Attachment description: Patch v4 for widget → Followup patch v1 for widget
Attachment #225142 - Attachment description: Patch v5 for widget → Followup patch v2 for widget
Attachment #225142 - Flags: superreview?(roc) → superreview+
Depends on: 341321
=>Fixed

Patch checked in by leon.

Great thanks for all the reviewers!
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.