Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2729613006:
When navigation focuses a web contents, also activate its window. (Closed)
|
Created:
3 years, 9 months ago by Sidney San Martín Modified:
3 years, 9 months ago CC:
chromium-reviews, awdf+watch_chromium.org, Peter Beverloo, mlamouri+watch-notifications_chromium.org, jam, darin-cc_chromium.org, mac-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWhen navigation focuses a web contents, also activate its window.
This fixes an issue where a popup opened by clicking a notification doesn't
activate Chrome (and appears behind the active app) on Mac.
BUG=661250
Review-Url: https://codereview.chromium.org/2729613006
Cr-Commit-Position: refs/heads/master@{#455632}
Committed: https://chromium.googlesource.com/chromium/src/+/6589abd5285d3008c44143269bdf5dc2605028cf
Patch Set 1 #
Total comments: 3
Patch Set 2 : Move common interactive_uitest stuff into its own file instead of notification_test_util #Patch Set 3 : Activate in a more specific place. #Patch Set 4 : Test cleanup. #
Total comments: 4
Patch Set 5 : Copyright + nits #
Messages
Total messages: 48 (38 generated)
The CQ bit was checked by sdy@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...
Description was changed from ========== [Mac] Make Chrome the active app when focusing a web contents. This fixes an issue where a popup opened by clicking a notification doesn't activate Chrome (and appears behind the active app). The change is a one-line addition to web_contents_view_mac.mm. The rest of this CL is a test, with refactoring of notification_interactive_uitest to allow for separate ObjC++ tests for Mac. BUG=661250 ========== to ========== [Mac] Activate Chrome when focusing a web contents view. This fixes an issue where a popup opened by clicking a notification doesn't activate Chrome (and appears behind the active app). The change is a one-line addition to web_contents_view_mac.mm. The rest of this CL is a test, with refactoring of notification_interactive_uitest to allow for separate ObjC++ tests for Mac. BUG=661250 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
sdy@chromium.org changed reviewers: + avi@chromium.org, yoshiki@chromium.org
PTAL
sdy@chromium.org changed reviewers: - avi@chromium.org, yoshiki@chromium.org
Sorry, I missed that I broke the Linux build. Will resubmit once I've fixed it.
avi@chromium.org changed reviewers: + avi@chromium.org
https://codereview.chromium.org/2729613006/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_view_mac.mm (right): https://codereview.chromium.org/2729613006/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_view_mac.mm:230: [NSApp activateIgnoringOtherApps:YES]; I could swear I've seen this change before. Who else calls Focus? Are we ok with that now forcing the window to the top of the pile no matter what?
The CQ bit was checked by sdy@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.
The CQ bit was checked by sdy@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.
sdy@chromium.org changed reviewers: + thakis@chromium.org, yoshiki@chromium.org
sdy@chromium.org changed required reviewers: + thakis@chromium.org, yoshiki@chromium.org
PTAL (avi@ — it looks like I don't need you as a reviewer anymore, but feel free to stay if you want.) https://codereview.chromium.org/2729613006/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_view_mac.mm (right): https://codereview.chromium.org/2729613006/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_view_mac.mm:230: [NSApp activateIgnoringOtherApps:YES]; On 2017/03/02 23:26:18, Avi wrote: > I could swear I've seen this change before. One of these? https://codereview.chromium.org/2412983004 https://codereview.chromium.org/2420733002 I haven't found the One True Change to cover all of these cases at once, yet. https://codereview.chromium.org/2729613006/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_view_mac.mm:230: [NSApp activateIgnoringOtherApps:YES]; On 2017/03/02 23:26:18, Avi wrote: > Who else calls Focus? Are we ok with that now forcing the window to the top of > the pile no matter what? I'd missed some callers… so, thanks! I found a more appropriate place.
Description was changed from ========== [Mac] Activate Chrome when focusing a web contents view. This fixes an issue where a popup opened by clicking a notification doesn't activate Chrome (and appears behind the active app). The change is a one-line addition to web_contents_view_mac.mm. The rest of this CL is a test, with refactoring of notification_interactive_uitest to allow for separate ObjC++ tests for Mac. BUG=661250 ========== to ========== When navigation focuses a web contents, also activate its window. This fixes an issue where a popup opened by clicking a notification doesn't activate Chrome (and appears behind the active app) on Mac. BUG=661250 ==========
Sorry, the CL title and description didn't match the most recent patch set. Fixed.
The CQ bit was checked by sdy@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 sdy@chromium.org
The CQ bit was checked by sdy@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...
Patchset #4 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm, but someone familiar with notifications (e.g. yoshiki) should take a look too. Nice test! https://codereview.chromium.org/2729613006/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_interactive_uitest_support.cc (right): https://codereview.chromium.org/2729613006/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_interactive_uitest_support.cc:23: // remove // https://codereview.chromium.org/2729613006/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_interactive_uitest_support.cc:264: for (ContentSettingsForOneType::const_iterator it = settings.begin(); can be a for-each loop
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
The CQ bit was unchecked by sdy@chromium.org
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Patchset #5 (id:100001) has been deleted
Thanks! https://codereview.chromium.org/2729613006/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_interactive_uitest_support.cc (right): https://codereview.chromium.org/2729613006/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_interactive_uitest_support.cc:23: // On 2017/03/08 16:23:25, Nico wrote: > remove // Done. https://codereview.chromium.org/2729613006/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_interactive_uitest_support.cc:264: for (ContentSettingsForOneType::const_iterator it = settings.begin(); On 2017/03/08 16:23:25, Nico wrote: > can be a for-each loop Done.
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.
lgtm
The CQ bit was checked by sdy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2729613006/#ps120001 (title: "Copyright + nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489023511693480,
"parent_rev": "8cc2674aa0a8f9da76f18fc55bd8adf4a4730a3b", "commit_rev":
"6589abd5285d3008c44143269bdf5dc2605028cf"}
Message was sent while issue was closed.
Description was changed from ========== When navigation focuses a web contents, also activate its window. This fixes an issue where a popup opened by clicking a notification doesn't activate Chrome (and appears behind the active app) on Mac. BUG=661250 ========== to ========== When navigation focuses a web contents, also activate its window. This fixes an issue where a popup opened by clicking a notification doesn't activate Chrome (and appears behind the active app) on Mac. BUG=661250 Review-Url: https://codereview.chromium.org/2729613006 Cr-Commit-Position: refs/heads/master@{#455632} Committed: https://chromium.googlesource.com/chromium/src/+/6589abd5285d3008c44143269bdf... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/6589abd5285d3008c44143269bdf... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
