76580 - Fix misplaced <command id="cmd_copyLink"/>, etc. in mailnews

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Scott MacGregor]

dr, I think your changes for the cmd_link stuff last night is causing some
assertions in mailnews when we build the edit menu. 

We assert for each of the new image link commands when the command dispatcher
attempts to ask if the command is enabled. Stack trace coming up:

Comment 1

[Scott MacGregor]

NTDLL! 77f9eea9()
nsDebug::Assertion(const char * 0x02934154, const char * 0x0293413c, const char
* 0x02934108, int 0x000013c0) line 286 + 13 bytes
nsDebug::WarnIfFalse(const char * 0x02934154, const char * 0x0293413c, const
char * 0x02934108, int 0x000013c0) line 392 + 21 bytes
DocumentViewerImpl::GetInLink(DocumentViewerImpl * const 0x022e43fc, int *
0x0012b0c4) line 5056 + 39 bytes
nsDOMWindowController::IsCommandEnabled(nsDOMWindowController * const
0x05f1e658, const nsAString & {...}, int * 0x0012b0c4) line 4569 + 36 bytes


We assert in GetInImage because there is no popup node. 

I see the assertion 3 or 4 times in a row before we build the edit menu.

Comment 2

[Dan Rosen]

crap, that's weird. i'll look into it. thanks.

Comment 3

[Dan Rosen]

Oh, okay. I have the <command>s in the edit menu, analogous to where I added
them in navigatorOverlay.xul, per jag's recommendation. There's no popup node
because we're not in the content area -- those <command>s should obviously be
somewhere completely different...

jag, your overlay-fu is stronger than mine -- can you help me figure this out?

Comment 4

[Peter "jag" Annema]

Are NS_ENSURE_SUCCESS and NS_ENSURE_TRUE correctly used here?

With those you're asserting in GetInLink that you should _always_ find a link
node, while that's what you're testing for.

Comment 5

[Peter "jag" Annema]

Hmmm... So to determine whether something is an image (GetInImage), you assume
the selected node is an image (GetPopupImageNode), then complain loudly when it
isn't, while the assumption is totally bogus. Put differently, the way
GetPopupImageNode is currently written you should only ever use it if you're
sure the selected node is an image node, not when you're trying to find out
whether it is. I think you should merge GetInImage and GetPopupImageNode to
something like:

GetInImage(nsIDOMNode** aNode) {
  nsCOMPtr<nsIDOMNode> node;
  nsresult rv = GetPopupNode(getter_AddRefs(node));
  if (NS_FAILED(rv)) return rv;

  nsCOMPtr<nsIDOMHTMLImageElement> img(do_QueryInterface(node));
  *aNode = img;
  NS_IF_ADDREF(*aNode);
  return NS_OK;
}

Dan: would you mind reopening your bug (or opening a new bug) and fix your code
per the above?

Anyway, those <command/>s indeed need to go somewhere else. The commandset
they're in now is used to update the menuitems in the Edit menu. Since they're
not related to menuitems in that menu, I've moved them into their own
commandset. Patch coming up which should fix this bug.

Comment 6

[Peter "jag" Annema]

Attachment: [patch] (untested) Don't spam console with assertions please

Comment 7

[Peter "jag" Annema]

This patch should work modulo c&p / typo errors. Dan, could you apply the patch
and verify it fixes the problem?

Comment 8

[Dan Rosen]

Attachment: dr's patch, tested but different. also fixes contextMenu/gMessagePaneContextMenu inconsistency.

Comment 9

[Dan Rosen]

jag: Yeah, I think I was being paranoid with a couple of those NS_ENSURE_SUCCESS
/ NS_ENSURE_TRUEs. I'll file a new bug to go back in and make them less noisy.

Regardless, I slapped together a simpler patch that seems to work just fine (and
fixes the gMessagePaneContextMenu problem you wanted me to fix). I add no
commandsets, I just have the commands live in the menu -- is that an entirely
incorrect thing to do, or is that okay?

Comment 10

[Dan Rosen]

patch attached to bug 76733 should quiet assertions independently of the
<command> problem. need r= and sr= asap to get that past drivers into 0.9.

Comment 11

[Dan Rosen]

76733 fixed, so assertions are gone, but i still need to fix the <command>s for
correctness's sake. changing summary... i'll still try to have this in for 0.9.

Comment 12

[Dan Rosen]

mscott: do either of my or jag's patches look right? the <command>s need to be
somewhere, i'm just not sure where.

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

aesthetically a little odd to have commands floating around in a XUL doc... I 
think I prefer jag's creation of a new commandset. 

Comment 14

[Dan Rosen]

r=dr on jag's patch (4/19 00:32). i'd like to get this in asap.

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

sr=ben

Comment 16

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to 0.9

Comment 17

[Dan Rosen]

fix checked in.

Comment 18

[R.K.Aa.]

*** Bug 76833 has been marked as a duplicate of this bug. ***