Closed Bug 54203 Opened 21 years ago Closed 21 years ago
Command dispatcher needs to be independent from XUL
102.72 KB, patch
|Details | Diff | Splinter Review|
104.22 KB, patch
|Details | Diff | Splinter Review|
99.38 KB, patch
|Details | Diff | Splinter Review|
We need to move the command dispatcher so that it is not dependant on XUL. This is so that we can removed XUL, RDF, and Chrome and still have focus management.
Is there any plans for this one?
Jud, what is the process for getting approval to work on embedding bugs?
Marking embed+, need to fix for embedding, okay to work on as long as it doesn't affect seamonkey
Target Milestone: --- → M19
we use the P field to indicate priority in the embedding world. I'm upping this to P1.
Priority: P3 → P1
putting back, hyatt is using that token.
Ignore the first patch. A second one is coming shortly.
Status: NEW → ASSIGNED
Ok. This is ready for an r= and an a=.
Lots of really good refactoring here. I like this patch. However: spelling error: "focus controllerer" wrong license in "nsIFocusController.h" --- should be MPL wrong license in "nsPIWindowRoot.h" --- should be MPL wrong license in "nsFocusController.cpp" --- should be MPL wrong license in "nsFocusController.h" --- should be MPL wrong license in "nsWindowRoot.cpp" --- should be MPL wrong license in "nsWindowRoot.h" --- should be MPL in all six cases, the preferred form for legal reasons and to better support scripts that parse file headers is: Contributors: David W. Hyatt <firstname.lastname@example.org> (original author) Please do not edit the boiler-plate to add an `Original Author:' section. By the way, do all your new files end with a newline? The patch makes it look like not all of them do. Ugh. Making a parameter list be |(void)| is an ugly C convention (in my opinion). In C, an empty parameter list essentially means the same thing as |(...)|. In C++, this is not the case. So putting something _in_ the parameter list, even |void|, is counter-intuitive. It increases the brain-print by making the reader have to _look_ to determine that the function actually takes _no_ parameters, even though there is something in the list. I also notice that you're not even consistent about this. E.g., |nsWindowRoot::~nsWindowRoot()|. Prefer the combined form NS_ADDREF(*aResult = controller); |NS_ADDREF| is specially designed to make this more efficient in the optimized case (in spite of Warren's comments). Similarly |NS_IF_ADDREF(*aWindow = mCurrentWindow);| You have many sites that will benefit from these two patterns. Don't, however, use it for, e.g., |NS_ADDREF(*aResult = new nsWindowRoot(aWindow));|. That introduces a leak into debug (only) builds. In |nsFocusController::UpdateCommands|, you might want to add a |#else| clause that either uses |#pragma unused| or actually does something with the parameter to reduce warnings. Prefer direct-initialization over copy-initialization, e.g., say nsCOMPtr<nsIDOMXULElement> xulElement( do_QueryInterface(... rather than using the |=| form. Similarly with |getter_Addrefs|, et al. You have many sites that need this. |nsPromiseFlatString| may do work. Evaluate whether you want this work to occur in the case that |command| is not used. There a lot of early returns in this function (|GetControllerForCommand|) and no comments explaining the different alternatives that are being explored; are you happy with the way it is structured? In |GetSuppressFocus|, why not *aSuppressFocus = mSupressFocus; ? Ah, because mSupressFocus is a `depth'. In that case, why not *aSupressFocus = (mSupressFocus > 0); That makes it obvious locally how |mSupressFocus| is being used. You don't _need_ to mark an |nsCOMPtr| member as |// [OWNER]| since that is explicit in the type. All |nsCOMPtr|s are owners. You don't have to change this for approval though. Looks like you might have some rogue tabs in "nsFocusController.h" Ugh. Old-style casts. Prefer |NS_STATIC_CAST(nsIDOMWindow*, this)| Prefer |do_CreateInstance| to calling |CreateInstance(..., getter_AddRefs(x))|. It's more efficient; it's typesafe (no need to say |NS_GET_IID| of anything) plus, you won't have to pass that ugly extra |nsnull| parameter. It is possible to say nsCOMPtr<nsIScriptObjectOwner> owner( do_QueryInterface( winRoot ? boundGlobal : aReceiver ) ); (forgive my formatting to make it fit this comment) assuming the types of |boundGlobal| and |aReceiver| are the same (or you can |NS_STATIC_CAST| them. This is mildly more efficient, and may be easier to read ... your call. In |EnsureFocusController| there is a serious COM violation. You may hold a weak reference, but you're not allowed to get it by taking an |AddRef|ed pointer and simply |Release()|ing it, as you've done here. Rather, you need an explicit routine that provides you with a weak reference. Sorry :-( I hope you don't use this pattern a lot. I look forward to your updated patch :-)
Ok, addressing some of this. (1) Several of your criticisms are levied at DOM IDLC-generated code. I can't do anything about that. (2) Much of your criticism is levied at code that is *moving* from the command dispatcher to focus controller. This code shows up in the diff, but it's just moving, not changing. I'm reluctant to make any changes to this code prior to checking in, since I know the code as written is well-tested and working properly. (3) I do not believe that there is a COM violation in EnsureFocusController. The lifetime of the focus controller is well-known, i.e., it is guaranteed to live longer than I do
This latest patch switches the licenses, incorporates all of the suggestions (e.g., use of void in parameter list, static casts, copy init vs. assignment init for do_QueryInterface, usage of do_CreateInstance instead of CreateInstance, the suppressFocus and scriptObjectOwner suggestions, et. al.)
scc, I wonder if you aren't seeing only crappy gcc -g or even -O (but not -O2) code that's slightly better for: NS_ADDREF(*_retval = aComPtr); compared to *_retval = aComPtr; NS_ADDREF(*retval); I don't see why any decent compiler will generate different code for these two sequences. I've already nagged hyatt to use XPIDL even for unscriptable interfaces, for better doxygen and similar language-neutral-tool results. There are tabs in the diff on + lines, probably because MSVC doesn't expand unless you shift the line right and left, or otherwise muck with the whole line. No big, fix if you can and everyone keep expunging tabs a bit at a time. Sounds like a Mac buddy may be needed -- or is camelot+tinderbox enough? email@example.com otherwise. /be
Fixed. Embedding guys, you should open a new bug against yourselves to figure out what to do with the activate/deactivate code in nsWebShellWindow. Focus is still going to have lots of glitches until you guys figure out what to do with the activate/deactivate code in webshellwindow.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Sorry I fell behind on this one; thanks for picking up my slack, Brendan. I'll do a comparison of the combined forms versus the split forms for |NS_ADDREF| and |NS_IF_ADDREF|.
With the optimizations we use for release builds, on the latest gcc and mac, the generated code shows no reason to prefer the combined form over the two step assign and |AddRef|. VC++ has a better optimizer than either of the compilers I tested with, so I imagine it does at least as well. I'll stop recommending the combined form.
You need to log in before you can comment on or make changes to this bug.