Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1780333006:
Check for null |tabstrip_| in BrowserView::IsTabStripVisible() (Closed)
|
Created:
4 years, 9 months ago by tdanderson Modified:
4 years, 9 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck for null |tabstrip_| in BrowserView::IsTabStripVisible()
Have BrowserView::IsTabStripVisible() return false
if |tabstrip_| is null, which is consistent with the
behaviour of other BrowserView::Is*Visible() functions.
BUG=594649
TEST=none
Committed: https://crrev.com/9b4aaba68babcfcee3887c04cc5397e6d5755450
Cr-Commit-Position: refs/heads/master@{#381235}
Patch Set 1 #
Total comments: 4
Patch Set 2 : DoesIntersectRect() implementations #
Total comments: 13
Patch Set 3 : only change Ash/Mus #
Messages
Total messages: 14 (6 generated)
tdanderson@chromium.org changed reviewers: + pkasting@chromium.org
LGTM. I'm kinda surprised this didn't cause more problems, given how many callers blindly deref the tabstrip when this returns true. https://codereview.chromium.org/1780333006/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc (right): https://codereview.chromium.org/1780333006/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc:409: tabstrip->IsRectInWindowCaption(rect_in_tabstrip_coords); This is functionally equivalent to the Ash code but not laid out quite the same. It would be nice to make these two exactly the same. https://codereview.chromium.org/1780333006/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1780333006/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:600: browser_->SupportsWindowFeature(Browser::FEATURE_TABSTRIP); Nit: I might reverse the order of these two conditions, since the SupportsWindowFeature() call is more of the "do we conceptually have a visible tabstrip" and the |tabstrip_| check is just a "and has it been created yet" check. We could maybe even add comments: // Does this window normally display a tabstrip? if (!browser_->SupportsWindowFeature(Browser::FEATURE_TABSTRIP)) return false; // And has that tabstrip been created yet (by InitViews())? Note that we // can't just check this alone, as the tabstrip is created unconditionally // even for windows that won't display it. return tabstrip_ != nullptr;
Description was changed from ========== Check for null |tabstrip_| in BrowserView::IsTabStripVisible() Have BrowserView::IsTabStripVisible() return false if |tabstrip_| is null, which is consistent with the behaviour of other BrowserView::Is*Visible() functions. BUG=none TEST=none ========== to ========== Check for null |tabstrip_| in BrowserView::IsTabStripVisible() Have BrowserView::IsTabStripVisible() return false if |tabstrip_| is null, which is consistent with the behaviour of other BrowserView::Is*Visible() functions. BUG=594649 TEST=none ==========
Peter, I tried to unify the Ash, Mus, and Opaque frame implementations of DoesIntersectRect(); please see my inline comments. I can also hold off on touching the opaque frame code in this CL if you'd prefer, since this is blocking https://codereview.chromium.org/1785643002/ https://codereview.chromium.org/1780333006/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc (right): https://codereview.chromium.org/1780333006/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc:409: tabstrip->IsRectInWindowCaption(rect_in_tabstrip_coords); On 2016/03/11 22:19:50, Peter Kasting wrote: > This is functionally equivalent to the Ash code but not laid out quite the same. > It would be nice to make these two exactly the same. Done. https://codereview.chromium.org/1780333006/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1780333006/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:600: browser_->SupportsWindowFeature(Browser::FEATURE_TABSTRIP); On 2016/03/11 22:19:50, Peter Kasting wrote: > Nit: I might reverse the order of these two conditions, since the > SupportsWindowFeature() call is more of the "do we conceptually have a visible > tabstrip" and the |tabstrip_| check is just a "and has it been created yet" > check. > > We could maybe even add comments: > > // Does this window normally display a tabstrip? > if (!browser_->SupportsWindowFeature(Browser::FEATURE_TABSTRIP)) > return false; > > // And has that tabstrip been created yet (by InitViews())? Note that we > // can't just check this alone, as the tabstrip is created unconditionally > // even for windows that won't display it. > return tabstrip_ != nullptr; Done. https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right): https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:487: // If the rect is outside the bounds of the client area, claim it. Lines 487-497 differ from Ash/Mus. Would removing lines 488-494 and replacing line 497 with 'return rect.y() < GetTopInset(false)' be logically equivalent here? https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:507: if (rect_in_tabstrip_coords.bottom() > tabstrip->GetLocalBounds().bottom()) { This is different than the corresponding check performed by Ash/Mus. Here you're saying "return false if any part of the rect is below the bottom of the tabstrip" whereas Ash/Mus says "return false if the entire rect is below the bottom of the tabstrip". I believe the Ash/Mus version is what we want here. https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:514: } GlassBFV::DoesIntersectRect() looks very different but (I think) it's essentially trying to do the same thing as the other three implementations, so perhaps we'd want to change its implementation to match these three?
I suggest keeping hit test behavior changes (even for the opaque frame) in their own CL and leaving this related to changing how the tabstrip visibility is checked and the cosmetic unification of Ash/Mus. LGTM with that. https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:417: // |rect| is entirely below the tabstrip. Nit: To me this comment basically restates the code. I would remove it. https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:604: // Has the tabstrip been created yet (by InitViews())? Note that we can't Nit: For parallel structure with the above, maybe change the first sentence to "Return false when the tabstrip has not yet been created (by InitViews()), since callers may otherwise try to access it." https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right): https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:487: // If the rect is outside the bounds of the client area, claim it. On 2016/03/14 20:38:33, tdanderson wrote: > Lines 487-497 differ from Ash/Mus. Would removing lines 488-494 and replacing > line 497 with 'return rect.y() < GetTopInset(false)' be logically equivalent > here? I doubt it. I think the difference may be that the opaque frame has visible frame area along the sides/bottom of the client area and Ash/Mus do not? If so these differences need to remain. https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:507: if (rect_in_tabstrip_coords.bottom() > tabstrip->GetLocalBounds().bottom()) { On 2016/03/14 20:38:33, tdanderson wrote: > This is different than the corresponding check performed by Ash/Mus. Here you're > saying "return false if any part of the rect is below the bottom of the > tabstrip" whereas Ash/Mus says "return false if the entire rect is below the > bottom of the tabstrip". I believe the Ash/Mus version is what we want here. I agree. https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:514: } On 2016/03/14 20:38:33, tdanderson wrote: > GlassBFV::DoesIntersectRect() looks very different but (I think) it's > essentially trying to do the same thing as the other three implementations, so > perhaps we'd want to change its implementation to match these three? I don't know. Maybe. I think I'd want to verify some of these behavioral differences by coming up with concrete sequences of actions that test the differences. Then we could probably just tell "should this return true or false when I do this" by what actually ends up happening.
https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:417: // |rect| is entirely below the tabstrip. On 2016/03/14 23:04:42, Peter Kasting wrote: > Nit: To me this comment basically restates the code. I would remove it. Done. https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:604: // Has the tabstrip been created yet (by InitViews())? Note that we can't On 2016/03/14 23:04:42, Peter Kasting wrote: > Nit: For parallel structure with the above, maybe change the first sentence to > "Return false when the tabstrip has not yet been created (by InitViews()), since > callers may otherwise try to access it." Done. https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right): https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:487: // If the rect is outside the bounds of the client area, claim it. On 2016/03/14 23:04:42, Peter Kasting wrote: > On 2016/03/14 20:38:33, tdanderson wrote: > > Lines 487-497 differ from Ash/Mus. Would removing lines 488-494 and replacing > > line 497 with 'return rect.y() < GetTopInset(false)' be logically equivalent > > here? > > I doubt it. I think the difference may be that the opaque frame has visible > frame area along the sides/bottom of the client area and Ash/Mus do not? If so > these differences need to remain. Ah, OK. It is true that the Ash/Mus frames do not have visible frame area along the sides and bottom. So I think we should be able to change the Ash/Mus implementations to match what we have here. Will explore that as a follow-up. https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:507: if (rect_in_tabstrip_coords.bottom() > tabstrip->GetLocalBounds().bottom()) { On 2016/03/14 23:04:42, Peter Kasting wrote: > On 2016/03/14 20:38:33, tdanderson wrote: > > This is different than the corresponding check performed by Ash/Mus. Here > you're > > saying "return false if any part of the rect is below the bottom of the > > tabstrip" whereas Ash/Mus says "return false if the entire rect is below the > > bottom of the tabstrip". I believe the Ash/Mus version is what we want here. > > I agree. Can address as follow-up. https://codereview.chromium.org/1780333006/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:514: } On 2016/03/14 23:04:42, Peter Kasting wrote: > On 2016/03/14 20:38:33, tdanderson wrote: > > GlassBFV::DoesIntersectRect() looks very different but (I think) it's > > essentially trying to do the same thing as the other three implementations, so > > perhaps we'd want to change its implementation to match these three? > > I don't know. Maybe. I think I'd want to verify some of these behavioral > differences by coming up with concrete sequences of actions that test the > differences. Then we could probably just tell "should this return true or false > when I do this" by what actually ends up happening. OK, I'll leave that alone then.
The CQ bit was checked by tdanderson@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/1780333006/#ps40001 (title: "only change Ash/Mus")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1780333006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1780333006/40001
Message was sent while issue was closed.
Description was changed from ========== Check for null |tabstrip_| in BrowserView::IsTabStripVisible() Have BrowserView::IsTabStripVisible() return false if |tabstrip_| is null, which is consistent with the behaviour of other BrowserView::Is*Visible() functions. BUG=594649 TEST=none ========== to ========== Check for null |tabstrip_| in BrowserView::IsTabStripVisible() Have BrowserView::IsTabStripVisible() return false if |tabstrip_| is null, which is consistent with the behaviour of other BrowserView::Is*Visible() functions. BUG=594649 TEST=none ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Check for null |tabstrip_| in BrowserView::IsTabStripVisible() Have BrowserView::IsTabStripVisible() return false if |tabstrip_| is null, which is consistent with the behaviour of other BrowserView::Is*Visible() functions. BUG=594649 TEST=none ========== to ========== Check for null |tabstrip_| in BrowserView::IsTabStripVisible() Have BrowserView::IsTabStripVisible() return false if |tabstrip_| is null, which is consistent with the behaviour of other BrowserView::Is*Visible() functions. BUG=594649 TEST=none Committed: https://crrev.com/9b4aaba68babcfcee3887c04cc5397e6d5755450 Cr-Commit-Position: refs/heads/master@{#381235} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9b4aaba68babcfcee3887c04cc5397e6d5755450 Cr-Commit-Position: refs/heads/master@{#381235} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
