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

RESOLVED FIXED

Status

Camino Graveyard
General
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Sean Murphy, Assigned: Sean Murphy)

Tracking

Details

Attachments

(1 attachment, 3 obsolete attachments)

11.96 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
Created attachment 325432 [details] [diff] [review]
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:|.
Attachment #325432 - Flags: review?(stuart.morgan)

Comment 1

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

Comment 2

10 years ago
(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

10 years ago
Yep, adding them to the protocol seems like the way to go.
(Assignee)

Comment 4

10 years ago
Created attachment 325774 [details] [diff] [review]
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.
Attachment #325432 - Attachment is obsolete: true
Attachment #325774 - Flags: review?(stuart.morgan)
Attachment #325432 - Flags: review?(stuart.morgan)
(Assignee)

Comment 5

10 years ago
Created attachment 325775 [details] [diff] [review]
Patch, v2

The last patch was diff'ed before I decided to add an extra comment.  Sorry about that!
Attachment #325774 - Attachment is obsolete: true
Attachment #325775 - Flags: review?(stuart.morgan)
Attachment #325774 - Flags: review?(stuart.morgan)

Comment 6

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

Looks good, r=smorgan
Attachment #325775 - Flags: review?(stuart.morgan) → review+

Updated

10 years ago
Attachment #325775 - Flags: superreview?(mikepinkerton)
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
Attachment #325775 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 8

10 years ago
(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 :-/!
(Assignee)

Comment 9

10 years ago
Created attachment 326492 [details] [diff] [review]
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.
Attachment #325775 - Attachment is obsolete: true
"Patch v2, for Check-in" landed on the trunk.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

9 years ago
Depends on: 452066
You need to log in before you can comment on or make changes to this bug.