439682 - Ability to respond to XUL command events sent from the browser content area

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Sean Murphy]

Attachment: Patch

Since Stuart needs this ability too, I'm breaking out the relevant code from my patch on 358299.

This gives us the opportunity to respond when XUL elements are activated (clicked, for instance) in the browser content area (such as error pages).  Ultimately, the BrowserWindowController can handle such behavior in |-performCommandForXULElementWithID:onPage:|.

Comment 1

[Stuart Morgan]

Comment on attachment 325432 [details] [diff] [review]
Patch

>+- (void)performCommandForXULElementWithID:(NSString*)elementIdentifier onPage:(NSString*)pageURI

I feel like this is probably too vague to send up to the BWC level. How about breaking down by page+id in BW's method, and creating individual methods for the BWC layer for each meaningful action (ignore danger warning, run away, add certificate override (in my case), etc.)? In this patch that would be nothing, obviously, but you and I can add them as necessary.

Comment 2

[Sean Murphy]

(In reply to comment #1)
> (From update of attachment 325432 [details] [diff] [review])
> >+- (void)performCommandForXULElementWithID:(NSString*)elementIdentifier onPage:(NSString*)pageURI
> 
> I feel like this is probably too vague to send up to the BWC level. How about
> breaking down by page+id in BW's method, and creating individual methods for
> the BWC layer for each meaningful action (ignore danger warning, run away, add
> certificate override (in my case), etc.)? In this patch that would be nothing,
> obviously, but you and I can add them as necessary.

Good point, I do like that approach better.  Were you thinking that those individual BWC methods would be part of the BrowserUIDelegate protocol?  I guess the other technique is IBAction type methods, but I don't feel the BrowserWrapper is coupled to BWC enough to make them a truly viable option.

Comment 3

[Stuart Morgan]

Yep, adding them to the protocol seems like the way to go.

Comment 4

[Sean Murphy]

Attachment: Patch, v2

I moved the |performCommandForXULElementWithID:onPage:| method into BW, so this can be where we dispatch the appropriate commands to the BWC.  This avoids a long onXULCommand:, and separates the DOM event interpretation from the actual action sending.

Comment 5

[Sean Murphy]

Attachment: Patch, v2

The last patch was diff'ed before I decided to add an extra comment.  Sorry about that!

Comment 6

[Stuart Morgan]

Comment on attachment 325775 [details] [diff] [review]
Patch, v2

Looks good, r=smorgan

Comment 7

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 325775 [details] [diff] [review]
Patch, v2

+  PRBool eventIsTrusted = PR_FALSE;
+  aDOMEvent->GetIsTrusted(&eventIsTrusted);

do we do this check elsewhere? maybe a comment about what this is and why.

sr=pink

Comment 8

[Sean Murphy]

(In reply to comment #7)
> (From update of attachment 325775 [details] [diff] [review])
> +  PRBool eventIsTrusted = PR_FALSE;
> +  aDOMEvent->GetIsTrusted(&eventIsTrusted);
> 
> do we do this check elsewhere? maybe a comment about what this is and why.

This is the only place we do this check, mainly because the GetIsTrusted method is only offered by the nsIDOMNSEvent interface, which we do not use elsewhere.

Documentation about this method is non-existent.  The implementation looks at the event flags on the DOM event, checking for NS_EVENT_FLAG_TRUSTED.  I originally imagined it provided a way to filter events sent from non chrome URLs, but a quick test revealed that a simple file:/// URL sent a trusted event from an XUL button.  So, I'm actually not sure now which types of events are set as trusted, and which are not?

I checked Firefox's source, and they do perform this check when handling XUL commands.  So, even though ensuring trusted events seems helpful, I'm ashamed to admit now that what I thought it was testing is not really the case.  If it stays, I'm not sure what to mention in the comment at this point :-/!

Comment 9

[Sean Murphy]

Attachment: Patch v2, for Check-in

After digging around some more, I can provide some more information about what constitutes a trusted event.  Basically, in our case here, the -[ChildView mouseUp:] method creates an event object, setting the trusted flag in the constructor.  From what I can tell, any genuine events, such as the GUI event here, sent from within the browser itself are labeled as trusted.  Synthesized events, faked as part of a security vulnerability or otherwise some unexpected code execution, will be classified as untrusted.

Even though we eventually check the error page URI to determine the appropriate action for a command, I think this check should stay just to be safe.  I added a comment explaining its usefulness now.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

"Patch v2, for Check-in" landed on the trunk.