1427188 - Search box in add-on manager not working in nightly

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: General, defect)

Description

[Jonathan Kamens]

With 2017-12-27 nightly, I get this error in the error console when I try to search on the add-ons page:

TypeError: chromewin.openLinkIn is not a function[Learn More]  extensions.js:2016:7
	initialize/< chrome://mozapps/content/extensions/extensions.js:2016:7
	_fireCommand chrome://global/content/bindings/textbox.xml:386:11
	_enterSearch chrome://global/content/bindings/textbox.xml:403:11
	onxblkeypress chrome://global/content/bindings/textbox.xml:441:11

Comment 1

[WaltS48 [:walts48]]

Same here using Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Thunderbird/59.0a1 BuildID: 20171227030203

Comment 2

[Richard Marti (:Paenglab)]

Attachment: Bug1427188.patch

Aceman, what do you think about this? I copied this from your code of bug 1419145. It didn't reuse the tab with the same search term. So I removed the whole check and leaved the tab opening. This is a very simple patch but it works.

Feel free to take the bug and improve the patch. My knowledge is too limited for more.

Comment 3

[:aceman]

Comment on attachment 8938965 [details] [diff] [review]
Bug1427188.patch

If you wanted to reuse a tab with existing search/URL you could call switchToTabHavingURI() from your new function. But I'm not sure that is needed.
I also wonder why you put it in mail/base/content/utilityOverlay.js and my function is in mail/base/content/mailWindowOverlay.js (also called from addon manager). I don't know which one is better placement and if there is any schema in it.

Comment 4

[Richard Marti (:Paenglab)]

I placed it there because FX has it there too https://dxr.mozilla.org/comm-central/source/mozilla/browser/base/content/utilityOverlay.js#207 and I thought it needs to be there to be found when chromewin.openLinkIn is called.

Comment 5

[Richard Marti (:Paenglab)]

Attachment: Bug1427188.patch

(In reply to :aceman [mostly away 23-29 Dec] from comment #3)
> Comment on attachment 8938965 [details] [diff] [review]
> Bug1427188.patch
> 
> If you wanted to reuse a tab with existing search/URL you could call
> switchToTabHavingURI() from your new function. But I'm not sure that is
> needed.

Tried it and it still opens a new tab with the same search term.

> I also wonder why you put it in mail/base/content/utilityOverlay.js and my
> function is in mail/base/content/mailWindowOverlay.js (also called from
> addon manager). I don't know which one is better placement and if there is
> any schema in it.

It works with mailWindowOverlay.js too -> moved it.

Comment 6

[:aceman]

Comment on attachment 8938965 [details] [diff] [review]
Bug1427188.patch

Review of attachment 8938965 [details] [diff] [review]:
-----------------------------------------------------------------

You are right, placing it in utilityOverlay.js seems better. We could also move switchToTabHavingURI() there, there are more of these tab opening functions. Jorg, would you agree with that and should we do it in this patch/bug (I've tested that the Preferenecs buttons in addon manager still works after the movement).

::: mail/base/content/utilityOverlay.js
@@ +324,5 @@
>    openContentTab(url, where, "^http://www.mozilla.org/");
>  }
> +
> +/* Used by the Add-on manager's search box */
> +function openLinkIn(aURI, aOpenNew, aOpenParams) {

Looking at the m-c definition of the function "openLinkIn(url, where, params", the second argument is 'where', containing values like "tab" or "window", so please rename the argument and pass it on to openContentTab.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8938965 [details] [diff] [review]
Bug1427188.patch

Why f? and not NI?

M-C have switchToTabHavingURI() in browser/base/content/browser.js.

Frankly, I don't have an opinion, you'd have to ask Magnus about the meaning of all these files. If you want to move it in this patch, you have my blessing. But since M-C have the two functions in different files, we might as well not move it.

Comment 8

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #5)
> > If you wanted to reuse a tab with existing search/URL you could call
> > switchToTabHavingURI() from your new function. But I'm not sure that is
> > needed.
> 
> Tried it and it still opens a new tab with the same search term.

I debugged this and it happens because the search box calls an URL of
https://addons.mozilla.org/en-US/thunderbird/search?q=column
but after the tab loads the URL is https://addons.mozilla.org/en-US/thunderbird/search/?q=column (notice the / after 'search'). So the urls are not equal and we open a new tab. The / may be added by some server redirect.

So I'll upload a new patch using switchToTabHavingURI() but it always opens a new tab for now. Notice the same also happens in Firefox nightly.

This can be solved by adding the / into extensions.getAddons.search.browseURL . But Firefox also hasn't done that yet.

Comment 9

[:aceman]

Attachment: 1427188.patch v2

This implements it all, but we need decision from Magnus if we can change the search URL.

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 8939164 [details] [diff] [review]
1427188.patch v2

Yes, it switches now to the already open tab when the same search is done and in the search tab nothing has changed.

Comment 11

[Jorg K (CEST = GMT+2)]

Maybe involve Andrei on the slash business. Andrei, please see comment #8 and the patch.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939164 [details] [diff] [review]
1427188.patch v2

Let's do a PLR for Magnus. This is busted, so we might as well fix it.

Comment 13

[:aceman]

Or you could land the patch without that pref change hunk as it is not critical to the fix, to be on the safer side. As said, we would behave better than Firefox does (it opens new tabs for each search), so there is no rush to land that part.

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 8939164 [details] [diff] [review]
1427188.patch v2

Review of attachment 8939164 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thx! r=mkmelin
(with this change we're just using what the server wants us to use)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dd64bc33f829
Add the openLinkIn() to utilityOverlay.js for add-on search to work. r=aceman,mkmelin

Comment 16

[Jorg K (CEST = GMT+2)]

This breaks "Tools > Add-on Options" when the add-on has tab options (3).
Backing out rev dd64bc33f829 makes tab options work again.

Please fix.

Comment 17

[:aceman]

Attachment: 1427188-2.patch

Fix

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939327 [details] [diff] [review]
1427188-2.patch

Fix :-)

Comment 19

[Richard Marti (:Paenglab)]

Thanks aceman.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bb53b1cf7bc0
Follow-up: open a new tab for add-on options if not already open. r=jorgk