Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2416313002:
Prepare chrome_main_app_mode_mac.mm for 10.9 (Closed)
DescriptionPrepare chrome_main_app_mode_mac.mm for 10.9
- Remove GetProcessForPID()
- Ping with the bundle identifier instead of the psn
BUG=652563
Committed: https://crrev.com/18e4d08f3ea5d85cce6676cd4dc6e893b7d7824c
Cr-Commit-Position: refs/heads/master@{#428741}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Added NSAppleEventDescriptor to the configuration #Patch Set 3 : Ping with bundle identifier #Patch Set 4 : Cleaned up #
Total comments: 12
Patch Set 5 : fix for tapted #Patch Set 6 : nit #
Total comments: 2
Patch Set 7 : Removed NSWorkspaceLaunchConfigurationAppleEvent #Patch Set 8 : Cleaned up #Patch Set 9 : nit #Messages
Total messages: 29 (14 generated)
spqchan@chromium.org changed reviewers: + tapted@chromium.org
Hey Trent, I’ve been trying to apply Erik’s CL, but I’m not familiar enough with this to know how to test this Erik mentioned that I should ping you for an app that I can install via chrome extensions. Do you have anything that I could use? Thanks!
On 2016/10/14 18:12:57, spqchan wrote: > Hey Trent, > > I’ve been trying to apply Erik’s CL, but I’m not familiar enough with this to > know how to test this > Erik mentioned that I should ping you for an app that I can install via chrome > extensions. > Do you have anything that I could use? > > Thanks! Something like google keep is a good test. e.g. Installing https://chrome.google.com/webstore/detail/google-keep-notes-and-lis/hmjkmjkep... should create a shortcut in ~/Applications (note: not /Applications). One big thing: the app shims don't work with is_component_build = true -- that needs to be set to `false` otherwise things will fail for other reasons. There is also an interactive_ui_test -- AppShimInteractiveTest.Launch -- it will be a disabled test in component builds, but it should run on the trybots. However, I can't guarantee that it has a check specifically for this case. I think if the ping reply fails the shim will still launch in the test process, but the test process can't interrogate the shim process it launched to ask whether it's still waiting for a ping reply [when that times out, the shim process will die with an error, but the test also can't wait for that :/] https://codereview.chromium.org/2416313002/diff/1/chrome/app_shim/chrome_main... File chrome/app_shim/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/2416313002/diff/1/chrome/app_shim/chrome_main... chrome/app_shim/chrome_main_app_mode_mac.mm:662: // NSAppleEventDescriptor* descriptor = [handler appleEventToSendToProcess]; I think the rest looks good - we just need to pass this in to OpenApplicationWithPath and include it on configuration[NSWorkspaceLaunchConfigurationAppleEvent]
Patchset #2 (id:20001) has been deleted
On 2016/10/17 04:44:07, tapted wrote: > On 2016/10/14 18:12:57, spqchan wrote: > > Hey Trent, > > > > I’ve been trying to apply Erik’s CL, but I’m not familiar enough with this to > > know how to test this > > Erik mentioned that I should ping you for an app that I can install via chrome > > extensions. > > Do you have anything that I could use? > > > > Thanks! > > Something like google keep is a good test. e.g. Installing > https://chrome.google.com/webstore/detail/google-keep-notes-and-lis/hmjkmjkep... > should create a shortcut in ~/Applications (note: not /Applications). One big > thing: the app shims don't work with is_component_build = true -- that needs to > be set to `false` otherwise things will fail for other reasons. > > There is also an interactive_ui_test -- AppShimInteractiveTest.Launch -- it will > be a disabled test in component builds, but it should run on the trybots. > However, I can't guarantee that it has a check specifically for this case. I > think if the ping reply fails the shim will still launch in the test process, > but the test process can't interrogate the shim process it launched to ask > whether it's still waiting for a ping reply [when that times out, the shim > process will die with an error, but the test also can't wait for that :/] > > https://codereview.chromium.org/2416313002/diff/1/chrome/app_shim/chrome_main... > File chrome/app_shim/chrome_main_app_mode_mac.mm (right): > > https://codereview.chromium.org/2416313002/diff/1/chrome/app_shim/chrome_main... > chrome/app_shim/chrome_main_app_mode_mac.mm:662: // NSAppleEventDescriptor* > descriptor = [handler appleEventToSendToProcess]; > I think the rest looks good - we just need to pass this in to > OpenApplicationWithPath and include it on > configuration[NSWorkspaceLaunchConfigurationAppleEvent] Thanks for the information! Current update: I added the NSAppleEventDescriptor. This solution mostly works, however the App Launcher will stop working if you quit Chromium and run it again. I'm currently looking for a solution for this case.
Description was changed from ========== (WIP) Prepare chrome_main_app_mode_mac.mm for 10.9 deployment target BUG=652563 ========== to ========== (WIP) Prepare chrome_main_app_mode_mac.mm for 10.9 deployment target Ping using the bundle identifier instead of the psn BUG=652563 ==========
Description was changed from ========== (WIP) Prepare chrome_main_app_mode_mac.mm for 10.9 deployment target Ping using the bundle identifier instead of the psn BUG=652563 ========== to ========== (WIP) Prepare chrome_main_app_mode_mac.mm for 10.9 deployment target Ping with the bundle identifier instead of the psn BUG=652563 ==========
Description was changed from ========== (WIP) Prepare chrome_main_app_mode_mac.mm for 10.9 deployment target Ping with the bundle identifier instead of the psn BUG=652563 ========== to ========== Prepare chrome_main_app_mode_mac.mm for 10.9 deployment target - Remove GetProcessForPID() - Ping with the bundle identifier instead of the psn BUG=652563 ==========
Description was changed from ========== Prepare chrome_main_app_mode_mac.mm for 10.9 deployment target - Remove GetProcessForPID() - Ping with the bundle identifier instead of the psn BUG=652563 ========== to ========== Prepare chrome_main_app_mode_mac.mm for 10.9 - Remove GetProcessForPID() - Ping with the bundle identifier instead of the psn BUG=652563 ==========
PTAL
https://codereview.chromium.org/2416313002/diff/80001/base/mac/launch_service... File base/mac/launch_services_util.mm (right): https://codereview.chromium.org/2416313002/diff/80001/base/mac/launch_service... base/mac/launch_services_util.mm:8: #include "base/mac/scoped_nsobject.h" nit: import https://codereview.chromium.org/2416313002/diff/80001/base/mac/launch_service... base/mac/launch_services_util.mm:36: [configuration setObject:launch_args optional: we can use the shiny new ObjC syntax for dictionaries now.. except I'm not sure if scoped_nsobject's conversion to an object pointer will kick in. I.e. configuration[NSWorkspaceLaunchConfigurationArguments] = launch_args; should work. But maybe only with NSMutableDictionary* configuration = [NSMutableDictionary dictionary]; above (which is also fine). https://codereview.chromium.org/2416313002/diff/80001/chrome/app_shim/chrome_... File chrome/app_shim/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/2416313002/diff/80001/chrome/app_shim/chrome_... chrome/app_shim/chrome_main_app_mode_mac.mm:545: NSString* chrome_bundle_id = [base::mac::OuterBundle() bundleIdentifier]; nit: I guess this should be `chromeBundleId ` since we're in @implementation (but initial_event below should be changed too). The file looks consistent apart from that. https://codereview.chromium.org/2416313002/diff/80001/chrome/app_shim/chrome_... chrome/app_shim/chrome_main_app_mode_mac.mm:575: forEventClass:'aevt' kCoreEventClass ? https://codereview.chromium.org/2416313002/diff/80001/chrome/app_shim/chrome_... chrome/app_shim/chrome_main_app_mode_mac.mm:576: andEventID:'ansr']; kAEAnswer I guess we should update the literals in closeWithSuccess as well :/ https://codereview.chromium.org/2416313002/diff/80001/chrome/app_shim/chrome_... chrome/app_shim/chrome_main_app_mode_mac.mm:729: [ReplyEventHandler pingProcessAndCall:on_ping_chrome_reply]; Can this line (and the two methods pingProcessAndCall / pingProcess) simply be removed now? (that was my understanding of how NSWorkspaceLaunchConfigurationAppleEvent should work..)
PTAL https://codereview.chromium.org/2416313002/diff/80001/base/mac/launch_service... File base/mac/launch_services_util.mm (right): https://codereview.chromium.org/2416313002/diff/80001/base/mac/launch_service... base/mac/launch_services_util.mm:8: #include "base/mac/scoped_nsobject.h" On 2016/10/26 00:14:48, tapted wrote: > nit: import Done. https://codereview.chromium.org/2416313002/diff/80001/base/mac/launch_service... base/mac/launch_services_util.mm:36: [configuration setObject:launch_args On 2016/10/26 00:14:48, tapted wrote: > optional: we can use the shiny new ObjC syntax for dictionaries now.. except I'm > not sure if scoped_nsobject's conversion to an object pointer will kick in. I.e. > > configuration[NSWorkspaceLaunchConfigurationArguments] = launch_args; > > should work. But maybe only with > > NSMutableDictionary* configuration = [NSMutableDictionary dictionary]; > > above (which is also fine). Done. https://codereview.chromium.org/2416313002/diff/80001/chrome/app_shim/chrome_... File chrome/app_shim/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/2416313002/diff/80001/chrome/app_shim/chrome_... chrome/app_shim/chrome_main_app_mode_mac.mm:545: NSString* chrome_bundle_id = [base::mac::OuterBundle() bundleIdentifier]; On 2016/10/26 00:14:48, tapted wrote: > nit: I guess this should be `chromeBundleId ` since we're in @implementation > (but initial_event below should be changed too). The file looks consistent apart > from that. Done. https://codereview.chromium.org/2416313002/diff/80001/chrome/app_shim/chrome_... chrome/app_shim/chrome_main_app_mode_mac.mm:575: forEventClass:'aevt' On 2016/10/26 00:14:48, tapted wrote: > kCoreEventClass ? Done. https://codereview.chromium.org/2416313002/diff/80001/chrome/app_shim/chrome_... chrome/app_shim/chrome_main_app_mode_mac.mm:576: andEventID:'ansr']; On 2016/10/26 00:14:48, tapted wrote: > kAEAnswer > > I guess we should update the literals in closeWithSuccess as well :/ Done. https://codereview.chromium.org/2416313002/diff/80001/chrome/app_shim/chrome_... chrome/app_shim/chrome_main_app_mode_mac.mm:729: [ReplyEventHandler pingProcessAndCall:on_ping_chrome_reply]; On 2016/10/26 00:14:48, tapted wrote: > Can this line (and the two methods pingProcessAndCall / pingProcess) simply be > removed now? (that was my understanding of how > NSWorkspaceLaunchConfigurationAppleEvent should work..) I removed it at first, but this runs into a problem where the app shim fails if you quit Chromium and then run it.
I mean to play with this today but ran out of time .... feel free to bump it my way if the following suggestions turn into dead ends. https://codereview.chromium.org/2416313002/diff/120001/chrome/app_shim/chrome... File chrome/app_shim/chrome_main_app_mode_mac.mm (right): https://codereview.chromium.org/2416313002/diff/120001/chrome/app_shim/chrome... chrome/app_shim/chrome_main_app_mode_mac.mm:580: targetDescriptor:nil It sounds like maybe this apple event simply isn't getting delivered. What happens if we populate the targetDescriptor here? Maybe the target needs to be +[NSAppleEventDescriptor descriptorWithApplicationURL:bundle_url] -- with |bundle_url| the url created inside OpenApplicationWithPath() https://codereview.chromium.org/2416313002/diff/120001/chrome/app_shim/chrome... chrome/app_shim/chrome_main_app_mode_mac.mm:694: if (pid == -1 && So I just got another crazy idea: I think we can just remove this `pid == -1` check. And in fact all the stuff above under "Find already running instances of Chrome". -[NSWorkspace launchApplicationAtURL:..] says it will find running versions of Chrome for us... That doesn't address the quitting-chrome issue, but it helps reduce the number of codepaths we need to worry about.
On 2016/10/26 06:28:29, tapted wrote: > I mean to play with this today but ran out of time .... feel free to bump it my > way if the following suggestions turn into dead ends. > > https://codereview.chromium.org/2416313002/diff/120001/chrome/app_shim/chrome... > File chrome/app_shim/chrome_main_app_mode_mac.mm (right): > > https://codereview.chromium.org/2416313002/diff/120001/chrome/app_shim/chrome... > chrome/app_shim/chrome_main_app_mode_mac.mm:580: targetDescriptor:nil > It sounds like maybe this apple event simply isn't getting delivered. What > happens if we populate the targetDescriptor here? > > Maybe the target needs to be +[NSAppleEventDescriptor > descriptorWithApplicationURL:bundle_url] -- with |bundle_url| the url created > inside OpenApplicationWithPath() > > https://codereview.chromium.org/2416313002/diff/120001/chrome/app_shim/chrome... > chrome/app_shim/chrome_main_app_mode_mac.mm:694: if (pid == -1 && > So I just got another crazy idea: I think we can just remove this `pid == -1` > check. And in fact all the stuff above under "Find already running instances of > Chrome". > > -[NSWorkspace launchApplicationAtURL:..] says it will find running versions of > Chrome for us... > > That doesn't address the quitting-chrome issue, but it helps reduce the number > of codepaths we need to worry about. Ah, I see what you mean. Keep in mind that descriptorWithApplicationURL is only available on 10.11+ Anyway, it doesn't make any difference. For some reason we're not receiving the ping that was sent when we added the event for NSWorkspaceLaunchConfigurationAppleEvent Not sure why, I'll dig into this. There's two options though: 1) Figure out NSWorkspaceLaunchConfigurationAppleEvent and get it to work 2) Just keep using pingProcess and don't use NSWorkspaceLaunchConfigurationAppleEvent I like 1) better since it's much cleaner, but if I can't get it to work, I'll just get rid of it and stick with 2
On 2016/10/26 20:02:58, spqchan wrote: > On 2016/10/26 06:28:29, tapted wrote: > > I mean to play with this today but ran out of time .... feel free to bump it > my > > way if the following suggestions turn into dead ends. > > > > > https://codereview.chromium.org/2416313002/diff/120001/chrome/app_shim/chrome... > > File chrome/app_shim/chrome_main_app_mode_mac.mm (right): > > > > > https://codereview.chromium.org/2416313002/diff/120001/chrome/app_shim/chrome... > > chrome/app_shim/chrome_main_app_mode_mac.mm:580: targetDescriptor:nil > > It sounds like maybe this apple event simply isn't getting delivered. What > > happens if we populate the targetDescriptor here? > > > > Maybe the target needs to be +[NSAppleEventDescriptor > > descriptorWithApplicationURL:bundle_url] -- with |bundle_url| the url created > > inside OpenApplicationWithPath() > > > > > https://codereview.chromium.org/2416313002/diff/120001/chrome/app_shim/chrome... > > chrome/app_shim/chrome_main_app_mode_mac.mm:694: if (pid == -1 && > > So I just got another crazy idea: I think we can just remove this `pid == -1` > > check. And in fact all the stuff above under "Find already running instances > of > > Chrome". > > > > -[NSWorkspace launchApplicationAtURL:..] says it will find running versions of > > Chrome for us... > > > > That doesn't address the quitting-chrome issue, but it helps reduce the number > > of codepaths we need to worry about. > > Ah, I see what you mean. Keep in mind that descriptorWithApplicationURL is only > available on 10.11+ > Anyway, it doesn't make any difference. For some reason we're not receiving the > ping that was sent when we added the event for > NSWorkspaceLaunchConfigurationAppleEvent > Not sure why, I'll dig into this. > > There's two options though: > 1) Figure out NSWorkspaceLaunchConfigurationAppleEvent and get it to work > 2) Just keep using pingProcess and don't use > NSWorkspaceLaunchConfigurationAppleEvent > > I like 1) better since it's much cleaner, but if I can't get it to work, I'll > just get rid of it and stick with 2 Ping - this is the last 10.9 deployment target blocker.
Patchset #9 (id:180001) has been deleted
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I experimented and no dice, I can't get it a response from the Apple Event sent with NSWorkspaceLaunchConfigurationAppleEvent =/ This solution works though, if you have any ideas or preference let me know. Thanks!
On 2016/10/28 22:02:54, spqchan wrote: > I experimented and no dice, I can't get it a response from the Apple Event sent > with NSWorkspaceLaunchConfigurationAppleEvent =/ > > This solution works though, if you have any ideas or preference let me know. > Thanks! lgtm - this is nice. Thanks a ton for investigating NSWorkspaceLaunchConfigurationAppleEvent - if I have some spare cycles I might poke at it a bit, but what you ended up with is quite neat as well, and I can't really think of any downsides.
On 2016/10/31 00:40:07, tapted wrote: > On 2016/10/28 22:02:54, spqchan wrote: > > I experimented and no dice, I can't get it a response from the Apple Event > sent > > with NSWorkspaceLaunchConfigurationAppleEvent =/ > > > > This solution works though, if you have any ideas or preference let me know. > > Thanks! > > lgtm - this is nice. Thanks a ton for investigating > NSWorkspaceLaunchConfigurationAppleEvent - if I have some spare cycles I might > poke at it a bit, but what you ended up with is quite neat as well, and I can't > really think of any downsides. thanks!
The CQ bit was checked by spqchan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Prepare chrome_main_app_mode_mac.mm for 10.9 - Remove GetProcessForPID() - Ping with the bundle identifier instead of the psn BUG=652563 ========== to ========== Prepare chrome_main_app_mode_mac.mm for 10.9 - Remove GetProcessForPID() - Ping with the bundle identifier instead of the psn BUG=652563 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Prepare chrome_main_app_mode_mac.mm for 10.9 - Remove GetProcessForPID() - Ping with the bundle identifier instead of the psn BUG=652563 ========== to ========== Prepare chrome_main_app_mode_mac.mm for 10.9 - Remove GetProcessForPID() - Ping with the bundle identifier instead of the psn BUG=652563 Committed: https://crrev.com/18e4d08f3ea5d85cce6676cd4dc6e893b7d7824c Cr-Commit-Position: refs/heads/master@{#428741} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/18e4d08f3ea5d85cce6676cd4dc6e893b7d7824c Cr-Commit-Position: refs/heads/master@{#428741} |
