Closed Bug 1344017 Opened 7 years ago Closed 7 years ago

editBookmarkOverlay.js - onItemMoved aItemOd is undefined & enable no-undef for browser/components

Categories

(Firefox :: Bookmarks & History, defect)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(2 files)

Bug 1344017 - Fix an issue where the folder picker in the edit bookmark overlay is not always be updated correctly.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1344017 - Turn on ESLint rule no-undef for browser/components.
59 bytes, text/x-review-board-request
jaws
: review+
Details 
In working towards enabling no-undef for eslint, I've come across this issue:

browser/components/places/content/editBookmarkOverlay.js
  1125:34  error  'aItemOd' is not defined.  no-undef (eslint)

This is here:

https://dxr.mozilla.org/mozilla-central/rev/e91de6fb2b3dce9c932428265b0fdb630ea470d7/browser/components/places/content/editBookmarkOverlay.js#1117

However, I've also noticed that earlier in that if statement, this part will always be true:

    !this._paneInfo.visibleRows.has("folderPicker")

this is because the "folderPicker" is for the hidden list, and in the visible rows it should be referred to as "folderRow".

So currently the code never gets past the if clause.

The code was added in bug 951651 (FF 40).

Note: I also attempted to test the effect of doing the selectedItem assignment (i.e. making the code work), however even though I hit it, I couldn't see a visible change - it seems the menus all update themselves anyway.
Flags: needinfo?(mak77)
The STR for this to happen are likely quite complex:
1. open a new window
2. Open a page with a frame
3. From the frame contextual menu This Frame > Bookmark this frame
4. The add bookmark dialog should open with the folder list on Bookmarks Menu
5. DO _NOT_ CLOSE THE BOOKMARK DIALOG
6. Go back to the first window, find the new bookmark in the Bookmarks Menu
7. Drag the bookmark to the toolbar

AR: The bookmark dialog folder list still tells that the bookmark is in the Bookmarks Menu
ER: The folder list should tell that the bookmark is now on the Bookmarks Toolbar
Flags: needinfo?(mak77)
in a test it would not be needed to open a new window, it would be enough to use PlacesUtils.bookmarks.update to change the bookmark position while the dialog is open.
Assignee: nobody → standard8
Thanks for the hints about the STR, Marco - that was very useful in writing the test.

Since this is the last no-undef issue in browser/components, I've included a separate patch to enable no-undef here to save a separate bug.
Summary: editBookmarkOverlay.js - onItemMoved aItemOd is undefined → editBookmarkOverlay.js - onItemMoved aItemOd is undefined & enable no-undef for browser/components
Comment on attachment 8844821 [details]
Bug 1344017 - Fix an issue where the folder picker in the edit bookmark overlay is not always be updated correctly.

https://reviewboard.mozilla.org/r/118154/#review120024

::: browser/components/places/tests/browser/browser_addBookmarkForFrame.js:13
(Diff revision 1)
> +const RIGHT_URL = BASE_URL + "/frameRight.html";
> +
> +function* withAddBookmarkForFrame(taskFn) {
> +  // Open a tab and wait for all the subframes to load.
> +  let tab = gBrowser.addTab(PAGE_URL);
> +  yield BrowserTestUtils.browserLoaded(tab.linkedBrowser, true);

To me looks like passing true as second argument to browserLoaded will resolve as soon as _any_ of the frames is loaded, not when all the frames are loaded.
The helper documentation doesn't help much, but the fact it's called "include" is a hint.
I suspect the scope for that bool is to be able to wait for a single specific frame but, if so, when includeSubFrames is true wantLoad _must_ be mandatory. This sounds like a footgun that should be fixed in a separate bug (some tests will need changes).

Regardless, if you really need to wait ALL the frames this won't work, I suggest passing the url of the frame you care about as third argument.

::: browser/components/places/tests/browser/browser_addBookmarkForFrame.js:14
(Diff revision 1)
> +
> +function* withAddBookmarkForFrame(taskFn) {
> +  // Open a tab and wait for all the subframes to load.
> +  let tab = gBrowser.addTab(PAGE_URL);
> +  yield BrowserTestUtils.browserLoaded(tab.linkedBrowser, true);
> +  yield BrowserTestUtils.switchTab(gBrowser, tab);

In general switchTab is a critical operation involving lots of parts, if possible please just use BrowserTestUtils.openNewForegroundTab()

::: browser/components/places/tests/browser/browser_addBookmarkForFrame.js:23
(Diff revision 1)
> +  yield withBookmarksDialog(true, function() {
> +      Task.spawn(function*() {
> +      let popupShownPromise = BrowserTestUtils.waitForEvent(contentAreaContextMenu, "popupshown");
> +      yield BrowserTestUtils.synthesizeMouse("#left", 5, 5,
> +        { type: "contextmenu", button: 2}, gBrowser.selectedBrowser);
> +      yield popupShownPromise;

I think you can move all of this code before yield withBookmarksDialog and just keep the click() calls inside the openFn argument (avoiding the Task.spawn).

::: browser/components/places/tests/browser/browser_addBookmarkForFrame.js:66
(Diff revision 1)
> +    let placesBundle =
> +      Services.strings.createBundle("chrome://places/locale/places.properties");
> +    let bookmarksMenuFolderName =
> +      placesBundle.GetStringFromName("BookmarksMenuFolderTitle");
> +    let toolbarFolderName =
> +      placesBundle.GetStringFromName("BookmarksToolbarFolderTitle");

just PlacesUtils.getString(key);

::: browser/components/places/tests/browser/browser_addBookmarkForFrame.js:68
(Diff revision 1)
> +    let bookmarksMenuFolderName =
> +      placesBundle.GetStringFromName("BookmarksMenuFolderTitle");
> +    let toolbarFolderName =
> +      placesBundle.GetStringFromName("BookmarksToolbarFolderTitle");
> +
> +    let uri = Services.io.newURI(LEFT_URL);

either NetUtil.newURI or makeURI

::: browser/components/places/tests/browser/browser_addBookmarkForFrame.js:76
(Diff revision 1)
> +    // Check the initial state of the folder picker.
> +    Assert.equal(folderPicker.selectedItem.label,
> +                 bookmarksMenuFolderName, "The folder is the expected one.");
> +
> +    // Check the bookmark has been created as expected.
> +    Assert.ok(PlacesUtils.bookmarks.isBookmarked(uri),

please use yield PlacesUtils.bookmarks.fetch (for docs see Bookmarks.jsm). what you are using is the old synchronous API for which we should avoid adding new consumers.

::: browser/components/places/tests/browser/browser_addBookmarkForFrame.js:79
(Diff revision 1)
> +
> +    // Check the bookmark has been created as expected.
> +    Assert.ok(PlacesUtils.bookmarks.isBookmarked(uri),
> +              "The page should be bookmarked.");
> +
> +    let bookmarkIds = PlacesUtils.bookmarks.getBookmarkIdsForURI(uri);

Not needed with the new API

::: browser/components/places/tests/browser/browser_addBookmarkForFrame.js:81
(Diff revision 1)
> +    Assert.ok(PlacesUtils.bookmarks.isBookmarked(uri),
> +              "The page should be bookmarked.");
> +
> +    let bookmarkIds = PlacesUtils.bookmarks.getBookmarkIdsForURI(uri);
> +
> +    Assert.equal(bookmarkIds.length, 1, "There should be exactly one bookmark.");

not needed with the new API (just check the returned bookmark object properties)

::: browser/components/places/tests/browser/browser_addBookmarkForFrame.js:83
(Diff revision 1)
> +
> +    let bookmarkIds = PlacesUtils.bookmarks.getBookmarkIdsForURI(uri);
> +
> +    Assert.equal(bookmarkIds.length, 1, "There should be exactly one bookmark.");
> +
> +    Assert.equal(PlacesUtils.bookmarks.getFolderIdForItem(bookmarkIds[0]),

ditto

::: browser/components/places/tests/browser/browser_addBookmarkForFrame.js:88
(Diff revision 1)
> +    Assert.equal(PlacesUtils.bookmarks.getFolderIdForItem(bookmarkIds[0]),
> +                 PlacesUtils.bookmarks.bookmarksMenuFolder,
> +                 "The bookmark should be in the unfiled folder.");
> +
> +    // Now move the bookmark and check the folder picker is updated correctly.
> +    PlacesUtils.bookmarks.moveItem(bookmarkIds[0],

yield PlacesUtils.bookmarks.update()

just change the parentGuid in the bookmark object you got from fetch, and pass it to update :)
Attachment #8844821 - Flags: review?(mak77)
Comment on attachment 8844822 [details]
Bug 1344017 - Turn on ESLint rule no-undef for browser/components.

https://reviewboard.mozilla.org/r/118156/#review120122
Attachment #8844822 - Flags: review?(jaws) → review+
Comment on attachment 8844821 [details]
Bug 1344017 - Fix an issue where the folder picker in the edit bookmark overlay is not always be updated correctly.

https://reviewboard.mozilla.org/r/118154/#review121038

::: browser/components/places/tests/browser/browser_addBookmarkForFrame.js:18
(Diff revision 2)
> +
> +  let contentAreaContextMenu = document.getElementById("contentAreaContextMenu");
> +
> +  let popupShownPromise = BrowserTestUtils.waitForEvent(contentAreaContextMenu, "popupshown");
> +  yield BrowserTestUtils.synthesizeMouse("#left", 5, 5,
> +    { type: "contextmenu", button: 2}, gBrowser.selectedBrowser);

BrowserUtils.synthesizeMouseAtCenter may allow you to avoid the "5, 5".

::: browser/components/places/tests/browser/browser_addBookmarkForFrame.js:42
(Diff revision 2)
> +    Assert.equal(namepicker.value, "Left frame", "Name field is correct.");
> +
> +    let placesBundle =
> +      Services.strings.createBundle("chrome://places/locale/places.properties");
> +    let expectedFolderName =
> +      placesBundle.GetStringFromName("BookmarksMenuFolderTitle");

This is still uneeded, just use PlacesUtils.getString("BookmarksMenuFolderTitle");

::: browser/components/places/tests/browser/browser_addBookmarkForFrame.js:72
(Diff revision 2)
> +    // Check the bookmark has been created as expected.
> +    let bookmark = yield PlacesUtils.bookmarks.fetch({url});
> +
> +    Assert.equal(bookmark.parentGuid,
> +                 PlacesUtils.bookmarks.menuGuid,
> +                 "The bookmark should be in the unfiled folder.");

The comment doesn't reflect the check, it's comparing with menuGuid
Attachment #8844821 - Flags: review?(mak77) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c44f810b07af
Fix an issue where the folder picker in the edit bookmark overlay is not always be updated correctly. r=mak
https://hg.mozilla.org/integration/autoland/rev/4ceb9062ea8f
Turn on ESLint rule no-undef for browser/components. r=jaws
https://hg.mozilla.org/mozilla-central/rev/c44f810b07af
https://hg.mozilla.org/mozilla-central/rev/4ceb9062ea8f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
20072017 - Bug Triage Day 

Bug 1344017, has been resolved 

Steps for confirming the triage:

1.The bookmark was added in new page

2.The bookmark was added to the toolbar while opening bookmark tab in first window by dragging and droping it in toolbar

3.Now, the bookmark was added to the toolbar 


status fixed in firefox beta 55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: