Closed Bug 54203 Opened 20 years ago Closed 19 years ago

Command dispatcher needs to be independent from XUL

Categories

(Core :: DOM: Navigation, defect, P1, blocker)

x86
Linux
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: dougt, Assigned: hyatt)

References

Details

(Keywords: embed, Whiteboard: [embed+])

Attachments

(3 files)

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.
Severity: normal → blocker
Keywords: embed
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
Whiteboard: [embed+]
Target Milestone: --- → M19
we use the P field to indicate priority in the embedding world. I'm upping this 
to P1.
Priority: P3 → P1
Whiteboard: [embed+]
putting back, hyatt is using that token.
Whiteboard: [embed+]
Ignore the first  patch.  A second one is coming shortly.
Status: NEW → ASSIGNED
Ok.  This is ready for an r= and an a=.
r=saari
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 <hyatt@netscape.com> (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?

sr=brendan@mozilla.org 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: 19 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.