Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Remove destroy from popup menus #9128

Merged
merged 1 commit into from
Jun 6, 2017
Merged

Remove destroy from popup menus #9128

merged 1 commit into from
Jun 6, 2017

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented May 29, 2017

Remove destroy from popup menus; it now happens when menu is hidden in Muon with brave/muon#161

Auditors: @bridiver, @bbondy

DON'T MERGE UNTIL brave/muon#161 IS MERGED

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

#5470

@bsclifton bsclifton requested review from bridiver and bbondy May 29, 2017 17:58
@bsclifton bsclifton self-assigned this May 29, 2017
@bsclifton bsclifton modified the milestones: 0.16.100 (Frozen, only critical adds from here), 0.16.200 May 29, 2017
@bsclifton bsclifton requested a review from darkdh June 5, 2017 22:43
darkdh added a commit to brave/muon that referenced this pull request Jun 6, 2017
```
All menus on aura platforms no longer use the nested message loop
code path of MenuController.

On Mac non-native menus also no longer use the nested message
loop. While native menus are handled vis menu_runner_impl_cocoa,
which does block.

This change removes the Nested Message Loop code paths from
MenuController.

Additionally some tests which specifically were for
nested message loops below asynchronous menus, have been
either transitioned, or removed if redundant.
```
https://chromium.googlesource.com/chromium/src.git/+/b31c887a2d14761b078f329629f1d3469106b891%5E%21/

It also forced menu to be async so we must make sure menu runner is alive until
menu closed.

requires brave/browser-laptop#9128

fix brave/browser-laptop#9100

Auditors: @bsclifton, @bridiver, @bbondy
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

depends on
brave/muon#205

@darkdh
Copy link
Member

darkdh commented Jun 6, 2017

moved to 0.16.x due to required by CR59 context menu regression

darkdh added a commit to brave/muon that referenced this pull request Jun 6, 2017
```
All menus on aura platforms no longer use the nested message loop
code path of MenuController.

On Mac non-native menus also no longer use the nested message
loop. While native menus are handled vis menu_runner_impl_cocoa,
which does block.

This change removes the Nested Message Loop code paths from
MenuController.

Additionally some tests which specifically were for
nested message loops below asynchronous menus, have been
either transitioned, or removed if redundant.
```
https://chromium.googlesource.com/chromium/src.git/+/b31c887a2d14761b078f329629f1d3469106b891%5E%21/

It also forced menu to be async so we must make sure menu runner is alive until
menu closed.

requires brave/browser-laptop#9128

fix brave/browser-laptop#9100

Auditors: @bsclifton, @bridiver, @bbondy
@darkdh
Copy link
Member

darkdh commented Jun 6, 2017

fix #9100

@luixxiul
Copy link
Contributor

luixxiul commented Jun 7, 2017

Removed the milestone as this PR was reverted. Let's add that for another PR which will be opened later.

@luixxiul
Copy link
Contributor

luixxiul commented Jun 7, 2017

Also don't we need an issue to track?

@luixxiul luixxiul added the needs-info Another team member needs information from the PR/issue opener. label Jun 7, 2017
@bridiver
Copy link
Collaborator

bridiver commented Jun 7, 2017

it was only reverted because I accidentally merged too soon

@luixxiul luixxiul removed the needs-info Another team member needs information from the PR/issue opener. label Jun 7, 2017
@darkdh
Copy link
Member

darkdh commented Jun 7, 2017

it is merged here 7eb2f28
in muon-4.0.x branch

@bsclifton bsclifton deleted the async-popup-browser-laptop branch June 7, 2017 06:34
@bsclifton
Copy link
Member Author

bsclifton commented Jun 7, 2017

@luixxiul the main issue is tracked here: #5470 (which was closed when Muon was accepted; this is the other important commit)

@bsclifton bsclifton added this to the 0.16.13 milestone Jul 27, 2017
@bsclifton
Copy link
Member Author

bsclifton commented Jul 27, 2017

Manually re-added with db2027a; I updated milestone appropriately (since it was deployed with 0.16.13)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants