Remove editBookmarksPanel instance from the locationBar

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Daniel Gherasim, Assigned: galgeek, Mentored)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox35 fixed)

Details

(Whiteboard: [lang=js], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
As requested in bug 1067411 Comment 8, we should remove the instance of bookmarksPanel out of the locationBar class in toolbars.js and use directly as a component of the toolbars (toolbars.editBookmarksPanel).

Once bug 1067411 get's landed, waitForNotificationPanel will be out of the locationBar scope so we can use it.

Also tests that uses the method have to be updated.
(Assignee)

Comment 1

3 years ago
Created attachment 8494649 [details] [diff] [review]
patches for tests that used locationBar.editBookmarksPanel

An update to toolbars.js has already made it into the git archive, but I don't see any updates to the tests there yet, so I'm submitting mine.

This is my first bug patch submission. I apologize if I'm making newbie mistakes!
(Assignee)

Updated

3 years ago
Attachment #8494649 - Flags: review?(andrei.eftimie)

Comment 2

3 years ago
Thanks for the patch galgeek.
I'm not sure exactly how broad the scope of this bug actually is.

The initial comment from Henrik is not clear to me.
Henrik, could you please explain what you asked for in bug 1067411 Comment 8?

Do we redefine what the `locationbar` is about? 
If we see it strictly as the Awesomebar (and co), there are several methods/properties that should be severed off this class, like: getElements =>
- bookmarksMenuButton,
- feedButton (I think this might not even exist anymore in the form we have it), 
- we should also break off all notification items from the location bar. From getElements > notificationPopup_buttonMenu, notificationPopup_menuItem, notification_element, notificationIcon, notification_popup... we'll have the favicon as a trigger, but that's it...
- starButton (this moved with Australis outside the locationbar)

Then we have several methods:
- getNotification
- getNotificationElement
- bookmarkWithAnimation
- waitForNotificationPanel

Which all should live outside the locationbar. Maybe we should have a `Notification` class that holds some of them.
Flags: needinfo?(hskupin)
None of those buttons and other elements are part of the location bar! Those are all part of the navigation bar. We should add a new class called NavigationToolbar, which includes class instances of LocationBar, SearchBar, etc... This newly created class could be integrated in the BrowserWindow class, so we have direct access to it, e.g. browser.navbar.locationbar.url.
Flags: needinfo?(hskupin)
(Assignee)

Comment 4

3 years ago
Hello, again!

This bug, 1069325, is titled "Remove editBookmarksPanel instance from the locationBar," submitted by  daniel.gherasim@softvision.ro and marked [good first bug]. Daniel submitted the fix for Bug 1067411, updates to toolbars.js, which looks like it removed both the editBookmarksPanel instance from locationBar as well as the waitForNotificationPanel method. 

My initial patch updates the tests that used locationBar.editBookmarksPanel. I could also work on updating tests that use locationBar.waitForNotificationPanel. Am I on the right track? These updates seem, also, like straightforward refactoring edits. But skimming the discussion on bug 908649, I wonder if your process for updating mozmill tests is more involved.
Flags: needinfo?(hskupin)
(Reporter)

Comment 5

3 years ago
(In reply to galgeek from comment #4)
> Hello, again!
> 
> This bug, 1069325, is titled "Remove editBookmarksPanel instance from the
> locationBar," submitted by  daniel.gherasim@softvision.ro and marked [good
> first bug]. Daniel submitted the fix for Bug 1067411, updates to
> toolbars.js, which looks like it removed both the editBookmarksPanel
> instance from locationBar as well as the waitForNotificationPanel method. 
> 

None of the changes you said here were made through the patch  on bug 1067411.
waitForNotificationPanel was indeed touched, but not removed. Part of the code was exposed as a global method, for reasons like here, so we can call it from the for the bookmark panel which will not be inside locationBar anymore.

> My initial patch updates the tests that used locationBar.editBookmarksPanel.
> I could also work on updating tests that use
> locationBar.waitForNotificationPanel. Am I on the right track? These updates
> seem, also, like straightforward refactoring edits. But skimming the
> discussion on bug 908649, I wonder if your process for updating mozmill
> tests is more involved.

That was the right track to go and why we started thig bug, but looks like things got complicated now and we are talking here about a bigger refactoring, we may file another bug for refactoring or hijack this one, but I'm not the one to decide so leaving the needinfo flag open.
As Daniel said there might be a bit more to do here, but I don't consider it as too complex. But it may not be a good first bug. So what has to be done on this bug would be:

* Create a new ui class (similar to LocationBar) called NavBar
* This created class should contain instances of the location bar and search bar class
* For the combined bookmarks button a separate class might be helpful
** It should contain the getElements() method, which can return the nodes for both buttons
** It contains the handling for the editBookmarksPanel, maybe even some helper methods e.g. 'starred', 'bookmark', 'edit', 'remove'.
* The Australis menu button would be optional here, and we should do it in another bug

galgeek, if you have interest to work on this, we would support you with any kind of information you need. It would be a good learning experience. But if you want to work on a good first bug, we can certainly find one for you.
Flags: needinfo?(hskupin)
Whiteboard: [lang=js][good first bug] → [lang=js]
(Assignee)

Comment 7

3 years ago
I apologize; I looked at the last of the patches submitted for bug 1067411, and certainly noticed that toolbars.js was updated and waitForNotificationPanel was now exposed as a global... then I did a git pull, and saw this error -- "locationBar.editBookmarksPanel is undefined" -- when I ran the master branch's tests I've updated against the new toolbars.js. So a patch that un-defined locationBar.editBookmarksPanel got applied somewhere, but I see that it's not part of Daniel's final patch for bug 1067411.

I could start with creating the new NavBar class. Should I plan to update tests referencing locationBar and searchBar, too? And is this a good place to start?

Thank you!
Flags: needinfo?(hskupin)
I think for now it would be good to not touch each and every test regarding locationbar and searchbar. It would blow up the patch a lot. But well, you are free to decide. Personally I would file a follow-up bug to get both properties added to the navbar, and all references updated.
Flags: needinfo?(hskupin)
(Assignee)

Comment 9

3 years ago
Created attachment 8498491 [details] [diff] [review]
patches for tests that used locationBar.editBookmarksPanel and locationBar.waitForNotificationPanel

I'm very sorry! It looks like, in my local repo, my little patch to toolbars.js got committed, and that's what was making locationBar.editBookmarksPanel undefined for tests in the github repo.

I've uploaded an updated patch, including my toolbars.js edit, that also removes references to locationBar.waitForNotificationPanel in those three tests that also reference locationBar.editBookmarksPanel. 

I thought it might be best to check my understanding using these edits, before I tried to do more.

I can look more at adding the navBar class tomorrow, and open another bug.
Attachment #8494649 - Attachment is obsolete: true
Attachment #8494649 - Flags: review?(andrei.eftimie)
(Assignee)

Updated

3 years ago
Attachment #8498491 - Flags: review?(andrei.eftimie)

Comment 10

3 years ago
Comment on attachment 8498491 [details] [diff] [review]
patches for tests that used locationBar.editBookmarksPanel and locationBar.waitForNotificationPanel

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

Ok then, lets go ahead with what Henrik suggested in comment 8.

We're going to to some smaller steps here (what was initially in the description), and I will shortly file another bug to for the rest
(mainly comment 2 and comment 6).

====

Thanks for the patch Barbara. There are a few issues to fix.
Besides those mentioned inline, we should also remove the entry from the locationbar.waitForNotificationPanel:
>      case "bookmark":
>        panel = this._editBookmarksPanel.getElement({type: "bookmarkPanel"});
>        break;

::: firefox/lib/toolbars.js
@@ +460,1 @@
>     */

Please remove this completely.

And also remove the reference saved in the locationBar constructor

::: firefox/tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
@@ +62,2 @@
>        starButton.click();
> +    }, {type: "bookmark", panel: editBookmarksPanel.getElement({type: "bookmarkPanel"})

Please save a reference to the panel before this function call, and reference the panel here and in the next call.

::: firefox/tests/functional/testAwesomeBar/testSuggestBookmarks.js
@@ +48,2 @@
>      controller.mainMenu.click("#menu_bookmarkThisPage");
> +  }, {type: "bookmark", panel: editBookmarksPanel.getElement({type: "bookmarkPanel"})

Same as the other test, save a reference to the panel.

::: firefox/tests/functional/testBookmarks/testAddBookmarkToMenu.js
@@ +35,2 @@
>      controller.mainMenu.click("#menu_bookmarkThisPage");
> +  }, {type: "bookmark", panel: editBookmarksPanel.getElement({type: "bookmarkPanel"})

Also here.
Attachment #8498491 - Flags: review?(andrei.eftimie) → review-

Updated

3 years ago
Blocks: 1076741
(Assignee)

Comment 11

3 years ago
Created attachment 8499053 [details] [diff] [review]
patches for toolbars.js and tests that used locationBar.editBookmarksPanel and locationBar.waitForNotificationPanel

I believe that this addresses all your comments, and fixes a typo in a comment, too!

Maybe still some coding style issues, and I wonder if there's a better name for var bookmarksPanel?
Attachment #8498491 - Attachment is obsolete: true
Attachment #8499053 - Flags: review?(andrei.eftimie)

Comment 12

3 years ago
Comment on attachment 8499053 [details] [diff] [review]
patches for toolbars.js and tests that used locationBar.editBookmarksPanel and locationBar.waitForNotificationPanel

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

Thanks Barbara for the patch.
We'll cover the rest in bug 1076741.

Landed: https://hg.mozilla.org/qa/mozmill-tests/rev/f17ca5e4fae9 (default)

With 2 changes:
- the r= part in the commit message is for the reviewer, the author is already mentioned in the specific Author field

::: firefox/tests/functional/testBookmarks/testAddBookmarkToMenu.js
@@ +46,3 @@
>      doneButton.click();
> +  }, {type: "bookmark", open: false, panel: bookmarksPanel}
> +  );

- and I've move all these closing braces at the end of the previous line.
Attachment #8499053 - Flags: review?(andrei.eftimie)
Attachment #8499053 - Flags: review+
Attachment #8499053 - Flags: checkin+

Comment 13

3 years ago
I think we're done here. I don't see we'll want to backport these sort of changes.

*Note: since you were asking for potential coding style issues, here is our coding style document:
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests/Mozmill_Style_Guide
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox35: --- → fixed
Resolution: --- → FIXED
Assignee: nobody → galgeek
(Assignee)

Comment 14

3 years ago
Andrei, thank you for that link, and for all your help!
You need to log in before you can comment on or make changes to this bug.