708582 - Add the 'Edit Bookmarks Panel' component to the toolbar module

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Maniac Vlad Florin (:vladmaniac)]

we don't want to directly instantiate objects in tests using elementslib and so on so forth, this is why we need to update the toolbar mozmill module to include the Edit Bookmarks Panel elements 

For starters I'd say we could have the UI map of the elements, which we can use in tests.

Comment 1

[Maniac Vlad Florin (:vladmaniac)]

I was thinking to include the Edit Bookmarks Panel element under the locationBar Class Ui Map.
The locationBar ui map uses elementslib to retrieve elements. should we be consistent or use the nodeCollector?  

Then add a function, under the same class, to wait for this panel, called waitForEditBookmarksPanel() 

Opinions are highly appreciated

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Same as for the autocomplete box I would say, yes. You can convert toolbar to use nodecollector or use the old style for now.

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.0

decided to go with the old style because p1 is now having endurance coverage

added a method to wait for the edit bookmarks dialog

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 580881 [details] [diff] [review]
patch v1.0

All that code has to be moved into a separate class, exactly like the autoCompleteResults class.

Comment 5

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2.0

Fixed to use a new class 

Still, kept the old style for grabbing elements in the UI map to be consistent with the other classes.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 581912 [details] [diff] [review]
patch v2.0

>+  waitForEditBookmarksPanel: function editBookmarksPanel_waitFor(aSpec) {
>+    var spec = aSpec || { };
>+    var timeout = (spec.timeout == undefined) ? TIMEOUT : spec.timeout;
>+
>+    mozmill.utils.waitFor(function() {
>+      return this._controller.window.top.StarUI._overlayLoaded;
>+    }, "Edit Bookmarks Panel has been loaded", timeout, undefined, this);

What does this function wait for? It has to be explicitly called out in the function name. Also in the message you should use "opened".

> function locationBar(controller)
> {
>   this._controller = controller;
>   this._autoCompleteResults = new autoCompleteResults(controller);
> }

We should do the same for the editBookmarksPanel class, so it gets created implicitly. 

> exports.locationBar = locationBar;
> exports.autoCompleteResults = autoCompleteResults;
>+exports.editBookmarksPanel = editBookmarksPanel;

Can you please completely re-order the list?

Also, I would like to see the testAddBookmarkToMenu.js test updated to use the new API. That will prove its functionality.

r- to get the remaining requests fixed.

Comment 7

[Maniac Vlad Florin (:vladmaniac)]

> > function locationBar(controller)
> > {
> >   this._controller = controller;
> >   this._autoCompleteResults = new autoCompleteResults(controller);
> > }
> 
> We should do the same for the editBookmarksPanel class, so it gets created
> implicitly. 
> 
We currently have this constructor

function editBookmarksPanel(controller) {
  this._controller = controller;
}

but I see no point of having a autoCompleteResults object instance if I do not use it.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No, I meant that we should add another line for the editBookmarksPanel class and an implicit instantiation.

Comment 9

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> No, I meant that we should add another line for the editBookmarksPanel class
> and an implicit instantiation.

I see, we use {} syntax for implicit object instantiation. 
so we are going to have this now 

function editBookmarksPanel(controller) 
{
  this._controller = controller;
}

Comment 10

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.0

i hope it's ok

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 582795 [details] [diff] [review]
patch v3.0

>+function editBookmarksPanel(controller) 
>+{

Why have you moved the bracket to the new line?


(In reply to Maniac Vlad Florin (:vladmaniac) from comment #9)
> (In reply to Henrik Skupin (:whimboo) from comment #8)
> > No, I meant that we should add another line for the editBookmarksPanel class
> > and an implicit instantiation.
> 
> I see, we use {} syntax for implicit object instantiation. 
> so we are going to have this now 
> 
> function editBookmarksPanel(controller) 
> {
>   this._controller = controller;
> }

We use what? Sorry, but I cannot follow your thoughts here.

> function locationBar(controller)
> {
>   this._controller = controller;
>   this._autoCompleteResults = new autoCompleteResults(controller);
> }

The line for the editBookmarksPanel instantiation has still not been added.

Comment 12

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v4.0

fixed

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 582805 [details] [diff] [review]
patch v4.0

It still misses the last item of review comment 6.

Comment 14

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #13)
> Comment on attachment 582805 [details] [diff] [review]
> patch v4.0
> 
> It still misses the last item of review comment 6.

Is that part of this patch ? I think modifying the test would be part of a new bug, this one is to unblock the endurance test in bug 705122, which is P1.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As I have already said, I want to see that this class works. And that change can be done quickly, compared to writing an API test.

Comment 16

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #15)
> As I have already said, I want to see that this class works. And that change
> can be done quickly, compared to writing an API test.

I get it now - we are doing unit tests for every major API change and that test would be a good unit one and we shoot two rabbits with one bullet. 

Thanks for clarifications

Comment 17

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v5.0

addressed the needs of comment 6

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 583783 [details] [diff] [review]
patch v5.0

> function locationBar(controller)
> {
>   this._controller = controller;
>   this._autoCompleteResults = new autoCompleteResults(controller);
>+  this._editBookmarksPanel = new editBookmarksPanel(controller);
> }

That's fine now, but please also add a getter to the locationBar class for this panel. See the one for the autoCompleteResults.

Otherwise it looks good. With that change it will get my r+.

Comment 19

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v6.0

fixed

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 584036 [details] [diff] [review]
patch v6.0

>+++ b/lib/toolbars.js
>@@ -260,20 +261,106 @@ autoCompleteResults.prototype = {
>+      case "editBookmarksPanel_doneButton":

Remove all 'editBookmarksPanel_' prefixes from each case. It's already part of the class name.

>+++ b/tests/functional/testBookmarks/testAddBookmarkToMenu.js
>@@ -33,25 +34,27 @@
> var setupModule = function() {
>   controller = mozmill.getBrowserController();
>+  editBookmarksPanel = new toolbars.editBookmarksPanel(controller);

To be mostly compatible to a future rewrite please instantiate the locationBar class here and access the editBookmarksPanel via its newly added property.

>+  editBookmarksPanel.waitForEditBookmarksPanel();

Please rename the class method to waitForPanel(). 'EditBookmarks' is already part of the class name and not necessary here.

Comment 21

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v7.0

All change requests addressed

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 592623 [details] [diff] [review]
patch v7.0

>+  waitForPanel: function editBookmarksPanel_waitForPanelLoad(aSpec) {

Looks good so far. The only thing I have to mention is the above internal name of the method which should be equal to the public one. Means please use 'editBookmarksPanel_waitForPanel(aSpec)'.

With that change r=me.

Comment 23

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v8.0

Nit fixed

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 593413 [details] [diff] [review]
patch v8.0

Applying the patch failed on default. Please update the patch.

Comment 25

[Maniac Vlad Florin (:vladmaniac)]

Attachment: fix patch v8.0

Something was wrong there I got 

  Hunk #1 FAILED at 14
1 out of 4 hunks FAILED -- saving rejects to file lib/toolbars.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Fixed now with re-patching on a clean repo. Tested applying patch and WFM now.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 593762 [details] [diff] [review]
fix patch v8.0

Works now. Thanks.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on default as:
http://hg.mozilla.org/qa/mozmill-tests/rev/1663964ca460

Please check todays results. If everything is green we can land it on the other branches too.

Comment 28

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #27)
> Landed on default as:
> http://hg.mozilla.org/qa/mozmill-tests/rev/1663964ca460
> 
> Please check todays results. If everything is green we can land it on the
> other branches too.

No failures so far today http://mozmill-release.blargon7.com/#/functional/top?branch=All&platform=All&from=2012-02-02&to=2012-02-02

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The patch has already been merged to Aurora so only checkins for beta and release are necessary:

http://hg.mozilla.org/qa/mozmill-tests/rev/4f7894f32871 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/ed740eb2f7bf (release)

Thanks Vlad for working on it.

Comment 30

[Maniac Vlad Florin (:vladmaniac)]

Did not see any failures across branches for any test using this helper class so marking as verified - will keep monitoring daily the default branch

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Vlad, please create a backport fix for esr10. I think that it is worth now given that multiple places would benefit from that class. So we could even land the endurance test on bug 705122 on esr10.

Comment 32

[Maniac Vlad Florin (:vladmaniac)]

Attachment: esr patch v1.0

just adding the class - this does not refactor any tests. should we want that, I can file another bug for it.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 663975 [details] [diff] [review]
esr patch v1.0

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

Backporting a patch should always include all the changes made in the original patch.

Comment 34

[Maniac Vlad Florin (:vladmaniac)]

Attachment: esr patch v1.1

* right, then this follow up patch matches so that it can be considered a backport patch, containing also the changes added to the bookmark test.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 664011 [details] [diff] [review]
esr patch v1.1

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

::: lib/toolbars.js
@@ +238,5 @@
> +
> +/**
> + * Edit Bookmarks Panel class
> + */
> +editBookmarksPanel.prototype = {

This patch misses the export of the editBookmarksPanel class.

Comment 36

[Maniac Vlad Florin (:vladmaniac)]

Attachment: esr patch v1.2

fixed to export the class

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 664375 [details] [diff] [review]
esr patch v1.2

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

Just as a FYI. Never put the branch name into the commit message especially for patches which will be transplanted. I will update it myself before pushing.

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on esr10:
http://hg.mozilla.org/qa/mozmill-tests/rev/a31be7a4c981