514308 - Support shift+click of the RSS icon in the address bar

Status: VERIFIED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[Tanner M. Young [:tmyoung]]

When testing Bug 304458, everything works except shift+click, which was requested in comment #2.  I don't know if this is a regression or if that feature never made it into the patch.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a2pre) Gecko/20090901 Namoroka/3.6a2pre (.NET CLR 3.5.30729) ID:20090901050855

Comment 1

[Tanner M. Young [:tmyoung]]

In testing this further, when shift+click is fired on a feed, this is in the console:

Error: uncaught exception: [Exception... "Could not convert JavaScript argument arg 0 [nsISupportsArray.AppendElement]"  nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame :: chrome://browser/content/utilityOverlay.js :: openUILinkIn :: line 195"  data: no]

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#195

Comment 2

[Tanner M. Young [:tmyoung]]

I think I've traced the fault to http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6133

I assume we need to make sure that the event and href are properly passed to the loadFeed function, which calls the error in openUILinkIn().  I will get a patch to sniff for the shiftKey and hand the correct arguments.

Assigning to me.

Comment 3

[Tanner M. Young [:tmyoung]]

Attachment: Added try/catch to utilityOverlay.js to prevent appending allowThirdPartyFixup

It's caused by the inability to append allowThirdPartyFixup to sa in the function definition openUILinkIn.

Comment 4

[Dão Gottwald [:dao]]

Should allowThirdPartyFixup be a nsISupportsPRBool?

Comment 5

[Tanner M. Young [:tmyoung]]

(In reply to comment #4)
> Should allowThirdPartyFixup be a nsISupportsPRBool?

I can't remember, what is it currently defined as?

Comment 6

[Tanner M. Young [:tmyoung]]

I tried setting it as a nsISupportsPRBool, but it didn't have any effect.  I did discover that the function cannot append a false value, but it can append a null value, if the null value is defined within the function, but if it is sent to the function as an argument, the function fails (despite the value of the argument being null).

Also, the openUILink function has 7 arguments, while only six were provided, but fix them didn't solve this problem.

Comment 7

[Dão Gottwald [:dao]]

I wonder why we're using an nsISupportsArray anyway. Seems unnecessary to me.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 401150 [details] [diff] [review]
Added try/catch to utilityOverlay.js to prevent appending allowThirdPartyFixup

This just hides the bug.

I cannot see why nsISupportsBool wouldn't work though... Can you post that patch?

Comment 9

[Tanner M. Young [:tmyoung]]

Attachment: Patch v 1.1

It doesn't work if you are dumb like me and forget to write to the value of allowThirdPartyFixup to the data attribute and not overwrite the new nsISupportsPRBool itself thus defeating the purpose of the new variable.

I think I will go somewhere quiet to contemplate my own stupidity...

Comment 10

[Asaf Romano (gone)]

Comment on attachment 411029 [details] [diff] [review]
Patch v 1.1

I would call it allowThirdPartyFixupSupports.

r=mano. A test would be nice.

Comment 11

[Tanner M. Young [:tmyoung]]

Ideally, we should test Bug 304458 which this was originally part of but was never tested.  The original bug added ctrl+click and several similar behaviors, so we should do a test for all of these at once.

I have requested in-testsuite on the first bug, so it's on my radar, and I'll also request on this so that it is all the more on my radar.

Comment 12

[Tanner M. Young [:tmyoung]]

Attachment: Patch v 1.2

Changed the variable name, and now it needs check-in.

Comment 13

[Tanner M. Young [:tmyoung]]

Attachment: Patch v 1.2 - correct

Yikes! Never mind...

The other file had the fix for another bug in it.  This one doesn't.

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/2d0da81ea17c

Comment 15

[Tanner M. Young [:tmyoung]]

Verified Fixed.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20091110 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20091110052734

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 411211 [details] [diff] [review]
Patch v 1.2 - correct

approval1.9.2 requests aren't currently being monitored, since we're nearing RC freeze and there are too many outstanding requests, so I'm clearing this request. Feel free to re-request approval if you are confident that it's worth drivers' time to consider whether this non-blocker needs to land for 1.9.2 at this stage.