201560 - convert global window controller to use commandtable (was commandManager)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kathleen :Brade]

The global window controller for commands needs to be converted to use
nsIControllerCommands which will give us more flexibility.

Comment 1

[Kathleen :Brade]

Attachment: patch v3

Comment 2

[Simon Fraser [no longer active]]

+#define NS_WINDOWCOMMANDTABLE_CID \
+ { /* 0DE2FBFA-6B7F-11D7-BBBA-0003938A9D96 */        \
+  0x0DE2FBFA, 0x6B7F, 0x11D7, {0xBB, 0xBA, 0x00, 0x03, 0x93, 0x8A, 0x9D, 0x96} }
+
+#define NS_WINDOWCONTROLLER_CID        \
+ { /* 7BD05C78-6A26-11D7-B16F-0003938A9D96 */       \
+  0x7BD05C78, 0x6A26, 0x11D7, {0xB1, 0x6F, 0x00, 0x03, 0x93, 0x8A, 0x9D, 0x96} }
+
+#define NS_WINDOWCONTROLLER_CONTRACTID "@mozilla.org/dom/window-controller;1"
+
+

None of these need to go into a header that's visible outside this library.
NS_WINDOWCOMMANDTABLE_CID can be internal to the module file.


+#include "nsITransferableOverrides.h"

Should we try to do this patch independently of the transferable override stuff
first?

+	nsWindowCommands.cpp    \
I think i'd prefer nsGlobalWindowCommands.cpp

+      context->SetCommandRefCon(nsnull);

this is SetCommandContext now.

+  NS_REGISTER_FIRST_COMMAND(nsSelectionCommands, sScrollTopString);
+  NS_REGISTER_NEXT_COMMAND(nsSelectionCommands, sScrollPageUpString);
+  NS_REGISTER_NEXT_COMMAND(nsSelectionCommands, sScrollPageDownString);
+  NS_REGISTER_NEXT_COMMAND(nsSelectionCommands, sMovePageUpString);
+  NS_REGISTER_NEXT_COMMAND(nsSelectionCommands, sMovePageDownString);
+  NS_REGISTER_NEXT_COMMAND(nsSelectionCommands, sScrollLineUpString);

alecf said in another bug that we should try to do this in a loop, for smaller
code size.
+    nsCOMPtr<nsIControllerContext> controllerContext =
do_QueryInterface(controller);
+    if (controllerContext)
+      controllerContext->SetCommandRefCon(windowAsISupports);

Now SetCommandContext
+    else
+      return NS_ERROR_FAILURE;

Prefer early return.


+NS_IMETHODIMP
+nsSelectionCommands::DoCommand(const char *aCommandName,
+                               nsISupports *aCommandRefCon)
+{
+  nsCOMPtr<nsIDOMWindow> window = do_QueryInterface(aCommandRefCon);
+  NS_ENSURE_TRUE(window, NS_ERROR_INVALID_ARG);       
+
+  nsCOMPtr<nsISelectionController> selCont;
+  nsresult rv = GetSelectionControllerFromWindow(window, getter_AddRefs(selCont));
+  // if we can't get a selection controller, that's bad
+  NS_ENSURE_SUCCESS(rv, rv);
+  NS_ENSURE_TRUE(selCont, NS_ERROR_NOT_INITIALIZED);       
+
+  PRBool doBrowseWithCaret = PR_FALSE;
+  nsCOMPtr<nsIEventStateManager> esm;
+  GetEventStateManagerForWindow(window, getter_AddRefs(esm));
+  if (esm)
+    esm->GetBrowseWithCaret(&doBrowseWithCaret);
+  
+  rv = NS_ERROR_FAILURE;
+  if (!nsCRT::strcmp(aCommandName, sScrollTopString))   
+    rv = (doBrowseWithCaret? selCont->CompleteMove(PR_FALSE, PR_FALSE)
+                           : selCont->CompleteScroll(PR_FALSE));
+  else if (!nsCRT::strcmp(aCommandName,sScrollBottomString))
+    rv = (doBrowseWithCaret? selCont->CompleteMove(PR_TRUE, PR_FALSE)

Sigh, it would be great to clean up this mess. Maybe give nsSelectionCommands a
couple of protected methods, one of which handles browsewithcaret mode, and one
not? Also, the fewer string compares you can do here, the better.

+nsresult
+GetClipboardFeedbackInterfaceFromRefCon(nsISupports *aRefCon,
nsIContentViewerEdit **aEditInterface)

Prefer '...FromContext'.

Some C++ inheritance would help reduce code duplication between all the
nsClipboard* commands.

+NS_IMETHODIMP
+nsWebNavigationCommands::IsCommandEnabled(const char * aCommandName,
+                                          nsISupports *aCommandRefCon,
+                                          PRBool *outCmdEnabled)

I think it's worth splitting 'back' and 'forward' into separate commands.

+static nsresult
+CreateWindowCommandTableConstructor(nsISupports *aOuter,
+                                    REFNSIID aIID, void **aResult)

+static nsresult
+CreateWindowControllerWithSingletonCommandTable(nsISupports *aOuter,

These will have to be
static NS_METHOD
or Windows will complain about __stdcall crap.

Comment 3

[Simon Fraser [no longer active]]

Attachment: global window controller patch

This patch doesn't contain any of transferable changes that are in the previous
one. It builds, but is currently untested.

Comment 4

[Kathleen :Brade]

Comment on attachment 120156 [details] [diff] [review]
global window controller patch

r=brade although I'd have made some of your command helper methods take the
context rather than doing the QI and then passing the window to the helper
methods.

Comment 5

[Simon Fraser [no longer active]]

Attachment: Updated patch, fixes nsClipboardBaseCommand to call the right methods

This patch is tested, and works!  :)

Kathy: I wanted to keep the code as small as I could, so did the QIs as few
times as possible.

Comment 6

[Kathleen :Brade]

Comment on attachment 120160 [details] [diff] [review]
Updated patch, fixes nsClipboardBaseCommand to call the right methods

r=brade

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 120160 [details] [diff] [review]
Updated patch, fixes nsClipboardBaseCommand to call the right methods

- In nsGlobalWindowCommands.cpp:

+const char * const sCopyString = "cmd_copy";
+const char * const sCutString = "cmd_cut";
+const char * const sPasteString = "cmd_paste";
...

You should make these static const char arrays and not pointers to string data,
that way they'll end up in the readonly section in the library and can be
shared among processes. I.e.:

+static const char sCopyString[] = "cmd_copy";
+static const char sCutString[] = "cmd_cut";
+static const char sPasteString[] = "cmd_paste";
...

+nsSelectionCommandsBase::IsCommandEnabled(const char * aCommandName,
+				       nsISupports *aCommandContext,
+				       PRBool *outCmdEnabled)

Fix the indentation of next-line arguments, there's a bunch of these, fix all
of them.

- In nsSelectionCommandsBase::GetPresShellFromWindow():

+  NS_ENSURE_SUCCESS(docShell->GetPresShell(aPresShell), NS_ERROR_FAILURE);
+  return NS_OK;

Why not just return docShell->GetPresShell() directly, do you really need the
warning from NS_ENSURE?

- In nsSelectionCommandsBase::GetSelectionControllerFromWindow():

+    nsCOMPtr<nsISelectionController> selController =
do_QueryInterface(presShell);
+    if (selController) 
+    {
+      *aSelCon = selController;
+      NS_ADDREF(*aSelCon);

Why not cal CallQueryInterface(presShell, aSelCon) here and avoid the extra
nsCOMPtr?

- In nsSelectionCommandsBase::GetEventStateManagerForWindow():

+      nsCOMPtr<nsIEventStateManager> esm;
+      presContext->GetEventStateManager(getter_AddRefs(esm));
+      *aEventStateManager = esm;
+      if (esm)
+      {
+	 NS_ADDREF(*aEventStateManager);
+	 return NS_OK;

Same here, no need for esm, QI directly into aEventStateManager.

- In nsClipboardCutCommand::IsClipboardCommandEnabled():

+{
+  *outCmdEnabled = PR_FALSE;
+  return aEdit->GetCutable(outCmdEnabled);

Setting *outCmdEnabled to PR_FALSE here should not be needed. Unless
aEdit->GetCuttable() is broken, it should always set it to the right value,
callers shouldn't need to care (even in failure cases). Same thing in a bunch
of these clipboard related methods.

sr=jst with those changes.

Comment 8

[Simon Fraser [no longer active]]

Attachment: Patch addressing comments, and changing nsGlobalChromeWindow::SetCursor for code size reduction

Comment 9

[Simon Fraser [no longer active]]

Checked in.

Comment 10

[Simon Fraser [no longer active]]

Attachment: Final bug fixes

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 120608 [details] [diff] [review]
Final bug fixes

sr=jst