Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2748103005:
Don't focus the omnibox when a subframe navigates on NTP. (Closed)
|
Created:
3 years, 9 months ago by alexmos Modified:
3 years, 9 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't focus the omnibox when a subframe navigates on NTP.
This fixes a bug where an NTP replacement page from an extension was
losing focus to the omnibox when a subframe on that page committed a
navigation. So if the user was typing into an <input> on the NTP
replacement page while a subframe navigated, the focus was
disruptively shifted to the omnibox in the middle of typing.
This matters only in OOPIF modes, as focusing of the omnibox was done
in RenderFrameHostManager::CommitPending, which is only used for
cross-process commits.
BUG=700124
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2748103005
Cr-Commit-Position: refs/heads/master@{#457222}
Committed: https://chromium.googlesource.com/chromium/src/+/5e8b5e6aebcb564897e311ae4f4397eed3555457
Patch Set 1 #
Total comments: 2
Patch Set 2 : Nasko's nit #
Total comments: 2
Patch Set 3 : Devlin's comment #
Messages
Total messages: 26 (19 generated)
Description was changed from ========== Don't focus the omnibar when a subframe navigates on NTP. BUG=700124 ========== to ========== Don't focus the omnibar when a subframe navigates on NTP. BUG=700124 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by alexmos@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.
Description was changed from ========== Don't focus the omnibar when a subframe navigates on NTP. BUG=700124 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't focus the omnibar when a subframe navigates on NTP. This fixes a bug where an NTP replacement page from an extension was losing focus to the omnibox when a subframe on that page committed a navigation. So if the user was typing into an <input> on the NTP replacement page while a subframe navigated, the focus was disruptively shifted to the omnibox in the middle of typing. BUG=700124 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Don't focus the omnibar when a subframe navigates on NTP. This fixes a bug where an NTP replacement page from an extension was losing focus to the omnibox when a subframe on that page committed a navigation. So if the user was typing into an <input> on the NTP replacement page while a subframe navigated, the focus was disruptively shifted to the omnibox in the middle of typing. BUG=700124 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't focus the omnibar when a subframe navigates on NTP. This fixes a bug where an NTP replacement page from an extension was losing focus to the omnibox when a subframe on that page committed a navigation. So if the user was typing into an <input> on the NTP replacement page while a subframe navigated, the focus was disruptively shifted to the omnibox in the middle of typing. This matters only in OOPIF modes, as focusing of the omnibox was done in RenderFrameHostManager::CommitPending, which is only used for cross-process commits. BUG=700124 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
alexmos@chromium.org changed reviewers: + nasko@chromium.org
Nasko, could you please take a look at this fix? It's short and hopefully straightforward.
Description was changed from ========== Don't focus the omnibar when a subframe navigates on NTP. This fixes a bug where an NTP replacement page from an extension was losing focus to the omnibox when a subframe on that page committed a navigation. So if the user was typing into an <input> on the NTP replacement page while a subframe navigated, the focus was disruptively shifted to the omnibox in the middle of typing. This matters only in OOPIF modes, as focusing of the omnibox was done in RenderFrameHostManager::CommitPending, which is only used for cross-process commits. BUG=700124 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't focus the omnibox when a subframe navigates on NTP. This fixes a bug where an NTP replacement page from an extension was losing focus to the omnibox when a subframe on that page committed a navigation. So if the user was typing into an <input> on the NTP replacement page while a subframe navigated, the focus was disruptively shifted to the omnibox in the middle of typing. This matters only in OOPIF modes, as focusing of the omnibox was done in RenderFrameHostManager::CommitPending, which is only used for cross-process commits. BUG=700124 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Don't focus the omnibox when a subframe navigates on NTP. This fixes a bug where an NTP replacement page from an extension was losing focus to the omnibox when a subframe on that page committed a navigation. So if the user was typing into an <input> on the NTP replacement page while a subframe navigated, the focus was disruptively shifted to the omnibox in the middle of typing. This matters only in OOPIF modes, as focusing of the omnibox was done in RenderFrameHostManager::CommitPending, which is only used for cross-process commits. BUG=700124 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't focus the omnibox when a subframe navigates on NTP. This fixes a bug where an NTP replacement page from an extension was losing focus to the omnibox when a subframe on that page committed a navigation. So if the user was typing into an <input> on the NTP replacement page while a subframe navigated, the focus was disruptively shifted to the omnibox in the middle of typing. This matters only in OOPIF modes, as focusing of the omnibox was done in RenderFrameHostManager::CommitPending, which is only used for cross-process commits. BUG=700124 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
LGTM with a nit. https://codereview.chromium.org/2748103005/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_override_apitest.cc (right): https://codereview.chromium.org/2748103005/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_override_apitest.cc:212: ui_test_utils::NavigateToURL(browser(), GURL("chrome://newtab/")); kChromeUINewTabURL
The CQ bit was checked by alexmos@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...
alexmos@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Thanks, Nasko. Devlin, could you please review the test for OWNERS? https://codereview.chromium.org/2748103005/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_override_apitest.cc (right): https://codereview.chromium.org/2748103005/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_override_apitest.cc:212: ui_test_utils::NavigateToURL(browser(), GURL("chrome://newtab/")); On 2017/03/15 17:14:24, nasko (slow) wrote: > kChromeUINewTabURL Done. At some point, it'd be nice to refactor the rest of this file to also use URL constants.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2748103005/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_override_apitest.cc (right): https://codereview.chromium.org/2748103005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_override_apitest.cc:211: ExtensionTestMessageListener listener(false); optional: If we know the expected message, we can use listener("controlled by first", false) to only listen to the expected message. Chances are nothing else will be sending a message, and up to you if you want to consider it a failure if something else does.
Thanks! https://codereview.chromium.org/2748103005/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_override_apitest.cc (right): https://codereview.chromium.org/2748103005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_override_apitest.cc:211: ExtensionTestMessageListener listener(false); On 2017/03/15 18:32:29, Devlin wrote: > optional: If we know the expected message, we can use listener("controlled by > first", false) to only listen to the expected message. Chances are nothing else > will be sending a message, and up to you if you want to consider it a failure if > something else does. Done.
The CQ bit was checked by alexmos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2748103005/#ps40001 (title: "Devlin's comment")
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": 40001, "attempt_start_ts": 1489607866570790,
"parent_rev": "d232d0c861bf43ce092894e33ba277906fa513b8", "commit_rev":
"5e8b5e6aebcb564897e311ae4f4397eed3555457"}
Message was sent while issue was closed.
Description was changed from ========== Don't focus the omnibox when a subframe navigates on NTP. This fixes a bug where an NTP replacement page from an extension was losing focus to the omnibox when a subframe on that page committed a navigation. So if the user was typing into an <input> on the NTP replacement page while a subframe navigated, the focus was disruptively shifted to the omnibox in the middle of typing. This matters only in OOPIF modes, as focusing of the omnibox was done in RenderFrameHostManager::CommitPending, which is only used for cross-process commits. BUG=700124 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't focus the omnibox when a subframe navigates on NTP. This fixes a bug where an NTP replacement page from an extension was losing focus to the omnibox when a subframe on that page committed a navigation. So if the user was typing into an <input> on the NTP replacement page while a subframe navigated, the focus was disruptively shifted to the omnibox in the middle of typing. This matters only in OOPIF modes, as focusing of the omnibox was done in RenderFrameHostManager::CommitPending, which is only used for cross-process commits. BUG=700124 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2748103005 Cr-Commit-Position: refs/heads/master@{#457222} Committed: https://chromium.googlesource.com/chromium/src/+/5e8b5e6aebcb564897e311ae4f43... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5e8b5e6aebcb564897e311ae4f43... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
