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

VERIFIED FIXED

Status

VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: vladmaniac, Assigned: vladmaniac)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr10 fixed)

Details

(Whiteboard: [lib])

Attachments

(2 attachments, 10 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
Assignee: nobody → vlad.mozbugs
Blocks: 705122
Status: NEW → ASSIGNED
(Assignee)

Updated

7 years ago
Whiteboard: [mozmill-shared-modules]
(Assignee)

Comment 1

7 years ago
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
Same as for the autocomplete box I would say, yes. You can convert toolbar to use nodecollector or use the old style for now.
Whiteboard: [mozmill-shared-modules]
(Assignee)

Comment 3

7 years ago
Created attachment 580881 [details] [diff] [review]
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
Attachment #580881 - Flags: review?(hskupin)
Summary: Toolbar module should include the Edit Bookmarks Panel component of the awesome bar → Add the 'Edit Bookmarks Panel' component to the toolbar module
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.
Attachment #580881 - Flags: review?(hskupin) → review-
(Assignee)

Comment 5

7 years ago
Created attachment 581912 [details] [diff] [review]
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.
Attachment #580881 - Attachment is obsolete: true
Attachment #581912 - Flags: review?(hskupin)
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.
Attachment #581912 - Flags: review?(hskupin) → review-
(Assignee)

Comment 7

7 years ago
> > 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.
No, I meant that we should add another line for the editBookmarksPanel class and an implicit instantiation.
(Assignee)

Comment 9

7 years ago
(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;
}
(Assignee)

Comment 10

7 years ago
Created attachment 582795 [details] [diff] [review]
patch v3.0

i hope it's ok
Attachment #581912 - Attachment is obsolete: true
Attachment #582795 - Flags: review?(hskupin)
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.
Attachment #582795 - Flags: review?(hskupin) → review-
(Assignee)

Comment 12

7 years ago
Created attachment 582805 [details] [diff] [review]
patch v4.0

fixed
Attachment #582795 - Attachment is obsolete: true
Attachment #582805 - Flags: review?(hskupin)
Comment on attachment 582805 [details] [diff] [review]
patch v4.0

It still misses the last item of review comment 6.
Attachment #582805 - Flags: review?(hskupin)
(Assignee)

Comment 14

7 years ago
(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.
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.
(Assignee)

Comment 16

7 years ago
(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
(Assignee)

Comment 17

7 years ago
Created attachment 583783 [details] [diff] [review]
patch v5.0

addressed the needs of comment 6
Attachment #582805 - Attachment is obsolete: true
Attachment #583783 - Flags: review?(hskupin)
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+.
Attachment #583783 - Flags: review?(hskupin) → review+
(Assignee)

Comment 19

7 years ago
Created attachment 584036 [details] [diff] [review]
patch v6.0

fixed
Attachment #583783 - Attachment is obsolete: true
Attachment #584036 - Flags: review?(hskupin)
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.
Attachment #584036 - Flags: review?(hskupin) → review-
(Assignee)

Comment 21

7 years ago
Created attachment 592623 [details] [diff] [review]
patch v7.0

All change requests addressed
Attachment #584036 - Attachment is obsolete: true
Attachment #592623 - Flags: review?(hskupin)
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.
Attachment #592623 - Flags: review?(hskupin) → review+
(Assignee)

Comment 23

7 years ago
Created attachment 593413 [details] [diff] [review]
patch v8.0

Nit fixed
Attachment #592623 - Attachment is obsolete: true
Attachment #593413 - Flags: review?(hskupin)
Attachment #593413 - Flags: review?(hskupin) → review+
Comment on attachment 593413 [details] [diff] [review]
patch v8.0

Applying the patch failed on default. Please update the patch.
Attachment #593413 - Flags: review+ → review-
(Assignee)

Comment 25

7 years ago
Created attachment 593762 [details] [diff] [review]
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.
Attachment #593413 - Attachment is obsolete: true
Attachment #593762 - Flags: review?(hskupin)
Comment on attachment 593762 [details] [diff] [review]
fix patch v8.0

Works now. Thanks.
Attachment #593762 - Flags: review?(hskupin) → review+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [landed default]
(Assignee)

Comment 28

7 years ago
(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
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.
Whiteboard: [landed default]
(Assignee)

Comment 30

7 years ago
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
Status: RESOLVED → VERIFIED
Component: Mozmill Shared Modules → Mozmill Tests
Whiteboard: [lib]
Blocks: 787924
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.
status-firefox-esr10: --- → affected
(Assignee)

Comment 32

6 years ago
Created attachment 663975 [details] [diff] [review]
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.
Attachment #663975 - Flags: review?(hskupin)
(Assignee)

Updated

6 years ago
Attachment #663975 - Flags: review?(dave.hunt)
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.
Attachment #663975 - Flags: review?(hskupin)
Attachment #663975 - Flags: review?(dave.hunt)
Attachment #663975 - Flags: review-
QA Contact: mozmill-shared-modules
(Assignee)

Comment 34

6 years ago
Created attachment 664011 [details] [diff] [review]
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.
Attachment #663975 - Attachment is obsolete: true
Attachment #664011 - Flags: review?(hskupin)
(Assignee)

Updated

6 years ago
Attachment #664011 - Flags: review?(dave.hunt)
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.
Attachment #664011 - Flags: review?(hskupin)
Attachment #664011 - Flags: review?(dave.hunt)
Attachment #664011 - Flags: review-
(Assignee)

Comment 36

6 years ago
Created attachment 664375 [details] [diff] [review]
esr patch v1.2

fixed to export the class
Attachment #664011 - Attachment is obsolete: true
Attachment #664375 - Flags: review?(hskupin)
(Assignee)

Updated

6 years ago
Attachment #664375 - Flags: review?(dave.hunt)
Attachment #664375 - Flags: review?(hskupin)
Attachment #664375 - Flags: review?(dave.hunt)
Attachment #664375 - Flags: review+
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.
Landed on esr10:
http://hg.mozilla.org/qa/mozmill-tests/rev/a31be7a4c981
status-firefox-esr10: affected → fixed
You need to log in before you can comment on or make changes to this bug.