Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737553004:
Set background color when adding a new tab. (Closed)
DescriptionSet background color when adding a new tab.
https://codereview.chromium.org/2466413009 covered some cases, but the
case of creating a new tab via ctrl-+ is covered by this CL.
BUG=470669
Review-Url: https://codereview.chromium.org/2737553004
Cr-Commit-Position: refs/heads/master@{#455623}
Committed: https://chromium.googlesource.com/chromium/src/+/125dfe5fa4f0d1ed3f0bcde1f8eaad419c581be5
Patch Set 1 #Patch Set 2 : none #
Total comments: 6
Patch Set 3 : none #Patch Set 4 : none #
Total comments: 2
Patch Set 5 : none #
Total comments: 2
Patch Set 6 : none #Messages
Total messages: 22 (11 generated)
Description was changed from ========== none none BUG= ========== to ========== Set background color when adding a new tab.e BUG=470669 ==========
The CQ bit was checked by chrishtr@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 ========== Set background color when adding a new tab.e BUG=470669 ========== to ========== Set background color when adding a new tab. https://codereview.chromium.org/2466413009 covered some cases, but the case of creating a new tab via ctrl-+ is covered by this CL. BUG=470669 ==========
chrishtr@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2737553004/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2737553004/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1052: if (old_view && new_view) { Can this condition actually fail? When? Nit: No {} https://codereview.chromium.org/2737553004/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_unittest.cc (right): https://codereview.chromium.org/2737553004/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_unittest.cc:87: // Color is only set once the tab is activated. Why is it important to test this? It doesn't seem like it would be a bug if we violated this in the future.
https://codereview.chromium.org/2737553004/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2737553004/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1052: if (old_view && new_view) { On 2017/03/07 at 00:28:36, Peter Kasting wrote: > Can this condition actually fail? When? I know of no cases. Would you prefer I skip the conditional? > Nit: No {} Fixed. https://codereview.chromium.org/2737553004/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser_unittest.cc (right): https://codereview.chromium.org/2737553004/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser_unittest.cc:87: // Color is only set once the tab is activated. On 2017/03/07 at 00:28:36, Peter Kasting wrote: > Why is it important to test this? It doesn't seem like it would be a bug if we violated this in the future. Fair point. Deleted this part.
https://codereview.chromium.org/2737553004/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2737553004/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1052: if (old_view && new_view) { On 2017/03/07 00:36:48, chrishtr wrote: > On 2017/03/07 at 00:28:36, Peter Kasting wrote: > > Can this condition actually fail? When? > > I know of no cases. Would you prefer I skip the conditional? My concern is that a null-check implies to future readers that these can be null. We should know for sure whether or not these can be null, so we know that the null-check is definitely needed (and when), and document that, or know that it's definitely not, so we can omit it or, if necessary, write DCHECKs. I'm not an expect in WebContents/RenderFrameHost, so I don't know the constraints here. But I'm sure someone else does.
chrishtr@chromium.org changed reviewers: + nasko@chromium.org
Ok. +nasko Nasko, any advice regarding Peter's comment?
https://codereview.chromium.org/2737553004/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2737553004/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1052: if (old_view && new_view) { On 2017/03/07 00:51:20, Peter Kasting wrote: > On 2017/03/07 00:36:48, chrishtr wrote: > > On 2017/03/07 at 00:28:36, Peter Kasting wrote: > > > Can this condition actually fail? When? > > > > I know of no cases. Would you prefer I skip the conditional? > > My concern is that a null-check implies to future readers that these can be > null. We should know for sure whether or not these can be null, so we know that > the null-check is definitely needed (and when), and document that, or know that > it's definitely not, so we can omit it or, if necessary, write DCHECKs. > > I'm not an expect in WebContents/RenderFrameHost, so I don't know the > constraints here. But I'm sure someone else does. The GetMainFrame() call is guaranteed to return non-null. However, the call to GetView() is not. There is a brief period between WebContents creation and the creation of the RWHV, which if you call GetView() you will get null. I also think that it happens when the process for the main frame goes away (killed or crashed). https://codereview.chromium.org/2737553004/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2737553004/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1047: // loaded any contents. As a result, we avoid flashing white when navigating nit: Is it when navigating or when switching tabs?
On 2017/03/08 at 23:11:32, nasko wrote: > https://codereview.chromium.org/2737553004/diff/20001/chrome/browser/ui/brows... > File chrome/browser/ui/browser.cc (right): > > https://codereview.chromium.org/2737553004/diff/20001/chrome/browser/ui/brows... > chrome/browser/ui/browser.cc:1052: if (old_view && new_view) { > On 2017/03/07 00:51:20, Peter Kasting wrote: > > On 2017/03/07 00:36:48, chrishtr wrote: > > > On 2017/03/07 at 00:28:36, Peter Kasting wrote: > > > > Can this condition actually fail? When? > > > > > > I know of no cases. Would you prefer I skip the conditional? > > > > My concern is that a null-check implies to future readers that these can be > > null. We should know for sure whether or not these can be null, so we know that > > the null-check is definitely needed (and when), and document that, or know that > > it's definitely not, so we can omit it or, if necessary, write DCHECKs. > > > > I'm not an expect in WebContents/RenderFrameHost, so I don't know the > > constraints here. But I'm sure someone else does. > > The GetMainFrame() call is guaranteed to return non-null. However, the call to GetView() is not. There is a brief period between WebContents creation and the creation of the RWHV, which if you call GetView() you will get null. I also think that it happens when the process for the main frame goes away (killed or crashed). Ok, I went ahead and left the checks for GetView() in, based on what you said. > > https://codereview.chromium.org/2737553004/diff/60001/chrome/browser/ui/brows... > File chrome/browser/ui/browser.cc (right): > > https://codereview.chromium.org/2737553004/diff/60001/chrome/browser/ui/brows... > chrome/browser/ui/browser.cc:1047: // loaded any contents. As a result, we avoid flashing white when navigating > nit: Is it when navigating or when switching tabs?
LGTM https://codereview.chromium.org/2737553004/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2737553004/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1050: RenderWidgetHostView* old_view = old_contents->GetMainFrame()->GetView(); NIt: Document the null check, e.g. "While GetMainFrame() is guaranteed to return non-null, GetView() is not, e.g. between WebContents creation and creation of the RenderWidgetHostView."
https://codereview.chromium.org/2737553004/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2737553004/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1047: // loaded any contents. As a result, we avoid flashing white when navigating On 2017/03/08 at 23:11:32, nasko (slow) wrote: > nit: Is it when navigating or when switching tabs? Fixed. https://codereview.chromium.org/2737553004/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2737553004/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1050: RenderWidgetHostView* old_view = old_contents->GetMainFrame()->GetView(); On 2017/03/09 at 00:02:03, Peter Kasting wrote: > NIt: Document the null check, e.g. "While GetMainFrame() is guaranteed to return non-null, GetView() is not, e.g. between WebContents creation and creation of the RenderWidgetHostView." Done.
The CQ bit was checked by chrishtr@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/2737553004/#ps100001 (title: "none")
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": 100001, "attempt_start_ts": 1489017898487090,
"parent_rev": "c2d78373f7f7b3e71c3f08707d7d0a27c082de1d", "commit_rev":
"125dfe5fa4f0d1ed3f0bcde1f8eaad419c581be5"}
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489017898487090,
"parent_rev": "c2d78373f7f7b3e71c3f08707d7d0a27c082de1d", "commit_rev":
"125dfe5fa4f0d1ed3f0bcde1f8eaad419c581be5"}
Message was sent while issue was closed.
Description was changed from ========== Set background color when adding a new tab. https://codereview.chromium.org/2466413009 covered some cases, but the case of creating a new tab via ctrl-+ is covered by this CL. BUG=470669 ========== to ========== Set background color when adding a new tab. https://codereview.chromium.org/2466413009 covered some cases, but the case of creating a new tab via ctrl-+ is covered by this CL. BUG=470669 Review-Url: https://codereview.chromium.org/2737553004 Cr-Commit-Position: refs/heads/master@{#455623} Committed: https://chromium.googlesource.com/chromium/src/+/125dfe5fa4f0d1ed3f0bcde1f8ea... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/125dfe5fa4f0d1ed3f0bcde1f8ea... |
