1719463 - Remove non-proton hamburger menu view and dependent code

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, task, P3)

Description

[:Gijs (he/him)]

After bug 1711508 we will never show the old hamburger view. We should remove the old markup and all the referencing CSS / images, insofar as that isn't already done.

Comment 1

[Sam Foster [:sfoster] (he/him)]

OK I've produced myself a long list of code, strings, CSS etc. to remove. But there are some references to some of these elements in e.g. UITour.jsm, and mozscreenshots that are presumably already broken or disabled or orphaned -what to do with those?

For example:

• appMenu-whatsnew-button
• appMenu-developer-button

I'll probably do this in 2 patches: first do the code removal, 2nd rename things from xxx2 back to xxx, or xxx-proton to xxx where it makes sense.

Comment 2

[:Gijs (he/him)]

(In reply to Sam Foster [:sfoster] (he/him) from comment #1)

OK I've produced myself a long list of code, strings, CSS etc. to remove. But there are some references to some of these elements in e.g. UITour.jsm, and mozscreenshots that are presumably already broken or disabled or orphaned -what to do with those?

(FWIW, I don't quickly see anything referencing mozscreenshots, but I definitely haven't looked as exhaustively as you!)

For example:

• appMenu-whatsnew-button

It's probably worth asking the new tab folks what's meant to happen here; there is no equivalent in the new hamburger menu, so I assume both the test and non-test code referring to this element need to go.

• appMenu-developer-button

This is clearer to me, we can "just" remove it.

I'll probably do this in 2 patches: first do the code removal, 2nd rename things from xxx2 back to xxx, or xxx-proton to xxx where it makes sense.

Yep, this makes sense to me; it may even be worth splitting the first patch up more, removing a few things at a time, to keep patch size down and make it more obvious what removals of markup imply what removals of CSS etc. Up to you.

Comment 3

[Sam Foster [:sfoster] (he/him)]

Hey jdescottes,
Part of the patch to remove the old (pre-proton) hamburger menu will remove the panelview#appmenu-moreView and all its contents, including the #appMenu-developer-button toolbarbutton. These have been replaced by the new vbox#appmenu-developer-tools-view at: https://searchfox.org/mozilla-central/source/browser/base/content/appmenu-viewcache.inc.xhtml#605-610

I'm trying to clean up all the references to the old elements I'm removing . There is code in https://searchfox.org/mozilla-central/source/devtools/startup/DevToolsStartup.jsm#488 which seems to still reference this old button. However, when I test in a new profile with the devtools.policy.disabled pref on, we already seem to do the right thing with the proton More tools menu. Is this removeDevToolsMenus method actually still in use?

Comment 4

[Julian Descottes [:jdescottes]]

Considering that proton is always on now, I think we can drop the removeDevToolsMenus method as part of this cleanup. I think it was only relevant when proton was off. DevTools items are now always relying on the More Tools panel view.

The only thing that should be impacted by this is that when devtools.policy.disabled is true, users will still see the Tools > Browser Tools menu with 2 items, Task Manager & Page Source. But I imagine this is rather an improvement than anything else.

Comment 5

[Sam Foster [:sfoster] (he/him)]

(In reply to Julian Descottes [:jdescottes] from comment #4)

Considering that proton is always on now, I think we can drop the removeDevToolsMenus method as part of this cleanup. I think it was only relevant when proton was off. DevTools items are now always relying on the More Tools panel view.

The only thing that should be impacted by this is that when devtools.policy.disabled is true, users will still see the Tools > Browser Tools menu with 2 items, Task Manager & Page Source. But I imagine this is rather an improvement than anything else.

Yeah that looks good to me, thanks. I'll flag you for review on this part.

Comment 6

[Sam Foster [:sfoster] (he/him)]

:mardak, I'm in the process of removing the old pre-proton app menu (hamburger menu. ) The "What's new" button got removed from that menu but its still referenced as APPMENU_BUTTON_ID in ToolbarPanelHub.jsm, where it seems to be possible to add and remove this feature from the menu somehow via about:newtab?. Presumably this is either broken or not in use at the moment, as the proton app menu is the default.

We do still have the panelview#PanelUI-whatsNew itself which this button used to open, and a toolbarbutton#whats-new-menu-button which is present but hidden by default in the new app menu. Should I just be updating the id referenced here?

Comment 7

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1719463 - Part 1: Remove the update banner items from the pre-proton appMenu view. r?Gijs

Comment 8

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1719463 - Part 2: Remove the Fxa items from the pre-proton appMenu view. r?Gijs

Depends on D121801

Comment 9

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1719463 - Part 3: Remove the Protections Report items from the pre-proton appMenu view. r?Gijs

Depends on D121802

Comment 10

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1719463 - Part 4: Remove the new window and session restore items from the pre-proton appMenu view. r?Gijs

Depends on D121803

Comment 11

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1719463 - Part 5: Remove the edit and zoom items from the pre-proton appMenu view. r?Gijs

Depends on D121804

Comment 12

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1719463 - Part 6: Remove the Library item from the pre-proton appMenu view. r?Gijs

Depends on D121805

Comment 13

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1719463 - Part 7: Remove the Logins, Addons, Preferences and Print buttons from the pre-proton appMenu view. r?Gijs

Depends on D121806

Comment 14

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1719463 - Part 8: Remove the File open, File save and Find buttons from the pre-proton appMenu view. r?Gijs

Depends on D121807

Comment 15

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1719463 - Part 9: Remove the More items button and its sub-view from the pre-proton appMenu view. r?Gijs

Depends on D121808

Comment 16

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1719463 - Part 10: Remove the devtools button from the pre-proton appMenu view. r?jdescottes,Gijs

Depends on D121809

Comment 17

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1719463 - Part 11: Remove the Help button from the pre-proton appMenu view. r?Gijs

Depends on D121810

Comment 18

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1719463 - Part 12: Remove the Quit button(s) from the pre-proton appMenu view. r?Gijs

Depends on D121811

Comment 19

[Sam Foster [:sfoster] (he/him)]

:Gijs, I'm pushing a whole stack of patches up for review. At this point they remove all but the What's new button and the panelview#appMenu-mainView itself. Its a lot, but also there's no complicated dependencies between the patches, and each is hopefully pretty trivial. I started looking for appropriate reviewers for each, but, surprise! you turn out to be a reasonable choice for all. However, if I can help split up the load by redirecting some please let me know.

Comment 20

[Sam Foster [:sfoster] (he/him)]

I guess no info needed, just wanted to make sure you saw this before you saw your review queue :)

Comment 21

[Sam Foster [:sfoster] (he/him)]

All the patches so far have r+, but as we're right on top of soft-freeze for 92, I'll hold off landing until after merge day to avoid unnecessary churn and conflicts.
I'll either add a patch for the what's new stuff or break that and any remaining work out into its own bug based on when those open questions get resolved.

Comment 22

[Ed Lee :Mardak]

andreio, see comment 6. I see there was some cleanup in bug 1708672, but we seem to intentionally(?) keep the code to show whats new panel, so I guess we should have the menu id updated? Or is there a separate bug to remove the whats new panel functionality?

https://searchfox.org/mozilla-central/search?q=APPMENU_BUTTON_ID

Comment 23

[Andrei Oprea [:andreio]]

Initially thought the what's new panel will still get some usage but now we know it should be removed. I've created bug 1724300 and I'll be working on this. I'm ok with any changes to the WNPanel code/tests to unblock this current patchlist.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8f75ffd35af
Part 1: Remove the update banner items from the pre-proton appMenu view. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/9a8785d39c6e
Part 2: Remove the Fxa items from the pre-proton appMenu view. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/ae0160a193ce
Part 3: Remove the Protections Report items from the pre-proton appMenu view. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/d9207378c73e
Part 4: Remove the new window and session restore items from the pre-proton appMenu view. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/6da90a5511f5
Part 5: Remove the edit and zoom items from the pre-proton appMenu view. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/c6c6ad8494fc
Part 6: Remove the Library item from the pre-proton appMenu view. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/e44097a4bb3e
Part 7: Remove the Logins, Addons, Preferences and Print buttons from the pre-proton appMenu view. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/6d7ae8d68ba9
Part 8: Remove the File open, File save and Find buttons from the pre-proton appMenu view. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/754b78618402
Part 9: Remove the More items button and its sub-view from the pre-proton appMenu view. r=Gijs,flod
https://hg.mozilla.org/integration/autoland/rev/e66854995469
Part 10: Remove the devtools button from the pre-proton appMenu view. r=jdescottes,Gijs,mkaply
https://hg.mozilla.org/integration/autoland/rev/539a94aefc53
Part 11: Remove the Help button from the pre-proton appMenu view. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/a831452272dd
Part 12: Remove the Quit button(s) from the pre-proton appMenu view. r=Gijs

Comment 25

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b8f75ffd35af
https://hg.mozilla.org/mozilla-central/rev/9a8785d39c6e
https://hg.mozilla.org/mozilla-central/rev/ae0160a193ce
https://hg.mozilla.org/mozilla-central/rev/d9207378c73e
https://hg.mozilla.org/mozilla-central/rev/6da90a5511f5
https://hg.mozilla.org/mozilla-central/rev/c6c6ad8494fc
https://hg.mozilla.org/mozilla-central/rev/e44097a4bb3e
https://hg.mozilla.org/mozilla-central/rev/6d7ae8d68ba9
https://hg.mozilla.org/mozilla-central/rev/754b78618402
https://hg.mozilla.org/mozilla-central/rev/e66854995469
https://hg.mozilla.org/mozilla-central/rev/539a94aefc53
https://hg.mozilla.org/mozilla-central/rev/a831452272dd

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.