1356587 - Updating commands takes a lot of CPU time

Status: RESOLVED FIXED

(Firefox :: Menus, enhancement, P1)

Description

[Florian Quèze [:florian]]

This profile (I was just clicking alternatively in the searchbar and urlbar to move focus around) shows we are spending a lot of time updating commands: https://perfht.ml/2of9x6x

From looking at the code history, it seems bug 404232 attempted to fix this, but it doesn't seem to work; somehow gEditUIVisible must not be getting set to false.

I think we should verify that this works (if it doesn't fix it), and add test coverage for it.

Comment 1

[Neil Deakin]

I investigated this recently as part of 1354564 so I'll look into it.

Comment 2

[Neil Deakin]

Attachment: Update commands once per focus change

Currently a command update can occur two or more times during a focus change, one after the blur and one after the focus. This patch queues up any command updates and skips any redundant ones.

I may also change this patch to use the locking added in bug 1354564, but I want to think about that some more.

I still need to update test_focus.xul to accommodate the changes.

Comment 3

[Neil Deakin]

It looks like test to check if the clipboard buttons are on the toolbar doesn't check if they are in the menu instead. We can update the test to check if the menu is open, and remember that any upcoming redesign should maintain this.

I'll add another check for this.

Comment 4

[Neil Deakin]

Attachment: Update gEditUIVisible state when panelUI opens and closes

Note that a found bug 1359790 while writing the test for this.

Comment 5

[Neil Deakin]

Comment on attachment 8859992 [details] [diff] [review]
Update commands once per focus change

I want to investigate this other patch a bit more and will use a different bug for this patch.

I'll leave this bug just for the edit controls optimization fix.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8861927 [details] [diff] [review]
Update gEditUIVisible state when panelUI opens and closes

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

::: browser/base/content/browser.js
@@ +4287,5 @@
>                     editMenuPopupState == "open" ||
>                     contextMenuPopupState == "showing" ||
>                     contextMenuPopupState == "open" ||
>                     placesContextMenuPopupState == "showing" ||
> +                   placesContextMenuPopupState == "open"

Missing semicolon

@@ +4291,5 @@
> +                   placesContextMenuPopupState == "open"
> +  if (!gEditUIVisible) {
> +    let editControlPlacement = CustomizableUI.getPlacementOfWidget("edit-controls");
> +    if (editControlPlacement) {
> +      if (editControlPlacement.area == CustomizableUI.AREA_PANEL) {

This check is going to be wrong soon, as we're changing the panel where customizable items live. Please use CUI.getAreaType and check that the type of the area is a panel.

@@ +4298,5 @@
> +          gEditUIVisible = true;
> +        }
> +      } else {
> +        // The edit controls are on a toolbar, so they are visible.
> +        gEditUIVisible = true;

This isn't technically necessarily true if they are in the overflow menu, but I don't know that we should care about this here.

::: browser/components/customizableui/content/panelUI.js
@@ +286,5 @@
>      }
>      switch (aEvent.type) {
>        case "popupshowing":
>          this._adjustLabelsForAutoHyphens();
> +        updateEditUIVisibility();

This looks wrong. Why do we need to do this whenever the menupanel shows up, even if the buttons aren't in the panel? What was wrong with having the check in the .show() code where it was before?

@@ +292,5 @@
>        case "popupshown":
>          // Fall through
>        case "popuphiding":
> +        if (aEvent.type == "popuphiding") {
> +          updateEditUIVisibility();

This needs a comment - why do we need to run this when the popup is hiding?

::: browser/components/customizableui/test/browser.ini
@@ +154,5 @@
>  [browser_panelUINotifications.js]
>  [browser_switch_to_customize_mode.js]
>  [browser_synced_tabs_menu.js]
>  [browser_check_tooltips_in_navbar.js]
> +[browser_editcontrols_update.js]

This needs to be in the clipboard set for automation, I expect?

::: browser/components/customizableui/test/browser_editcontrols_update.js
@@ +3,5 @@
> +let isMac = navigator.platform.indexOf("Mac") == 0;
> +
> +function checkState(updateExpected, allowCut, desc) {
> +  is(askedForCommandState, updateExpected, desc);
> +  is(document.getElementById("cmd_cut").getAttribute("disabled") == "true", !allowCut, desc + " - cut");

Isn't there a disabled property we can use to make this easier to read?

@@ +9,5 @@
> +
> +  askedForCommandState = false;
> +}
> +
> +add_task(function*() {

Several named subtests and some helpers for the adding/removing of the override controller would make this test a lot easier to read. Please also add docstring comments about what the individual portions of the test are testing.

@@ +30,5 @@
> +  gURLBar.focus();
> +  gURLBar.value = "test";
> +
> +  // Note that the default value of the cut command is true on non-Mac.
> +  // Mac always updates commands even when the panels are not open.

Why? I don't see anything about this in the context of the code changes.

@@ +35,5 @@
> +  checkState(false, true, "No update when edit-controls is on panel and not visible");
> +
> +  let shownPromise = promisePanelShown(window);
> +  PanelUI.show();
> +  yield shownPromise;

Just yield PanelUI.show();

@@ +38,5 @@
> +  PanelUI.show();
> +  yield shownPromise;
> +
> +  // Wait for the things that happen during panel shown to finish.
> +  yield new Promise(r => { SimpleTest.executeSoon(r); });

This looks really hacky, and the comment doesn't help explain. What things that happen?

@@ +48,5 @@
> +
> +  let hiddenPromise = promisePanelHidden(window);
> +  PanelUI.hide();
> +  yield hiddenPromise;
> +  yield new Promise(r => { SimpleTest.executeSoon(r); });

Not sure why we need this.

@@ +83,5 @@
> +  let palette = document.getElementById("customization-palette");
> +  simulateItemDrag(editControls, palette);
> +  yield endCustomizing();
> +
> +  // updateEditUIVisibility should be called when customization ends but isn't. See bug 1359790.

Why (should it be called), especially if the controls are now in the palette?

@@ +86,5 @@
> +
> +  // updateEditUIVisibility should be called when customization ends but isn't. See bug 1359790.
> +  updateEditUIVisibility();
> +
> +  goSetCommandEnabled("cmd_delete", true);

Why do we need to do this?

Comment 7

[Neil Deakin]

Attachment: Update gEditUIVisible state when panelUI opens and closes, v2

Comment 8

[Neil Deakin]

::: browser/components/customizableui/content/panelUI.js
>        case "popupshowing":
>          this._adjustLabelsForAutoHyphens();
> +        updateEditUIVisibility();

> This looks wrong. Why do we need to do this whenever the menupanel shows up, even if the buttons aren't in the panel? What was wrong with having the check in the .show() code where it was before?

It could be left as is but then updateEditUIVisibility would need to have code to handle this case where the panel is not yet open.


@@ +292,5 @@
>        case "popupshown":
>          // Fall through
>        case "popuphiding":
> +        if (aEvent.type == "popuphiding") {
> +          updateEditUIVisibility();

> This needs a comment - why do we need to run this when the popup is hiding?

Because the edit controls on the panel are no longer visible so gEditUIVisible should be made false. This is the actual bug being fixed.


> Isn't there a disabled property we can use to make this easier to read?

No.


> Several named subtests and some helpers for the adding/removing of the override controller would make this test a lot easier to read. Please also add docstring comments about what the individual portions of the test are testing.


I reworked the test quite a bit into separate tasks and to better listen for updates.


> +  let palette = document.getElementById("customization-palette");
> +  simulateItemDrag(editControls, palette);
> +  yield endCustomizing();
> +
> +  // updateEditUIVisibility should be called when customization ends but isn't. See bug 1359790.

Why (should it be called), especially if the controls are now in the palette?

As before, to update gEditUIVisible now that the edit controls are no longer visible.


> +  goSetCommandEnabled("cmd_delete", true);

>  Why do we need to do this?

I think this is leftover. I have removed it.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8862439 [details] [diff] [review]
Update gEditUIVisible state when panelUI opens and closes, v2

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

::: browser/components/customizableui/test/browser_editcontrols_update.js
@@ +52,5 @@
> +  });
> +});
> +
> +// Test updating in the initial state when the edit-controls are on the panel but
> +// have not yet been created.

Especially in this directory, there's no guarantee that this is the case unless you open a new window to run this test in. That is, in practice when run inside the directory, the edit controls will have been created in the main window by the time this code runs.

@@ +70,5 @@
> +
> +// Test updating when the panel is open with the edit-controls on the panel.
> +// Updates should occur.
> +add_task(function* test_panelui_opened() {
> +  // Two updates occur while opening the panel; wait for the second one.

Is this a (followup) bug? Why do we update twice?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56b70ce1a2ab
update the gEditUIVisible flag when the panelUI is opened and closed so that the command updating optimization actually applies. Currently it never applies once the panel has been opened at least once, r=gijs

Comment 11

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/56b70ce1a2ab