1056035 - Unnecessarily line displayed above Find Updates option for h264 plugin

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Hossain Al Ikram [:ikram] (QA Contact)]

Attachment: screenshot.jpg

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/36.0.1985.143 Safari/537.36 OPR/23.0.1522.77

Steps to reproduce:

Install OpenH264 Video Codec plugin . Open Add-ons Manager . Double click on the OpenH264 Plugin . Now in the information area of this plugin , in a space , click right of the mouse button.


Actual results:

You will see a extra line above find updates


Expected results:

The line shouldn't be here.

Comment 1

[Hossain Al Ikram [:ikram] (QA Contact)]

Reproducible on:

latest Nightly (Build ID: 20140819030202) : Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0

latest Aurora (Build ID: ) : User agent

Comment 2

[Hossain Al Ikram [:ikram] (QA Contact)]

Reproducible on latest Aurora (Build ID: 20140819004001) : Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0

Comment 3

[Georg Fritzsche [:gfritzsche]]

The broken assumption here is that we only need to hide the separator if we have only one active context menu entry:
http://hg.mozilla.org/mozilla-central/annotate/ffdd1a398105/toolkit/mozapps/extensions/content/extensions.js#l448
Instead we should only count the number of enabled entries that come *before* the separator.

We can add a check for this to the detail view tests here:
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_openH264.js

This file shows how to get the context menu in a browser test (gContextMenu):
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_bug591465.js

Comment 4

[Mike Quested]

Hi, I am new to Bugzilla. I just graduated with a degree in computer science and want to get involved. I've built mozilla-central, can run the nightly build of Firefox, can reproduce the bug, and have located the files relevant to this bug.

I'd like to have a crack at it. Would it be possible to have some guidance to get me started?

Cheers!

Comment 5

[Georg Fritzsche [:gfritzsche]]

Great, welcome!

I would ignore the tests at first and test whether the first paragraph of comment 3 works out.
If in [1] we break out of the loop when we encounter |menuSep| (which is one of the children of the context menu per [2]) and adjust the .hidden condition below the context, the bug should be fixed. 

Feel free to also contact me directly per mail or IRC (gfritzsche there) if something is unclear.

[1] http://hg.mozilla.org/mozilla-central/annotate/5bd6e09f074e/toolkit/mozapps/extensions/content/extensions.js#l441
[2] http://hg.mozilla.org/mozilla-central/annotate/5bd6e09f074e/toolkit/mozapps/extensions/content/extensions.xul#l57

Comment 6

[Mike Quested]

Attachment: Patch v1 for bug 1056035

Thanks Georg.

I have attached a patch which hides the separator. Is this along the right lines?

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8493233 [details] [diff] [review]
Patch v1 for bug 1056035

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

This looks like it should work right to me.

::: toolkit/mozapps/extensions/content/extensions.js
@@ +438,3 @@
>  
>        var menuSep = document.getElementById("addonitem-menuseparator");
>        var countEnabledMenuCmds = 0;

The meaning for this variable changed, i think we should rename it accordingly.

@@ +442,5 @@
>          if (child.nodeName == "menuitem" &&
>            gViewController.isCommandEnabled(child.command)) {
>              countEnabledMenuCmds++;
>          }
> +        if (child.nodeName == "menuseparator") {

You should be able to just compare them directly, |child == menuSep|.
Move this check up before the previous one or make it an |else if ...| to avoid double comparisons.

Comment 8

[Mike Quested]

Attachment: bug1056035v1.1.patch

Ok, I made the suggested adjustments. Is the new variable name I provided appropriate?

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8493793 [details] [diff] [review]
bug1056035v1.1.patch

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

This looks good, have you tried it out?
Are the comments on the test changes helpful?

::: toolkit/mozapps/extensions/content/extensions.js
@@ +436,5 @@
>        var addon = gViewController.currentViewObj.getSelectedAddon();
>        contextMenu.setAttribute("addontype", addon.type);
>  
>        var menuSep = document.getElementById("addonitem-menuseparator");
> +      var countMenuItemsAboveSep = 0;

I would maybe use "before" instead of "above".

Comment 10

[Mike Quested]

Yes I have compiled the code and tried it out and it works. I will make the change to the variable name.

I'm not quite sure what needs to be done with browser_openH264.js to add the test. Could you clarify for me?

Comment 11

[Georg Fritzsche [:gfritzsche]]

I would probably just add this to |testInstalledDetails()|.

This already opens the detail view. In there get the context menu like comment 3 shows and check that the menu separator is |.hidden|.

Comment 12

[Mike Quested]

Attachment: bug1056035v1.2.patch

Here is the latest patch which includes the working mochitest. I think it is good...

Thanks for all your help Georg

Comment 13

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8495375 [details] [diff] [review]
bug1056035v1.2.patch

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

Thanks, this looks good!
I'd also not mention the count explicitly in the patch message here - what we really care about is whether we have any visible items before the separator.

After addressing the below comments you can request review from Unfocused / Blair McBride on the new patch.

::: toolkit/mozapps/extensions/content/extensions.js
@@ +448,4 @@
>          }
>        }
>  
> +      // If menu items above separator is 0, hide separator

"Hide the separator if there are no visible menu items before it" ?

::: toolkit/mozapps/extensions/test/browser/browser_openH264.js
@@ +111,5 @@
>    gIsEnUsLocale = chrome.getSelectedLocale("global") == "en-US";
>  
>    Services.obs.addObserver(gOptionsObserver, AddonManager.OPTIONS_NOTIFICATION_DISPLAYED, false);
>  
> +  //	 Start out with OpenH264 not being installed, disabled and automatic updates disabled.

What's with this change?

@@ +204,5 @@
> +    contextMenu.removeEventListener("popupshown", listener, false);
> +    deferred.resolve();
> +  };
> +  contextMenu.addEventListener("popupshown", listener, false);
> +  

Unneeded whitespace here.

@@ +209,5 @@
> +  el = doc.getElementsByClassName("detail-view-container")[0];
> +  EventUtils.synthesizeMouse(el, 4, 4, { }, gManagerWindow);
> +  EventUtils.synthesizeMouse(el, 4, 4, { type: "contextmenu", button: 2 }, gManagerWindow);
> +  yield deferred.promise;
> +  

Unneeded whitespace here.

Comment 14

[Mike Quested]

Attachment: bug1056035v1.3.patch

Ok I have addressed the issues you raised, updated the patch, and requested review. Thanks.

> ::: toolkit/mozapps/extensions/test/browser/browser_openH264.js
> @@ +111,5 @@
> >    gIsEnUsLocale = chrome.getSelectedLocale("global") == "en-US";
> >  
> >    Services.obs.addObserver(gOptionsObserver, AddonManager.OPTIONS_NOTIFICATION_DISPLAYED, false);
> >  
> > +  //	 Start out with OpenH264 not being installed, disabled and automatic updates disabled.
> 
> What's with this change?
Whoops, an accidental insertion of a tab character. I've gotten rid of it.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8495882 [details] [diff] [review]
bug1056035v1.3.patch

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

Looks good - thanks!

Comment 16

[Georg Fritzsche [:gfritzsche]]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=83a3fa6754f6

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8495882 [details] [diff] [review]
bug1056035v1.3.patch

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

This should be r=unfocused in the patch message now.

Comment 18

[Georg Fritzsche [:gfritzsche]]

So, try looks all green & good, thanks!
Can you fix the commit message and set checkin-needed when you uploaded the new patch?

Comment 19

[Mike Quested]

Attachment: bug1056035v1.4.patch

Updated reviewer field in commit message

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/139c533264d7

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/139c533264d7

Comment 22

[Ada [:adalucinet]]

Verified as fixed with latest Nightly 2014-09-30 on Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5.