Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110208 Firefox/4.0b12pre ID:20110208030358 When I was testing Bug 627065, I encountered the following problem. Huge context menu pops up when right click after selected text in sidebar web page. After even once displayed another context menu, the problem does not occur. Reproducible: Always Steps to Reproduce: 1. Start Minefield with new profile 2. Bookmark any page and set "Load this bookmark in the sidebar" 3. Open the bookmark in the sidebar 4. Select text 5. Right click Actual Results: Huge context menu pops up. Expected Results: Normal context menu should pop up. Regression window(cached m-c hourly): Works: http://hg.mozilla.org/mozilla-central/rev/00955067e4b5 Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100504 Minefield/3.7a5pre ID:20100505053644 Fails: http://hg.mozilla.org/mozilla-central/rev/d7393e28fb2d Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100504 Minefield/3.7a5pre ID:20100505071655 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=00955067e4b5&tochange=d7393e28fb2d Suspected bug; d7393e28fb2d Asaf Romano — Bug 528884 - Remove places' menu and toolbar bindings. r=mak

Error shows when popup huge context menu in error console. Error: BrowserSearch is undefined Source file: chrome://browser/content/nsContextMenu.js Line: 1128 var ss = Cc["@mozilla.org/browser/search-service;1"]. getService(Ci.nsIBrowserSearchService); >> if (isElementVisible(BrowserSearch.searchBar)) engineName = ss.currentEngine.name; else

the error could make sense but the relation with the Places patch is not that clear

In addition to comment #0 >After even once displayed another context menu, the problem does not occur. However, It is not context menu related to a selected text.

(In reply to comment #2) > the error could make sense but the relation with the Places patch is not that > clear In local build, I confirmed that the following cset causes the problem. d7393e28fb2d Asaf Romano — Bug 528884 - Remove places' menu and toolbar bindings. r=mak build from d7393e28fb2d : fails build from 2c98eeb202b9 : works build from 00955067e4b5 : works

This pains me to say, but it's a softblocker unless there's some more common path to trigger it. Mano - what's the likelihood that this is worse than it seems?

The real error is: Error: PlacesMenu is not defined Source File: chrome://browser/content/browser.js Line: 2935 That triggers "BrowserSearch is not defined" later because browser.js failed to load completely.

I don't think any of the other global-scripts are dangerous to include, and this might help avoid this kind of trouble in the future? Maybe overkill, though...

(In reply to comment #7) > Created attachment 510705 [details] [diff] [review] > option #1 Er, this patch is reversed somehow. But you get the idea!

ah, the PlacesMenu issue is tracked by bug 610187 as well (see bug 610187 comment 2), I think Mano had ideas regarding that, but there is no clear winner and we left that for post FX4. Personally I'd just include global-script.

Actually, we could even just do a mock PlacesMenu in the sidebar context, as I pointed out in bug 610187... What happens here is that we have a object that inherits from PlacesMenu, when the file is read the prototype is created, and here we fail... but we don't need that object in the sidebar. We could even set the inheriting proto in the constructor I guess?

I think this is a less scary option, if we want to delay solving the borken browser dependencies we have here to FX5, in the meanwhile we could take this and leave Bug 610187 to properly solve the issue with more thoughts. both history menu and the context menu work fine after the fix.

Gavin, do we know for sure that there's any benifit from #including browser-places rathan having this script separated? Another options is to move HistoryMenu to browserPlacesViews.

Comment on attachment 510705 [details] [diff] [review] option #1 There is no reason to load browserPlacesViews.js in the sidebar.

Comment on attachment 510728 [details] [diff] [review] option #3 If we don't do any of the options I propossed above, I would take this one.

Moving HistoryMenu has other dependency implications, since it includes code from Sync and Session Restore. Most likely it would work fine but at this late stage in the product I'd take the workaround and let the final solution to Bug 610187 for post-FX4.

I really cannot see how would it break anything, at all. We use browserPlacesViews only in browser.xul (by design). Thus, any dependencies of HistoryMenu on browser.xul scripts wouldn't be affected. There's no reason to nastyify the code if the good fix isn't risky.

Moving things around is inherently risky, it's nearly impossible to be sure that it won't break things. Let's just go with Marco's patch for now (though I'd use |this| rather than |HistoryMenu|).

Comment on attachment 510728 [details] [diff] [review] option #3 Well, fine with me. r=mano.

(In reply to comment #18) > I'd use |this| rather than |HistoryMenu|). that doesn't work, I tried a patch with "this" in the Console and it didn't work... I tried in the Error Console though so it's possible new js makes that possible. will check.

This works for me: function a() { } a.prototype = { x: 1}; function b() { this.__proto__.__proto__ = a.prototype } b.prototype = { y: 2 }; var c = new b(); alert(c.x + " " + c.y); But if you're really looking for correctness - I don't think you should - use Object.create.

yeah that works, also arguments.callee.prototype.__proto__ works...

Mano I still think my original fix is more readable than both "this.__proto__.__proto__" and "arguments.callee.prototype.__proto__" approaches... I'd go for the original one if you're fine with it, Gavin said he has faith in us (just a little bit) ;)

Ah I missed the Object.create suggestion, looking into it.

Object.create unfortunately is still a bit factory-ish. Let's stop the bikeshed discussion, I took a look around to see what's the most used standard for this kind of inheritance and looks like __proto__.__proto__ is pretty much used. So I'll go for this.__proto__.__proto__, I'll directly fix that in the push. Thank you!

Comment 19 said just "r=mano", not "r=mano with gavin's nit addressed". I'm fine with all options. Here's another one: window.a = function() { }; a.prototype = { x: 3 }; var b = function() { }; b.prototype = { __proto__: "a" in window ? window.a.prototype : null }; alert((new b()).x) vs var b = function() { }; b.prototype = { __proto__: "a" in window ? window.a.prototype : null }; alert((new b()).x)

ok, let's stop overengineering this thing, there is better stuff for a headache. http://hg.mozilla.org/mozilla-central/rev/98cf4955a4b5