Closed Bug 201560 Opened 21 years ago Closed 21 years ago

convert global window controller to use commandtable (was commandManager)

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Brade, Assigned: Brade)

References

Details

(Keywords: topembed)

Attachments

(3 files, 2 obsolete files)

The global window controller for commands needs to be converted to use
nsIControllerCommands which will give us more flexibility.
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #120121 - Flags: review?(sfraser)
+#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.
Attached patch global window controller patch (obsolete) — Splinter Review
This patch doesn't contain any of transferable changes that are in the previous
one. It builds, but is currently untested.
Attachment #120121 - Attachment is obsolete: true
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.
Attachment #120156 - Flags: review+
Attachment #120121 - Flags: review?(sfraser)
Attachment #120156 - Flags: superreview?(jst)
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.
Attachment #120156 - Attachment is obsolete: true
Attachment #120160 - Flags: superreview?(jst)
Comment on attachment 120160 [details] [diff] [review]
Updated patch, fixes nsClipboardBaseCommand to call the right methods

r=brade
Attachment #120160 - Flags: review+
Attachment #120156 - Flags: superreview?(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.
Attachment #120160 - Flags: superreview?(jst) → superreview+
Checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment on attachment 120608 [details] [diff] [review]
Final bug fixes

sr=jst
Attachment #120608 - Flags: superreview+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: