If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

better UI for registerContentHandler

RESOLVED FIXED in Firefox 3 alpha1

Status

()

Firefox
RSS Discovery and Preview
P2
major
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: philor, Assigned: mano)

Tracking

({fixed1.8.1})

2.0 Branch
Firefox 3 alpha1
fixed1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 6 obsolete attachments)

203 bytes, text/html
Details
24.77 KB, patch
Details | Diff | Splinter Review
6.39 KB, patch
Details | Diff | Splinter Review
21.55 KB, image/png
beltzner
: ui-review+
Details
25.55 KB, image/png
beltzner
: ui-review+
Details
32.51 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
Three problems, off the top of my head:

1. The dialog isn't associated with the tab that spawned it, so I can have a link that opens FooReader, and sets a timeout to open my phishy registerContentHandler, making it look like it came from FooReader itself.

2. The unresizable dialog is too small to adequately confirm the URL (moreso because it makes faking too easy, by not stripping username:password, which I'll file separately).

3. A page can spawn an annoying infinite number of dialogs that have to be closed separately, while even if a notificationbox implementation doesn't limit a page to just one, infinite notificationboxes can be closed by just closing the tab.
(Reporter)

Updated

11 years ago
Flags: blocking-firefox2?
(Reporter)

Comment 1

11 years ago
Created attachment 234640 [details]
Spawns 50 tiresome dialogs

Just in case anyone feels the need to click "No" 50 times.
(Reporter)

Comment 2

11 years ago
Filed bug 349383 for stripping username:password, though I don't think just that's enough.
Flags: blocking-firefox2? → blocking-firefox2+
The hope was to be able to do this without string changes, just by doing something like:

| Add "%S" as a feed reader?     ( Yes ) ( No ) |

I think we can just ditch the optimization to make it the default feed reader for now, but after adding it we should probably make it the last selected feed reader type in the drop down (does that make sense to you, Mano?)
Assignee: nobody → bugs.mano
Priority: -- → P2
Status: NEW → ASSIGNED
Created attachment 235032 [details] [diff] [review]
Required DOM changes
Attachment #235032 - Flags: superreview?(bzbarsky)
Attachment #235032 - Flags: review?(bzbarsky)
Comment on attachment 235032 [details] [diff] [review]
Required DOM changes

>Index: dom/src/base/nsGlobalWindow.cpp
>+  if (registrar) {
>+    if (mDocShell) {

Some && love here would reduce the indenting needed.  Same in the other method.

Looks ok with those nits.
Attachment #235032 - Flags: superreview?(bzbarsky)
Attachment #235032 - Flags: superreview+
Attachment #235032 - Flags: review?(bzbarsky)
Attachment #235032 - Flags: review+
Mano's pointed out the spoofing issue, ugh. If we were loosed from the bonds of string freeze, the optimal notification message would look like:

| Add "%S" (%S) as a Feed Reader?     (( Add Feed Reader )) ( Cancel ) |

f.e, for bloglines:

| Add "Bloglines" (www.bloglines.com) as a Feed Reader?     (( Add Feed Reader )) ( Cancel ) |
Morphing this a bit to cover both trunk and branch fix approaches.

The branch fix would:
  1) Make sure the dialog is modal to the browser window
  2) focus the requesting tab before we show the dialog
  3) fix bug 349383
  4) don't limit the dialog width (should be fine together with bug 349383 fixed).

The trunk fix would be comment 6 with bug 349383 fixed.
Summary: registerContentHandler should spawn a notificationbox for confirmation, not a dialog → better UI for registerContentHandler
Created attachment 235160 [details] [diff] [review]
branch UI patch
Attachment #235160 - Flags: review?(mconnor)
Comment on attachment 235160 [details] [diff] [review]
branch UI patch

r=me with the makeURI bits moved to the caller so we don't open the dialog at all for a bogus URL.
Attachment #235160 - Flags: review?(mconnor) → review+
Created attachment 235169 [details] [diff] [review]
v2
Attachment #235160 - Attachment is obsolete: true
Attachment #235169 - Flags: review?(mconnor)
Attachment #235169 - Attachment is obsolete: true
Attachment #235169 - Flags: review?(mconnor)
Attachment #235169 - Attachment is obsolete: false
Attachment #235169 - Flags: review?(mconnor)
Attachment #235032 - Flags: approval1.8.1?
Blocks: 349383
Comment on attachment 235169 [details] [diff] [review]
v2

>Index: browser/components/feeds/src/WebContentConverter.js

>+  _makeURI: function(aURL, aOriginCharset, aBaseURI) {
>+    var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>+                              .getService(Components.interfaces.nsIIOService);
>+    return ioService.newURI(aURL, aOriginCharset, aBaseURI);
>+  },

Need to deal with errors here (and at the caller), since the input comes from content.
navigator.registerContentHandler("application/vnd.mozilla.maybe.feed","foo","foo");
with this patch causes an exception that exposes the appdir location.

>+  _confirmAddHandler: function WCCR__confirmAddHandler(contentType, title, uriString, contentWindow) {

>     var shouldAdd = paramBlock.GetInt(PARAM_SHOULD_ADD_HANDLER) == 1;
>     if (shouldAdd&& contentType == TYPE_MAYBE_FEED && 
>         paramBlock.GetInt(PARAM_SHOULD_MAKE_DEFAULT) == 1) {
>       // User chose to use the reader as their default, so update the 
>       // chosen reader preference, too.
>-      this._updateDefaultReader(uri);
>+      this._updateDefaultReader(uri.spec);

uriString?

>   registerContentHandler: 
>-  function WCCR_registerContentHandler(contentType, uri, title) {
>+  function WCCR_registerContentHandler(contentType, uri, title, contentWindow) {
>     LOG("registerContentHandler(" + contentType + "," + uri + "," + title + ")");
>     
>     // XXXben - for Firefox 2 we only support feed types
>     contentType = this._resolveContentType(contentType);
>     if (contentType != TYPE_MAYBE_FEED)
>       return;    
> 
>-    if (!this._checkForDuplicateContentType(contentType, uri, title) ||
>-        !this._confirmAddHandler(contentType, title, uri))
>+    if (!this._checkForDuplicateContentType(contentType, uri, title, contentWindow) ||
>+        !this._confirmAddHandler(contentType, title, uri, contentWindow))
>       return;
>     
>     // Reset the auto handler so that the user is asked again the next time
>     // they load content of this type.
>     if (this.getAutoHandler(contentType)) 
>       this.setAutoHandler(contentType, null);
> 
>     this._registerContentHandler(contentType, uri, title);

Forgot to pass contentWindow here (even though it isn't used)? Either that, or revert:

>   _registerContentHandler: 
>-  function WCCR__registerContentHandler(contentType, uri, title) {
>+  function WCCR__registerContentHandler(contentType, uri, title, win) {
Attachment #235169 - Flags: review?(mconnor) → review+
Comment on attachment 235032 [details] [diff] [review]
Required DOM changes

a=beltzner on behalf of drivers for the MOZILLA_1_8_BRANCH
Attachment #235032 - Flags: approval1.8.1? → approval1.8.1+
Attachment #235169 - Flags: approval1.8.1?
Comment on attachment 235169 [details] [diff] [review]
v2

a=mconnor on behalf of drivers for checkin to the 1.8 branch
Attachment #235169 - Flags: approval1.8.1? → approval1.8.1+
Created attachment 235267 [details] [diff] [review]
branch patch for checkin
Attachment #235169 - Attachment is obsolete: true
1.8 branch:
mozilla/dom/public/idl/sidebar/nsIWebContentHandlerRegistrar.idl 1.1.2.2
mozilla/dom/src/base/nsGlobalWindow.cpp 1.761.2.57
mozilla/browser/components/feeds/content/addFeedReader.js 1.1.2.3
mozilla/browser/components/feeds/content/addFeedReader.xul 1.1.2.2
mozilla/browser/components/feeds/src/WebContentConverter.js 1.1.2.13
Keywords: fixed1.8.1
Created attachment 238322 [details] [diff] [review]
updated DOM changes for trunk (checked in)

mozilla/dom/public/idl/sidebar/nsIWebContentHandlerRegistrar.idl 1.3
mozilla/dom/src/base/nsGlobalWindow.cpp 1.881
mozilla/browser/components/feeds/src/WebContentConverter.js 1.15
Attachment #235032 - Attachment is obsolete: true
Created attachment 240539 [details] [diff] [review]
checkpoint
Created attachment 240727 [details]
screenshot - already registered case
Attachment #240727 - Flags: ui-review?(beltzner)
Created attachment 240728 [details]
screenshot - add me, add me! case
Attachment #240728 - Flags: ui-review?(beltzner)
Created attachment 240729 [details] [diff] [review]
patch

I may need to fix button-icon appearance in winstripe before landing this (no idea how much it is broken right now), see bug 354947 for the pinstripe fix.
Attachment #240539 - Attachment is obsolete: true
Attachment #240729 - Flags: review?(gavin.sharp)
Attachment #240729 - Flags: superreview?(mconnor)
Depends on: 354947
Comment on attachment 240727 [details]
screenshot - already registered case

nit: drop the "" marks, since it's pretty clear what the object in that sentence is
Attachment #240727 - Flags: ui-review?(beltzner) → ui-review+
(and yeah, I know I asked for 'em in the first place ... what can I say? I'm fickle!)
Comment on attachment 240728 [details]
screenshot - add me, add me! case

nit: drop the RSS icon from the button; I want that to represent a feed, not a feed reader.

Let's go with this, though I have to say that not having a "no" button throws me a little (I know that the (X) serves as that, but I think people are going to find it weird - might as well start this way, though)
Attachment #240728 - Flags: ui-review?(beltzner) → ui-review+
Target Milestone: Firefox 2 → Firefox 3 alpha1
Comment on attachment 240729 [details] [diff] [review]
patch

>Index: browser/components/feeds/src/WebContentConverter.js

> var WebContentConverterRegistrar = {
>+  _stringsBundle: null,
>+
>+  get stringsBundle() {
>+    if (!this._stringsBundle) {
>+      this._stringsBundle = Cc["@mozilla.org/intl/stringbundle;1"].
>+                            getService(Ci.nsIStringBundleService).
>+                            createBundle(STRINGS_BUNDLE_URI);
>+    }
>+
>+    return this._stringsBundle;
>+  },

nit: s/stringsBundle/stringBundle/ (in the constant name too) ?

>+  function WCCR_registerContentHandler(aContentType, aURIString, aTitle, aContentWindow) {
>+    LOG("registerContentHandler(" + aContentType + "," + aURIString + "," + aTitle + ")");
>+
>+    // We only support feed types at present.
>+    var contentType = this._resolveContentType(aContentType);
>     if (contentType != TYPE_MAYBE_FEED)
>       return;
> 
>-    if (!this._checkForDuplicateContentType(contentType, uri, title) ||
>-        !this._confirmAddHandler(contentType, title, uri))
>-      return;
>+    try {
>+      var uri = this._makeURI(aURIString);
>+    }
>+    catch(ex) { return; }

I guess making this not fail silently is bug 350273?

>+    // For security reasons we reject non-http(s) urls (see bug Bug 354316),
>+    // we may need to revise this once we support more content types
>+    if (uri.scheme != "http" &&  uri.scheme != "https")
>+      throw("Permission denied to add " + uri.spec + "as a content handler");
>+
>+    var browserWindow = this._getBrowserWindowFromContentWindow(aContentWindow);
>+    var browserElement = this._getBrowserForContentWindow(browserWindow, aContentWindow);

nit: "from" in one, but "for" in the other? _getBrowserFromContentWindow?

>+    var notifactionBox = browserWindow.getBrowser().getNotificationBox(browserElement);
>+    this._appendFeedReaderNotification(uri, aTitle, notifactionBox);
>+    return;
> 
>     // Reset the auto handler so that the user is asked again the next time
>     // they load content of this type.
>     if (this.getAutoHandler(contentType)) 
>-      this.setAutoHandler(contentType, null);
>+      this.setAutoHandler(contentType, null);    
>+  },

Should this just be removed, or is that return misplaced?

>+  /**
>+   * Returns the <xul:browser> elemenet associated with the given content

nit: "element"

Other than those small nits think this looks pretty good, though I'd like to look over it again one more time (with a smaller delay this time, I promise :).
Created attachment 244517 [details] [diff] [review]
patch
Attachment #240729 - Attachment is obsolete: true
Attachment #244517 - Flags: review?(gavin.sharp)
Attachment #240729 - Flags: superreview?(mconnor)
Attachment #240729 - Flags: review?(gavin.sharp)
(Ignore the changes to FeedWrtier.js)
Blocks: 354316
Comment on attachment 244517 [details] [diff] [review]
patch

You forgot to remove addFeedReader.css, and looks to me like _wrapString is now unused in WebContentConverter.js.

>Index: browser/components/feeds/src/WebContentConverter.js

>+const STRINGS_BUNDLE_URI = "chrome://browser/locale/feeds/subscribe.properties";

uber-nit: STRING_BUNDLE, not STRINGS_BUNDLE

>+    var browserWindow = this._getBrowserWindowForContentWindow(aContentWindow);
>+    var browserElement = this._getBrowserForContentWindow(browserWindow, aContentWindow);
>+    var notifactionBox = browserWindow.getBrowser().getNotificationBox(browserElement);
>+    this._appendFeedReaderNotification(uri, aTitle, notifactionBox);

nit: s/notifactionBox/notificationBox/, and remove trailing whitespace.

>+    var buttons, message;
>+    if (this.getWebContentHandlerByURI(TYPE_MAYBE_FEED, uriSpec))
>+      message = this._getFormattedString("handlerRegistered", [aName]);
>+    else {
>+      message = this._getFormattedString("addHandler", [aName, aURI.host]);
>+      var self = this;
>+      aNotificationBox.ownerDocument.defaultView.alert(self._getString("addHandlerAddButtonAccesskey"));

oops? :)

>+            // Make the new handler the last-selected reader in the preview page
>+            // and make sure the preview page is shown the next time a feed is visited
>+            var pb = Cc["@mozilla.org/preferences-service;1"].
>+                     getService(Ci.nsIPrefService).getBranch(null);
>+            pb.setCharPref(PREF_SELECTED_READER, "web");
>+            pb.setCharPref(PREF_SELECTED_WEB, uri);
>+            pb.setCharPref(PREF_SELECTED_ACTION, "ask");

Are you sure you don't need to use setComplexValue here, for PREF_SELECTED_WEB? The old code did. I think newURI always returns punycoded strings, so this may be alright, but I'm not sure. Probably safer to use setComplexValue either way...

Also, do you know why the old code avoided setting the PREF_SELECTED_ACTION pref if it wasn't already set? http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/feeds/src/WebContentConverter.js&rev=1.16#313

Are you sure that this doesn't cause component->notificationbox->buttonhandler->component reference loops, and thus leaks? Having the notificationbox hold on to the component via a closure scares me a bit.

>Index: browser/locales/en-US/chrome/browser/feeds/subscribe.properties

>+addHandler=Add %S (%S) as a Feed Reader?
>+addHandlerAddButton=Add Feed Reader
>+addHandlerAddButtonAccesskey=A
>+handlerRegistered=%S is already registered as a Feed Reader

I think it might make sense to quote the names in these strings, since that makes it harder to spoof the dialog (name="Firefox thinks you suck! Foo" would be kinda funny).

r=me with those comments addressed.
Attachment #244517 - Flags: review?(gavin.sharp) → review+
(In reply to comment #27)

> Are you sure you don't need to use setComplexValue here, for PREF_SELECTED_WEB?
> The old code did. I think newURI always returns punycoded strings, so this may
> be alright, but I'm not sure. Probably safer to use setComplexValue either
> way...

This is a mess: the old addFeedReader code read it as a localized-string pref, the preferences window reads it as nsISupportsString pref, the preview page reads it as a simple char pref.

So, done, but also fixed FeedWriter.js to read it as nsISupportsString.

> Also, do you know why the old code avoided setting the PREF_SELECTED_ACTION
> pref if it wasn't already set?
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/feeds/src/WebContentConverter.js&rev=1.16#313
>

It kept the "Always Use" checkbox checked in the preview page but made it so we don't skip the preview. With this patch, we also make sure the preference state is correct.

> Are you sure that this doesn't cause
> component->notificationbox->buttonhandler->component reference loops, and thus
> leaks? Having the notificationbox hold on to the component via a closure scares
> me a bit.

I don't think so, but it is hard to tell (Leak Monitor doesn't track JS components IIRC), so I've added:

          // avoid reference cycles
          aButtonInfo._outer = null;
 
> >Index: browser/locales/en-US/chrome/browser/feeds/subscribe.properties
> 
> >+addHandler=Add %S (%S) as a Feed Reader?
> >+addHandlerAddButton=Add Feed Reader
> >+addHandlerAddButtonAccesskey=A
> >+handlerRegistered=%S is already registered as a Feed Reader
> 
> I think it might make sense to quote the names in these strings, since that
> makes it harder to spoof the dialog (name="Firefox thinks you suck! Foo" would
> be kinda funny).
>

Agreed, so I've added them back. Do note that beltzner asked me to remove them in the previous cycle. I guess we can tweak that later.
Created attachment 245077 [details] [diff] [review]
patch
Attachment #244517 - Attachment is obsolete: true
Attachment #245077 - Flags: review?(gavin.sharp)
Comment on attachment 245077 [details] [diff] [review]
patch

r=me, but I'd probably just inline _wrapString if there's only one caller.
Attachment #245077 - Flags: review?(gavin.sharp) → review+
with _wrapString inlined:
mozilla/browser/components/feeds/jar.mn 1.6
mozilla/browser/components/feeds/content/addFeedReader.js delete
mozilla/browser/components/feeds/content/addFeedReader.xul delete
mozilla/browser/components/feeds/src/FeedWriter.js 1.25
mozilla/browser/components/feeds/src/WebContentConverter.js 1.17
mozilla/browser/themes/pinstripe/browser/jar.mn 1.33
mozilla/browser/themes/pinstripe/browser/feeds/addFeedReader.css delete
mozilla/browser/themes/winstripe/browser/jar.mn 1.31
mozilla/browser/themes/winstripe/browser/feeds/addFeedReader.css delete
mozilla/browser/locales/jar.mn 1.61
mozilla/browser/locales/en-US/chrome/browser/feeds/addFeedReader.dtd delete
mozilla/browser/locales/en-US/chrome/browser/feeds/subscribe.properties 1.5
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 32

11 years ago
(In reply to comment #19)
> Created an attachment (id=240728) [edit]
> screenshot - add me, add me! case
> 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061110 Minefield/3.0a1 ID:2006111003 [cairo]

I cannot see this (this is not displayed)
tell me how to display this.

screenshot : http://img95.imageshack.us/img95/8310/ssek2.jpg
This isn't a patch for the feed preview UI, see the first testcase here.
You need to log in before you can comment on or make changes to this bug.