Closed Bug 1719463 Opened 3 months ago Closed 2 months ago

Remove non-proton hamburger menu view and dependent code

Categories

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

Desktop
All
task
Points:
2

Tracking

()

RESOLVED FIXED
93 Branch
Tracking Status
firefox91 --- wontfix
firefox92 --- wontfix
firefox93 --- fixed

People

(Reporter: Gijs, Assigned: sfoster)

References

(Depends on 1 open bug)

Details

(Whiteboard: [proton-cleanups])

Attachments

(12 files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

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.

Severity: -- → N/A
Priority: -- → P3
Assignee: nobody → sfoster
Status: NEW → ASSIGNED

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:

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.

(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:

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.

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.

Points: --- → 2

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?

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Flags: needinfo?(jdescottes)
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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.

Flags: needinfo?(jdescottes)

(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.

: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?

Flags: needinfo?(edilee)

: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.

Flags: needinfo?(gijskruitbosch+bugs)

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

Flags: needinfo?(gijskruitbosch+bugs)

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.

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

Flags: needinfo?(edilee) → needinfo?(andrei.br92)

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.

Flags: needinfo?(andrei.br92)
See Also: → 1625478
Depends on: 1724854
Pushed by sfoster@mozilla.com:
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

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

You need to log in before you can comment on or make changes to this bug.