Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26751003:
Factor out [min|max]_size into ShellWindow::SizeConstraints (Closed)
|
Created:
7 years, 2 months ago by jackhou1 Modified:
7 years, 2 months ago CC:
chromium-reviews, tfarina, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFactor out [min|max]_size into ShellWindow::SizeConstraints
This encapsulates the logic for managing window size constraints.
BUG=305477
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228654
Patch Set 1 #Patch Set 2 : Restrict this change to apps/ #
Total comments: 15
Patch Set 3 : Address comments #
Total comments: 14
Patch Set 4 : Address comments #
Total comments: 4
Patch Set 5 : Address comments, fix bug #Messages
Total messages: 11 (0 generated)
https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.cc File apps/shell_window.cc (right): https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.cc#newco... apps/shell_window.cc:78: result.set_width(std::max(result.width(), minimum_size().width())); I think this and the next line is just `result.SetToMax(minimum_size)` https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.cc#newco... apps/shell_window.cc:85: minimum_size().height() != kUnboundedSize; nit(I think): only 4 spaces of indent for these https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.cc#newco... apps/shell_window.cc:156: gfx::Rect bounds = params.bounds; While we're refactoring.... I think this is the CL where it makes sense to decompose the crazy & massive constructor logic from this point down. The relationship between bounds, params.bounds, new_params.bounds, and cached_bounds is really unclear. Your change is an improvement, but the changes will be easier to follow if this is tackled too. In particular, here we break `bounds` off params.bounds, change it, then copy the original params into new_params (which now has a stale bounds). Then we change the broken off bounds (line 188) AND the unrelated new_params (line 189). But, then, in the code you've changed so far, we add the original (and altered) `bounds` back onto `new_params`, and couple them together again. What I'm thinking would be good is: window_key_ = params.window_key; CreateParams new_params = LoadDefaultsAndConstrain(params); native_app_window_.reset(delegate_->CreateNativeAppWindow(this, new_params)); if (!new_params.hidden) { /* etc */ which is a const member function: ShellWindow::CreateParams ShellWindow::LoadDefaultsAndConstrain(CreateParams params) const { /* mess with params */ return params; } WDYT? It might be OK in a follow-up too, but I think it needs to be done at some point. https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.h File apps/shell_window.h (right): https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.h#newcode96 apps/shell_window.h:96: static const int kUnboundedSize = 0; not sure if visual studio is happy with inline initializers yet.. https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.h#newcod... apps/shell_window.h:114: bool MinAndMaxEqual() const; Perhaps `HasFixedSize`? But only if that's the only thing it's used for, otherwise I think this name is OK. https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.h#newcod... apps/shell_window.h:116: gfx::Size minimum_size() const; nit: the the accessor functions should match the variable name (i.e. both `minimum` or both `min`). I don't really have a preference for one over the other though.
https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.cc File apps/shell_window.cc (right): https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.cc#newco... apps/shell_window.cc:78: result.set_width(std::max(result.width(), minimum_size().width())); On 2013/10/14 03:17:10, tapted wrote: > I think this and the next line is just `result.SetToMax(minimum_size)` SetToMax is in gfx::Size but not gfx::Rect. https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.cc#newco... apps/shell_window.cc:85: minimum_size().height() != kUnboundedSize; On 2013/10/14 03:17:10, tapted wrote: > nit(I think): only 4 spaces of indent for these Done. https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.cc#newco... apps/shell_window.cc:156: gfx::Rect bounds = params.bounds; On 2013/10/14 03:17:10, tapted wrote: > While we're refactoring.... I think this is the CL where it makes sense to > decompose the crazy & massive constructor logic from this point down. The > relationship between bounds, params.bounds, new_params.bounds, and cached_bounds > is really unclear. Your change is an improvement, but the changes will be easier > to follow if this is tackled too. > > In particular, here we break `bounds` off params.bounds, change it, then copy > the original params into new_params (which now has a stale bounds). Then we > change the broken off bounds (line 188) AND the unrelated new_params (line 189). > But, then, in the code you've changed so far, we add the original (and altered) > `bounds` back onto `new_params`, and couple them together again. > > > What I'm thinking would be good is: > > window_key_ = params.window_key; > CreateParams new_params = LoadDefaultsAndConstrain(params); > native_app_window_.reset(delegate_->CreateNativeAppWindow(this, new_params)); > if (!new_params.hidden) { > /* etc */ > > > which is a const member function: > > ShellWindow::CreateParams ShellWindow::LoadDefaultsAndConstrain(CreateParams > params) const { > /* mess with params */ > return params; > } > > > WDYT? It might be OK in a follow-up too, but I think it needs to be done at some > point. Done. https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.h File apps/shell_window.h (right): https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.h#newcode96 apps/shell_window.h:96: static const int kUnboundedSize = 0; On 2013/10/14 03:17:10, tapted wrote: > not sure if visual studio is happy with inline initializers yet.. This type of declaration appears occassionally in chrome/. https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.h#newcod... apps/shell_window.h:114: bool MinAndMaxEqual() const; On 2013/10/14 03:17:10, tapted wrote: > Perhaps `HasFixedSize`? But only if that's the only thing it's used for, > otherwise I think this name is OK. Done. https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.h#newcod... apps/shell_window.h:116: gfx::Size minimum_size() const; On 2013/10/14 03:17:10, tapted wrote: > nit: the the accessor functions should match the variable name (i.e. both > `minimum` or both `min`). I don't really have a preference for one over the > other though. Done.
looking really good - I like it! These are mostly nits, but some questions too. https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.cc File apps/shell_window.cc (right): https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.cc#newco... apps/shell_window.cc:78: result.set_width(std::max(result.width(), minimum_size().width())); On 2013/10/14 04:13:06, jackhou1 wrote: > On 2013/10/14 03:17:10, tapted wrote: > > I think this and the next line is just `result.SetToMax(minimum_size)` > > SetToMax is in gfx::Size but not gfx::Rect. Ah. Drat. But... what about changing the function signature to accept and return gfx::Size? Since, it's not changing the Rect origin.. and the class is a `SizeConstraints` not a `BoundsConstraints` ;). https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.h File apps/shell_window.h (right): https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.h#newcode96 apps/shell_window.h:96: static const int kUnboundedSize = 0; On 2013/10/14 04:13:06, jackhou1 wrote: > On 2013/10/14 03:17:10, tapted wrote: > > not sure if visual studio is happy with inline initializers yet.. > > This type of declaration appears occassionally in chrome/. Sweet - you're right! I think I sheriffed a link error from one of these recently, but maybe its address was being taken as well.. https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.cc File apps/shell_window.cc (right): https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.cc#newco... apps/shell_window.cc:101: gfx::Size ShellWindow::SizeConstraints::maximum_size() const { drat- I missed that this is not a simple accessor. Should we call these Get{Max,Min}imumSize? https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.cc#newco... apps/shell_window.cc:159: size_constraints_ = SizeConstraints(new_params.minimum_size, move this to the next CL too? https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.cc#newco... apps/shell_window.cc:661: const ShellWindow::CreateParams& create_params) const { nit: the ShellWindow:: prefix on the create_params argument is redundant (but sadly not redundant on the return type). Since ShellWindow::Init doesn't have it, we should do the same here I guess. https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.cc#newco... apps/shell_window.cc:662: CreateParams params(create_params); You could also pass-by-value, call the argument `params` and skip this line. I read an article once that you could get better code generation this way while still playing nice with NRVO. But I think it only applies when the function is inlineable.. #uptoyou https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.cc#newco... apps/shell_window.cc:698: params.bounds = size_constraints.ClampBounds(bounds); this could then become new_params.bounds.set_size(size_constraints_.ClampSize(bounds.size()); https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.h File apps/shell_window.h (right): https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.h#newcod... apps/shell_window.h:363: CreateParams LoadDefaultsAndConstrain( Hm.. the non-overrides are already mixed in with a bunch of overrides. Since this is part of `Init`, and const, I feel stronger about it being at the top of the private: section. Same argument applies for `AdjustBoundsToBeVisibleOnScreen` though. But really all the private non-virtuals should be at the start of the private: section. The function order is all messed up in the .cc as well. Ummm.. I'd suggest moving this to come after the existing AdjustBoundsToBeVisibleOnScreen for now -- likewise in the .cc file so there's not a static function mixed in. Then, things can be organised properly in a follow-up (which would keep the two functions together still, just put them somewhere more appropriate). https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.h#newcod... apps/shell_window.h:423: SizeConstraints size_constraints_; maybe move this to the next CL
https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.cc File apps/shell_window.cc (right): https://codereview.chromium.org/26751003/diff/3001/apps/shell_window.cc#newco... apps/shell_window.cc:78: result.set_width(std::max(result.width(), minimum_size().width())); On 2013/10/14 05:13:43, tapted wrote: > On 2013/10/14 04:13:06, jackhou1 wrote: > > On 2013/10/14 03:17:10, tapted wrote: > > > I think this and the next line is just `result.SetToMax(minimum_size)` > > > > SetToMax is in gfx::Size but not gfx::Rect. > > Ah. Drat. But... what about changing the function signature to accept and return > gfx::Size? Since, it's not changing the Rect origin.. and the class is a > `SizeConstraints` not a `BoundsConstraints` ;). Done. https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.cc File apps/shell_window.cc (right): https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.cc#newco... apps/shell_window.cc:101: gfx::Size ShellWindow::SizeConstraints::maximum_size() const { On 2013/10/14 05:13:43, tapted wrote: > drat- I missed that this is not a simple accessor. Should we call these > Get{Max,Min}imumSize? Done. https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.cc#newco... apps/shell_window.cc:159: size_constraints_ = SizeConstraints(new_params.minimum_size, On 2013/10/14 05:13:43, tapted wrote: > move this to the next CL too? Done. https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.cc#newco... apps/shell_window.cc:661: const ShellWindow::CreateParams& create_params) const { On 2013/10/14 05:13:43, tapted wrote: > nit: the ShellWindow:: prefix on the create_params argument is redundant (but > sadly not redundant on the return type). Since ShellWindow::Init doesn't have > it, we should do the same here I guess. Done. https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.cc#newco... apps/shell_window.cc:662: CreateParams params(create_params); On 2013/10/14 05:13:43, tapted wrote: > You could also pass-by-value, call the argument `params` and skip this line. I > read an article once that you could get better code generation this way while > still playing nice with NRVO. But I think it only applies when the function is > inlineable.. #uptoyou Done. https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.cc#newco... apps/shell_window.cc:698: params.bounds = size_constraints.ClampBounds(bounds); On 2013/10/14 05:13:43, tapted wrote: > this could then become > > new_params.bounds.set_size(size_constraints_.ClampSize(bounds.size()); Done. https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.h File apps/shell_window.h (right): https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.h#newcod... apps/shell_window.h:363: CreateParams LoadDefaultsAndConstrain( On 2013/10/14 05:13:43, tapted wrote: > Hm.. the non-overrides are already mixed in with a bunch of overrides. Since > this is part of `Init`, and const, I feel stronger about it being at the top of > the private: section. Same argument applies for > `AdjustBoundsToBeVisibleOnScreen` though. > > But really all the private non-virtuals should be at the start of the private: > section. The function order is all messed up in the .cc as well. > > Ummm.. I'd suggest moving this to come after the existing > AdjustBoundsToBeVisibleOnScreen for now -- likewise in the .cc file so there's > not a static function mixed in. Then, things can be organised properly in a > follow-up (which would keep the two functions together still, just put them > somewhere more appropriate). Done. https://codereview.chromium.org/26751003/diff/8001/apps/shell_window.h#newcod... apps/shell_window.h:423: SizeConstraints size_constraints_; On 2013/10/14 05:13:43, tapted wrote: > maybe move this to the next CL Done.
+chrome-apps-syd-reviews [missed it until now!] nice - lgtm. Perhaps worth updating the CL description, and referencing the BUG= for the API change, since part of this is a precursor for that. https://codereview.chromium.org/26751003/diff/14001/apps/shell_window.cc File apps/shell_window.cc (right): https://codereview.chromium.org/26751003/diff/14001/apps/shell_window.cc#newc... apps/shell_window.cc:72: if (GetMaximumSize().width() != kUnboundedSize) nit: (since it does a bit of work), perhaps a temporary, const gfx::Size current_max_size = GetMaximumSize(); https://codereview.chromium.org/26751003/diff/14001/apps/shell_window.h File apps/shell_window.h (right): https://codereview.chromium.org/26751003/diff/14001/apps/shell_window.h#newco... apps/shell_window.h:357: // Loads the appropriate default or cached window bounds and constraints them nit: constraints -> constrains (or clamps)
https://codereview.chromium.org/26751003/diff/14001/apps/shell_window.cc File apps/shell_window.cc (right): https://codereview.chromium.org/26751003/diff/14001/apps/shell_window.cc#newc... apps/shell_window.cc:72: if (GetMaximumSize().width() != kUnboundedSize) On 2013/10/15 03:33:04, tapted wrote: > nit: (since it does a bit of work), perhaps a temporary, > > const gfx::Size current_max_size = GetMaximumSize(); Done. https://codereview.chromium.org/26751003/diff/14001/apps/shell_window.h File apps/shell_window.h (right): https://codereview.chromium.org/26751003/diff/14001/apps/shell_window.h#newco... apps/shell_window.h:357: // Loads the appropriate default or cached window bounds and constraints them On 2013/10/15 03:33:04, tapted wrote: > nit: constraints -> constrains (or clamps) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/26751003/24001
Message was sent while issue was closed.
Change committed as 228654
Message was sent while issue was closed.
lgtm
On Oct 17, 2013 6:55 PM, "Charles Radloff" <mr.c.radloff@gmail.com> wrote: > > 120312<3 > > On Oct 17, 2013 2:12 PM, "Charles Radloff" <mr.c.radloff@gmail.com> wrote: > > > > ---------- Forwarded message ---------- > > From: "Mail Delivery Subsystem" <mailer-daemon@googlemail.com> > > Date: Oct 15, 2013 11:37 PM > > Subject: Delivery Status Notification (Failure) > > To: <mr.c.radloff@gmail.com> > > Cc: > > > > Delivery to the following recipient failed permanently: > > > > email@locatebigdiscounts.com > > > > Technical details of permanent failure: > > Google tried to deliver your message, but it was rejected by the server for the recipient domain locatebigdiscounts.com by mxa.mailgun.org. [198.61.253.48]. > > > > The error that the other server returned was: > > 550 5.1.0 Recipient rejected: <email@locatebigdiscounts.com> > > > > ----- Original message ----- > > > > DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; > > d=gmail.com; s=20120113; > > h=mime-version:in-reply-to:references:date:message-id:subject:from:to > > :content-type; > > bh=/8yVZdyJyTatWUzmFFCLOARgmTXR54vhFJG4rxkLHOw=; > > b=OgDpIyAlM2RI3AwOrg02542/vXwMtMCE1v+aexbiXE7bIvBF9gUL/IFYcfhSozBLO1 > > 0FTsiH6ekMWVnjCDCrDaxd+T0HbPvE+PuW1XbWFTyh5uJgyXSoJDN4WgVcl6GdYaLlkY > > ni0f65n3JS3CzeFtdcwVOejld1OD6tL/jv05O4laZ1Xy7/Q6PNJG8hLb79Dn5CzCfnL7 > > hXtrwodQIIdw1K4Zbxf9Gh/tX8f/mUEsxiEhOdg76k8CXMDopJQzTG5TKSsktpHPaS60 > > D4DZDIhDXNF6+HNfN2iKPsNgbMPXJ19N7hayfUOOxy0KTVvNwvZhLRLXGdUIjnG+eCtl > > YLgA== > > MIME-Version: 1.0 > > X-Received: by 10.180.104.34 with SMTP id gb2mr22586838wib.3.1381894675247; > > Tue, 15 Oct 2013 20:37:55 -0700 (PDT) > > Received: by 10.227.27.132 with HTTP; Tue, 15 Oct 2013 20:37:55 -0700 (PDT) > > Received: by 10.227.27.132 with HTTP; Tue, 15 Oct 2013 20:37:55 -0700 (PDT) > > In-Reply-To: <20131015201603.23951.99082@locatebigdiscounts.com> > > References: <20131015201603.23951.99082@locatebigdiscounts.com> > > Date: Tue, 15 Oct 2013 23:37:55 -0400 > > Message-ID: <CABFJ9ruebtbiZPde5t7+1FqMtnGLgzTZh+Ty7-p+bW+ ykPxsaw@mail.gmail.com> > > Subject: =?UTF-8?Q?Re=3A_=E2=9D=A4_Jeep_Wrangler_=E2=9D=A4_Clearance_Sale_Today=21?= > > From: Charles Radloff <mr.c.radloff@gmail.com> > > To: LocateBigDiscounts <email@locatebigdiscounts.com> > > Content-Type: multipart/alternative; boundary=f46d044271b03c38a204e8d36d65 > > > > On Oct 15, 2013 11:17 PM, "LocateBigDiscounts" < email@locatebigdiscounts.com> > > wrote: > > > > > ** > > > 2013 Jeep Wrangler inventory selloff now happening near Marine City 123 > > > *FindACar*<http://locatebigdiscounts.com/pb/rd/ cfac/offers/email?make=Jeep&model=Wrangler&zipcode=48039& edmid=357197695&id=625&arrival_id=311457548&lead_id=1233001081&intent=used& service=162&template=CFAC_category_icons_images.html> > > > New Cars > > > < http://locatebigdiscounts.com/pb/rd/cfac/offers/email? zipcode=48039&edmid=357197695&id=625&arrival_id=311457548& lead_id=1233001081&intent=new&service=162&searchquery= newcars&imgnewcarsclick=yes&template=CFAC_category_icons_images.html> > > > Used Cars > > > < http://locatebigdiscounts.com/pb/rd/cfac/offers/email? zipcode=48039&edmid=357197695&id=625&arrival_id=311457548& lead_id=1233001081&intent=used&service=162&searchquery= usedcars&imgusedcarsclick=yes&template=CFAC_category_icons_images.html> > > > [image: JeepWrangler Deals]<http://locatebigdiscounts.com/pb/rd/ cfac/offers/email?make=Jeep&model=Wrangler&zipcode=48039& edmid=357197695&id=625&arrival_id=311457548&lead_id=1233001081&intent=used& service=162&imgclick=yes&template=CFAC_category_icons_images.html> > > > Best Deals By Vehicle Type: [image: SUV] SUV > > > < http://locatebigdiscounts.com/pb/rd/cfac/offers/email? zipcode=48039&edmid=357197695&id=625&intent=used&service= 162&template=LBD_category_icons_images_dynamic.html& imgclickgmc=yes&searchquery=SUVs> > > > [image: Luxury] Luxury > > > < http://locatebigdiscounts.com/pb/rd/cfac/offers/email? zipcode=48039&edmid=357197695&id=625&intent=used&service= 162&template=LBD_category_icons_images_dynamic.html& imgclickgmc=yes&searchquery=Luxury%20Vehicles> > > > [image: Hybrid] Hybrid > > > < http://locatebigdiscounts.com/pb/rd/cfac/offers/email? zipcode=48039&edmid=357197695&id=625&intent=used&service= 162&template=LBD_category_icons_images_dynamic.html& imgclickgmc=yes&searchquery=Hybrids> > > > [image: Diesel] Diesel > > > < http://locatebigdiscounts.com/pb/rd/cfac/offers/email? zipcode=48039&edmid=357197695&i > > https://m.facebook.com/home.php?refsrc=https%3A%2F%2Fm. facebook.com%2Ftorsten.rambaum&refid=8&_rdr > > ----- Message truncated ----- > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
