1498904 - Add-ons Manager's category box is no more completely painted with the background color.

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Richard Marti (:Paenglab)]

Bug 1483335 introduced buttons to open from Add-ons manager the prefs and vice versa.

This needs some changes on our side.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 1498904-prefsAddonLinks.patch

This fixes almost all issues:
- Give the whole extension category box a background color.
- Set the correct prefs icon.
- Hide the Help button. We have no corresponding page to show.
- Add Add-ons button to the prefs.

We have one issue: When you click on the prefs button in Add-on Manager it opens a different prefs tab than we open because we open it differently. Best visible when you have already a open prefs tab and the click on the Add-ons manager's prefs button. Then a second tab opens with the prefs (without the correct icon in the tab). Additional clicks on the button switches now to the new prefs.

Aceman, what could we do? Somehow overwrite the opening code with our code in extensions.js or should we adpt our code to the m-c way? I don't see how to do this and you are our tabs specialist (besides many other areas). Please, could you provide the code for the best solution?

Comment 2

[Richard Marti (:Paenglab)]

Attachment: 1498904-prefsAddonLinks.patch

Updated after landing of bug 1489825.

Comment 3

[:aceman]

Comment on attachment 9017096 [details] [diff] [review]
1498904-prefsAddonLinks.patch

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

Thanks, this makes the addon manager a bit more bearable again. I'd really like to see a m-c change that is for the better.
Hiding the Addon support link again isn't atomic, it shows for a while and then hides. Where in the patch do you hide it?

I also get a few messages of "No chrome package registered for chrome://browser/skin/preferences/in-content/general.svg" in the console when playing with the addon manager. Is that caused by us?

Comment 4

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #1)
> We have one issue: When you click on the prefs button in Add-on Manager it
> opens a different prefs tab than we open because we open it differently.

What function does the addon manager call to open the prefs?

Comment 5

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #3)
> I also get a few messages of "No chrome package registered for
> chrome://browser/skin/preferences/in-content/general.svg" in the console
> when playing with the addon manager. Is that caused by us?

They linked in toolkit to a icon in browser:
https://searchfox.org/comm-central/source/mozilla/toolkit/themes/shared/in-content/common.inc.css#923
My patch links to our icon. Maybe better would be I override this in manifest instead.

(In reply to :aceman from comment #4)
> What function does the addon manager call to open the prefs?

They use this:
https://hg.mozilla.org/mozilla-central/diff/661d9ca2bed9/toolkit/mozapps/extensions/content/extensions.js

Comment 6

[Richard Marti (:Paenglab)]

Attachment: 1498904-prefsAddonLinks.patch

This is with the override of the icon. With this the "No chrome package registered for chrome://browser/skin/preferences/in-content/general.svg" should be gone.

Comment 7

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #5)
> They linked in toolkit to a icon in browser:
> https://searchfox.org/comm-central/source/mozilla/toolkit/themes/shared/in-
> content/common.inc.css#923
> My patch links to our icon.

Where?

> Maybe better would be I override this in manifest instead.

I think we can override browser/ URLs too in our jar.mn, as we did for the builtin_addon_list.json file.
 
> (In reply to :aceman from comment #4)
> > What function does the addon manager call to open the prefs?
> 
> They use this:
> https://hg.mozilla.org/mozilla-central/diff/661d9ca2bed9/toolkit/mozapps/
> extensions/content/extensions.js

Ok, so that is switchToTabHavingURI() and we use openPreferencesTab(), which boils down to tabmail.openTab() ?

Butthe new link also opens a new prefs tab, just does not see if there is already one open.
Maybe at https://dxr.mozilla.org/comm-central/source/comm/mail/base/content/mailWindow.js#743 you need to check for "preferencesTab" in not only "contentTab" as that is the type of our prefs tab?

Comment 8

[Richard Marti (:Paenglab)]

Attachment: 1498904-prefsAddonLinks.patch

This checks now if we have already a prefs tab open. But now, when we open the prefs from Add-ons manager and then from menu open the prefs, we get two tabs again. What do you think can we do here?

You can also see that the prefs opened from Add-on manager has no prefs favicon because we check for the preferencesTab to show a icon. Depending what we do here, I'll fix this in a follow-up bug.

Comment 9

[:aceman]

Comment on attachment 9017469 [details] [diff] [review]
1498904-prefsAddonLinks.patch

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

::: mail/base/content/mailWindow.js
@@ +743,5 @@
>      let tabInfo = tabmail.tabInfo;
>  
>      // Check if we already have the same URL open in a content tab.
>      for (let tabIndex = 0; tabIndex < tabInfo.length; tabIndex++) {
> +      if (tabInfo[tabIndex].mode.name == "contentTab" || "preferencesTab") {

This should not work.
It needs to be:
tabInfo[tabIndex].mode.name == "contentTab" ||
tabInfo[tabIndex].mode.name == "preferencesTab"

Comment 10

[Richard Marti (:Paenglab)]

Attachment: 1498904-prefsAddonLinks.patch v3

I only tested to open the prefs and it worked. Now with your proposal.

Comment 11

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #8)
> This checks now if we have already a prefs tab open. But now, when we open
> the prefs from Add-ons manager and then from menu open the prefs, we get two
> tabs again. What do you think can we do here?

Yeah, that's sad.
We would have to make openPreferencesTab() call switchToTabHavingURI() (if that is what m-c prefers).
The third argument could be made to pass the arguments we need (like paneId).
Or actually make openOptionsDialog() call switchToTabHavingURI() instead of openPreferencesTab() and make 
switchToTabHavingURI() call openPreferencesTab() instead of openContentTab() IF aUrl == "about:preferences".

Comment 12

[Richard Marti (:Paenglab)]

Attachment: 1498904-prefsAddonLinks.patch v4

This looks as it works for me.

Comment 13

[:aceman]

Comment on attachment 9017566 [details] [diff] [review]
1498904-prefsAddonLinks.patch v4

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

::: mail/base/content/mailWindow.js
@@ +745,5 @@
> +    // about:preferences should be opened through openPreferencesTab().
> +    if (openURI = "about:preferences") {
> +      openPreferencesTab();
> +      return;
> +    }

Does openPreferencesTab() check if the tab is already open? So we do not need to find it ourselves via the code below this?

There is some check in tabmail.openTab() is no more than maxTab of that type are open, which could work for the case here (if set to 1), but I do not see maxTab = 1 setting for preferencesTab type.

Comment 14

[Richard Marti (:Paenglab)]

It switches to the already open prefs tab, so it looks it does.

Comment 15

[:aceman]

Comment on attachment 9017566 [details] [diff] [review]
1498904-prefsAddonLinks.patch v4

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

Thanks, this seems to work fine now.
I just observed 2 oddities:
1.The "Daily preferences" label in Addons is smaller font size than "Extensions & Themes" in Preferences.
2.If you open Tools->Addons, then Options and click "Extensions & Themes", it switches to the already open Addons tab, but reloads it. Since then whether Tools->Addons or "Extensions & Themes" does no longer reload it, just switches to it. Is the tab URL in any way different so that the first "Extensions & Themes" click needs to reload it?

Comment 16

[Richard Marti (:Paenglab)]

Attachment: 1498904-prefsAddonLinks.patch v5

Fixed the font size on all platforms.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f77ce17097fa
Fix issues after bug 1483335 (about:preferences to about:addons links). r=aceman

Comment 18

[Richard Marti (:Paenglab)]

Attachment: 1498904-fix-logic.patch

Aceman pointed to a wrong logic. Fixed it now.

Comment 19

[:aceman]

Comment on attachment 9019147 [details] [diff] [review]
1498904-fix-logic.patch

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

Thanks:)

Comment 20

[Richard Marti (:Paenglab)]

Thanks to you to find it.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5a8c03dfb2ac
Fix logic to be a comparison and not an assignment. r=aceman DONTBUILD