Closed Bug 1444228 Opened 6 years ago Closed 6 years ago

Remove editBookmarkOverlay.xul

Categories

(Firefox :: Bookmarks & History, enhancement, P1)

Product:
Firefox
Component:
Bookmarks & History
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: bdahl, Assigned: bdahl)

References

Details

Attachments

(1 file)

Bug 1444228 - Remove editBookmarkOverlay.xul.
59 bytes, text/x-review-board-request
standard8
: review+
Details 
The overlay is currently used by:
  - bookmarkProperties.xul
  - places.xul
  - browser.xul (loaded by JS)

This can be removed by moving most of the overlay into an include file and inlining the rest. The one tricky piece is the dynamically loaded overlay, but I'm hoping that having hidden="true" on the panel causes no talos issues when inlined.
As you imagined, the reason for the lazy overlay is the fact it's loaded by the star panel. We must check the various startup and winopen talos tests.

This may allow us to remove the current workaround to align stuff in the star panel(bug 484022)
I've got talos tests underway.

The other tricky thing with this is fixing the various chrome tests that use this overlay. I'll be moving editBookmarkPanelContent into an include file, but it seems rather difficult to use the preprocessor within a chrome test.
heh, we used chrome tests because then we didn't have to open a full dialog window, the Library or the star panel, the overlay made that trivial. If we can't go the overlay path, we may have to convert these tests to mochitest-browser.
For example
https://searchfox.org/mozilla-central/source/browser/components/places/tests/browser/browser_bookmarkProperties_addFolderDefaultButton.js (and the other browser_bookmarkProperties_* tests)
Talos results look good:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=c7fb4fe473b4de5127eed31ae72bb11c937c84e9&newProject=try&newRevision=50997eb7b9037bed7e83ddab82d749a54c8c4819&framework=1

(In reply to Marco Bonardo [::mak] from comment #3)
> heh, we used chrome tests because then we didn't have to open a full dialog
> window, the Library or the star panel, the overlay made that trivial. If we
> can't go the overlay path, we may have to convert these tests to
> mochitest-browser.

I looked into using the preprocessor and preprocessing the test file itself is not really possible. We'd need to preprocess a support file and load that. I was just going to ask you if you'd be fine with me converting the files to browser test files.
Somewhat of an aside, but I originally was planning to replace this overlay by using web components. Web components shipping date was looking somewhat iffy so I decided to go the include route (for now this is also less change). In the future I think this still would be a good candidate for a web component though.
A note, test_editBookmarkOverlay_keywords.xul required the most change since the the keyword UI isn't shown for the start panel. To make sure the test still was exercising the behavior in the original bug, I reverted the patch in bug 1343256 and made sure the new test failed here as well.
Comment on attachment 8958148 [details]
Bug 1444228 - Remove editBookmarkOverlay.xul.

https://reviewboard.mozilla.org/r/227094/#review233272

I'm forwarding the review to Mark for a deeper look, I'm only looking at the whole picture here.

::: browser/base/content/browser.xul:16
(Diff revision 1)
>  <?xml-stylesheet href="chrome://browser/content/places/places.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://browser/content/usercontext/usercontext.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://browser/skin/controlcenter/panel.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://browser/skin/customizableui/panelUI.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://browser/skin/downloads/downloads.css"?>
> +<?xml-stylesheet href="chrome://browser/skin/places/places.css"?>

worth investigating why places.css is necessary here. As far as I can tell it may be just for the trees part, since editBookmarkOverlay includes a tree.

Maybe we should also include the trees part in editBookmarkOverlay.css, or split places.css in placesTrees.css and places.css.

::: browser/base/content/browser.xul:17
(Diff revision 1)
>  <?xml-stylesheet href="chrome://browser/content/usercontext/usercontext.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://browser/skin/controlcenter/panel.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://browser/skin/customizableui/panelUI.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://browser/skin/downloads/downloads.css"?>
> +<?xml-stylesheet href="chrome://browser/skin/places/places.css"?>
> +<?xml-stylesheet href="chrome://browser/skin/places/editBookmarkOverlay.css"?>

Likely this css should be renamed to just editBookmark.css since it's no more an overlay

::: browser/components/places/content/editBookmarkOverlay.js:227
(Diff revision 1)
>      // For sanity ensure that the implementer has uninited the panel before
>      // trying to init it again, or we could end up leaking due to observers.
>      if (this.initialized)
>        this.uninitPanel(false);
>  
> +    document.getElementById("editBookmarkPanelContent").hidden = false;

The lazy loading problem is just for the star panel, would we get the same perf benefit just setting/unsetting hidden in the star panel, rather than on this object? It sounds like cleaner, if possible.

::: browser/components/places/tests/browser/browser_bug427633_no_newfolder_if_noip.js:28
(Diff revision 1)
> +  let shownPromise = promisePopupShown(bookmarkPanel);
> +
> +  let bookmarkStar = BookmarkingUI.star;
> +  bookmarkStar.click();
> +
> +  await shownPromise;

The only fear I have about using the star panel in these tests is the fact it's animated, and due to that it can cause intermittent failures, please check it's not the case (--test-verify or manual retriggers). I think the star has an "animate" attribute one can set in tests.
Alternatively, using the Properties dialog on a bookmark would allow to test this more safely.
Attachment #8958148 - Flags: review?(mak77)
Attachment #8958148 - Flags: review?(standard8)
Comment on attachment 8958148 [details]
Bug 1444228 - Remove editBookmarkOverlay.xul.

https://reviewboard.mozilla.org/r/227094/#review233272

> worth investigating why places.css is necessary here. As far as I can tell it may be just for the trees part, since editBookmarkOverlay includes a tree.
> 
> Maybe we should also include the trees part in editBookmarkOverlay.css, or split places.css in placesTrees.css and places.css.

Just looked with the inspector, seems to mostly be used by browser.xul: '#bhtTitleText' and 'menupopup[placespopup="true"]' are active. Also, the 'tree[type="places"]' after the bookmark panel has been opened.

'.toolbar-drop-indicator' doesn't appear to exist anywhere in mozcentral.

> Likely this css should be renamed to just editBookmark.css since it's no more an overlay

K, I've renamed the js file too, but I plan to leave the DTD as that causes pain for the localizers.

> The lazy loading problem is just for the star panel, would we get the same perf benefit just setting/unsetting hidden in the star panel, rather than on this object? It sounds like cleaner, if possible.

I'm not sure about this, mossop mentioned the hidden per benefit may only be for panels, but enn would know for sure.
Comment on attachment 8958148 [details]
Bug 1444228 - Remove editBookmarkOverlay.xul.

https://reviewboard.mozilla.org/r/227094/#review233580

Looks good, r=Standard8 with the nits fixed, and assuming this passes on try.

::: commit-message-493e4:1
(Diff revision 2)
> +Bug 1444228 - Remove editBookmarkOverlay.xul. r?mak

nit: if you change this to r?Standard8, then it doesn't keep pinging Marco.

::: browser/components/places/tests/browser/browser.ini:117
(Diff revision 2)
>  [browser_toolbar_overflow.js]
>  [browser_toolbarbutton_menu_context.js]
>  [browser_views_iconsupdate.js]
>  skip-if = (os == 'win' && ccov) # Bug 1423667
> +[browser_bug485100-change-case-loses-tag.js]
> +[browser_editBookmarkOverlay_tags_liveUpdate.js]

As we're moving these anyway, could we drop the 'Overlay' from their names please?

::: browser/components/places/tests/browser/browser.ini:118
(Diff revision 2)
>  [browser_toolbarbutton_menu_context.js]
>  [browser_views_iconsupdate.js]
>  skip-if = (os == 'win' && ccov) # Bug 1423667
> +[browser_bug485100-change-case-loses-tag.js]
> +[browser_editBookmarkOverlay_tags_liveUpdate.js]
> +skip-if = (os == 'win' && ccov) # Bug 1423667

This skip-if shouldn't be needed now, but verifying on try would be best (xref bug 1445652)

::: browser/components/places/tests/browser/browser.ini:121
(Diff revision 2)
> +[browser_bug485100-change-case-loses-tag.js]
> +[browser_editBookmarkOverlay_tags_liveUpdate.js]
> +skip-if = (os == 'win' && ccov) # Bug 1423667
> +[browser_bug427633_no_newfolder_if_noip.js]
> +[browser_editBookmarkOverlay_keywords.js]
> +skip-if = (os == 'win' && ccov) # Bug 1423667

ditto regarding shouldn't be needed.
Attachment #8958148 - Flags: review+
Attachment #8958148 - Flags: review?(mak77)
Neil,

We were wondering about some of the optimizations for hidden="true" in XUL. Would we still have any perf benefit if we set hidden="true" within JS instead of having it within XUL? I should note this is on <vbox>, mossop had mentioned that some of the optimizations only applied to panels.
Flags: needinfo?(enndeakin)
Priority: -- → P1
The edit bookmarks panel already has hidden="true":

https://searchfox.org/mozilla-central/source/browser/base/content/browser.xul#197
Flags: needinfo?(enndeakin)
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c039062bce14
Remove editBookmarkOverlay.xul. r=standard8
https://hg.mozilla.org/mozilla-central/rev/c039062bce14
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: