Closed Bug 816084 Opened 8 years ago Closed 7 years ago

Add new IdentityPopup class in toolbars.js

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

defect

Tracking

(firefox31 fixed, firefox32 fixed, firefox33 fixed, firefox34 fixed, firefox-esr24 wontfix, firefox-esr31 fixed)

RESOLVED FIXED
Tracking Status
firefox31 --- fixed
firefox32 --- fixed
firefox33 --- fixed
firefox34 --- fixed
firefox-esr24 --- wontfix
firefox-esr31 --- fixed

People

(Reporter: daniela.p98911, Assigned: danisielm, Mentored)

References

Details

(Whiteboard: [lib][lang=js])

Attachments

(2 files, 24 obsolete files)

33.83 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
34.43 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
The test testDVCertificate.js contains "identity-popup-encryption". The lib/toolbars locationBar.getElement needs to be updated for this test as it was updated for the old testEncryptedPageWarning.js
Summary: Modify lib/toolbars.js for testDVCertificate.js test → Update toolbars.js share module to add more elements in locationBar.getElement
Whiteboard: [lib]
Whiteboard: [lib] → [lib][mentor=andreeamatei][lang=js][good first bug]
Hi andreeamatei,

I'm banas from IRC. I'll need help with this bug, it'll be my first. Could you assign it to me?

Thanks!
Assignee: nobody → sbanskota08
Status: NEW → ASSIGNED
Hi Sarup, thanks for your interest!
Here are some more details:
We need to update this test [1] in order to move the elements (those got with elementslib.ID) to the lib/toolbars.js library [2] in getElement() method. Most of them are with ids: identity-popup-* and security-identity-*.

Let me know if you have some more questions in getting started. Please work with the latest nightly downloaded from here [3] and always start with a clean repository (we change it mostly every day with new pushes).

[1] http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/functional/testSecurity/testDVCertificate.js
[2] http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/toolbars.js#l458
[3] ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central/

Helpful style guide:
https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide

I'm also available on IRC if you need help.
Sure, I'll check them out, and ping you on IRC for help :)
Added  elements from tests/functional/testSecurity/testDVCertificate.js to getElement() module in toolbars.js
Attachment #727826 - Flags: feedback?(andreea.matei)
Comment on attachment 727826 [details] [diff] [review]
Patch containing additions to the getElement() method's switch case in toolbars.js

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

Nice start Sarup for your first patch!

Some overall comments:
The indentation has to be aligned with the other cases, it needs shifting to the left. Also we use spaces instead of tabs, please change your editor's settings to be easier for you.

Could you please sort the cases alphabetically in the getElement() method?

Giving these changes, we want to also update the security tests that uses them, in order to get the elements through this method instead of elementslib.ID as it is right now. testSecurity/testDVCertificate.js is one of them, I see testGreenLarry.js also uses one element, would be nice to check all of them.

Thanks for your work so far!
Attachment #727826 - Flags: feedback?(andreea.matei) → feedback-
Hi Andreea,

I had actually focused on maintaining the alignment, but I don't know what went wrong. I'll look into it again :)

Yes, I'll now sort them alphabetically.

About updating the security tests, I was thinking the same, but I wasn't sure. I'll update that, but is it to be done as part of the same patch, or is the part of another patch?

Also, something's wrong with my IRC. Connections to Moznet at port 6697 are being refused. So if you've been replying to my messages, I'm sorry I'm not currently able to see them. I'll ping you once I fix that :)

Thank you!
Most likely the tabs are the ones that massed up the alignment.
You can update the tests in the same patch. Just continue working on your repository, after you're done with a hg qrefresh and hg export in the same patch you should have those added there.

I have seen now that your details do not appear in the patch at the top. Like username, email and so on. Not sure why since your configuration is correct.
We would also need a commit message. Better do 
> hg qrefresh -m "Bug 816084 - Update toolbars.js shared module for moving elements from security tests. r=amatei"
and then export.
Summary: Update toolbars.js share module to add more elements in locationBar.getElement → Update toolbars.js shared module to move elements from security tests in locationBar.getElement()
Sarup, could you confirm that you're still working on this?
Flags: needinfo?(sbanskota08)
Yes Josh, I started work on this, and intend to complete it. I am working on my GSoC proposal + have exams at the moment, so I just might need a little more time to come back to this.
Flags: needinfo?(sbanskota08)
Could you please unassign me from this bug - I'm afraid I'm working on my GSoC project and unable to find the time.
Sure Sarup, thanks for your contribution so far! Good luck on your project.
Assignee: sbanskota08 → nobody
Status: ASSIGNED → NEW
Whiteboard: [lib][mentor=andreeamatei][lang=js][good first bug] → [lib][mentor=andreea.matei][lang=js][good first bug]
Hi Andreea,
Can you please assign this bug to me?
Flags: needinfo?(andreea.matei)
Samvedana, if you don't have edit permissions on Bugzilla please speak to Marcia. You should got those, really.
Assignee: nobody → samvedana.gohil
Status: NEW → ASSIGNED
Flags: needinfo?(andreea.matei)
(In reply to Henrik Skupin (:whimboo) from comment #13)
> Samvedana, if you don't have edit permissions on Bugzilla please speak to
> Marcia. You should got those, really.

Hi Henrik,
I do have permission but I thought that I should ask first before doing any change.
Thanks for assigning me this bug.
Attached patch bug-816084-fix.patch (obsolete) — Splinter Review
Attachment #779082 - Flags: feedback?(andreea.matei)
Comment on attachment 779082 [details] [diff] [review]
bug-816084-fix.patch

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

Good start Samvedana! You have introduced some whitespaces we try to avoid and we will need all the security tests updated, not just testDVCertificate.js.

::: lib/toolbars.js
@@ +180,4 @@
>          break;
>        case "results":
>          elem = new elementslib.Lookup(this._controller.window.document,
> +        AUTOCOMPLETE_POPUP + '/anon({"anonid":"richlistbox"})');

No need to change this element, it was well intended before.

@@ +478,5 @@
>        case "goButton":
>          elem = new elementslib.ID(this._controller.window.document, "urlbar-go-button");
>          break;
>        case "historyDropMarker":
> +        elem = new elementslib.Lookup(this._controller.window.document,                                      URLBAR_CONTAINER + '/id("urlbar")/anon({"anonid":"historydropmarker"})');

No need to change this element, it was well intended before.

@@ +506,5 @@
> +        elem = new elementslib.ID(this._controller.window.document, "identity-popup-encryption-label");
> +        break;	
> +      case "identityPopupMoreInfoButton":
> +        elem = new elementslib.ID(this._controller.window.document, "identity-popup-more-info-button");
> +        break;		

You have trailing whitespaces at the end of each break line mostly, please erase them.

@@ +525,5 @@
> +        break;
> +      case "securityIdentityVerifierValue":
> +        elem = new elementslib.ID(this._controller.window.document, "security-identity-verifier-value");
> +        break; 			
> +	  case "securityTab":

Same here.

@@ +540,5 @@
>          break;
>        case "urlbar_input":
> +        elem = new elementslib.Lookup(this._controller.window.document, URLBAR_INPUTBOX 
> +		+ 
> +		                              '/anon({"anonid":"input"})');

No need to change this element, it was well intended before.

::: tests/functional/testSecurity/testDVCertificate.js
@@ +8,4 @@
>  
>  // Include necessary modules
>  var { assert, expect } = require("../../../lib/assertions");
> +var toolbars = require("../../../lib/toolbars_modified");

It's still toolbars, maybe you used another file for testing purposes?

@@ +102,4 @@
>    assert.ok(securityTab.getNode().selected, "The Security tab is selected by default");
>  
>    // Check the Web Site label against the Cert CName
> +  var webIDDomainLabel = locationBar.getElement({type:"securityIdentityDomainValue"});										

Trailing whitespaces.

@@ +110,4 @@
>                 "Expected web site label found");
>  
>    // Check the Owner label for "This web site does not supply ownership information."
> +  var webIDOwnerLabel = locationBar.getElement({type:"securityIdentityOwnerValue"});									   

Same here.

@@ +116,5 @@
>    expect.equal(webIDOwnerLabel.getNode().value, securityOwner,
>                 "Expected owner label found");
>  
>    // Check the Verifier label against the Cert Issuer
> +  var webIDVerifierLabel = locationBar.getElement({type:"securityIdentityVerifierValue"});										  

And here.
Attachment #779082 - Flags: feedback?(andreea.matei) → feedback-
Comment on attachment 779082 [details] [diff] [review]
bug-816084-fix.patch

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

This looks good. Just one thing to note. See my inline comments. Otherwise f+ on the process.

::: lib/toolbars.js
@@ +492,5 @@
>          break;
> +	  case "identityPopupEncryptionIcon":
> +        elem = new elementslib.ID(this._controller.window.document, "identity-popup-encryption-icon");
> +        break;
> +      case "identityPopupContentHost":

I would name that identityPopup_contentHost, so that we can read the groups a bit easier. This would apply to all instances.
Attachment #779082 - Flags: feedback+
Hi Andreea and Henrik,
I am trying to change other tests, but when I run mozmill, tests are not passing which already passing previously. Sometime tests are skipped and sometimes failed. So, I can not check after doing modification that modified test is running or not. Can you help me in this? I am getting following problem:


mozilla@SGOHIL ~/Desktop/mozmill_repository/mozmill-tests/tests/functional/testS
ecurity
$ mozmill -t testDVCertificate.js -b "C:\Program Files\Nightly\firefox.exe"
←[1;34mTEST-START←[0m | c:\Users\mozilla\Desktop\mozmill_repository\mozmill-test
s\tests\functional\testSecurity\testDVCertificate.js | setupModule
←[1;31mERROR←[0m | Test Failure | {
  "exception": {
    "stack": [
      "TimeoutError@resource://mozmill/stdlib/utils.js:243",
      "waitFor@resource://mozmill/stdlib/utils.js:293",
      "MozMillController@resource://mozmill/driver/controller.js:249",
      "getBrowserController@resource://mozmill/driver/mozmill.js:182",
      "setupModule@resource://mozmill/modules/frame.js -> file:///c:/Users/mozil
la/Desktop/mozmill_repository/mozmill-tests/tests/functional/testSecurity/testDV
Certificate.js:16",
      "Runner.prototype.wrapper@resource://mozmill/modules/frame.js:633",
      "Runner.prototype.runTestModule@resource://mozmill/modules/frame.js:678",

      "Runner.prototype.runTestFile@resource://mozmill/modules/frame.js:603",
      "runTestFile@resource://mozmill/modules/frame.js:750",
      "Bridge.prototype._execFunction@resource://jsbridge/modules/Bridge.jsm:140
",
      "Bridge.prototype.execFunction@resource://jsbridge/modules/Bridge.jsm:147"
,
      "@resource://jsbridge/modules/Server.jsm:33",
      ""
    ],
    "message": "controller(): Window could not be initialized.",
    "fileName": "resource://mozmill/stdlib/utils.js",
    "name": "TimeoutError",
    "lineNumber": 243
  }
}
←[1;34mTEST-START←[0m | c:\Users\mozilla\Desktop\mozmill_repository\mozmill-test
s\tests\functional\testSecurity\testDVCertificate.js | testLarryBlue
←[1;33mTEST-SKIPPED←[0m | testLarryBlue | setupModule failed.
TEST-END | c:\Users\mozilla\Desktop\mozmill_repository\mozmill-tests\tests\funct
ional\testSecurity\testDVCertificate.js | finished in 5005ms
RESULTS | Passed: 0
RESULTS | Failed: 0
RESULTS | Skipped: 1
Flags: needinfo?(hskupin)
Well, without the patch I cannot give a fully qualified answer but as it looks like one of your changes prevents us from opening a new window. And that times out here. So you might want to revert your changes step by step until you found the culprit. On the other side you can also add dump() statements to the test and library modules, to figure out at which step the test fails.
Flags: needinfo?(hskupin)
(In reply to Henrik Skupin (:whimboo) [away 07/29 - 08/11] from comment #19)
> Well, without the patch I cannot give a fully qualified answer but as it
> looks like one of your changes prevents us from opening a new window. And
> that times out here. So you might want to revert your changes step by step
> until you found the culprit. On the other side you can also add dump()
> statements to the test and library modules, to figure out at which step the
> test fails.

Thanks Henrik.
For me, original test is also not working. I tried original testDVCertificate.js
It was skipped.
I have downloaded new repository.
Are you seeing any issue with original test?
Flags: needinfo?(hskupin)
Attached patch bug-816084-fix.patch (obsolete) — Splinter Review
Attachment #782868 - Flags: review?(andreea.matei)
Attached patch bug_816084_fix.patch (obsolete) — Splinter Review
Attachment #782890 - Flags: review?(andreea.matei)
Flags: needinfo?(hskupin)
Attachment #782868 - Flags: review?(andreea.matei)
Attachment #782890 - Flags: review?(andreea.matei)
Attachment #782868 - Flags: review?(andreea.matei)
Comment on attachment 782868 [details] [diff] [review]
bug-816084-fix.patch

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

Great work Samvedana!

Heh, now I understand why the two patches :)

So if you need to update your current working patch, you do the changes and then just run:
hg qrefresh
hg export tip > working_patch

That will update it and still be compared to the original repository. We need one single patch, against the original mozmill-tests repository in the end. 
Thanks!

::: lib/toolbars.js
@@ +464,5 @@
>         */
> +      case "certDomain_link":
> +        elem = new elementslib.ID(this._controller.tabs.activeTab, "cert_domain_link");
> +        break;
> +	  case "contextMenu":

Indentation, please align this with the other cases.

@@ +471,5 @@
>        case "contextMenu_entry":
>          elem = new elementslib.Lookup(this._controller.window.document, CONTEXT_MENU +
>                                        '/{"cmd":"cmd_' + spec.subtype + '"}');
> +        break;      
> +      case "errorShort_descText":

Trailing whitespaces

@@ +479,5 @@
> +        elem = new elementslib.ID(this._controller.tabs.activeTab, "errorTitleText");
> +        break;
> +      case "errorTry_again":
> +        elem = new elementslib.ID(this._controller.tabs.activeTab, "errorTryAgain");
> +        break;      

Trailing whitespaces

@@ +494,5 @@
> +        elem = new elementslib.ID(this._controller.tabs.activeTab, "getMeOutButton");
> +        break;
> +      case "getMe_outOf_hereButton":
> +        elem = new elementslib.ID(this._controller.tabs.activeTab, "getMeOutOfHereButton");
> +        break;      

Trailing whitespaces

@@ +507,5 @@
>          elem = new elementslib.ID(this._controller.window.document, "identity-box");
>          break;
> +      case "identityIcon_label":
> +        elem = new elementslib.ID(this._controller.window.document, "identity-icon-label");
> +        break;      

Trailing whitespaces

@@ +513,5 @@
> +        elem = new elementslib.ID(this._controller.window.document, "identity.identified.verifier");
> +        break;
> +      case "identity-popup":
> +        elem = new elementslib.ID(this._controller.window.document, "identity-popup");
> +        break;      

Trailing whitespaces

@@ +522,5 @@
> +        elem = new elementslib.ID(this._controller.window.document, "identity-popup-content-owner");
> +        break;
> +      case "identityPopup_contentVerifier":
> +        elem = new elementslib.ID(this._controller.window.document, "identity-popup-content-verifier");
> +        break; 

Trailing whitespaces

@@ +534,5 @@
> +        elem = new elementslib.ID(this._controller.window.document, "identity-popup-encryption-label");
> +        break;
> +      case "identityPopup_moreInfo_button":
> +        elem = new elementslib.ID(this._controller.window.document, "identity-popup-more-info-button");
> +        break;      

Trailing whitespaces. Lets call this identityPopup_moreInfoButton as the other cases have this template.

@@ +547,5 @@
>                                        spec.subtype);
>          break;
>        case "notification_popup":
>          elem = new elementslib.Lookup(this._controller.window.document, NOTIFICATION_POPUP);
> +        break;      

Trailing whitespaces

@@ +549,5 @@
>        case "notification_popup":
>          elem = new elementslib.Lookup(this._controller.window.document, NOTIFICATION_POPUP);
> +        break;      
> +      case "q":
> +        elem = new elementslib.ID(this._controller.tabs.activeTab, "q");

This is for testing I assume?

@@ +559,5 @@
> +        elem = new elementslib.ID(this._controller.tabs.activeTab, "reportButton");
> +        break;
> +      case "result":
> +        elem = new elementslib.ID(this._controller.tabs.activeTab, "result");
> +        break;      

I don't think this needs to be here, as I recall "result" is an id from a local page, not related to the security items.

@@ +562,5 @@
> +        elem = new elementslib.ID(this._controller.tabs.activeTab, "result");
> +        break;      
> +      case "searchTerm":
> +        elem = new elementslib.ID(this._controller.tabs.activeTab, "search-term");
> +        break;

Same for this one.

::: tests/functional/testSecurity/testDVCertificate.js
@@ +12,3 @@
>  var utils = require("../../../lib/utils");
>  
> +

No need for this extra line

@@ +100,4 @@
>   */
>  function checkSecurityTab(controller) {
>    var securityTab = new elementslib.ID(controller.window.document, "securityTab");
> +  //var securityTab = locationBar.getElement({type:"securityTab"});

You can remove these comments in the next patch as described at the beginning.

::: tests/functional/testSecurity/testGreyLarry.js
@@ +36,4 @@
>  
>    // Make sure the doorhanger is "open" before continuing
> +  var doorhanger = locationBar.getElement({type:"identity-popup"});
> +  

Whitespace

::: tests/functional/testSecurity/testSafeBrowsingNotificationBar.js
@@ +6,4 @@
>  
>  // Include necessary modules
>  var { assert, expect } = require("../../../lib/assertions");
> +

No need for the extra line

@@ +24,3 @@
>    aModule.tabBrowser = new tabs.tabBrowser(aModule.controller);
>    aModule.tabBrowser.closeAllTabs();
> +  

Whitespaces

@@ +67,4 @@
>  var checkIgnoreWarningButton = function(badUrl) {
>    // Verify the element is loaded onto the page and go to the phishing site
>    var ignoreWarningButton = new elementslib.ID(controller.tabs.activeTab, "ignoreWarningButton");
> +    

Wthitespaces

@@ +101,4 @@
>      controller.waitForPageLoad(controller.tabs.getTab(1));
>  
>      var urlField = new elementslib.ID(controller.tabs.activeTab, "url");
> +	controller.waitForElement(urlField);

Indentation, has to be under "var"

::: tests/functional/testSecurity/testSecurityNotification.js
@@ +44,5 @@
>    controller.waitForPageLoad();
>  
>    // Verify the info in Technical Details contains the invalid cert page
> +  var text = locationBar.getElement({type:"technicalContent_text"});
> +  

Whitespaces
Attachment #782868 - Flags: review?(andreea.matei) → review-
Attached patch bug-816084-fix.patch (obsolete) — Splinter Review
Comment on attachment 783930 [details] [diff] [review]
bug-816084-fix.patch

Thanks Andreea for reviewing patch. I have modified it and attached again.
Attachment #783930 - Flags: review?(andreea.matei)
Attached patch bug-816084-fix.patch (obsolete) — Splinter Review
Attached patch bug-816084-fix.patch (obsolete) — Splinter Review
Attachment #783930 - Attachment is obsolete: true
Attachment #783945 - Attachment is obsolete: true
Attachment #783930 - Flags: review?(andreea.matei)
Attachment #783948 - Flags: review?(andreea.matei)
Comment on attachment 783948 [details] [diff] [review]
bug-816084-fix.patch

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

The patch is missing the changes to the library. Please check it before uploading so you're sure it's the right one, cause I see you've creating multiple patches for this. In the future you can just hg qrefresh and export over the same patch, it's easier to update it. Thanks!

::: tests/functional/testSecurity/testEnablePrivilege.js
@@ +13,5 @@
>  const TEST_DATA = BASE_URL + "security/enable_privilege.html";
>  
>  function setupModule(aModule) {
>    aModule.controller = mozmill.getBrowserController();
> +  aModule.locationBar = new toolbars.locationBar(aModule.controller);

I don't think we need the changes since we don't move any element here.

::: tests/functional/testSecurity/testSafeBrowsingNotificationBar.js
@@ +23,3 @@
>    aModule.tabBrowser = new tabs.tabBrowser(aModule.controller);
>    aModule.tabBrowser.closeAllTabs();
> +

No need for this new line.

@@ +66,4 @@
>  var checkIgnoreWarningButton = function(badUrl) {
>    // Verify the element is loaded onto the page and go to the phishing site
>    var ignoreWarningButton = new elementslib.ID(controller.tabs.activeTab, "ignoreWarningButton");
> +

Same here.
Attachment #783948 - Flags: review?(andreea.matei) → review-
Attached patch bug-816084-fix.patch (obsolete) — Splinter Review
Hi Andreea, I had one question. Can I see diff in local machine?
Above two commands are not working for me to refresh patch, but I think the patch attached now should be fine.
Attachment #787660 - Flags: review?(andreea.matei)
Comment on attachment 787660 [details] [diff] [review]
bug-816084-fix.patch

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

With hg diff or hg qdiff you can see your changes in the console. 
What do you mean hg qrefresh and export does not work? What's the output?
All these commands need to be done inside the working repository.

The patch is overwriting some changes we've done for australis and I'm sure it's not passing as it's missing some cases. Not sure how many patches you have applied to your working repository, I can see it's up to date cause you have the changes with australis. 

I would appreciate if after having your patch ready, you could do a functional testrun to see if you have all tests passing. That's how you can make sure everything is set.
Here you can see how:
https://developer.mozilla.org/en-US/docs/Mozmill_Tests#Running_Mozmill_tests

Please also see:
https://developer.mozilla.org/en-US/docs/Mercurial_Queues
https://developer.mozilla.org/en-US/docs/Mozmill_Tests#Configuring_Mercurial

hgext.color will help you see the whitespaces with red in the console when you do hg diff or qdiff.

::: lib/toolbars.js
@@ +18,5 @@
>  const URLBAR_CONTAINER = '/id("main-window")/id("tab-view-deck")/[0]' +
> +                         '/id("navigator-toolbox")/id("nav-bar")/id("urlbar-container")';
> +const URLBAR_INPUTBOX = URLBAR_CONTAINER + '/id("urlbar")' +
> +                                           '/anon({"anonid":"textbox-container"})' +
> +                                           '/anon({"anonid":"textbox-input-box"})';

This is not needed, it's overwriting the changes we've done.

@@ +488,5 @@
>        case "identityPopup":
>          elem = new elementslib.ID(this._controller.window.document, "identity-popup-encryption");
>          break;
> +      case "identityPopupEncryption":
> +        elem = new elementslib.ID(this._controller.window.document, "identity-popup-encryption");

We are missing some cases that an older patch of yours had..

@@ +527,5 @@
> +        break;
> +      case "securityIdentityVerifierValue":
> +        elem = new elementslib.ID(this._controller.window.document, "security-identity-verifier-value");
> +        break; 			
> +	  case "securityTab":

Most of the cases have whitespaces after "break", please remove them.

::: tests/functional/testSecurity/testEnablePrivilege.js
@@ +17,1 @@
>    tabs.closeAllTabs(aModule.controller);

We don't need changes in this test.

::: tests/functional/testSecurity/testSafeBrowsingNotificationBar.js
@@ +150,4 @@
>    // Click on the x button and verify the notification bar dissapears
>    var button = tabBrowser.getTabPanelElement(tabBrowser.selectedIndex,
>                                               '/{"value":"blocked-badware-page"}/anon({"type":"critical"})' +
> +                                             '/{"class":"messageCloseButton tabbable"}');

This should not change our current code for australis.
Attachment #787660 - Flags: review?(andreea.matei) → review-
Comment on attachment 782868 [details] [diff] [review]
bug-816084-fix.patch

Please mark as obsolete the older patch (r- / f-) when you add a new one.
Attachment #782868 - Attachment is obsolete: true
Attachment #782890 - Attachment is obsolete: true
Attachment #783948 - Attachment is obsolete: true
Attachment #727826 - Attachment is obsolete: true
Samvedana, to ensure that you are working on the latest code revision of the repository, make sure to pop all the patches from the queue and run 'hg pull -u' to update your local copy. Then qpush the local patches again and fix possible merge conflicts. If you have further questions please find me on IRC.
Attached patch bug_816084-fix.patch (obsolete) — Splinter Review
Attachment #779082 - Attachment is obsolete: true
Attachment #787660 - Attachment is obsolete: true
Attachment #793667 - Flags: review?(hskupin)
Attachment #793667 - Flags: review?(andreea.matei)
Forgot to give following info:

testSafeBrowsingNotificationBar.js skips 3 tests in original code

so, I have not modified it. All other tests are passing after modification.
(In reply to Samvedana (:Samvedana) from comment #34)
> testSafeBrowsingNotificationBar.js skips 3 tests in original code

Do you mean that the test is skipped? That's because of bug 905033. It will be re-enabled soon. So if this test needs similar changes as the other security tests, you should do those changes.

> so, I have not modified it. All other tests are passing after modification.

Can you please run the tests and submit reports to http://mozmill-crowd.blargon7.com/db/ ? Thanks.
(In reply to Henrik Skupin (:whimboo) from comment #35)
> (In reply to Samvedana (:Samvedana) from comment #34)
> > testSafeBrowsingNotificationBar.js skips 3 tests in original code
> 
> Do you mean that the test is skipped? That's because of bug 905033. It will
> be re-enabled soon. So if this test needs similar changes as the other
> security tests, you should do those changes.
> 
> > so, I have not modified it. All other tests are passing after modification.
> 
> Can you please run the tests and submit reports to
> http://mozmill-crowd.blargon7.com/db/ ? Thanks.

Yes, Tests were skipped.

I can not open http://mozmill-crowd.blargon7.com/db/ ? 
I think url is not correct. I tried http://mozmill-crowd.blargon7.com/db/?
I don't see any options there too.

Which results you want me to submit? Is it the same which I get in command window?

I don't think I need to change anything here as patch doesn't have any content for testSafeBrowsingNotificationBar.js file. Mario Garbi is already creating patch for bug 905033.
Flags: needinfo?(hskupin)
(In reply to Samvedana (:Samvedana) from comment #36)
> > Can you please run the tests and submit reports to
> > http://mozmill-crowd.blargon7.com/db/ ? Thanks.
> 
> Yes, Tests were skipped.

So please update them too with the changes you have made.

> I can not open http://mozmill-crowd.blargon7.com/db/ ? 
> I think url is not correct. I tried http://mozmill-crowd.blargon7.com/db/?
> I don't see any options there too.
> 
> Which results you want me to submit? Is it the same which I get in command
> window?

When you run our functional testrun scripts from the mozmill-automation repository:
https://developer.mozilla.org/en-US/docs/Mozmill_Tests#Running_Mozmill_tests

> I don't think I need to change anything here as patch doesn't have any
> content for testSafeBrowsingNotificationBar.js file. Mario Garbi is already
> creating patch for bug 905033.

Not sure I understand but I will have a look at latest tomorrow morning.
Flags: needinfo?(hskupin)
Comment on attachment 793667 [details] [diff] [review]
bug_816084-fix.patch

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

::: lib/toolbars.js
@@ +492,5 @@
> +      case "getMe_outButton":
> +        elem = new elementslib.ID(this._controller.window.document, "getMeOutButton");
> +        break;
> +      case "getMe_outOf_hereButton":
> +        elem = new elementslib.ID(this._controller.window.document, "getMeOutOfHereButton");

This looks good now, although some of these cases are hard to read due to the long name and the upper/lowercase letters. 
I'd also like to see the testrun results and wait for Henrik's thoughts.
Comment on attachment 793667 [details] [diff] [review]
bug_816084-fix.patch

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

It's a good patch, Samvedana! Sadly it seems like we missed to fully specify where those element specifiers have to live. You can see a lot of inline comments directly in the toolbar.js part. Given that all those comments automatically apply to the callers of getElement(), I haven't repeated those. For the mentioned security module please take care of visual blocks and areas (pages). So different classes might be necessary.

::: lib/toolbars.js
@@ +481,5 @@
> +        elem = new elementslib.ID(this._controller.window.document, "errorTryAgain");
> +        break;
> +      case "exceptionDialog_button":
> +        elem = new elementslib.ID(this._controller.window.document, "exceptionDialogButton");
> +        break;

Why have all of those entries been added to the locationBar class? Elements like those are not part of the location bar. Those are contained in a xul document visible in content space. So if you want to add element getters you should create a new security ui module. This is out of place here.

Further they have to be referenced via 'controller.tabs.activeTab' and not 'controller.window.document'. The latter is chrome while the former is content.

@@ +492,5 @@
> +      case "getMe_outButton":
> +        elem = new elementslib.ID(this._controller.window.document, "getMeOutButton");
> +        break;
> +      case "getMe_outOf_hereButton":
> +        elem = new elementslib.ID(this._controller.window.document, "getMeOutOfHereButton");

Both buttons are contained in a remote web page. So they will both not be added to any of our ui modules. We only create ui modules for elements in chrome documents.

@@ +511,5 @@
>        case "identityPopup":
>          elem = new elementslib.ID(this._controller.window.document, "identity-popup-encryption");
>          break;
> +      case "identity-popup":
> +        elem = new elementslib.ID(this._controller.window.document, "identity-popup");

It's not clear what the difference between 'identityPopup' and 'identity-popup' is. As it looks like the former needs an update given that it is not the popup itself.

@@ +532,5 @@
> +      case "identityPopup_moreInfoButton":
> +        elem = new elementslib.ID(this._controller.window.document, "identity-popup-more-info-button");
> +        break;
> +      case "ignoreWarning_button":
> +        elem = new elementslib.ID(this._controller.window.document, "ignoreWarningButton");

Again this is content and not chrome.

@@ +536,5 @@
> +        elem = new elementslib.ID(this._controller.window.document, "ignoreWarningButton");
> +        break;
> +      case "mainFeature":
> +        elem = new elementslib.ID(this._controller.window.document, "main-feature");
> +        break;

Same here.

@@ +548,5 @@
>        case "reloadButton":
>          elem = new elementslib.ID(this._controller.window.document, "urlbar-reload-button");
>          break;
> +      case "reportButton":
> +        elem = new elementslib.ID(this._controller.window.document, "reportButton");

And here.

@@ +557,5 @@
>        case "stopButton":
>          elem = new elementslib.ID(this._controller.window.document, "urlbar-stop-button");
>          break;
> +      case "technicalContent_text":
> +        elem = new elementslib.ID(this._controller.window.document, "technicalContentText");

This would be another candidate for a security ui module, given that it is an in-content page.
Attachment #793667 - Flags: review?(hskupin)
Attachment #793667 - Flags: review?(andreea.matei)
Attachment #793667 - Flags: review-
Ok, so after reading comment 0 again, it might be a good idea to only concentrate on those elements which are really part of the location bar in this bug. Everything which is related to security ui should be moved to another one.
Samvedana, will you be able to continue work on this bug?
Since Samvedana hasn't answered lately, I'll put this back in the good-first-bug list so that another contributor can finish the work on it.
Assignee: samvedana.gohil → nobody
Status: ASSIGNED → NEW
Daniel will work on this.
Assignee: nobody → daniel.gherasim
Status: NEW → ASSIGNED
Attached patch update_ToolbarsJS_v1.patch (obsolete) — Splinter Review
Until we have time to continue and review this, I'm uploading here wwhat I did, basically the only elements that are related to the locationBar are the ones from the identity popup box.

So I did the changes in the library.

All this and the patch from bug 863139 should replace all id's and a lot of code we constantly repeat in our security tests.
Attachment #793667 - Attachment is obsolete: true
Attached patch update_ToolbarsJS_v1.1.patch (obsolete) — Splinter Review
Updated the tests too and asked for review.
Only elements related to the locationBar (identity popup) were included in this library, as we handle the rest of them in the security library (bug 863139).
Attachment #8379653 - Attachment is obsolete: true
Attachment #8392909 - Flags: review?(andrei.eftimie)
Attachment #8392909 - Flags: review?(andreea.matei)
Comment on attachment 8392909 [details] [diff] [review]
update_ToolbarsJS_v1.1.patch

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

::: firefox/lib/toolbars.js
@@ +428,5 @@
> +      case "identityPopup_ownerLocation":
> +        elem = new elementslib.ID(document, "identity-popup-content-supplemental");
> +        break;
> +      case "identityPopup_label":
> +        elem = new elementslib.ID(document, "identity-popup-encryption-label");

These should be sorted alphabetically and since we're in the identityPopup class, I'd drop that part from the name cases.

::: firefox/tests/functional/testSecurity/testGreyLarry.js
@@ +24,5 @@
>    // Go to a "grey" website
>    controller.open(TEST_DATA);
>    controller.waitForPageLoad();
>  
> +  var identityPopup = locationBar.identityPopup;

Please move this down where you're using it or even in setupModule.

::: firefox/tests/functional/testSecurity/testIdentityPopupOpenClose.js
@@ +9,5 @@
>  "use strict";
>  
>  // Include necessary modules
>  var { assert, expect } = require("../../../../lib/assertions");
> +var toolbars = require("../../../lib/toolbars");

toolbars comes after tabs.

@@ +18,5 @@
>  const TEST_DATA = BASE_URL + "layout/mozilla.html";
>  
>  var setupModule = function(aModule) {
>    aModule.controller = mozmill.getBrowserController();
> +  aModule.locationBar = new toolbars.locationBar(aModule.controller);

Please also add a blank like which separates block of declarations from the one calling methods.
Attachment #8392909 - Flags: review?(andrei.eftimie)
Attachment #8392909 - Flags: review?(andreea.matei)
Attachment #8392909 - Flags: review-
Attached patch update_ToolbarsJS_v1.2.patch (obsolete) — Splinter Review
Thanks, I made the changes.
Attachment #8392909 - Attachment is obsolete: true
Attachment #8400475 - Flags: review?(andrei.eftimie)
Attachment #8400475 - Flags: review?(andreea.matei)
Comment on attachment 8400475 [details] [diff] [review]
update_ToolbarsJS_v1.2.patch

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

::: firefox/lib/toolbars.js
@@ +384,5 @@
> +        elem = new elementslib.ID(document, "identity-icon-country-label");
> +        break;
> +      case "cssInfoLockImage":
> +        var lockIcon = new elementslib.ID(document, "identity-popup-encryption-icon");
> +        elem = utils.getElementStyle(lockIcon, 'list-style-image');

I don't think this is the right place to return styles, we're expecting elements.
We already return this element as "lockIcon". We should leave this as part of the test.

nit: please use double quotes for consistency

@@ +464,5 @@
>  
>    /**
> +   * Returns the edit bookmarks panel object
> +   *
> +   * @returns editBookmarksPanel object

This description needs to be changed.

@@ +627,5 @@
>          return [new elementslib.ID(root, "page-proxy-favicon")];
> +      case "faviconImage":
> +        var favicon = new elementslib.ID(this._controller.window.document,
> +                                         "page-proxy-favicon");
> +        return [utils.getElementStyle(favicon, 'list-style-image')];

Same as above, this is not an element.

::: firefox/tests/remote/testSecurity/testDVCertificate.js
@@ +37,5 @@
>    expect.ok(!favicon.getNode().hasAttribute("hidden"),
>              "Lock icon is visible in identity box");
>  
> +  var faviconImage = locationBar.getElement({type: "faviconImage"});
> +  expect.contain(faviconImage, "identity-icons-https.png",

Since we're here please update the check to exclude ".png".
On retina displays the name of the image is "identity-icons-https@2X.png" and this test currently fails.

::: firefox/tests/remote/testSecurity/testMixedContentPage.js
@@ +32,1 @@
>      return faviconImage.indexOf("identity-icons-https-mixed-display.png") !== -1;

Same here, please exclude ".png" from the checked string.
Attachment #8400475 - Flags: review?(andrei.eftimie)
Attachment #8400475 - Flags: review?(andreea.matei)
Attachment #8400475 - Flags: review-
Attached patch update_ToolbarsJS_v1.3.patch (obsolete) — Splinter Review
Moved the style lines in tests & finished requested changes.
Attachment #8400475 - Attachment is obsolete: true
Attachment #8401125 - Flags: review?(andrei.eftimie)
Attachment #8401125 - Flags: review?(andreea.matei)
Comment on attachment 8401125 [details] [diff] [review]
update_ToolbarsJS_v1.3.patch

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

With those 2 nits this has my r+.

Please make the update and ask Henrik for a review.
Also please provide some full testrun results.

::: firefox/tests/remote/testSecurity/testDVCertificate.js
@@ +37,5 @@
>    expect.ok(!favicon.getNode().hasAttribute("hidden"),
>              "Lock icon is visible in identity box");
>  
> +  var favicon = locationBar.getElement({type: "favicon"});
> +  expect.contain(utils.getElementStyle(favicon, 'list-style-image'),

nit: as asked in the previous review, please update this to double quotes for consistency

@@ +58,5 @@
>                 "The Larry UI is domain verified (aka Blue)");
>  
>    // Check for the Lock icon is visible
> +  var lockIcon = locationBar.identityPopup.getElement({type: "lockIcon"});
> +  expect.notEqual(utils.getElementStyle(lockIcon, 'list-style-image'),

nit: double quotes please
Attachment #8401125 - Flags: review?(andrei.eftimie)
Attachment #8401125 - Flags: review?(andreea.matei)
Attachment #8401125 - Flags: review+
Attached patch update_ToolbarsJS_v1.4.patch (obsolete) — Splinter Review
Thanks!

An updated small description of this patch: we create a new IdentityPopup class (which is part of the location bar) in toolbars.js  and replace the corresponding elements from security tests.

I'll run sume testruns and provide the results here.
Attachment #8401125 - Attachment is obsolete: true
Attachment #8403199 - Flags: review?(hskupin)
Comment on attachment 8403199 [details] [diff] [review]
update_ToolbarsJS_v1.4.patch

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

Not that much to say here. It looks great! There are some things to get updated but it looks like it will be ready soon. Great work. Please update the summary of the bug and the commit message to better reflect the what we currently do here.

::: firefox/lib/toolbars.js
@@ +331,5 @@
> + * @param {MozmillController} controller
> + *        MozMillController of the window to operate on
> + */
> +function IdentityPopup(aController) {
> +  this._controller = aController;

I would assert here for a valid controller.

@@ +413,5 @@
> +      case "verifier":
> +        elem = new elementslib.ID(document, "identity-popup-content-verifier");
> +        break;
> +      default:
> +        assert.fail("Unknown element type - " + spec.type);

We now also have cookie permissions here. I miss that select element.

@@ +427,5 @@
>   * @param {MozmillController} controller
>   *        MozMillController of the window to operate on
>   */
> +function locationBar(aController) {
> +  this._controller = aController;

Same here for assert to make it safer.

::: firefox/tests/functional/testSecurity/testGreyLarry.js
@@ +29,3 @@
>    expect.ok(!favicon.getNode().hidden, "The globe favicon is visible");
>  
> +  var orgLabel = locationBar.identityPopup.getElement({type: "organizationLabel"});

Lets cache identityPopup in setupModule.

::: firefox/tests/functional/testSecurity/testIdentityPopupOpenClose.js
@@ +40,3 @@
>    assert.waitFor(function () {
>      return popup.getNode().state === 'open';
>    }, "Identity popup has been opened");

Can we have open() and close() methods in the IdentityPopup class?
Attachment #8403199 - Flags: review?(hskupin) → review-
Summary: Update toolbars.js shared module to move elements from security tests in locationBar.getElement() → Add new IdentityPopup class in toolbars.js and move the elements from the tests there
Summary: Add new IdentityPopup class in toolbars.js and move the elements from the tests there → Add new IdentityPopup class in toolbars.js
Attached patch update_ToolbarsJS_v3.patch (obsolete) — Splinter Review
I've added an waitForPanel function to the IdentityPopup to wait for the propper events when opening the panel.

Also the test testIdentityPanelOpenClose was refactored.

All security tests from bug 994040 can be enabled.
Attachment #8403199 - Attachment is obsolete: true
Attachment #8408824 - Flags: review?(andrei.eftimie)
Attachment #8408824 - Flags: review?(andreea.matei)
Blocks: 863139
Comment on attachment 8408824 [details] [diff] [review]
update_ToolbarsJS_v3.patch

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

::: firefox/lib/toolbars.js
@@ +350,5 @@
> +
> +  /**
> +   * Check if the identity popup is open
> +   *
> +   * @returns {Boolen} True if the panel is open

nit: boolean

@@ +449,5 @@
> +   *
> +   * @param {Boolean} aForce
> +   *        Force closing the identity popup
> +   */
> +  close : function identityPopup_close(aForce) {

Please move this above open.

@@ +456,5 @@
> +        this._popup.getNode().hidePopup();
> +      }
> +      else {
> +        this.waitForPanel(() => {
> +          this._controller.keypress(this._popup, "VK_ESCAPE", {});

Please use this._popup.keypress here.

@@ +468,5 @@
> +   *
> +   * @param {function} aCallback
> +   *        Function that triggeres the panel to open/close
> +   * @param {Boolean} [aState="open"]
> +   *        State of the panel we are waiting for

I don't think we need this to be separate, we have waitForNotification() in locationBar which waits for any popup notification to open/close.
If you have good reason to keep it though, please use aState as Boolean as you mentioned, so it cannot be "open", just true for opening or false for closed.

@@ +470,5 @@
> +   *        Function that triggeres the panel to open/close
> +   * @param {Boolean} [aState="open"]
> +   *        State of the panel we are waiting for
> +   */
> +  waitForPanel: function downloadsPanel_waitForPanel(aCallback, aState) {

waitForNotification, there's no panel and no downloadsPanel class.

@@ +474,5 @@
> +  waitForPanel: function downloadsPanel_waitForPanel(aCallback, aState) {
> +    assert.equal(typeof aCallback, "function", "Callback function is defined");
> +
> +    var state = aState || "open";
> +    var eventType = (state === "open") ? "popupshown" : "popuphidden";

You can use aState to choose the event here.

@@ +486,5 @@
> +
> +      assert.waitFor(() => {
> +        return checkPopupEvent &&
> +               this._popup.getNode().state === state;
> +      }, "Downloads panel has been opened");

The message should be about the identity popup.

::: firefox/tests/functional/testSecurity/testGreyLarry.js
@@ +30,3 @@
>    expect.ok(!favicon.getNode().hidden, "The globe favicon is visible");
>  
> +  var orgLabel = identityPopup.getElement({type: "organizationLabel"});

I'd prefer organizationLabel for the name of the variable.

::: firefox/tests/remote/testSecurity/testDVCertificate.js
@@ +38,5 @@
>    expect.ok(!favicon.getNode().hasAttribute("hidden"),
>              "Lock icon is visible in identity box");
>  
> +  var favicon = locationBar.getElement({type: "favicon"});
> +  expect.contain(utils.getElementStyle(favicon, "list-style-image"),

Please leave faviconImage variable for the utils call. Same applies to other instances in the patch.
Attachment #8408824 - Flags: review?(andrei.eftimie)
Attachment #8408824 - Flags: review?(andreea.matei)
Attachment #8408824 - Flags: review-
This will enable 5 tests that are currently skipped.
Priority: -- → P1
Whiteboard: [lib][mentor=andreea.matei][lang=js][good first bug] → [lib][mentor=andreea.matei][lang=js]
Attached patch update_ToolbarsJS_v4.patch (obsolete) — Splinter Review
In 50 runs by platform all looks ok on linux, osx & windows.
Attachment #8422278 - Flags: review?(andrei.eftimie)
Attachment #8422278 - Flags: review?(andreea.matei)
Attachment #8408824 - Attachment is obsolete: true
Comment on attachment 8422278 [details] [diff] [review]
update_ToolbarsJS_v4.patch

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

There's a conflict in testSecurity/manifest.ini

::: firefox/lib/toolbars.js
@@ +351,5 @@
> +   *
> +   * @returns {boolen} True if the panel is open
> +   */
> +  get isOpened() {
> +    return (this._popup.getNode().state === 'open');

nit: please use double quotes for consistency

@@ +359,5 @@
> +   * Retrieve an UI element based on the given specification
> +   *
> +   * @param {Object} aSpec
> +   *        Information of the UI elements which should be retrieved
> +   * @parma {String} aSpec.type

nit: native types should be lowercase, please check all

@@ +387,5 @@
> +    var document = this._controller.window.document;
> +
> +    switch(spec.type) {
> +      case "box":
> +        elem = new elementslib.ID(document, "identity-box");

Use `findElement` instead of `elementslib`

@@ +389,5 @@
> +    switch(spec.type) {
> +      case "box":
> +        elem = new elementslib.ID(document, "identity-box");
> +        break;
> +      case "countryLabel":

Could we keep all names in sync with the ids?
We can still strip the prefix "identity" since it's common among all items, and we are in the Identity class.

I would keep a camelCased version of all ids without the mentioned prefix.

@@ +444,5 @@
> +      if (aForce) {
> +        this._popup.getNode().hidePopup();
> +      }
> +      else {
> +        this.waitForPanel(() => {

Do we still need to wait here if the panel is opened?

@@ +468,5 @@
> +   *
> +   * @param {function} aCallback
> +   *        Function that triggeres the panel to open/close
> +   */
> +  waitForPanel: function IdentityPopup_waitForPanel(aCallback, aSpec) {

Wondering if we should also have the desired state, with that we would wait for both the transitionEnd and for (!)isOpen

::: firefox/tests/functional/testSecurity/testGreyLarry.js
@@ +24,2 @@
>    expect.ok(!favicon.getNode().hidden, "The globe favicon is visible");
> +  var orgLabel = identityPopup.getElement({type: "organizationLabel"});

Why not keep the name?
The returned element's id is "identity-icon-label"... I see we stripped the prefix "identity" from the names, so it should be "iconLabel"

::: firefox/tests/functional/testSecurity/testIdentityPopupOpenClose.js
@@ +21,3 @@
>    tabs.closeAllTabs(aModule.controller);
>  }
> +function tearDownModule(aModule) {

`teardownModule`
Attachment #8422278 - Flags: review?(andrei.eftimie)
Attachment #8422278 - Flags: review?(andreea.matei)
Attachment #8422278 - Flags: review-
Comment on attachment 8422278 [details] [diff] [review]
update_ToolbarsJS_v4.patch

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

::: firefox/lib/toolbars.js
@@ +389,5 @@
> +    switch(spec.type) {
> +      case "box":
> +        elem = new elementslib.ID(document, "identity-box");
> +        break;
> +      case "countryLabel":

Not sure to what you are referring here. In general we should use names which are best to understand. And no, we should not be in sync given this reason and that ids can always change.
Those tests were disabled in bug 994040, so lowing down the priority on this.
Priority: P1 → P2
Comment on attachment 8439133 [details] [diff] [review]
update_ToolbarsJS_v4.1.patch

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

Looks good overall, but we have to address some things.

::: firefox/lib/toolbars.js
@@ +338,5 @@
> +   * Check if the identity popup is open
> +   *
> +   * @returns {boolen} True if the panel is open
> +   */
> +  get isOpened() {

I think this should be called `isOpen`

@@ +339,5 @@
> +   *
> +   * @returns {boolen} True if the panel is open
> +   */
> +  get isOpened() {
> +    return (this._popup.getNode().state === 'open');

We can omit the outer parentheses.
Also please use double quotes.

@@ +347,5 @@
> +   * Retrieve an UI element based on the given specification
> +   *
> +   * @param {Object} aSpec
> +   *        Information of the UI elements which should be retrieved
> +   * @parma {String} aSpec.type

Use lowercase for native types. `object`, `string`

@@ +366,5 @@
> +   *        Information of the UI elements which should be retrieved
> +   * @parma {String} aSpec.type
> +   *        Identifier of the element
> +   *
> +   * @returns {[ElemBase]} Elements which have been found

{ElemBase[]}

@@ +373,5 @@
> +    var spec = aSpec || { };
> +    var elem = null;
> +    var document = this._controller.window.document;
> +
> +    switch(spec.type) {

nit: missing space after the `switch` keyword

@@ +375,5 @@
> +    var document = this._controller.window.document;
> +
> +    switch(spec.type) {
> +      case "box":
> +        elem = new elementslib.ID(document, "identity-box");

Please use findElement

::: firefox/tests/functional/testSecurity/testIdentityPopupOpenClose.js
@@ +25,4 @@
>    tabs.closeAllTabs(aModule.controller);
>  }
>  
> +function tearDownModule(aModule) {

Typo: should be `teardownModule`

::: firefox/tests/remote/testSecurity/testMixedContentPage.js
@@ +40,2 @@
>  
> +  var encryptionPopup = identityPopup.getElement({type:"popup"});

nit: missing space after colon

::: firefox/tests/remote/testSecurity/testSecurityInfoViaMoreInformation.js
@@ +13,5 @@
>  
>  var setupModule = function(aModule) {
>    aModule.controller = mozmill.getBrowserController();
>    aModule.locationBar = new toolbars.locationBar(aModule.controller);
> +  aModule.identityPopup = locationBar.identityPopup;

aModule.locationBar

Please check all changed files, I think this is needed on all of them.

::: firefox/tests/remote/testSecurity/testSecurityNotification.js
@@ +19,5 @@
>  
>  var setupModule = function(aModule) {
>    aModule.controller = mozmill.getBrowserController();
> +  aModule.locationBar = new toolbars.locationBar(aModule.controller);
> +  aModule.identityPopup = locationBar.identityPopup;

aModule.locationBar
Attachment #8439133 - Flags: review?(andrei.eftimie)
Attachment #8439133 - Flags: review?(andreea.matei)
Attachment #8439133 - Flags: review-
Mentor: andreea.matei
Whiteboard: [lib][mentor=andreea.matei][lang=js] → [lib][lang=js]
Attached patch v4.2.patch (obsolete) — Splinter Review
Updated based on review.
Attachment #8439133 - Attachment is obsolete: true
Attachment #8449205 - Flags: review?(andrei.eftimie)
Attachment #8449205 - Flags: review?(andreea.matei)
Comment on attachment 8449205 [details] [diff] [review]
v4.2.patch

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

Something is not right with this patch. Please check if you uploaded the final version?
I'm missing most changes from the last review and it doesn't apply cleanly.
Attachment #8449205 - Flags: review?(andrei.eftimie)
Attachment #8449205 - Flags: review?(andreea.matei)
Attachment #8449205 - Flags: review-
Attached patch v4.3.patch (obsolete) — Splinter Review
Sorry! Forgot to refresh the changes.
Fix that & we are good to go.

http://mozmill-crowd.blargon7.com/#/functional/report/b4d880bccb8d9ea0732eac8cd801e74e
http://mozmill-crowd.blargon7.com/#/remote/report/b4d880bccb8d9ea0732eac8cd801e6a8
Attachment #8449205 - Attachment is obsolete: true
Attachment #8452141 - Flags: review?(andrei.eftimie)
Attachment #8452141 - Flags: review?(andreea.matei)
Comment on attachment 8452141 [details] [diff] [review]
v4.3.patch

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

This looks good to me.

Henrik, please review this patch.
Attachment #8452141 - Flags: review?(hskupin)
Attachment #8452141 - Flags: review?(andrei.eftimie)
Attachment #8452141 - Flags: review?(andreea.matei)
Attachment #8452141 - Flags: review+
Comment on attachment 8452141 [details] [diff] [review]
v4.3.patch

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

::: firefox/lib/toolbars.js
@@ +387,5 @@
> +      case "host":
> +        elem = findElement.ID(parent, "identity-popup-content-host");
> +        break;
> +      case "label":
> +        elem = findElement.ID(parent, "identity-popup-encryption-label");

This is identical to the case above.

@@ +389,5 @@
> +        break;
> +      case "label":
> +        elem = findElement.ID(parent, "identity-popup-encryption-label");
> +        break;
> +      case "lockIcon":

I would stay with encryptionIcon.

::: firefox/tests/functional/testSecurity/testIdentityPopupOpenClose.js
@@ +49,4 @@
>  
> +  var moreInfo = identityPopup.getElement({type: "moreInfoButton"});
> +  assert.equal(moreInfo.getNode().localName, "button",
> +               "More information button has been found");

I don't see why we need this assert here. It should be part of the unit test for the identity popup class. And that one I miss in this patch.

::: firefox/tests/remote/testSecurity/testDVCertificate.js
@@ +17,3 @@
>    aModule.controller = mozmill.getBrowserController();
>    aModule.locationBar = new toolbars.locationBar(aModule.controller);
> +  aModule.identityPopup = aModule.locationBar.identityPopup;

nit: order.

@@ +103,4 @@
>   *        MozMillController of the window to operate on
>   */
> +function checkSecurityTab(aController) {
> +  var securityTab = findElement.ID(aController.window.document, "securityTab");

Those changes are totally unrelated to this patch. I would love if we could skip those things in the future and do it separately. We mentioned that already a couple of times.

::: firefox/tests/remote/testSecurity/testGreenLarry.js
@@ +15,5 @@
>  
>  var setupModule = function(aModule) {
>    aModule.controller = mozmill.getBrowserController();
>    aModule.locationBar = new toolbars.locationBar(aModule.controller);
> +  aModule.identityPopup = aModule.locationBar.identityPopup;

order.

@@ +45,5 @@
>  
>    // Check the label displays
>    // Format: Organization (CountryCode)
> +  var identOrgLabel = identityPopup.getElement({type: "organizationLabel"});
> +  var identCountryLabel = identityPopup.getElement({type: "countryLabel"});

orgLabel and countryLabel as names should also be fine.

::: firefox/tests/remote/testSecurity/testSecurityInfoViaMoreInformation.js
@@ +46,4 @@
>      identityBox.click();
>    }, {type: "identity"});
>  
> +  var doorhanger = identityPopup.getElement({type: "popup"});

I don't see that we use that at all.
Attachment #8452141 - Flags: review?(hskupin) → review-
Attached patch v5.patch (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) [away 07/19 - 08/01] from comment #67)
> Comment on attachment 8452141 [details] [diff] [review]
> v4.3.patch
> 
> Review of attachment 8452141 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: firefox/tests/remote/testSecurity/testDVCertificate.js
> @@ +17,3 @@
> >    aModule.controller = mozmill.getBrowserController();
> >    aModule.locationBar = new toolbars.locationBar(aModule.controller);
> > +  aModule.identityPopup = aModule.locationBar.identityPopup;
> 
> nit: order.
> 

identityPopup needs locationBar to be instantiated

> @@ +103,4 @@
> >   *        MozMillController of the window to operate on
> >   */
> > +function checkSecurityTab(aController) {
> > +  var securityTab = findElement.ID(aController.window.document, "securityTab");
> 
> Those changes are totally unrelated to this patch. I would love if we could
> skip those things in the future and do it separately. We mentioned that
> already a couple of times.
> 

I don't see the problem with making those changes here as we already touch this files!
But I will revert them, as you requested.

> ::: firefox/tests/remote/testSecurity/testGreenLarry.js
> @@ +15,5 @@
> >  
> >  var setupModule = function(aModule) {
> >    aModule.controller = mozmill.getBrowserController();
> >    aModule.locationBar = new toolbars.locationBar(aModule.controller);
> > +  aModule.identityPopup = aModule.locationBar.identityPopup;
> 
> order.
> 

same.
Attachment #8452141 - Attachment is obsolete: true
Attachment #8460207 - Flags: review?(andrei.eftimie)
Attachment #8460207 - Flags: review?(andreea.matei)
Comment on attachment 8460207 [details] [diff] [review]
v5.patch

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

Just a few items mentioned inline, otherwise this looks good to me.

+
I've noticed something in the waitForNotificationPanel method fromt toolbars.js
This is not strictly related to this patch, so we might open a new bug to enhance it a bit.
Basically if the no panel is open and you try to close it, the failure message is that we failed to close the panel (which we did, but because the panel was not open in the first place).
We should file a bug to get that enhanced (based on the state we wait for, we should assert at the beginning of that method that the panel is in the reversed state).

::: firefox/lib/tests/testIndentity.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

There is a typo in the filename. I think it wanted to be "Identity"

@@ +4,5 @@
> +
> +// Include required modules
> +var toolbars = require("../toolbars");
> +
> +const BASE_URL = collector.addHttpResource("../../../data/");

We don't seem to be using any local files in this test.

@@ +5,5 @@
> +// Include required modules
> +var toolbars = require("../toolbars");
> +
> +const BASE_URL = collector.addHttpResource("../../../data/");
> +const TEST_DATA = "www.mozilla.org";

Can we switch to a mozqa controlled subdomain?
The test passes with "ssl-dv.mozqa.com" (not sure sure though this is the one we should use, or one of the other ssl related subdomains)
Attachment #8460207 - Flags: review?(andrei.eftimie)
Attachment #8460207 - Flags: review?(andreea.matei)
Attachment #8460207 - Flags: review-
Attached patch v5.1.patch (obsolete) — Splinter Review
I will file a bug for that.
Attachment #8460207 - Attachment is obsolete: true
Attachment #8463313 - Flags: review?(andrei.eftimie)
Attachment #8463313 - Flags: review?(andreea.matei)
Comment on attachment 8463313 [details] [diff] [review]
v5.1.patch

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

Looks good to me.
Just one nit as mentioned inline, with that updated please request a review from Henrik.

Thanks!

::: firefox/lib/tests/testIdentity.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Include required modules
> +var toolbars = require("../toolbars");
> +var utils = require("../utils");

This is not used. You can remove the include.
Attachment #8463313 - Flags: review?(andrei.eftimie)
Attachment #8463313 - Flags: review?(andreea.matei)
Attachment #8463313 - Flags: review+
Attached patch v5.2.patch (obsolete) — Splinter Review
Attachment #8463313 - Attachment is obsolete: true
Attachment #8465424 - Flags: review?(hskupin)
Comment on attachment 8465424 [details] [diff] [review]
v5.2.patch

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

The libs and tests look fine now. But please make the changes as mentioned below. No need to ask for review from me again.

::: firefox/lib/tests/testIdentity.js
@@ +4,5 @@
> +
> +// Include required modules
> +var toolbars = require("../toolbars");
> +
> +const TEST_DATA = "https://ssl-dv.mozqa.com";

Also both following const definitions are part of the TEST_DATA. It's not only the URL. So please combine so we have .url, .elements, and .popup_elements
Attachment #8465424 - Flags: review?(hskupin) → review+
Attached patch v5.3.patch (obsolete) — Splinter Review
Final patch.
Attachment #8465424 - Attachment is obsolete: true
Attachment #8469251 - Flags: review?(andrei.eftimie)
Attachment #8469251 - Flags: review?(andreea.matei)
Comment on attachment 8469251 [details] [diff] [review]
v5.3.patch

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

Just one small thing and we can land this.

::: firefox/lib/tests/testIdentity.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Oh, seems we missed something, please add this test to the manifest.
Attachment #8469251 - Flags: review?(andrei.eftimie)
Attachment #8469251 - Flags: review?(andreea.matei)
Attachment #8469251 - Flags: review-
Attached patch v5.4.patchSplinter Review
Attachment #8469251 - Attachment is obsolete: true
Attachment #8469962 - Flags: review?(andrei.eftimie)
Comment on attachment 8469962 [details] [diff] [review]
v5.4.patch

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

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/5b6f0d0101e1 (default)
Attachment #8469962 - Flags: review?(andrei.eftimie)
Attachment #8469962 - Flags: review+
Attachment #8469962 - Flags: checkin+
Works fine on aurora, transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/2caa6e3d88d3 (mozilla-aurora)

Lets land this on lower branches early next week.
Comment on attachment 8470778 [details] [diff] [review]
beta-release.patch

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

Sorry, this doesn't apply on beta or release cleanly.
Have you uploaded the right patch?
Attachment #8470778 - Flags: review?(andrei.eftimie) → review-
Yes, sorry, wrong patch there.
Attachment #8470778 - Attachment is obsolete: true
Attachment #8470831 - Flags: review?(andrei.eftimie)
Comment on attachment 8470831 [details] [diff] [review]
beta-release.patch

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

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/c2a432b6d015 (mozilla-beta)
https://hg.mozilla.org/qa/mozmill-tests/rev/95f137a11891 (mozilla-release)
https://hg.mozilla.org/qa/mozmill-tests/rev/73a9c393764c (mozilla-esr31)
Attachment #8470831 - Flags: review?(andrei.eftimie)
Attachment #8470831 - Flags: review+
Attachment #8470831 - Flags: checkin+
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.