Mac async popup #161

bsclifton · 2017-02-19T08:51:32Z

Manual application of electron patch electron/electron#8702
Additional updates (per electron/electron#8702)

Default async to true for popup menus
fix lint errors
1st commit: menus do not destroy themselves
2nd commit: menus destroy themselves

bsclifton · 2017-02-27T09:10:34Z

Right clicking anywhere inside a tab's webcontents causes this:

[66741:775:0227/015159.905320:ERROR:CONSOLE(219)] "Uncaught Error: Could not call remote function ''. Check that the function signature is correct. Underlying error: Error processing argument at index 1, conversion failure from -1
Error: Could not call remote function ''. Check that the function signature is correct. Underlying error: Error processing argument at index 1, conversion failure from -1
    at callFunction (/Users/clifton/Documents/browser-laptop-bootstrap/src/out/Release/Brave.app/Contents/Resources/electron.asar/browser/rpc-server.js:226:11)
    at EventEmitter.<anonymous> (/Users/clifton/Documents/browser-laptop-bootstrap/src/out/Release/Brave.app/Contents/Resources/electron.asar/browser/rpc-server.js:316:5)
    at emitMany (events.js:127:13)
    at EventEmitter.emit (events.js:201:7)
    at WebContents.<anonymous> (/Users/clifton/Documents/browser-laptop-bootstrap/src/out/Release/Brave.app/Contents/Resources/electron.asar/browser/api/web-contents.js:173:37)
    at emitTwo (events.js:106:13)
    at WebContents.emit (events.js:191:7)", source: extensions::remote (219)

kevinsawicki · 2017-02-27T16:50:43Z

[66741:775:0227/015159.905320:ERROR:CONSOLE(219)] "Uncaught Error: Could not call remote function ''. Check that the function signature is correct. Underlying error: Error processing argument at index 1, conversion failure from -1

This might be because you need this native mate change: electron/native-mate#10

bsclifton · 2017-02-28T00:26:25Z

Thanks for the update @kevinsawicki, that definitely got me past that 😄

Seems now though that the menu is closing itself quickly after it opens. Will look at this later tonite

kevinsawicki · 2017-02-28T00:35:03Z

Seems now though that the menu is closing itself quickly after it opens. Will look at this later tonite

Haven't seen this before, this could happen if the native menu object gets destroyed too early, that is why they are kept around in a map of nscoped_object until the menu is closed or closePopupAt is called.

bridiver · 2017-03-25T15:41:35Z

what is the status of this? Still trying to resolve the closing quickly issue?

bsclifton · 2017-03-26T23:21:13Z

@bridiver yes- I haven't tried it recently though. I'm not sure I'll have time to look in this upcoming week

Additional updates (per electron/electron#8702) Fixes brave/browser-laptop#5470 - Default `async` to true for popup menus - fix lint errors - menus do not destroy themselves

bsclifton · 2017-05-24T08:58:43Z

Ready for review! 😄

There are two ways we can attempt to use this:

We can use the PR as-is; it will do the destroy when the popup closes. However, I had to grant friend access so that objects could call Destroy
We can remove the 2nd commit and then make the caller (ex: browser-laptop) explicitly call menuObject.destroy() when it's ready to be destroyed

bridiver · 2017-05-24T13:19:31Z

DEPS

@@ -4,7 +4,7 @@ use_relative_paths = True

 deps = {
  "vendor/node": "https://github.com/brave/node.git@f51b9ab8ff446ca7b13be0de1bc12b854b23938d",
-  "vendor/native_mate": "https://github.com/zcbenz/native-mate.git@b5e5de626c6a57e44c7e6448d8bbaaac475d493c",
+  "vendor/native_mate": "https://github.com/zcbenz/native-mate.git@ad0fd825663932ee3fa29ff935dfec99933bdd8c",


is this intentional?

Yes- it's needed per the comment here:
#161 (comment)

bsclifton · 2017-05-29T17:58:44Z

browser-laptop portion of this change captured with brave/browser-laptop#9128

bsclifton · 2017-06-05T22:43:34Z

cc: @darkdh

bridiver · 2017-06-06T00:41:06Z

would there be any benefit to not destroying them automatically?

darkdh

I wonder why not destroying them in OnMenuClosed?
Just like #205 without window id.
If you intentionally make MenuController::Cancel return at first line, you can see the context menu is not window specific but a desktop global on Aura.

darkdh · 2017-06-06T16:10:17Z

atom/browser/api/atom_api_menu_views.cc

@@ -38,21 +43,32 @@ void MenuViews::PopupAt(Window* window, int x, int y, int positioning_item) {
    location = gfx::Point(origin.x() + x, origin.y() + y);
  }

+  int flags = MenuRunner::CONTEXT_MENU |
+              MenuRunner::HAS_MNEMONICS |
+              MenuRunner::ASYNC;


ASYNC flag is redundant since 59
see https://chromium.googlesource.com/chromium/src.git/+/5ab7c2d09320aa6f4fc6ba0118908ac7e378e77b

bridiver · 2017-06-06T20:28:13Z

brave/common/extensions/url_bindings.cc

@@ -330,7 +330,8 @@ void URLBindings::Parse(
  GURL gurl(url_string);
  gin::Dictionary dict = gin::Dictionary::CreateEmpty(isolate);
  if (gurl.has_username())
-    dict.Set("auth", gurl.username() + (gurl.has_password() ? ":" + gurl.password() : ""));
+    dict.Set("auth", gurl.username() + (gurl.has_password()


just saw this now- this just moves part of it onto the next line (since it went over 80 chars length, lint failed)

@bridiver

…n Muon with brave/muon#161 Auditors: @bridiver, @bbondy

@bridiver

…n Muon with brave/muon#161 Auditors: @bridiver, @bbondy

@bridiver

…n Muon with brave/muon#161 Auditors: @bridiver, @bbondy

bridiver force-pushed the master branch from 0491e17 to bd0b1f9 Compare February 21, 2017 16:11

kevinsawicki mentioned this pull request Feb 22, 2017

Add async menu.popup option electron/electron#8702

Merged

7 tasks

bsclifton force-pushed the mac-async-popup branch from 2cae00c to 20f6b43 Compare February 23, 2017 22:59

bridiver force-pushed the master branch from d8b3e75 to 349c547 Compare February 25, 2017 01:34

bsclifton force-pushed the mac-async-popup branch from 20f6b43 to 594b268 Compare February 28, 2017 00:25

bsclifton added a commit that referenced this pull request Feb 28, 2017

Updated per #161 (comment)

594b268

bsclifton mentioned this pull request Apr 19, 2017

Make new tab context menu native brave/browser-laptop#8397

Merged

4 tasks

bridiver force-pushed the master branch from e6a5892 to 9a475be Compare April 29, 2017 03:21

This was referenced May 2, 2017

Convert React ContextMenu items to native context menu brave/browser-laptop#8641

Closed

Opening a context menu freezes video (ex: Youtube) brave/browser-laptop#5470

Closed

bsclifton force-pushed the mac-async-popup branch from 594b268 to 9bf6809 Compare May 9, 2017 18:10

bsclifton added a commit that referenced this pull request May 9, 2017

Updated per #161 (comment)

9bf6809

bsclifton force-pushed the mac-async-popup branch from 3517a7f to faa2b45 Compare May 24, 2017 08:23

bsclifton added a commit that referenced this pull request May 24, 2017

Updated per #161 (comment)

2eb1f4b

bsclifton added 2 commits May 24, 2017 01:51

Manual application of electron patch electron/electron#8702

d45bf9f

Additional updates (per electron/electron#8702) Fixes brave/browser-laptop#5470 - Default `async` to true for popup menus - fix lint errors - menus do not destroy themselves

menus destroy themselves when closed

2436b71

bsclifton force-pushed the mac-async-popup branch from 1151052 to 2436b71 Compare May 24, 2017 08:52

bsclifton changed the title ~~(NEEDS TESTING) Mac async popup~~ Mac async popup May 24, 2017

bsclifton requested review from bridiver, bbondy and darkdh May 24, 2017 08:54

bridiver reviewed May 24, 2017

View reviewed changes

bsclifton mentioned this pull request May 29, 2017

Remove destroy from popup menus brave/browser-laptop#9128

Merged

7 tasks

darkdh reviewed Jun 6, 2017

View reviewed changes

darkdh self-assigned this Jun 6, 2017

darkdh mentioned this pull request Jun 6, 2017

Address change of removing nested message loop #205

Closed

bridiver reviewed Jun 6, 2017

View reviewed changes

bridiver merged commit 6f1e346 into master Jun 6, 2017

bridiver deleted the mac-async-popup branch June 6, 2017 21:01

darkdh pushed a commit to brave/browser-laptop that referenced this pull request Jun 6, 2017

Remove destroy from popup menus; it now happens when menu is hidden i…

5f97976

…n Muon with brave/muon#161 Auditors: @bridiver, @bbondy

darkdh pushed a commit to brave/browser-laptop that referenced this pull request Jun 6, 2017

Remove destroy from popup menus; it now happens when menu is hidden i…

7eb2f28

…n Muon with brave/muon#161 Auditors: @bridiver, @bbondy

bsclifton added a commit to brave/browser-laptop that referenced this pull request Jun 7, 2017

Remove destroy from popup menus; it now happens when menu is hidden i…

db2027a

…n Muon with brave/muon#161 Auditors: @bridiver, @bbondy

This was referenced Jun 11, 2017

Right click context menu appears twice brave/browser-laptop#8872

Closed

clicking menu bar items in 0.16.x and 0.17.1 does nothing brave/browser-laptop#9470

Closed

darkdh mentioned this pull request Jul 6, 2017

Chromium60.0.3112.50 #237

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mac async popup #161

Mac async popup #161

bsclifton commented Feb 19, 2017 •

edited

Loading

bsclifton commented Feb 27, 2017

kevinsawicki commented Feb 27, 2017

bsclifton commented Feb 28, 2017 •

edited

Loading

kevinsawicki commented Feb 28, 2017

bridiver commented Mar 25, 2017

bsclifton commented Mar 26, 2017

bsclifton commented May 24, 2017

bridiver May 24, 2017

bsclifton May 29, 2017

bsclifton commented May 29, 2017

bsclifton commented Jun 5, 2017

bridiver commented Jun 6, 2017

darkdh left a comment •

edited

Loading

darkdh Jun 6, 2017

bridiver Jun 6, 2017

bsclifton Jun 15, 2017

Mac async popup #161

Mac async popup #161

Conversation

bsclifton commented Feb 19, 2017 • edited Loading

bsclifton commented Feb 27, 2017

kevinsawicki commented Feb 27, 2017

bsclifton commented Feb 28, 2017 • edited Loading

kevinsawicki commented Feb 28, 2017

bridiver commented Mar 25, 2017

bsclifton commented Mar 26, 2017

bsclifton commented May 24, 2017

bridiver May 24, 2017

Choose a reason for hiding this comment

bsclifton May 29, 2017

Choose a reason for hiding this comment

bsclifton commented May 29, 2017

bsclifton commented Jun 5, 2017

bridiver commented Jun 6, 2017

darkdh left a comment • edited Loading

Choose a reason for hiding this comment

darkdh Jun 6, 2017

Choose a reason for hiding this comment

bridiver Jun 6, 2017

Choose a reason for hiding this comment

bsclifton Jun 15, 2017

Choose a reason for hiding this comment

bsclifton commented Feb 19, 2017 •

edited

Loading

bsclifton commented Feb 28, 2017 •

edited

Loading

darkdh left a comment •

edited

Loading