Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2624373002:
Don't focus the location bar for NTP navigations in non-selected tabs. (Closed)
|
Created:
3 years, 11 months ago by Charlie Reis Modified:
3 years, 11 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, estark Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't focus the location bar for NTP navigations in non-selected tabs.
BUG=677716
TEST=See bug for repro steps.
Review-Url: https://codereview.chromium.org/2624373002
Cr-Commit-Position: refs/heads/master@{#443338}
Committed: https://chromium.googlesource.com/chromium/src/+/8f3a9a68b2dcdd2c54cf49a41ad34729ab576702
Patch Set 1 : Initial patch #
Total comments: 6
Patch Set 2 : Fix nits #
Messages
Total messages: 19 (12 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by creis@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...
creis@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@, can you take a look? This is a variation on https://codereview.chromium.org/1678233003/ from https://crbug.com/567445.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2624373002/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2624373002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1554: // Focus if we're going to the NTP, but only if it's in the active tab. Nit: This might be clearer as "Navigations in background tabs shouldn't change the focus state of the omnibox, since it's associated with the foreground tab." https://codereview.chromium.org/2624373002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1556: return false; Does this mean that, when a user switches to a now-navigated-to-the-NTP background tab, the location bar won't be focused, but it should be? Of course that wouldn't have worked before either, I'm just wondering if there's a secondary bug here that a background NTP navigation should mark the saved state for that omnibox as "focused". https://codereview.chromium.org/2624373002/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/2624373002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:749: const GURL url1(embedded_test_server()->GetURL("/title1.html")); Nit: Prefer = to () for initializing things like GURLs and strings; see https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... . (several places) Personally, for things like "url1" where the name is not much clearer than seeing the code used to initialize this, I'd just inline the initializer into the usage below directly, unless things are longer overall that way. But up to you.
Patchset #2 (id:60001) has been deleted
Thanks! https://codereview.chromium.org/2624373002/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2624373002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1554: // Focus if we're going to the NTP, but only if it's in the active tab. On 2017/01/12 18:24:04, Peter Kasting wrote: > Nit: This might be clearer as "Navigations in background tabs shouldn't change > the focus state of the omnibox, since it's associated with the foreground tab." Done. https://codereview.chromium.org/2624373002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1556: return false; On 2017/01/12 18:24:04, Peter Kasting wrote: > Does this mean that, when a user switches to a now-navigated-to-the-NTP > background tab, the location bar won't be focused, but it should be? Of course > that wouldn't have worked before either, I'm just wondering if there's a > secondary bug here that a background NTP navigation should mark the saved state > for that omnibox as "focused". Yes, you're right that there's a secondary bug, and it affects Stable and Canary today. I've filed https://crbug.com/680624 for it. https://codereview.chromium.org/2624373002/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/2624373002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:749: const GURL url1(embedded_test_server()->GetURL("/title1.html")); On 2017/01/12 18:24:04, Peter Kasting wrote: > Nit: Prefer = to () for initializing things like GURLs and strings; see > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab... > . (several places) > > Personally, for things like "url1" where the name is not much clearer than > seeing the code used to initialize this, I'd just inline the initializer into > the usage below directly, unless things are longer overall that way. But up to > you. Thanks, done. I updated the source of the pattern (i.e., test above) as well.
The CQ bit was checked by creis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2624373002/#ps80001 (title: "Fix 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": 80001, "attempt_start_ts": 1484249236065790,
"parent_rev": "d0fa9cf8f06c7cba55a18246a71038618e0463f5", "commit_rev":
"8f3a9a68b2dcdd2c54cf49a41ad34729ab576702"}
Message was sent while issue was closed.
Description was changed from ========== Don't focus the location bar for NTP navigations in non-selected tabs. BUG=677716 TEST=See bug for repro steps. ========== to ========== Don't focus the location bar for NTP navigations in non-selected tabs. BUG=677716 TEST=See bug for repro steps. Review-Url: https://codereview.chromium.org/2624373002 Cr-Commit-Position: refs/heads/master@{#443338} Committed: https://chromium.googlesource.com/chromium/src/+/8f3a9a68b2dcdd2c54cf49a41ad3... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:80001) as https://chromium.googlesource.com/chromium/src/+/8f3a9a68b2dcdd2c54cf49a41ad3...
Message was sent while issue was closed.
(Due to a drover issue, this landed in https://codereview.chromium.org/2640773003/ instead.)
Message was sent while issue was closed.
On 2017/01/18 23:59:41, Charlie Reis wrote: > (Due to a drover issue, this landed in > https://codereview.chromium.org/2640773003/ instead.) Oops, posted to wrong CL again. :) That message was about the merge to M56. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
