54203 - Command dispatcher needs to be independent from XUL

Status: RESOLVED FIXED

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

Description

[Doug Turner (:dougt)]

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.

Comment 1

[Brian King [:kinger]]

Is there any plans for this one?

Comment 2

[Peter Trudelle]

Jud, what is the process for getting approval to work on embedding bugs?

Comment 3

[Peter Trudelle]

Marking embed+, need to fix for embedding, okay to work on as long as it doesn't
affect seamonkey

Comment 4

[Judson Valeski]

we use the P field to indicate priority in the embedding world. I'm upping this 
to P1.

Comment 5

[Judson Valeski]

putting back, hyatt is using that token.

Comment 6

[David Hyatt]

Attachment: Patch to remove XUL dependencies.

Comment 7

[David Hyatt]

Ignore the first  patch.  A second one is coming shortly.

Comment 8

[David Hyatt]

Attachment: Second patch. Now it really works. :)

Comment 9

[David Hyatt]

Ok.  This is ready for an r= and an a=.

Comment 10

[saari (gone)]

r=saari

Comment 11

[Scott Collins]

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 <[e-mail]> (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 :-)

Comment 12

[David Hyatt]

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

Comment 13

[David Hyatt]

Attachment: Patch that incorporates scc's corrections.

Comment 14

[David Hyatt]

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.)

Comment 15

[Brendan Eich [:brendan]]

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=[e-mail] otherwise.

/be

Comment 16

[David Hyatt]

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.

Comment 17

[Scott Collins]

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|.

Comment 18

[Scott Collins]

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.