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)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: Brade, Assigned: Brade)
References
Details
(Keywords: topembed)
Attachments
(3 files, 2 obsolete files)
60.45 KB,
patch
|
Brade
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
65.53 KB,
patch
|
Details | Diff | Splinter Review | |
1.51 KB,
patch
|
jst
:
superreview+
|
Details | Diff | Splinter Review |
The global window controller for commands needs to be converted to use nsIControllerCommands which will give us more flexibility.
Assignee | ||
Comment 1•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #120121 -
Flags: review?(sfraser)
Comment 2•21 years ago
|
||
+#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•21 years ago
|
||
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
Assignee | ||
Comment 4•21 years ago
|
||
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+
Assignee | ||
Updated•21 years ago
|
Attachment #120121 -
Flags: review?(sfraser)
Assignee | ||
Updated•21 years ago
|
Attachment #120156 -
Flags: superreview?(jst)
Comment 5•21 years ago
|
||
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
Updated•21 years ago
|
Attachment #120160 -
Flags: superreview?(jst)
Assignee | ||
Comment 6•21 years ago
|
||
Comment on attachment 120160 [details] [diff] [review] Updated patch, fixes nsClipboardBaseCommand to call the right methods r=brade
Attachment #120160 -
Flags: review+
Assignee | ||
Updated•21 years ago
|
Attachment #120156 -
Flags: superreview?(jst)
Comment 7•21 years ago
|
||
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+
Comment 8•21 years ago
|
||
Comment 9•21 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 10•21 years ago
|
||
Comment 11•21 years ago
|
||
Comment on attachment 120608 [details] [diff] [review] Final bug fixes sr=jst
Attachment #120608 -
Flags: superreview+
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•