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

RESOLVED FIXED in mozilla0.9

Status

()

Core
XUL
P1
normal
RESOLVED FIXED
17 years ago
7 years ago

People

(Reporter: Scott MacGregor, Assigned: Dan Rosen)

Tracking

Trunk
mozilla0.9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: important to 0.9)

Attachments

(2 attachments)

(Reporter)

Description

17 years ago
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:
(Reporter)

Comment 1

17 years ago
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.
(Assignee)

Comment 2

17 years ago
crap, that's weird. i'll look into it. thanks.
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9
(Assignee)

Comment 3

17 years ago
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?
OS: Windows 2000 → All
Priority: P2 → P1
Hardware: PC → All

Comment 4

17 years ago
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

17 years ago
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

17 years ago
Created attachment 31417 [details] [diff] [review]
[patch] (untested) Don't spam console with assertions please

Comment 7

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

Updated

17 years ago
Keywords: mozilla0.9
Whiteboard: important to 0.9
(Assignee)

Comment 8

17 years ago
Created attachment 31499 [details] [diff] [review]
dr's patch, tested but different. also fixes contextMenu/gMessagePaneContextMenu inconsistency.
(Assignee)

Comment 9

17 years ago
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?
(Assignee)

Updated

17 years ago
Depends on: 76733
(Assignee)

Comment 10

17 years ago
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.
(Assignee)

Comment 11

17 years ago
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.
Summary: Many assertions trying to bring up the Edit Menu in mailnews → Fix misplaced <command id="cmd_copyLink"/>, etc. in mailnews
(Assignee)

Comment 12

17 years ago
mscott: do either of my or jag's patches look right? the <command>s need to be
somewhere, i'm just not sure where.
aesthetically a little odd to have commands floating around in a XUL doc... I 
think I prefer jag's creation of a new commandset. 
(Assignee)

Comment 14

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

Comment 16

17 years ago
a= asa@mozilla.org for checkin to 0.9
(Assignee)

Comment 17

17 years ago
fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 18

17 years ago
*** Bug 76833 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.