609789 - inBaseCommand for all your transaction prototype needs

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Colby Russell :crussell (no longer Mozilla-ing)]

From bug 607080, comment 7:

grepping for txnMerge shows 26 consumers (1 occurrence for the definition), with all of them roughly reproducing this boilerplate:

./resources/content/viewers/dom/dom.js-  // required for nsITransaction
./resources/content/viewers/dom/dom.js-  QueryInterface: txnQueryInterface,
./resources/content/viewers/dom/dom.js-  merge: txnMerge,
./resources/content/viewers/dom/dom.js:  isTransient: true,
./resources/content/viewers/dom/dom.js-  redoTransaction: txnRedoTransaction,

Additionally, from bug 588071 and bug 606821, there are 6 providers for cmdEditInspectInNewWindow.  All of them essentially look like the implementation found in dom.js:

> function cmdEditInspectInNewWindow()
> {
>   this.mNode = viewer.selectedNode;
> }
> 
> cmdEditInspectInNewWindow.prototype = {
>   isTransient: true,
>   merge: txnMerge,
>   QueryInterface: txnQueryInterface,
> 
>   doTransaction: function InspectInNewWindow_DoTransaction()
>   {
>     if (this.mNode) {
>       inspectObject(this.mNode);
>     }
>   }
> };

With baseCommands.js, these can all be reduced to their ~three- to four-line constructors (and the line to set the prototype).

With this patch, there will be two cmdEditCopyFileURI/cmdEditViewFileURI providers.

utils.js currently contains the definitions for txnMerge, txnQueryInterface, and txnRedoTransaction, and a copy transaction.  With the addition of the three new base classes, the code mentioned has enough in common and is sizeable enough to split it off into a pseudo-module concerned with command implementations, leaving utils.js to contain the sort of miscellaneous things that remain.

Comment 1

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 1 - clean up computedStyle.xul

Comment 2

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 2 - make some required additions to inBaseCommand

Comment 3

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 3 - switch txn* consumers over to inBaseCommand

Comment 4

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 4 - get rid of the rest of the transaction stuff in utils.js, moving to baseCommands.js where appropriate

Comment 5

[[e-mail]]

Comment on attachment 490058 [details] [diff] [review]
phase 2 - make some required additions to inBaseCommand

I take it most "transactions" are transient?

Comment 6

[[e-mail]]

Comment on attachment 490059 [details] [diff] [review]
phase 3 - switch txn* consumers over to inBaseCommand

Any chance of a -w diff?

>+cmdEditInspectInNewWindow.prototype = new inBaseCommand();
Shouldn't this use cmdEditInspectInNewWindowBase?

Comment 7

[[e-mail]]

Comment on attachment 490060 [details] [diff] [review]
phase 4 - get rid of the rest of the transaction stuff in utils.js, moving to baseCommands.js where appropriate

>+// TODO: Move these (or similar) to jsutil/system/clipboardFlavors.js or
>+// something as part of bug 328878.
What does this refer to?

Comment 8

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #5)
> Comment on attachment 490058 [details] [diff] [review]
> phase 2 - make some required additions to inBaseCommand
> 
> I take it most "transactions" are transient?

Nope.

styleRules.js:cmdEditInsert.prototype = new inBaseCommand(false);
styleRules.js:cmdEditDelete.prototype = new inBaseCommand(false);
styleRules.js:cmdEditEdit.prototype = new inBaseCommand(false);
styleRules.js:cmdTogglePriority.prototype = new inBaseCommand(false);
accessibleProps.js:cmdEditInspectInNewWindow.prototype = new inBaseCommand();
domNode.js:cmdEditCut.prototype = new inBaseCommand(false);
domNode.js:cmdEditPaste.prototype = new inBaseCommand(false);
domNode.js:cmdEditInsert.prototype = new inBaseCommand(false);
domNode.js:cmdEditDelete.prototype = new inBaseCommand(false);
domNode.js:cmdEditEdit.prototype = new inBaseCommand(false);
domNode.js:cmdEditTextValue.prototype = new inBaseCommand(false);
dom.js:cmdEditDelete.prototype = new inBaseCommand(false);
dom.js:cmdEditCut.prototype = new inBaseCommand(false);
dom.js:cmdEditCopy.prototype = new inBaseCommand();
dom.js:cmdEditPaste.prototype = new inBaseCommand(false);
dom.js:cmdEditPasteBefore.prototype = new inBaseCommand(false);
dom.js:cmdEditPasteReplace.prototype = new inBaseCommand(false);
dom.js:cmdEditPasteFirstChild.prototype = new inBaseCommand(false);
dom.js:cmdEditPasteLastChild.prototype = new inBaseCommand(false);
dom.js:cmdEditPasteAsParent.prototype = new inBaseCommand(false);
dom.js:InsertNode.prototype = new inBaseCommand(false);
baseCommands.js:cmdEditInspectInNewWindowBase.prototype = new inBaseCommand();
baseCommands.js:cmdEditCopySimpleStringBase.prototype = new inBaseCommand();
baseCommands.js:cmdEditCopy.prototype = new inBaseCommand();
baseCommands.js:cmdEditViewFileURIBase.prototype = new inBaseCommand();

I waffled on this a bit.  I went with opt-in for non-transient because
1. I had already gone for transient-is-default for Bug 607080.
2. It seemed like it makes sense to opt-in for undo and redo to actually mean anything, rather than opting out (to imply undo and redo would be no-ops).

I don't care either way, though, really.

(In reply to comment #7)
> Comment on attachment 490060 [details] [diff] [review]
> phase 4 - get rid of the rest of the transaction stuff in utils.js, moving to
> baseCommands.js where appropriate
> 
> >+// TODO: Move these (or similar) to jsutil/system/clipboardFlavors.js or
> >+// something as part of bug 328878.
> What does this refer to?

The CSSDeclaration and the DOMAttribute flavors.  The context lines in the patch don't really show this.

Comment 9

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: phase 3 - -w diff

> >+cmdEditInspectInNewWindow.prototype = new inBaseCommand();
> Shouldn't this use cmdEditInspectInNewWindowBase?

That'd be helpful, but Inspect in New Window is implemented differently for that viewer.  There's some "PropViewerMgr" layer to go through there, and the viewer offloads the logic to it.

Comment 10

[[e-mail]]

Comment on attachment 490059 [details] [diff] [review]
phase 3 - switch txn* consumers over to inBaseCommand

>+  if (this.mPropViewerMgr) {
>+    this.mPropViewerMgr.inspectInNewView();
I see what you mean; there's no method to get the selected object. (Although I suspect it wouldn't be to difficult to arrange, since it looks like a case of renaming the inspectInNewView methods and making them return the object.)

Comment 11

[[e-mail]]

Comment on attachment 490060 [details] [diff] [review]
phase 4 - get rid of the rest of the transaction stuff in utils.js, moving to baseCommands.js where appropriate

>+  QueryInterface:
>+    XPCOMUtils.generateQI([Components.interfaces.nsITransaction]),
>+ 
Nit: there's a space on this "blank" line.

Comment 12

[Colby Russell :crussell (no longer Mozilla-ing)]

Pushed:
http://hg.mozilla.org/dom-inspector/rev/98dc299d6aeb
http://hg.mozilla.org/dom-inspector/rev/647291885c53
http://hg.mozilla.org/dom-inspector/rev/c5842a5f3d7e
http://hg.mozilla.org/dom-inspector/rev/e04610722831

Comment 13

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: remove TODO item mentioning this bug

This was left in.

Comment 14

[Colby Russell :crussell (no longer Mozilla-ing)]

Pushed:
http://hg.mozilla.org/dom-inspector/rev/c1674499b69d