Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7846010:
Fixed crash when a SinglesplitView gets embdedded inside SingleSplitView (Closed)
|
Created:
9 years, 3 months ago by Gajen Modified:
9 years ago CC:
chromium-reviews, dhollowa Visibility:
Public. |
DescriptionFixed crash when a SinglesplitView gets embdedded inside SingleSplitView
BUG=none
TEST=added test example double_split_view in views/examples
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101298
Patch Set 1 #
Total comments: 19
Patch Set 2 : incorporated all review comments by Sky and Tfarina #
Total comments: 10
Patch Set 3 : Incorporated Reviews comments by Tfarina and Oshima #Patch Set 4 : Comments incorporated as per Sky review #
Total comments: 2
Patch Set 5 : Incorporated final comment #Patch Set 6 : Sync up with trunk #
Total comments: 2
Patch Set 7 : Examples order changed #
Messages
Total messages: 37 (0 generated)
Hi, I would like you guys to review my patch.
http://codereview.chromium.org/7846010/diff/1/views/controls/single_split_vie... File views/controls/single_split_view.cc (right): http://codereview.chromium.org/7846010/diff/1/views/controls/single_split_vie... views/controls/single_split_view.cc:253: return std::min(divider_offset, Put the std::max call here, eg std::max(0, divider_offset) ... http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... File views/examples/double_split_view_example.cc (right): http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... views/examples/double_split_view_example.cc:47: remove one of these lines. http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... views/examples/double_split_view_example.cc:70: SizeToPreferredSize(); Is this really necessary? Layout is mean to size your children, not your own bounds. http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... File views/examples/double_split_view_example.h (right): http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... views/examples/double_split_view_example.h:20: virtual std::wstring GetExampleTitle(); OVERRIDE on both of these. http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... views/examples/double_split_view_example.h:33: virtual gfx::Size GetPreferredSize(); OVERRIDE on all of these. http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... views/examples/double_split_view_example.h:37: remove this. http://codereview.chromium.org/7846010/diff/1/views/examples/examples_main.cc File views/examples/examples_main.cc (right): http://codereview.chromium.org/7846010/diff/1/views/examples/examples_main.cc... views/examples/examples_main.cc:169: remove both of these lines.
http://codereview.chromium.org/7846010/diff/1/views/controls/single_split_vie... File views/controls/single_split_view.cc (right): http://codereview.chromium.org/7846010/diff/1/views/controls/single_split_vie... views/controls/single_split_view.cc:249: // Prevents negative check fail in gfx:Size()for embedded SingleSplitView add space between gfx::Size() and for and "for an embedded SingleSplitView." ? http://codereview.chromium.org/7846010/diff/1/views/controls/single_split_vie... views/controls/single_split_view.cc:251: return std::max(0, ((primary_axis_size - kDividerSize) / 2)); is there an extra (unnecessary) parentheses around the second argument of std::max?! http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... File views/examples/double_split_view_example.h (right): http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... views/examples/double_split_view_example.h:25: class SplittedView : public views::View { Could you move this class to an unnamed namespace in the source file instead?
Sky, Tfarina Thank you very much for quick review comments. Incorporated both of your comments in one single patch, hope it doesn't break any guidelines. http://codereview.chromium.org/7846010/diff/1/views/controls/single_split_vie... File views/controls/single_split_view.cc (right): http://codereview.chromium.org/7846010/diff/1/views/controls/single_split_vie... views/controls/single_split_view.cc:249: // Prevents negative check fail in gfx:Size()for embedded SingleSplitView On 2011/09/08 20:39:45, tfarina wrote: > add space between gfx::Size() and for > > and "for an embedded SingleSplitView." ? Done. http://codereview.chromium.org/7846010/diff/1/views/controls/single_split_vie... views/controls/single_split_view.cc:251: return std::max(0, ((primary_axis_size - kDividerSize) / 2)); On 2011/09/08 20:39:45, tfarina wrote: > is there an extra (unnecessary) parentheses around the second argument of > std::max?! Done. http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... File views/examples/double_split_view_example.cc (right): http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... views/examples/double_split_view_example.cc:47: On 2011/09/08 14:27:11, sky wrote: > remove one of these lines. Done. http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... views/examples/double_split_view_example.cc:70: SizeToPreferredSize(); On 2011/09/08 14:27:11, sky wrote: > Is this really necessary? Layout is mean to size your children, not your own > bounds. Right, will remove this http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... File views/examples/double_split_view_example.h (right): http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... views/examples/double_split_view_example.h:20: virtual std::wstring GetExampleTitle(); On 2011/09/08 14:27:11, sky wrote: > OVERRIDE on both of these. Done. http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... views/examples/double_split_view_example.h:25: class SplittedView : public views::View { On 2011/09/08 20:39:45, tfarina wrote: > Could you move this class to an unnamed namespace in the source file instead? Done. http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... views/examples/double_split_view_example.h:33: virtual gfx::Size GetPreferredSize(); On 2011/09/08 14:27:11, sky wrote: > OVERRIDE on all of these. Done. http://codereview.chromium.org/7846010/diff/1/views/examples/double_split_vie... views/examples/double_split_view_example.h:37: On 2011/09/08 14:27:11, sky wrote: > remove this. Done. http://codereview.chromium.org/7846010/diff/1/views/examples/examples_main.cc File views/examples/examples_main.cc (right): http://codereview.chromium.org/7846010/diff/1/views/examples/examples_main.cc... views/examples/examples_main.cc:169: On 2011/09/08 14:27:11, sky wrote: > remove both of these lines. Done.
views/examples LGTM, but sky should approve as OWNER. http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... File views/examples/double_split_view_example.h (right): http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... views/examples/double_split_view_example.h:9: #include "views/controls/single_split_view.h" Please, instead of including this here, just forward declare it. namespace views { class SingleSplitView; } http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... views/examples/double_split_view_example.h:20: virtual std::wstring GetExampleTitle() OVERRIDE; could you include base/compiler_specific.h for OVERRIDE? http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... views/examples/double_split_view_example.h:28: DISALLOW_COPY_AND_ASSIGN(DoubleSplitViewExample); could you include base/basictypes.h for DISALLOW?
LGTM with the following change. http://codereview.chromium.org/7846010/diff/8001/views/controls/single_split_... File views/controls/single_split_view.cc (right): http://codereview.chromium.org/7846010/diff/8001/views/controls/single_split_... views/controls/single_split_view.cc:249: // Prevents negative check fail in gfx::Size() for an embedded For a comment, how about 'primary_axis_size may < kDividerSize during initial layout'
LGTM once you addressed sky/tfarina's comment. http://codereview.chromium.org/7846010/diff/8001/views/examples/examples_main.cc File views/examples/examples_main.cc (right): http://codereview.chromium.org/7846010/diff/8001/views/examples/examples_main... views/examples/examples_main.cc:111: remove extra empty line
Hi, Incorporated Tfarina and Oshima review comments. Sky, I have few thoughts on your reviews comments below. Please let me know if it makes sense...?? http://codereview.chromium.org/7846010/diff/8001/views/controls/single_split_... File views/controls/single_split_view.cc (right): http://codereview.chromium.org/7846010/diff/8001/views/controls/single_split_... views/controls/single_split_view.cc:249: // Prevents negative check fail in gfx::Size() for an embedded On 2011/09/09 15:32:23, sky wrote: > For a comment, how about 'primary_axis_size may < kDividerSize during initial > layout' Agreed but it won't add more value b'coz 0)Adds one more flag and a condition which gets checked every time in subsequent Layout and MousePressed events too. 1)Currently there is no other code path than this Method(NormalizeDividerOffset) which depends on this condition. This way we save bit space and few instructions to be executed. I guess, if in future more dependency comes on this condition then we may think of adding this condition. Please let me know your comments/suggestions http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... File views/examples/double_split_view_example.h (right): http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... views/examples/double_split_view_example.h:9: #include "views/controls/single_split_view.h" On 2011/09/09 15:15:31, tfarina wrote: > Please, instead of including this here, just forward declare it. > > namespace views { > class SingleSplitView; > } Done. http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... views/examples/double_split_view_example.h:20: virtual std::wstring GetExampleTitle() OVERRIDE; On 2011/09/09 15:15:31, tfarina wrote: > could you include base/compiler_specific.h for OVERRIDE? Done. http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... views/examples/double_split_view_example.h:28: DISALLOW_COPY_AND_ASSIGN(DoubleSplitViewExample); On 2011/09/09 15:15:31, tfarina wrote: > could you include base/basictypes.h for DISALLOW? Done. http://codereview.chromium.org/7846010/diff/8001/views/examples/examples_main.cc File views/examples/examples_main.cc (right): http://codereview.chromium.org/7846010/diff/8001/views/examples/examples_main... views/examples/examples_main.cc:111: On 2011/09/09 17:50:27, oshima wrote: > remove extra empty line Done.
On Mon, Sep 12, 2011 at 8:13 AM, <wxjg68@motorola.com> wrote: > Hi, > Incorporated Tfarina and Oshima review comments. > > Sky, I have few thoughts on your reviews comments below. Please let me know > if > it makes sense...?? > > > http://codereview.chromium.org/7846010/diff/8001/views/controls/single_split_... > File views/controls/single_split_view.cc (right): > > http://codereview.chromium.org/7846010/diff/8001/views/controls/single_split_... > views/controls/single_split_view.cc:249: // Prevents negative check fail > in gfx::Size() for an embedded > On 2011/09/09 15:32:23, sky wrote: >> >> For a comment, how about 'primary_axis_size may < kDividerSize during > > initial >> >> layout' > > Agreed but it won't add more value b'coz > 0)Adds one more flag and a condition which gets checked every time in > subsequent Layout and MousePressed events too. > > 1)Currently there is no other code path than this > Method(NormalizeDividerOffset) which depends on this condition. I wasn't suggesting a new branch, only changing the comment. It's not the gfx::Size check you want to fix, rather that divider offset should always be >= 0. That's why your comment isn't particularly helpful. -Scott > This way we save bit space and few instructions to be executed. > > I guess, if in future more dependency comes on this condition then we > may think of adding this condition. > > Please let me know your comments/suggestions > > http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... > File views/examples/double_split_view_example.h (right): > > http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... > views/examples/double_split_view_example.h:9: #include > "views/controls/single_split_view.h" > On 2011/09/09 15:15:31, tfarina wrote: >> >> Please, instead of including this here, just forward declare it. > >> namespace views { >> class SingleSplitView; >> } > > Done. > > http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... > views/examples/double_split_view_example.h:20: virtual std::wstring > GetExampleTitle() OVERRIDE; > On 2011/09/09 15:15:31, tfarina wrote: >> >> could you include base/compiler_specific.h for OVERRIDE? > > Done. > > http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... > views/examples/double_split_view_example.h:28: > DISALLOW_COPY_AND_ASSIGN(DoubleSplitViewExample); > On 2011/09/09 15:15:31, tfarina wrote: >> >> could you include base/basictypes.h for DISALLOW? > > Done. > > http://codereview.chromium.org/7846010/diff/8001/views/examples/examples_main.cc > File views/examples/examples_main.cc (right): > > http://codereview.chromium.org/7846010/diff/8001/views/examples/examples_main... > views/examples/examples_main.cc:111: > On 2011/09/09 17:50:27, oshima wrote: >> >> remove extra empty line > > Done. > > http://codereview.chromium.org/7846010/ >
On 2011/09/12 15:56:41, sky_google.com wrote: > On Mon, Sep 12, 2011 at 8:13 AM, <mailto:wxjg68@motorola.com> wrote: > > Hi, > > Incorporated Tfarina and Oshima review comments. > > > > Sky, I have few thoughts on your reviews comments below. Please let me know > > if > > it makes sense...?? > > > > > > > http://codereview.chromium.org/7846010/diff/8001/views/controls/single_split_... > > File views/controls/single_split_view.cc (right): > > > > > http://codereview.chromium.org/7846010/diff/8001/views/controls/single_split_... > > views/controls/single_split_view.cc:249: // Prevents negative check fail > > in gfx::Size() for an embedded > > On 2011/09/09 15:32:23, sky wrote: > >> > >> For a comment, how about 'primary_axis_size may < kDividerSize during > > > > initial > >> > >> layout' > > > > Agreed but it won't add more value b'coz > > 0)Adds one more flag and a condition which gets checked every time in > > subsequent Layout and MousePressed events too. > > > > 1)Currently there is no other code path than this > > Method(NormalizeDividerOffset) which depends on this condition. > > I wasn't suggesting a new branch, only changing the comment. It's not > the gfx::Size check you want to fix, rather that divider offset should > always be >= 0. That's why your comment isn't particularly helpful. > > -Scott Yeah,agreed with you. So, I assume the final comment would be 'primary_axis_size may < kDividerSize during initial layout' INSTEAD of 'Prevents negative check fail in gfx::Size() for an embedded SingleSplitView on very first Layout'. Please let me know the correction before I publish the patch. > > > This way we save bit space and few instructions to be executed. > > > > I guess, if in future more dependency comes on this condition then we > > may think of adding this condition. > > > > Please let me know your comments/suggestions > > > > > http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... > > File views/examples/double_split_view_example.h (right): > > > > > http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... > > views/examples/double_split_view_example.h:9: #include > > "views/controls/single_split_view.h" > > On 2011/09/09 15:15:31, tfarina wrote: > >> > >> Please, instead of including this here, just forward declare it. > > > >> namespace views { > >> class SingleSplitView; > >> } > > > > Done. > > > > > http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... > > views/examples/double_split_view_example.h:20: virtual std::wstring > > GetExampleTitle() OVERRIDE; > > On 2011/09/09 15:15:31, tfarina wrote: > >> > >> could you include base/compiler_specific.h for OVERRIDE? > > > > Done. > > > > > http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... > > views/examples/double_split_view_example.h:28: > > DISALLOW_COPY_AND_ASSIGN(DoubleSplitViewExample); > > On 2011/09/09 15:15:31, tfarina wrote: > >> > >> could you include base/basictypes.h for DISALLOW? > > > > Done. > > > > > http://codereview.chromium.org/7846010/diff/8001/views/examples/examples_main.cc > > File views/examples/examples_main.cc (right): > > > > > http://codereview.chromium.org/7846010/diff/8001/views/examples/examples_main... > > views/examples/examples_main.cc:111: > > On 2011/09/09 17:50:27, oshima wrote: > >> > >> remove extra empty line > > > > Done. > > > > http://codereview.chromium.org/7846010/ > >
On Mon, Sep 12, 2011 at 10:30 PM, <wxjg68@motorola.com> wrote: > On 2011/09/12 15:56:41, sky_google.com wrote: >> >> On Mon, Sep 12, 2011 at 8:13 AM, <mailto:wxjg68@motorola.com> wrote: >> > Hi, >> > Incorporated Tfarina and Oshima review comments. >> > >> > Sky, I have few thoughts on your reviews comments below. Please let me >> > know >> > if >> > it makes sense...?? >> > >> > >> > > > http://codereview.chromium.org/7846010/diff/8001/views/controls/single_split_... >> >> > File views/controls/single_split_view.cc (right): >> > >> > > > http://codereview.chromium.org/7846010/diff/8001/views/controls/single_split_... >> >> > views/controls/single_split_view.cc:249: // Prevents negative check fail >> > in gfx::Size() for an embedded >> > On 2011/09/09 15:32:23, sky wrote: >> >> >> >> For a comment, how about 'primary_axis_size may < kDividerSize during >> > >> > initial >> >> >> >> layout' >> > >> > Agreed but it won't add more value b'coz >> > 0)Adds one more flag and a condition which gets checked every time in >> > subsequent Layout and MousePressed events too. >> > >> > 1)Currently there is no other code path than this >> > Method(NormalizeDividerOffset) which depends on this condition. > >> I wasn't suggesting a new branch, only changing the comment. It's not >> the gfx::Size check you want to fix, rather that divider offset should >> always be >= 0. That's why your comment isn't particularly helpful. > >> -Scott > > Yeah,agreed with you. > > So, I assume the final comment would be 'primary_axis_size may < > kDividerSize > during initial layout' INSTEAD of 'Prevents negative check fail in > gfx::Size() > for an embedded SingleSplitView on very first Layout'. > Please let me know the correction before I publish the patch. Yes. -Scott > > > >> > This way we save bit space and few instructions to be executed. >> > >> > I guess, if in future more dependency comes on this condition then we >> > may think of adding this condition. >> > >> > Please let me know your comments/suggestions >> > >> > > > http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... >> >> > File views/examples/double_split_view_example.h (right): >> > >> > > > http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... >> >> > views/examples/double_split_view_example.h:9: #include >> > "views/controls/single_split_view.h" >> > On 2011/09/09 15:15:31, tfarina wrote: >> >> >> >> Please, instead of including this here, just forward declare it. >> > >> >> namespace views { >> >> class SingleSplitView; >> >> } >> > >> > Done. >> > >> > > > http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... >> >> > views/examples/double_split_view_example.h:20: virtual std::wstring >> > GetExampleTitle() OVERRIDE; >> > On 2011/09/09 15:15:31, tfarina wrote: >> >> >> >> could you include base/compiler_specific.h for OVERRIDE? >> > >> > Done. >> > >> > > > http://codereview.chromium.org/7846010/diff/8001/views/examples/double_split_... >> >> > views/examples/double_split_view_example.h:28: >> > DISALLOW_COPY_AND_ASSIGN(DoubleSplitViewExample); >> > On 2011/09/09 15:15:31, tfarina wrote: >> >> >> >> could you include base/basictypes.h for DISALLOW? >> > >> > Done. >> > >> > > > http://codereview.chromium.org/7846010/diff/8001/views/examples/examples_main.cc >> >> > File views/examples/examples_main.cc (right): >> > >> > > > http://codereview.chromium.org/7846010/diff/8001/views/examples/examples_main... >> >> > views/examples/examples_main.cc:111: >> > On 2011/09/09 17:50:27, oshima wrote: >> >> >> >> remove extra empty line >> > >> > Done. >> > >> > http://codereview.chromium.org/7846010/ >> > > > > > http://codereview.chromium.org/7846010/ >
Sky, Incorporated you comments.
lgtm http://codereview.chromium.org/7846010/diff/19001/views/controls/single_split... File views/controls/single_split_view.cc (right): http://codereview.chromium.org/7846010/diff/19001/views/controls/single_split... views/controls/single_split_view.cc:249: // primary_axis_size may < kDividerSize during initial layout end with a '.'.
Hi, Sky. Should I submit one more patch.
Yes, updated it then I'll make sure it passes the try bots and land it.
http://codereview.chromium.org/7846010/diff/19001/views/controls/single_split... File views/controls/single_split_view.cc (right): http://codereview.chromium.org/7846010/diff/19001/views/controls/single_split... views/controls/single_split_view.cc:249: // primary_axis_size may < kDividerSize during initial layout On 2011/09/14 15:01:16, sky wrote: > end with a '.'. Done.
Your patch no longer cleanly applies, can you sync to trunk and update it.
Hi, Sky and All. Did sync with trunk but now can not do "git cl upload", got below errors: ///////////////////////////////// ** Presubmit ERRORS ** views/examples/double_split_view_example.cc, line 59: new code should not use wstrings. If you are calling an API that accepts a wstring, fix the API. views/examples/double_split_view_example.h, line 25: new code should not use wstrings. If you are calling an API that accepts a wstring, fix the API. ////////////////////////////////////////// As ExampleBase class defines GetExampleTitle() as a pure virtual function, virtual std::wstring GetExampleTitle() = 0; So, changing that would break all other views examples. Do we have some tweak to pass this patch in, later I can take up a new CL to convert the wstring issue.
On 2011/09/15 12:48:38, Gajen wrote: > Hi, Sky and All. > Did sync with trunk but now can not do "git cl upload", got below errors: > ///////////////////////////////// > ** Presubmit ERRORS ** > views/examples/double_split_view_example.cc, line 59: new code should not use > wstrings. If you are calling an API that accepts a wstring, fix the API. > > views/examples/double_split_view_example.h, line 25: new code should not use > wstrings. If you are calling an API that accepts a wstring, fix the API. > ////////////////////////////////////////// > As ExampleBase class defines GetExampleTitle() as a pure virtual function, > virtual std::wstring GetExampleTitle() = 0; > > So, changing that would break all other views examples. > Just skip the presubmit errors for now. > Do we have some tweak to pass this patch in, later I can take up a new CL to > convert the wstring issue. I plan to convert this API to string16, but if you can take it first that is fine too.
On 2011/09/15 13:04:48, tfarina wrote: > On 2011/09/15 12:48:38, Gajen wrote: > > Hi, Sky and All. > > Did sync with trunk but now can not do "git cl upload", got below errors: > > ///////////////////////////////// > > ** Presubmit ERRORS ** > > views/examples/double_split_view_example.cc, line 59: new code should not use > > wstrings. If you are calling an API that accepts a wstring, fix the API. > > > > views/examples/double_split_view_example.h, line 25: new code should not use > > wstrings. If you are calling an API that accepts a wstring, fix the API. > > ////////////////////////////////////////// > > As ExampleBase class defines GetExampleTitle() as a pure virtual function, > > virtual std::wstring GetExampleTitle() = 0; > > > > So, changing that would break all other views examples. > > > Just skip the presubmit errors for now. > > > Do we have some tweak to pass this patch in, later I can take up a new CL to > > convert the wstring issue. > > I plan to convert this API to string16, but if you can take it first that is > fine too. Yes, I will take this up..
On 2011/09/15 13:30:38, Gajen wrote: > Yes, I will take this up.. Not in this patch I suppose? It's very big and beyond the scope of this CL.
Done Sync up with trunk.
http://codereview.chromium.org/7846010/diff/24001/views/examples/examples_mai... File views/examples/examples_main.cc (right): http://codereview.chromium.org/7846010/diff/24001/views/examples/examples_mai... views/examples/examples_main.cc:119: AddExample(&double_split_view_example); Put this example after the SingleSplitView, after line 155.
On 2011/09/15 13:32:51, tfarina wrote: > On 2011/09/15 13:30:38, Gajen wrote: > > Yes, I will take this up.. > > Not in this patch I suppose? It's very big and beyond the scope of this CL. Yeah, understood. So, Would you assign a new BUG to me or this CL should be linked to http://code.google.com/p/chromium/issues/detail?id=23581.
http://codereview.chromium.org/7846010/diff/24001/views/examples/examples_mai... File views/examples/examples_main.cc (right): http://codereview.chromium.org/7846010/diff/24001/views/examples/examples_mai... views/examples/examples_main.cc:119: AddExample(&double_split_view_example); On 2011/09/15 13:46:31, tfarina wrote: > Put this example after the SingleSplitView, after line 155. Done.
Presubmit check for 7846010-28001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** views/examples/double_split_view_example.h, line 25: new code should not use wstrings. If you are calling an API that accepts a wstring, fix the API. views/examples/double_split_view_example.cc, line 59: new code should not use wstrings. If you are calling an API that accepts a wstring, fix the API. Presubmit checks took 1.3s to calculate.
Thiago, it looks like you tried to land this but it failed because of presubmit errors. Are you going to land the old fashioned way, or should I? -Scott On Thu, Sep 15, 2011 at 7:14 AM, <commit-bot@chromium.org> wrote: > Presubmit check for 7846010-28001 failed and returned exit status 1. > > Running presubmit commit checks ... > > ** Presubmit ERRORS ** > views/examples/double_split_view_example.h, line 25: new code should not use > wstrings. If you are calling an API that accepts a wstring, fix the API. > > views/examples/double_split_view_example.cc, line 59: new code should not > use > wstrings. If you are calling an API that accepts a wstring, fix the API. > > Presubmit checks took 1.3s to calculate. > > > > http://codereview.chromium.org/7846010/ >
Hi, Sky. Would this be committed now..!!!
On 2011/09/15 14:53:21, sky_google.com wrote: > Thiago, it looks like you tried to land this but it failed because of > presubmit errors. Are you going to land the old fashioned way, or > should I? No, I didn't check the commit-box. I can try to land this manually for him.
On 2011/09/15 14:53:21, http://sky_google.com wrote: > Are you going to land the old fashioned way, or should I? I can land this patch from here http://codereview.chromium.org/7911001/
On 2011/09/15 14:53:21, sky_google.com wrote: > Thiago, it looks like you tried to land this but it failed because of > presubmit errors. Are you going to land the old fashioned way, or > should I? > > -Scott Please do the needful. Just out of curiosity, I tried Non-chromium committer option as per this link http://www.chromium.org/developers/testing/commit-queue. Now burnt my fingers.. > > On Thu, Sep 15, 2011 at 7:14 AM, <mailto:commit-bot@chromium.org> wrote: > > Presubmit check for 7846010-28001 failed and returned exit status 1. > > > > Running presubmit commit checks ... > > > > ** Presubmit ERRORS ** > > views/examples/double_split_view_example.h, line 25: new code should not use > > wstrings. If you are calling an API that accepts a wstring, fix the API. > > > > views/examples/double_split_view_example.cc, line 59: new code should not > > use > > wstrings. If you are calling an API that accepts a wstring, fix the API. > > > > Presubmit checks took 1.3s to calculate. > > > > > > > > http://codereview.chromium.org/7846010/ > >
Go for it. THanks, -Scott On Thu, Sep 15, 2011 at 7:56 AM, <tfarina@chromium.org> wrote: > On 2011/09/15 14:53:21, sky_google.com wrote: >> >> Thiago, it looks like you tried to land this but it failed because of >> presubmit errors. Are you going to land the old fashioned way, or >> should I? > > No, I didn't check the commit-box. I can try to land this manually for him. > > > http://codereview.chromium.org/7846010/ >
Landed in r101298. Thanks. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
