Simon Montagu wrote:
> Steve, if you, Chris Waterson and Kevin McCluskey could send more
> detailed comments directly to me at [EMAIL PROTECTED] it would be
> very helpful.
>
> Best regards,
> Simon
From jst:
==============================================
I'll be over in Finland the following two weeks so I won't be able to
make it on thursday but I did have a look at the patch and it seems like
there's not a whole lot of changes to DOM code so feel free to proceed
w/o me, I'll be reading email so if there's a need to contact me just
send me email.
Here are my comments about the DOM related changes:
nsDocument.cpp, the only changes to that file is:
+#ifdef OLDIBMBIDI
+#include "nsIPref.h"
+#include "nsIServiceManager.h"
+#include "nsIDocumentViewer.h"
+#include "nsIUBidiUtils.h"
+#include "nsIWebShell.h"
+
+static NS_DEFINE_CID(kPrefCID, NS_PREF_CID);
+static NS_DEFINE_IID(kIWebShellIID, NS_IWEB_SHELL_IID);
+#endif // IBMBIDI
+
and that seems like a completely unnecessary change to me, OLDIBMBIDI
doesn't seem to be used anywhere else so the whole change could AFAIK,
if not then someone better explain why this change is in the diff.
nsDocument.h has some changes that add inline implementations of virtual
methods, please move the implementations into the .cpp file.
This one I just noticed when browsing the diff, what's up with this
weird indentation?
+
+nsIFrame*
+ nsFrameList::GetNextVisualFor(nsIFrame* aFrame) const
+{
In nsHTMLSpanElement.cpp, this is no big deal but the code could be
simplified a bit (and sped up ever so slightly):
...
+
+#ifdef IBMBIDI
+ // Instead, we could derive class nsHTMLBdoElement : public
nsHTMLSpanElement
+ // But this would add more code...
remove >+ nsCOMPtr<nsIAtom> tag;
remove >+ GetTag(*getter_AddRefs(tag) );
+
change >+ if (mNodeInfo->Equals(nsHTMLAtoms::bdo))
+ aMapFunc = &MapBdoAttributesInto;
+ else
+#endif // IBMBIDI
aMapFunc = &MapAttributesInto;
return NS_OK;
}
In nsHTMLDocument.cpp there's this line that looks wrong
+ if (charset.RFind("864", PR_TRUE )) {
First of all, why not use the faster Find() in stead of RFind()? And why
do an expensive case-insensitive search when you're looking for numbers?
Also, it looks like the assumption is that RFind() returns true or false
depending on whether the string is found or not but that's not what it
does, it returns the index in the string where the substring was found,
or -1 if the result isn't found. The same is true for Find().
nsComboboxControlFrame.cpp, line ~510, why this weird indentation change?
if (presShell) {
- presShell->ScrollFrameIntoView(this,
+ presShell->ScrollFrameIntoView(this,
NS_PRESSHELL_SCROLL_IF_NOT_VISIBLE,NS_PRESSHELL_SCROLL_IF_NOT_VISIBLE);
- }
}
}
+}
And a similar thing here:
@@ -2015,7 +2076,7 @@
const nsStyleUserInterface* uiStyle;
GetStyleData(eStyleStruct_UserInterface, (const nsStyleStruct
*&)uiStyle);
if (uiStyle->mUserInput == NS_STYLE_USER_INPUT_NONE ||
uiStyle->mUserInput == NS_STYLE_USER_INPUT_DISABLED)
- return nsAreaFrame::HandleEvent(aPresContext, aEvent, aEventStatus);
+ return nsAreaFrame::HandleEvent(aPresContext, aEvent, aEventStatus);
return NS_OK;
}
And yet more such changes around line 2530.
... and one in nsFormFrame.cpp around line 905. Please undo the weird
and confusing indentation changes.
Other than these issues I'm ok with the DOM and content model related
changes.
>
