Closed Bug 304727 Opened 19 years ago Closed 19 years ago

Attempting to install an extension no longer triggers yellow info bar (notification) if a site isn't whitelisted or if software installation is disabled

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox1.5

People

(Reporter: ria.klaassen, Assigned: Gavin)

References

()

Details

(Keywords: fixed1.8, regression)

Attachments

(1 file, 3 obsolete files)

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050815 Firefox/1.0+ Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050815 Firefox/1.0+ Online extension installation does not trigger the yellow notifier bar anymore if the site is not in the whitelist. But installation works if the domain is listed under the Allowed Sites. Reproducible: Always Steps to Reproduce: 1. Create a new profile with default settings. 2. Go to http://www.extensionsmirror.nl/index.php?s=f25b7e09f1d6079595b79117a6397c52&showtopic=13 and click on the Install button. 3. An hourglass appears for fraction of a second but further nothing. Expected Results: The notifier bar should appear on the top of the screen.
Dup of bug 303058?
The two are fairly unrelated, and the fix for that bug is specific to the plugin bar, so I'd say no. Ria, do you have a regression range?
Between 2005081112 (works) and 2005081121 (fails).
Keywords: regression
This was broken with Ben's change to findChildShell, due to the checkin for bug 303848.
Assignee: nobody → gavin.sharp
Status: UNCONFIRMED → NEW
Component: Extension/Theme Manager → General
Ever confirmed: true
OS: Windows XP → All
Priority: -- → P1
QA Contact: extension.manager → general
Hardware: PC → All
Summary: Online extension installation does not trigger the yellow notification bar anymore if the site is not in the whitelist → "Blocked popup" and extension installation info-bars (yellow notification bar) no longer appear
Target Milestone: --- → Firefox1.1
Version: unspecified → Trunk
Oops, I was mistaken in thinking this affected the blocked popup notification as well.
Summary: "Blocked popup" and extension installation info-bars (yellow notification bar) no longer appear → Extension installation info-bars (yellow notification bar) no longer appear
Attached patch patch (obsolete) (deleted) — Splinter Review
Fixes this bug, and a couple strict warnings. This is needed trunk&branch. I've tested that all the findChildShell callers (popup blocked message, extension install message, feedview) work as they should with this change.
Attachment #192766 - Flags: review?(mconnor)
Status: NEW → ASSIGNED
Summary: Extension installation info-bars (yellow notification bar) no longer appear → Attempting to install an extension no longer triggers yellow info bar (notification) if a site isn't whitelisted or if software installation is disabled
Flags: blocking1.8b4?
Summary: Attempting to install an extension no longer triggers yellow info bar (notification) if a site isn't whitelisted or if software installation is disabled → Extension installation info-bars (yellow notification bar) no longer appear
Summary: Extension installation info-bars (yellow notification bar) no longer appear → Attempting to install an extension no longer triggers yellow info bar (notification) if a site isn't whitelisted or if software installation is disabled
Flags: blocking1.8b4? → blocking1.8b4+
Same problem with FF 1.0.6. Mozilla/5.0 (Windows; U; Windows NT 5.0; de-DE; rv:1.7.10) Gecko/20050717 Firefox/1.0.6
(In reply to comment #6) > Created an attachment (id=192766) [edit] > patch This patch will fail when you have two identical tabs open, but it works when you lookup/match the docShell ;)
(In reply to comment #8) > (In reply to comment #6) > > Created an attachment (id=192766) [edit] [edit] > > patch > > This patch will fail when you have two identical tabs open, but it works when > you lookup/match the docShell ;) Great, I looked at the wrong patch, sorry for the bug spam (:
(In reply to comment #8) > This patch will fail when you have two identical tabs open, but it works when > you lookup/match the docShell ;) As far as I see, that comment is correct... I tried to take a shortcut without making it check for a provided document, and that leads to simply checking the URI, which is wrong.
Attachment #192766 - Attachment is obsolete: true
Attachment #192766 - Flags: review?(mconnor)
Depends on: 305134
Attached patch Patch I don't really like (obsolete) (deleted) — Splinter Review
This fixes it, but isn't very pretty. Backing out the changes made by the feedview landing (bug 305134) is probably a better bet.
*** Bug 305648 has been marked as a duplicate of this bug. ***
Flags: blocking1.8b5+
As a datapoint, the backout of feedview didn't seem to repair this bug. Gavin's on the case, star that he is.
Attached patch patch (obsolete) (deleted) — Splinter Review
This seems to work for all cases, but it's late (early!), so I'm going to test this again later once I've gotten some sleep.
Attachment #193387 - Attachment is obsolete: true
Attached patch patch 2 (deleted) — Splinter Review
This works in my testing. Rather trying to maintain a super findChildShell that uses three different ways to test docShell equality, I split it into two, one that compares either a document or a URI (for Live Bookmarks and Popup blocking, respectively), and the other that just compares docShells directly (for extension install). This effectively reverts extension install to pre-Feedview, and modifies the function previously used for "Popup blocking" to be more general, for use with Live bookmarks.
Attachment #193793 - Attachment is obsolete: true
Attachment #193812 - Flags: review?(mconnor)
Attachment #193812 - Flags: review?(mconnor) → review+
What about this one? _getBrowser: function (aDocShell) { var tabbrowser = getBrowser(); var browsers = tabbrowser.browsers; for (var i = 0; i < browsers.length; ++i) { var browser = tabbrowser.getBrowserAtIndex(i); if (this.findChildShell(browser.docShell, aDocShell)) return browser; } return null; },
(In reply to comment #16) > What about this one? What about it? If you're suggesting that I not use the temporary "shell" variable, that's fine by me. Is there anything else?
Attachment #193812 - Flags: approval1.8b4?
(In reply to comment #17) > (In reply to comment #16) > > What about this one? > > What about it? If you're suggesting that I not use the temporary "shell" > variable, that's fine by me. Is there anything else? Yes, because that makes it a little faster. See also: browsers.length instead of tabbrowser.browsers.length Also note that _findChildShell is an exact copy of the function we used for MultiZilla for over I don't know already, so that should be ok and not fail. Who is the original developer of this line: var shellInfo = this._getContentShell(targetDoc, feedURI); Why did he use feedURI (because he thought that having more feeds on a page would make any difference perhaps?). p.s. why are these included: aDocShell.QueryInterface(Components.interfaces.nsIWebNavigation); aDocShell.QueryInterface(Components.interfaces.nsIInterfaceRequestor); Why is that?
(In reply to comment #18) > Who is the original developer of this line: > var shellInfo = this._getContentShell(targetDoc, feedURI); Ben Goodger > Why did he use feedURI (because he thought that having more feeds on a page > would make any difference perhaps?). Most likely because the clumsily merged findChildShell took a URI parameter, but it's redundant, since that URI is just built from document.location.href. > p.s. why are these included: > aDocShell.QueryInterface(Components.interfaces.nsIWebNavigation); > aDocShell.QueryInterface(Components.interfaces.nsIInterfaceRequestor); nsIWebNavigation provides currrentURI and nsIInterfaceRequestor provides getInterface. Better safe than sorry.
BTW; is there a spec for this? I mean, what about the location bar, history items and bookmarks? Should that also trigger a BIM (Browser Info Message)?
(In reply to comment #20) > BTW; is there a spec for this? I mean, what about the location bar, history > items and bookmarks? Should that also trigger a BIM (Browser Info Message)? A spec for what? Why would those show browser messages? How is it related to this bug? :)
Comment on attachment 193812 [details] [diff] [review] patch 2 a=shaver for branch landing.
Attachment #193812 - Flags: approval1.8b4? → approval1.8b4+
(In reply to comment #21) > (In reply to comment #20) > > BTW; is there a spec for this? I mean, what about the location bar, history > > items and bookmarks? Should that also trigger a BIM (Browser Info Message)? > > A spec for what? A spec that describes how this should work, i.e. when a BIM should be displayed. > Why would those show browser messages? Steps to reproduce: 1) copy the following link into your locationbar: http://downloads.mozdev.org/multizilla/multiviews-v1810.xpi and press enter 2) cancel the installation (no BIM displayed) 3) add the link as bookmark 4) click on the bookmark (no BIM displayed) 5) visit www.mozilla.org 6) go back in history and select the XPInstall link (no BIM displayed) How is it related to this bug? :) That seems pretty obvious to me ;)
(In reply to comment #23) Oh great, that is caused by some extension I had installed (:
Trunk: Checking in browser/base/content/browser.js; /cvsroot/mozilla/browser/base/content/browser.js,v <-- browser.js new revision: 1.492; previous revision: 1.491 done 1.8 Branch: Checking in browser/base/content/browser.js; /cvsroot/mozilla/browser/base/content/browser.js,v <-- browser.js new revision: 1.479.2.13; previous revision: 1.479.2.12 done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: fixed1.8
*** Bug 305965 has been marked as a duplicate of this bug. ***
Keywords: fixed1.8
Whiteboard: fixed1.8
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: