Default zoom was never read from chrome://settings

Default zoom was never read from chrome://settings

The fix basically includes reading default zoom percent of a tab  from a helper function in zoom_controller.

BUG=427440
TESTs = Go to chrome://settings
              Expand to Webcontents under Advanced Settings
              Change the default Zoom percentage
              Expected result: All the open tabs would be changed to that particular zoom   value and the "Actual Size" under View menu would be disabled.

Committed: https://crrev.com/daadc7150bd4955f19af36c67d104e68fef5d5fe
Cr-Commit-Position: refs/heads/master@{#318191}

[sarka, 2014-10-28 11:44:44]

a.sarkar.arun@gmail.com changed reviewers:
+ asvitkine@chromium.org

[sarka, 2014-10-28 11:45:34]

On 2014/10/28 11:44:44, sarka wrote:
> mailto:a.sarkar.arun@gmail.com changed reviewers:
> + mailto:asvitkine@chromium.org

PTAL.
Thanks

[Alexei Svitkine (slow), 2014-10-28 13:15:54]

https://codereview.chromium.org/678343002/diff/1/chrome/browser/ui/browser_co...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/678343002/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_commands.cc:615:
zoom_controller->GetDefaultZoomPercent();
Can't you use zoom_controller->IsAtDefaultZoom() instead of doing the same logic
here?

In fact, I find the name of that method more intuitive than this one - so maybe
rename this method to the same thing (i.e. IsAtDefaultZoom()).

[sarka, 2014-10-29 01:57:05]

On 2014/10/28 13:15:54, Alexei Svitkine wrote:
>
https://codereview.chromium.org/678343002/diff/1/chrome/browser/ui/browser_co...
> File chrome/browser/ui/browser_commands.cc (right):
> 
>
https://codereview.chromium.org/678343002/diff/1/chrome/browser/ui/browser_co...
> chrome/browser/ui/browser_commands.cc:615:
> zoom_controller->GetDefaultZoomPercent();
> Can't you use zoom_controller->IsAtDefaultZoom() instead of doing the same
logic
> here?
> 
> In fact, I find the name of that method more intuitive than this one - so
maybe
> rename this method to the same thing (i.e. IsAtDefaultZoom()).

Done. Btw CL uploaded only 2 files, and not all of them, is this a new feature?
Thanks

[Alexei Svitkine (slow), 2014-10-29 03:44:25]

https://codereview.chromium.org/678343002/diff/20001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/678343002/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands.cc:611: bool ActualSize(content::WebContents*
contents) {
I was suggesting we rename this function to also be called IsAtDefaultZoom() -
which is a better name, imho.

[sarka, 2014-10-29 14:28:23]

On 2014/10/29 03:44:25, Alexei Svitkine wrote:
>
https://codereview.chromium.org/678343002/diff/20001/chrome/browser/ui/browse...
> File chrome/browser/ui/browser_commands.cc (right):
> 
>
https://codereview.chromium.org/678343002/diff/20001/chrome/browser/ui/browse...
> chrome/browser/ui/browser_commands.cc:611: bool
ActualSize(content::WebContents*
> contents) {
> I was suggesting we rename this function to also be called IsAtDefaultZoom() -
> which is a better name, imho.

Ahh. I misunderstood the second part. Thanks for clarifying!

[sarka, 2014-10-30 02:16:34]

On 2014/10/29 14:28:23, sarka wrote:
> On 2014/10/29 03:44:25, Alexei Svitkine wrote:
> >
>
https://codereview.chromium.org/678343002/diff/20001/chrome/browser/ui/browse...
> > File chrome/browser/ui/browser_commands.cc (right):
> > 
> >
>
https://codereview.chromium.org/678343002/diff/20001/chrome/browser/ui/browse...
> > chrome/browser/ui/browser_commands.cc:611: bool
> ActualSize(content::WebContents*
> > contents) {
> > I was suggesting we rename this function to also be called IsAtDefaultZoom()
-
> > which is a better name, imho.
> 
> Ahh. I misunderstood the second part. Thanks for clarifying!

Done. I also renamed the other function names to make it consistent.
Thanks

[Alexei Svitkine (slow), 2014-10-30 13:49:42]

Please add a TEST= line to the CL description that describes how QA can verify
that the fix is working. Thanks!

https://codereview.chromium.org/678343002/diff/40001/chrome/browser/browser_c...
File chrome/browser/browser_commands_unittest.cc (right):

https://codereview.chromium.org/678343002/diff/40001/chrome/browser/browser_c...
chrome/browser/browser_commands_unittest.cc:288:
EXPECT_EQ(zoom_controller->GetZoomPercent(), 100.f);
Nit: Your only change to this file now is 100.0f -> 100.f.

That's not a meaningful change - please revert it.

https://codereview.chromium.org/678343002/diff/40001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/678343002/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands.cc:613: return
!zoom_controller->IsAtDefaultZoom();
IsAtDefaultZoom(contents) shouldn't negate the result of
zoom_controller_->IsAtDefaultZoom().

Instead, negate the result of this function in the code:
command_updater_.UpdateCommandEnabled(IDC_ZOOM_NORMAL,

[sarka, 2014-10-31 02:39:12]

On 2014/10/30 13:49:42, Alexei Svitkine wrote:
> Please add a TEST= line to the CL description that describes how QA can verify
> that the fix is working. Thanks!
> 
>
https://codereview.chromium.org/678343002/diff/40001/chrome/browser/browser_c...
> File chrome/browser/browser_commands_unittest.cc (right):
> 
>
https://codereview.chromium.org/678343002/diff/40001/chrome/browser/browser_c...
> chrome/browser/browser_commands_unittest.cc:288:
> EXPECT_EQ(zoom_controller->GetZoomPercent(), 100.f);
> Nit: Your only change to this file now is 100.0f -> 100.f.
> 
> That's not a meaningful change - please revert it.
> 
Done.
>
https://codereview.chromium.org/678343002/diff/40001/chrome/browser/ui/browse...
> File chrome/browser/ui/browser_commands.cc (right):
> 
>
https://codereview.chromium.org/678343002/diff/40001/chrome/browser/ui/browse...
> chrome/browser/ui/browser_commands.cc:613: return
> !zoom_controller->IsAtDefaultZoom();
> IsAtDefaultZoom(contents) shouldn't negate the result of
> zoom_controller_->IsAtDefaultZoom().
> 
> Instead, negate the result of this function in the code:
> command_updater_.UpdateCommandEnabled(IDC_ZOOM_NORMAL,

Done.
Thanks

[Alexei Svitkine (slow), 2014-10-31 05:16:18]

lgtm, thanks

you also need an owner approval

[sarka, 2014-10-31 14:10:28]

a.sarkar.arun@gmail.com changed reviewers:
+ sky@chromium.org

[sarka, 2014-10-31 14:10:57]

On 2014/10/31 05:16:18, Alexei Svitkine wrote:
> lgtm, thanks
> 
> you also need an owner approval

Added @sky.

PTAL.

Thanks

[sky, 2014-11-01 17:26:28]

sky@chromium.org changed reviewers:
+ wjmaclean@chromium.org

[sky, 2014-11-01 17:26:30]

How about test coverage?
+wjmaclean

[wjmaclean, 2014-11-01 17:45:47]

lgtm

https://codereview.chromium.org/678343002/diff/60001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/678343002/diff/60001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands.cc:613: return
zoom_controller->IsAtDefaultZoom();
Thanks, this is definitely preferred.

[sarka, 2014-11-03 15:30:03]

On 2014/11/01 17:45:47, wjmaclean wrote:
> lgtm
> 
>
https://codereview.chromium.org/678343002/diff/60001/chrome/browser/ui/browse...
> File chrome/browser/ui/browser_commands.cc (right):
> 
>
https://codereview.chromium.org/678343002/diff/60001/chrome/browser/ui/browse...
> chrome/browser/ui/browser_commands.cc:613: return
> zoom_controller->IsAtDefaultZoom();
> Thanks, this is definitely preferred.

@sky I would upload a test .

Thanks

[Alexei Svitkine (slow), 2014-11-13 23:21:44]

Ping, what's the status of this?

[sarka, 2014-11-13 23:37:28]

Sorry, Have been pretty busy lately. Is there an ETA? I would like to
upload a test by this weekend if that okay with you.

Thanks

On Thu, Nov 13, 2014 at 6:21 PM, <asvitkine@chromium.org> wrote:

> Ping, what's the status of this?
>
> https://codereview.chromium.org/678343002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2014-11-13 23:43:49]

This weekend sounds good, thanks!

On Thu, Nov 13, 2014 at 6:37 PM, arunoday sarkar <a.sarkar.arun@gmail.com>
wrote:

> Sorry, Have been pretty busy lately. Is there an ETA? I would like to
> upload a test by this weekend if that okay with you.
>
> Thanks
>
> On Thu, Nov 13, 2014 at 6:21 PM, <asvitkine@chromium.org> wrote:
>
>> Ping, what's the status of this?
>>
>> https://codereview.chromium.org/678343002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sarka, 2014-11-16 18:51:33]

On 2014/11/13 23:43:49, Alexei Svitkine wrote:
> This weekend sounds good, thanks!
> 
> On Thu, Nov 13, 2014 at 6:37 PM, arunoday sarkar
<mailto:a.sarkar.arun@gmail.com>
> wrote:
> 
> > Sorry, Have been pretty busy lately. Is there an ETA? I would like to
> > upload a test by this weekend if that okay with you.
> >
> > Thanks
> >
> > On Thu, Nov 13, 2014 at 6:21 PM, <mailto:asvitkine@chromium.org> wrote:
> >
> >> Ping, what's the status of this?
> >>
> >> https://codereview.chromium.org/678343002/
> >>
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Upload a test. PTAL

Thanks

[Alexei Svitkine (slow), 2014-11-17 01:25:20]

lgtm, thanks!

[tfarina, 2014-11-17 01:57:11]

tfarina@chromium.org changed reviewers:
+ tfarina@chromium.org

[tfarina, 2014-11-17 01:57:12]

https://codereview.chromium.org/678343002/diff/100001/chrome/test/base/testin...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/678343002/diff/100001/chrome/test/base/testin...
chrome/test/base/testing_profile.h:14: #include
"chrome/browser/ui/zoom/chrome_zoom_level_prefs.h"
why did you include this here? for ChromeZoomLevelPrefs pointer? If it is, I'm
sure you don't need this include.

Could you try removing it and see if it still compiles?

Thanks,

[sarka, 2014-11-17 02:01:21]

On 2014/11/17 01:57:12, tfarina wrote:
>
https://codereview.chromium.org/678343002/diff/100001/chrome/test/base/testin...
> File chrome/test/base/testing_profile.h (right):
> 
>
https://codereview.chromium.org/678343002/diff/100001/chrome/test/base/testin...
> chrome/test/base/testing_profile.h:14: #include
> "chrome/browser/ui/zoom/chrome_zoom_level_prefs.h"
> why did you include this here? for ChromeZoomLevelPrefs pointer? If it is, I'm
> sure you don't need this include.
> 
> Could you try removing it and see if it still compiles?
> 
It doesn't compile removing those headers.
I get these errors
../../chrome/test/base/testing_profile.cc:641:11: error: allocation of
incomplete type 'chrome::ChromeZoomLevelPrefs'
      new chrome::ChromeZoomLevelPrefs(GetPrefs(), GetPath(), partition_path));
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../chrome/browser/profiles/profile.h:40:7: note: forward declaration of
'chrome::ChromeZoomLevelPrefs'
class ChromeZoomLevelPrefs;
      ^
../../chrome/test/base/testing_profile.cc:775:10: error: static_cast from
'content::ZoomLevelDelegate *' to 'chrome::ChromeZoomLevelPrefs *' is not
allowed

Thanks

> Thanks,

[tfarina, 2014-11-17 02:03:22]

On Mon, Nov 17, 2014 at 12:01 AM, <a.sarkar.arun@gmail.com> wrote:

> On 2014/11/17 01:57:12, tfarina wrote:
>
> https://codereview.chromium.org/678343002/diff/100001/
> chrome/test/base/testing_profile.h
>
>> File chrome/test/base/testing_profile.h (right):
>>
>
>
> https://codereview.chromium.org/678343002/diff/100001/
> chrome/test/base/testing_profile.h#newcode14
>
>> chrome/test/base/testing_profile.h:14: #include
>> "chrome/browser/ui/zoom/chrome_zoom_level_prefs.h"
>> why did you include this here? for ChromeZoomLevelPrefs pointer? If it
>> is, I'm
>> sure you don't need this include.
>>
>
>  Could you try removing it and see if it still compiles?
>>
>
> It doesn't compile removing those headers.
> I get these errors
> ../../chrome/test/base/testing_profile.cc:641:11: error: allocation of
> incomplete type 'chrome::ChromeZoomLevelPrefs'
>       new chrome::ChromeZoomLevelPrefs(GetPrefs(), GetPath(),
> partition_path));
>           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../../chrome/browser/profiles/profile.h:40:7: note: forward declaration of
> 'chrome::ChromeZoomLevelPrefs'
> class ChromeZoomLevelPrefs;
>       ^
> ../../chrome/test/base/testing_profile.cc:775:10: error: static_cast from
> 'content::ZoomLevelDelegate *' to 'chrome::ChromeZoomLevelPrefs *' is not
> allowed
>
> Yes, it will compile. You just need to include it in the source file
(testing_profile.cc) rather than in the header file (which you don't use
there).

Could you make that change?

-- 
Thiago Farina

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sarka, 2014-11-17 02:33:39]

On 2014/11/17 02:03:22, tfarina wrote:
> On Mon, Nov 17, 2014 at 12:01 AM, <mailto:a.sarkar.arun@gmail.com> wrote:
> 
> > On 2014/11/17 01:57:12, tfarina wrote:
> >
> > https://codereview.chromium.org/678343002/diff/100001/
> > chrome/test/base/testing_profile.h
> >
> >> File chrome/test/base/testing_profile.h (right):
> >>
> >
> >
> > https://codereview.chromium.org/678343002/diff/100001/
> > chrome/test/base/testing_profile.h#newcode14
> >
> >> chrome/test/base/testing_profile.h:14: #include
> >> "chrome/browser/ui/zoom/chrome_zoom_level_prefs.h"
> >> why did you include this here? for ChromeZoomLevelPrefs pointer? If it
> >> is, I'm
> >> sure you don't need this include.
> >>
> >
> >  Could you try removing it and see if it still compiles?
> >>
> >
> > It doesn't compile removing those headers.
> > I get these errors
> > ../../chrome/test/base/testing_profile.cc:641:11: error: allocation of
> > incomplete type 'chrome::ChromeZoomLevelPrefs'
> >       new chrome::ChromeZoomLevelPrefs(GetPrefs(), GetPath(),
> > partition_path));
> >           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ../../chrome/browser/profiles/profile.h:40:7: note: forward declaration of
> > 'chrome::ChromeZoomLevelPrefs'
> > class ChromeZoomLevelPrefs;
> >       ^
> > ../../chrome/test/base/testing_profile.cc:775:10: error: static_cast from
> > 'content::ZoomLevelDelegate *' to 'chrome::ChromeZoomLevelPrefs *' is not
> > allowed
> >
> > Yes, it will compile. You just need to include it in the source file
> (testing_profile.cc) rather than in the header file (which you don't use
> there).
> 
> Could you make that change?
> 
> -- 
Ah I see. That worked.
Thanks
> Thiago Farina
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sarka, 2014-11-17 02:36:59]

On 2014/11/17 02:33:39, sarka wrote:
> On 2014/11/17 02:03:22, tfarina wrote:
> > On Mon, Nov 17, 2014 at 12:01 AM, <mailto:a.sarkar.arun@gmail.com> wrote:
> > 
> > > On 2014/11/17 01:57:12, tfarina wrote:
> > >
> > > https://codereview.chromium.org/678343002/diff/100001/
> > > chrome/test/base/testing_profile.h
> > >
> > >> File chrome/test/base/testing_profile.h (right):
> > >>
> > >
> > >
> > > https://codereview.chromium.org/678343002/diff/100001/
> > > chrome/test/base/testing_profile.h#newcode14
> > >
> > >> chrome/test/base/testing_profile.h:14: #include
> > >> "chrome/browser/ui/zoom/chrome_zoom_level_prefs.h"
> > >> why did you include this here? for ChromeZoomLevelPrefs pointer? If it
> > >> is, I'm
> > >> sure you don't need this include.
> > >>
> > >
> > >  Could you try removing it and see if it still compiles?
> > >>
> > >
> > > It doesn't compile removing those headers.
> > > I get these errors
> > > ../../chrome/test/base/testing_profile.cc:641:11: error: allocation of
> > > incomplete type 'chrome::ChromeZoomLevelPrefs'
> > >       new chrome::ChromeZoomLevelPrefs(GetPrefs(), GetPath(),
> > > partition_path));
> > >           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ../../chrome/browser/profiles/profile.h:40:7: note: forward declaration of
> > > 'chrome::ChromeZoomLevelPrefs'
> > > class ChromeZoomLevelPrefs;
> > >       ^
> > > ../../chrome/test/base/testing_profile.cc:775:10: error: static_cast from
> > > 'content::ZoomLevelDelegate *' to 'chrome::ChromeZoomLevelPrefs *' is not
> > > allowed
> > >
> > > Yes, it will compile. You just need to include it in the source file
> > (testing_profile.cc) rather than in the header file (which you don't use
> > there).
> > 
> > Could you make that change?
> > 
> > -- 
> Ah I see. That worked.
> Thanks
> > Thiago Farina
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

Done.

[tfarina, 2014-11-18 15:47:05]

thanks, testing_profile.* changes lgtm!

[sky, 2014-11-24 15:46:57]

https://codereview.chromium.org/678343002/diff/120001/chrome/browser/browser_...
File chrome/browser/browser_commands_unittest.cc (right):

https://codereview.chromium.org/678343002/diff/120001/chrome/browser/browser_...
chrome/browser/browser_commands_unittest.cc:354:
EXPECT_EQ(zoom_controller->GetZoomPercent(), 125);
Format for assertions in tests is 'expected, actual.' I think you have it
backwards throughout this test.

https://codereview.chromium.org/678343002/diff/120001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/678343002/diff/120001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands.cc:604: contents->GetMaximumZoomPercent() +
1;
Why the +1 here?

[Alexei Svitkine (slow), 2015-01-07 17:25:30]

Ping, what's the status of this?

[sarka, 2015-01-07 18:57:19]

On 2015/01/07 17:25:30, Alexei Svitkine wrote:
> Ping, what's the status of this?
Couple of questions.
@sky, @asvitkine Based on the comments the should look like EXPECT_EQ(125,
zoom_controller->GetZoomPercent());

In that case I may have it wrong for all the other tests I wrote. Am I correct
here?

@sky I had to do +1 instead of  contents->GetMaximumZoomPercent() because to my
knowledge GetMaximumZoomPercent() return 499 and 500.
I could make further changes to make sure it returns 500.0f.

Once I have my questions resolved I could submit a patch by this weekend if
thats not too late.

Thanks again!

[wjmaclean, 2015-01-07 19:13:53]

On 2015/01/07 18:57:19, sarka wrote:
> On 2015/01/07 17:25:30, Alexei Svitkine wrote:
> > Ping, what's the status of this?
> Couple of questions.
> @sky, @asvitkine Based on the comments the should look like EXPECT_EQ(125,
> zoom_controller->GetZoomPercent());
> 
> In that case I may have it wrong for all the other tests I wrote. Am I correct
> here?
> 
> @sky I had to do +1 instead of  contents->GetMaximumZoomPercent() because to
my
> knowledge GetMaximumZoomPercent() return 499 and 500.
> I could make further changes to make sure it returns 500.0f.
> 
> Once I have my questions resolved I could submit a patch by this weekend if
> thats not too late.
> 
> Thanks again!

W.r.t. the "EXPECT_EQ(125, ..." issue, it's standard to put the expected value
as the first argument, and the actual value as the second.

As for the +1, it seems a bit dangerous, for example if the rounding worked out
cleanly you could return false when it should be true. Perhaps better to test to
see if the two values differ by less than some tolerance (in this case 1, since
the integer rounding in RenderViewImpl::zoomLimitsChanged() means you could be
off by one). So something like

return abs(zoom_controller->GetZoomPercent() -
contents->GetMaximumZoomPercent()) < kZoomPercentEpsilon;

I guess the difference between 99 and 100 is fairly negligible. It would be
better if ViewHostMsg_UpdateZoomLimits carried a float value perhaps.

[Alexei Svitkine (slow), 2015-01-07 19:44:05]

Could you use EXPECT_FLOAT_EQ?

On Wed, Jan 7, 2015 at 2:13 PM, <wjmaclean@chromium.org> wrote:

> On 2015/01/07 18:57:19, sarka wrote:
>
>> On 2015/01/07 17:25:30, Alexei Svitkine wrote:
>> > Ping, what's the status of this?
>> Couple of questions.
>> @sky, @asvitkine Based on the comments the should look like EXPECT_EQ(125,
>> zoom_controller->GetZoomPercent());
>>
>
>  In that case I may have it wrong for all the other tests I wrote. Am I
>> correct
>> here?
>>
>
>  @sky I had to do +1 instead of  contents->GetMaximumZoomPercent()
>> because to
>>
> my
>
>> knowledge GetMaximumZoomPercent() return 499 and 500.
>> I could make further changes to make sure it returns 500.0f.
>>
>
>  Once I have my questions resolved I could submit a patch by this weekend
>> if
>> thats not too late.
>>
>
>  Thanks again!
>>
>
> W.r.t. the "EXPECT_EQ(125, ..." issue, it's standard to put the expected
> value
> as the first argument, and the actual value as the second.
>
> As for the +1, it seems a bit dangerous, for example if the rounding
> worked out
> cleanly you could return false when it should be true. Perhaps better to
> test to
> see if the two values differ by less than some tolerance (in this case 1,
> since
> the integer rounding in RenderViewImpl::zoomLimitsChanged() means you
> could be
> off by one). So something like
>
> return abs(zoom_controller->GetZoomPercent() -
> contents->GetMaximumZoomPercent()) < kZoomPercentEpsilon;
>
> I guess the difference between 99 and 100 is fairly negligible. It would be
> better if ViewHostMsg_UpdateZoomLimits carried a float value perhaps.
>
> https://codereview.chromium.org/678343002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sarka, 2015-01-12 00:46:03]

On 2015/01/07 19:44:05, Alexei Svitkine wrote:
> Could you use EXPECT_FLOAT_EQ?
> 
> On Wed, Jan 7, 2015 at 2:13 PM, <mailto:wjmaclean@chromium.org> wrote:
> 
> > On 2015/01/07 18:57:19, sarka wrote:
> >
> >> On 2015/01/07 17:25:30, Alexei Svitkine wrote:
> >> > Ping, what's the status of this?
> >> Couple of questions.
> >> @sky, @asvitkine Based on the comments the should look like EXPECT_EQ(125,
> >> zoom_controller->GetZoomPercent());
> >>
> >
> >  In that case I may have it wrong for all the other tests I wrote. Am I
> >> correct
> >> here?
> >>
> >
> >  @sky I had to do +1 instead of  contents->GetMaximumZoomPercent()
> >> because to
> >>
> > my
> >
> >> knowledge GetMaximumZoomPercent() return 499 and 500.
> >> I could make further changes to make sure it returns 500.0f.
> >>
> >
> >  Once I have my questions resolved I could submit a patch by this weekend
> >> if
> >> thats not too late.
> >>
> >
> >  Thanks again!
> >>
> >
> > W.r.t. the "EXPECT_EQ(125, ..." issue, it's standard to put the expected
> > value
> > as the first argument, and the actual value as the second.
> >
> > As for the +1, it seems a bit dangerous, for example if the rounding
> > worked out
> > cleanly you could return false when it should be true. Perhaps better to
> > test to
> > see if the two values differ by less than some tolerance (in this case 1,
> > since
> > the integer rounding in RenderViewImpl::zoomLimitsChanged() means you
> > could be
> > off by one). So something like
> >
> > return abs(zoom_controller->GetZoomPercent() -
> > contents->GetMaximumZoomPercent()) < kZoomPercentEpsilon;
> >
> > I guess the difference between 99 and 100 is fairly negligible. It would be
> > better if ViewHostMsg_UpdateZoomLimits carried a float value perhaps.
> >
> > https://codereview.chromium.org/678343002/
> >
> 
@wjmaclean	
 Thanks for your comments. I am seeing something weird happening with
ZoomLevelToZoomFactor defined in page_zoom.cc. Apparently 

static_cast<int>(
      ZoomLevelToZoomFactor(maximum_level) * 100); always returns 499 where
maximum_level is 8.82747, it returns 500 on Mac using either GNU C++ / C++ 98
compiler. Also do you think its fair to use static_cast<int>(kMaximumZoomFactor
* 100) instead.

Let me know what your thoughts are. 

Thanks


> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sarka, 2015-01-12 00:46:42]

On 2015/01/12 00:46:03, sarka wrote:
> On 2015/01/07 19:44:05, Alexei Svitkine wrote:
> > Could you use EXPECT_FLOAT_EQ?
Yes
> > On Wed, Jan 7, 2015 at 2:13 PM, <mailto:wjmaclean@chromium.org> wrote:
> > 
> > > On 2015/01/07 18:57:19, sarka wrote:
> > >
> > >> On 2015/01/07 17:25:30, Alexei Svitkine wrote:
> > >> > Ping, what's the status of this?
> > >> Couple of questions.
> > >> @sky, @asvitkine Based on the comments the should look like
EXPECT_EQ(125,
> > >> zoom_controller->GetZoomPercent());
> > >>
> > >
> > >  In that case I may have it wrong for all the other tests I wrote. Am I
> > >> correct
> > >> here?
> > >>
> > >
> > >  @sky I had to do +1 instead of  contents->GetMaximumZoomPercent()
> > >> because to
> > >>
> > > my
> > >
> > >> knowledge GetMaximumZoomPercent() return 499 and 500.
> > >> I could make further changes to make sure it returns 500.0f.
> > >>
> > >
> > >  Once I have my questions resolved I could submit a patch by this weekend
> > >> if
> > >> thats not too late.
> > >>
> > >
> > >  Thanks again!
> > >>
> > >
> > > W.r.t. the "EXPECT_EQ(125, ..." issue, it's standard to put the expected
> > > value
> > > as the first argument, and the actual value as the second.
> > >
> > > As for the +1, it seems a bit dangerous, for example if the rounding
> > > worked out
> > > cleanly you could return false when it should be true. Perhaps better to
> > > test to
> > > see if the two values differ by less than some tolerance (in this case 1,
> > > since
> > > the integer rounding in RenderViewImpl::zoomLimitsChanged() means you
> > > could be
> > > off by one). So something like
> > >
> > > return abs(zoom_controller->GetZoomPercent() -
> > > contents->GetMaximumZoomPercent()) < kZoomPercentEpsilon;
> > >
> > > I guess the difference between 99 and 100 is fairly negligible. It would
be
> > > better if ViewHostMsg_UpdateZoomLimits carried a float value perhaps.
> > >
> > > https://codereview.chromium.org/678343002/
> > >
> > 
> @wjmaclean	
>  Thanks for your comments. I am seeing something weird happening with
> ZoomLevelToZoomFactor defined in page_zoom.cc. Apparently 
> 
> static_cast<int>(
>       ZoomLevelToZoomFactor(maximum_level) * 100); always returns 499 where
> maximum_level is 8.82747, it returns 500 on Mac using either GNU C++ / C++ 98
> compiler. Also do you think its fair to use
static_cast<int>(kMaximumZoomFactor
> * 100) instead.
> 
> Let me know what your thoughts are. 
> 
> Thanks
> 
> 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2015-01-12 16:29:17]

> @wjmaclean	
>  Thanks for your comments. I am seeing something weird happening with
> ZoomLevelToZoomFactor defined in page_zoom.cc. Apparently 
> 
> static_cast<int>(
>       ZoomLevelToZoomFactor(maximum_level) * 100); always returns 499 where
> maximum_level is 8.82747, it returns 500 on Mac using either GNU C++ / C++ 98
> compiler. Also do you think its fair to use
static_cast<int>(kMaximumZoomFactor
> * 100) instead.

What are the underlying float values before the static_cast<int>? I guess in one
case it's 499.9 or something and the other case its 500.0? Those types of
discrepancies are normal with floating point numbers.

I think that's why the actual code does: static_cast<int>(zoom_factor * 100 +
0.5) in zoom_controller.cc - which will round up or down to the nearest int
(rather than always down as in your pasted code). (It's correct to keep the +
0.5 in the static_cast<int>() call.

> 
> Let me know what your thoughts are. 
> 
> Thanks

[Alexei Svitkine (slow), 2015-01-12 16:33:50]

I bet the error is in web_contents_impl.cc in the following code:

maximum_zoom_percent_(static_cast<int>(kMaximumZoomFactor * 100)),

Since kMaximumZoomFactor is a floating point number, the above float op probably
results in 499 on some platforms, so probably the right fix to do the + 0.5
there to make sure its rounded to the nearest int rather than down in some
cases.

[sarka, 2015-01-12 16:34:59]

On 2015/01/12 16:33:50, Alexei Svitkine wrote:
> I bet the error is in web_contents_impl.cc in the following code:
> 
> maximum_zoom_percent_(static_cast<int>(kMaximumZoomFactor * 100)),
> 
> Since kMaximumZoomFactor is a floating point number, the above float op
probably
> results in 499 on some platforms, so probably the right fix to do the + 0.5
> there to make sure its rounded to the nearest int rather than down in some
> cases.

Yes that sounds like a good idea. I will make those changes.

Thanks.

[sarka, 2015-01-13 05:02:32]

On 2015/01/12 16:34:59, sarka wrote:
> On 2015/01/12 16:33:50, Alexei Svitkine wrote:
> > I bet the error is in web_contents_impl.cc in the following code:
> > 
> > maximum_zoom_percent_(static_cast<int>(kMaximumZoomFactor * 100)),
> > 
> > Since kMaximumZoomFactor is a floating point number, the above float op
> probably
> > results in 499 on some platforms, so probably the right fix to do the + 0.5
> > there to make sure its rounded to the nearest int rather than down in some
> > cases.
> 
> Yes that sounds like a good idea. I will make those changes.
> 
> Thanks.

PTAL.
Thanks

[Alexei Svitkine (slow), 2015-01-13 14:51:13]

lgtm

https://codereview.chromium.org/678343002/diff/140001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/678343002/diff/140001/content/renderer/render...
content/renderer/render_view_impl.cc:3763: ZoomLevelToZoomFactor(minimum_level)
* 100);
This should probably be rounded too.

https://codereview.chromium.org/678343002/diff/140001/content/renderer/render...
content/renderer/render_view_impl.cc:3766: ZoomLevelToZoomFactor(maximum_level)
* 100 + .5);
Nit: 0.5

[sarka, 2015-01-14 03:04:04]

On 2015/01/13 14:51:13, Alexei Svitkine wrote:
> lgtm
> 
>
https://codereview.chromium.org/678343002/diff/140001/content/renderer/render...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/678343002/diff/140001/content/renderer/render...
> content/renderer/render_view_impl.cc:3763:
ZoomLevelToZoomFactor(minimum_level)
> * 100);
> This should probably be rounded too.
> 
Done
>
https://codereview.chromium.org/678343002/diff/140001/content/renderer/render...
> content/renderer/render_view_impl.cc:3766:
ZoomLevelToZoomFactor(maximum_level)
> * 100 + .5);
> Nit: 0.5
Done
Thanks

[Alexei Svitkine (slow), 2015-01-14 20:42:40]

sky, could you take another look at the CL? I think your comments have been
addressed by Arunoday.

On Tue, Jan 13, 2015 at 10:04 PM, <a.sarkar.arun@gmail.com> wrote:

> On 2015/01/13 14:51:13, Alexei Svitkine wrote:
>
>> lgtm
>>
>
>
> https://codereview.chromium.org/678343002/diff/140001/
> content/renderer/render_view_impl.cc
>
>> File content/renderer/render_view_impl.cc (right):
>>
>
>
> https://codereview.chromium.org/678343002/diff/140001/
> content/renderer/render_view_impl.cc#newcode3763
>
>> content/renderer/render_view_impl.cc:3763:
>>
> ZoomLevelToZoomFactor(minimum_level)
>
>> * 100);
>> This should probably be rounded too.
>>
>
> Done
>
> https://codereview.chromium.org/678343002/diff/140001/
> content/renderer/render_view_impl.cc#newcode3766
>
>> content/renderer/render_view_impl.cc:3766:
>>
> ZoomLevelToZoomFactor(maximum_level)
>
>> * 100 + .5);
>> Nit: 0.5
>>
> Done
> Thanks
>
> https://codereview.chromium.org/678343002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-01-14 20:57:39]

On 2015/01/07 18:57:19, sarka wrote:
> On 2015/01/07 17:25:30, Alexei Svitkine wrote:
> > Ping, what's the status of this?
> Couple of questions.
> @sky, @asvitkine Based on the comments the should look like EXPECT_EQ(125,
> zoom_controller->GetZoomPercent());
> 
> In that case I may have it wrong for all the other tests I wrote. Am I correct
> here?
> 
> @sky I had to do +1 instead of  contents->GetMaximumZoomPercent() because to
my
> knowledge GetMaximumZoomPercent() return 499 and 500.
> I could make further changes to make sure it returns 500.0f.

That seems wrong. Seems like it should always return consistent values.

> 
> Once I have my questions resolved I could submit a patch by this weekend if
> thats not too late.
> 
> Thanks again!

[Alexei Svitkine (slow), 2015-01-14 21:01:20]

I believe the above is no longer the case in his latest patch set. (i.e. he
no longer does the +1 as a result of the fixes on the most recent patch set)

On Wed, Jan 14, 2015 at 3:57 PM, <sky@chromium.org> wrote:

> On 2015/01/07 18:57:19, sarka wrote:
>
>> On 2015/01/07 17:25:30, Alexei Svitkine wrote:
>> > Ping, what's the status of this?
>> Couple of questions.
>> @sky, @asvitkine Based on the comments the should look like EXPECT_EQ(125,
>> zoom_controller->GetZoomPercent());
>>
>
>  In that case I may have it wrong for all the other tests I wrote. Am I
>> correct
>> here?
>>
>
>  @sky I had to do +1 instead of  contents->GetMaximumZoomPercent()
>> because to
>>
> my
>
>> knowledge GetMaximumZoomPercent() return 499 and 500.
>> I could make further changes to make sure it returns 500.0f.
>>
>
> That seems wrong. Seems like it should always return consistent values.
>
>
>  Once I have my questions resolved I could submit a patch by this weekend
>> if
>> thats not too late.
>>
>
>  Thanks again!
>>
>
>
>
> https://codereview.chromium.org/678343002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-01-15 00:45:49]

https://codereview.chromium.org/678343002/diff/160001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/678343002/diff/160001/content/renderer/render...
content/renderer/render_view_impl.cc:3762: // Round the double before
dispatching the async IPC.
You're documenting what the code is doing. Document why the rounding is
important.

[sarka, 2015-01-22 01:51:28]

On 2015/01/15 00:45:49, sky wrote:
>
https://codereview.chromium.org/678343002/diff/160001/content/renderer/render...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/678343002/diff/160001/content/renderer/render...
> content/renderer/render_view_impl.cc:3762: // Round the double before
> dispatching the async IPC.
> You're documenting what the code is doing. Document why the rounding is
> important.

Done.
Thanks

[sky, 2015-01-22 17:30:35]

https://codereview.chromium.org/678343002/diff/180001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/678343002/diff/180001/content/renderer/render...
content/renderer/render_view_impl.cc:3765: ZoomLevelToZoomFactor(minimum_level)
* 100 + 0.5);
If we're rounding, why not use a round api?

[Alexei Svitkine (slow), 2015-01-22 20:46:54]

Do we have a round API?

On Thu, Jan 22, 2015 at 12:30 PM, <sky@chromium.org> wrote:

>
> https://codereview.chromium.org/678343002/diff/180001/
> content/renderer/render_view_impl.cc
> File content/renderer/render_view_impl.cc (right):
>
> https://codereview.chromium.org/678343002/diff/180001/
> content/renderer/render_view_impl.cc#newcode3765
> content/renderer/render_view_impl.cc:3765:
> ZoomLevelToZoomFactor(minimum_level) * 100 + 0.5);
> If we're rounding, why not use a round api?
>
> https://codereview.chromium.org/678343002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-01-22 21:20:28]

round() ?

On Thu, Jan 22, 2015 at 12:46 PM, Alexei Svitkine
<asvitkine@chromium.org> wrote:
> Do we have a round API?
>
> On Thu, Jan 22, 2015 at 12:30 PM, <sky@chromium.org> wrote:
>>
>>
>>
>>
https://codereview.chromium.org/678343002/diff/180001/content/renderer/render...
>> File content/renderer/render_view_impl.cc (right):
>>
>>
>>
https://codereview.chromium.org/678343002/diff/180001/content/renderer/render...
>> content/renderer/render_view_impl.cc:3765:
>> ZoomLevelToZoomFactor(minimum_level) * 100 + 0.5);
>> If we're rounding, why not use a round api?
>>
>> https://codereview.chromium.org/678343002/
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sarka, 2015-01-23 04:11:32]

On 2015/01/22 21:20:28, sky wrote:
> round() ?
> 
> On Thu, Jan 22, 2015 at 12:46 PM, Alexei Svitkine
> <mailto:asvitkine@chromium.org> wrote:
> > Do we have a round API?
> >
> > On Thu, Jan 22, 2015 at 12:30 PM, <mailto:sky@chromium.org> wrote:
> >>
> >>
> >>
> >>
>
https://codereview.chromium.org/678343002/diff/180001/content/renderer/render...
> >> File content/renderer/render_view_impl.cc (right):
> >>
> >>
> >>
>
https://codereview.chromium.org/678343002/diff/180001/content/renderer/render...
> >> content/renderer/render_view_impl.cc:3765:
> >> ZoomLevelToZoomFactor(minimum_level) * 100 + 0.5);
> >> If we're rounding, why not use a round api?
> >>
There are several places within the code base where static_cast<int> has been
used.Not that they are all doing similar stuff to this particular use case. To
honor developer guidelines do I use round() or 
static_cast<int>.
PS: round() solves the problem as well.

Thanks
> >> https://codereview.chromium.org/678343002/
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-01-23 17:17:12]

On Thu, Jan 22, 2015 at 8:11 PM,  <a.sarkar.arun@gmail.com> wrote:
> On 2015/01/22 21:20:28, sky wrote:
>>
>> round() ?
>
>
>> On Thu, Jan 22, 2015 at 12:46 PM, Alexei Svitkine
>> <mailto:asvitkine@chromium.org> wrote:
>> > Do we have a round API?
>> >
>> > On Thu, Jan 22, 2015 at 12:30 PM, <mailto:sky@chromium.org> wrote:
>> >>
>> >>
>> >>
>> >>
>
>
>
https://codereview.chromium.org/678343002/diff/180001/content/renderer/render...
>>
>> >> File content/renderer/render_view_impl.cc (right):
>> >>
>> >>
>> >>
>
>
>
https://codereview.chromium.org/678343002/diff/180001/content/renderer/render...
>>
>> >> content/renderer/render_view_impl.cc:3765:
>> >> ZoomLevelToZoomFactor(minimum_level) * 100 + 0.5);
>> >> If we're rounding, why not use a round api?
>> >>
>
> There are several places within the code base where static_cast<int> has
> been
> used.Not that they are all doing similar stuff to this particular use case.
> To
> honor developer guidelines do I use round() or
> static_cast<int>.
> PS: round() solves the problem as well.

static_cast<int> isn't what you want, right? The reason I prefer
round() here is your comment says 'Round', which to me says you should
be using the round function and not rolling your own.
>
> Thanks
>>
>> >> https://codereview.chromium.org/678343002/
>> >
>> >
>
>
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email
>>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/678343002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sarka, 2015-01-24 18:30:20]

On 2015/01/23 17:17:12, sky wrote:
> On Thu, Jan 22, 2015 at 8:11 PM,  <mailto:a.sarkar.arun@gmail.com> wrote:
> > On 2015/01/22 21:20:28, sky wrote:
> >>
> >> round() ?
> >
> >
> >> On Thu, Jan 22, 2015 at 12:46 PM, Alexei Svitkine
> >> <mailto:asvitkine@chromium.org> wrote:
> >> > Do we have a round API?
> >> >
> >> > On Thu, Jan 22, 2015 at 12:30 PM, <mailto:sky@chromium.org> wrote:
> >> >>
> >> >>
> >> >>
> >> >>
> >
> >
> >
>
https://codereview.chromium.org/678343002/diff/180001/content/renderer/render...
> >>
> >> >> File content/renderer/render_view_impl.cc (right):
> >> >>
> >> >>
> >> >>
> >
> >
> >
>
https://codereview.chromium.org/678343002/diff/180001/content/renderer/render...
> >>
> >> >> content/renderer/render_view_impl.cc:3765:
> >> >> ZoomLevelToZoomFactor(minimum_level) * 100 + 0.5);
> >> >> If we're rounding, why not use a round api?
> >> >>
> >
> > There are several places within the code base where static_cast<int> has
> > been
> > used.Not that they are all doing similar stuff to this particular use case.
> > To
> > honor developer guidelines do I use round() or
> > static_cast<int>.
> > PS: round() solves the problem as well.
> 
> static_cast<int> isn't what you want, right? The reason I prefer
> round() here is your comment says 'Round', which to me says you should
> be using the round function and not rolling your own.
> >

Done.

> > Thanks
> >>
> >> >> https://codereview.chromium.org/678343002/
> >> >
> >> >
> >
> >
> >> To unsubscribe from this group and stop receiving emails from it, send an
> >
> > email
> >>
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> >
> > https://codereview.chromium.org/678343002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-01-26 16:00:54]

LGTM

[sarka, 2015-01-26 19:50:56]

The CQ bit was checked by a.sarkar.arun@gmail.com

[commit-bot: I haz the power, 2015-01-26 19:51:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/678343002/200001

[commit-bot: I haz the power, 2015-01-26 20:00:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-26 20:00:09]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sarka, 2015-01-27 19:54:56]

a.sarkar.arun@gmail.com changed reviewers:
+ jeremy@chromium.org

[sarka, 2015-01-27 19:55:40]

On 2015/01/24 18:30:20, sarka wrote:
> On 2015/01/23 17:17:12, sky wrote:
> > On Thu, Jan 22, 2015 at 8:11 PM,  <mailto:a.sarkar.arun@gmail.com> wrote:
> > > On 2015/01/22 21:20:28, sky wrote:
> > >>
> > >> round() ?
> > >
> > >
> > >> On Thu, Jan 22, 2015 at 12:46 PM, Alexei Svitkine
> > >> <mailto:asvitkine@chromium.org> wrote:
> > >> > Do we have a round API?
> > >> >
> > >> > On Thu, Jan 22, 2015 at 12:30 PM, <mailto:sky@chromium.org> wrote:
> > >> >>
> > >> >>
> > >> >>
> > >> >>
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/678343002/diff/180001/content/renderer/render...
> > >>
> > >> >> File content/renderer/render_view_impl.cc (right):
> > >> >>
> > >> >>
> > >> >>
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/678343002/diff/180001/content/renderer/render...
> > >>
> > >> >> content/renderer/render_view_impl.cc:3765:
> > >> >> ZoomLevelToZoomFactor(minimum_level) * 100 + 0.5);
> > >> >> If we're rounding, why not use a round api?
> > >> >>
> > >
> > > There are several places within the code base where static_cast<int> has
> > > been
> > > used.Not that they are all doing similar stuff to this particular use
case.
> > > To
> > > honor developer guidelines do I use round() or
> > > static_cast<int>.
> > > PS: round() solves the problem as well.
> > 
> > static_cast<int> isn't what you want, right? The reason I prefer
> > round() here is your comment says 'Round', which to me says you should
> > be using the round function and not rolling your own.
> > >
> 
> Done.
> 
> > > Thanks
> > >>
> > >> >> https://codereview.chromium.org/678343002/
> > >> >
> > >> >
> > >
> > >
> > >> To unsubscribe from this group and stop receiving emails from it, send an
> > >
> > > email
> > >>
> > >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > >
> > >
> > > https://codereview.chromium.org/678343002/
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
Added renderer
@jeremy PTAL.
Thanks

[jeremy, 2015-01-28 05:59:25]

LGTM

https://codereview.chromium.org/678343002/diff/200001/chrome/browser/browser_...
File chrome/browser/browser_commands_unittest.cc (right):

https://codereview.chromium.org/678343002/diff/200001/chrome/browser/browser_...
chrome/browser/browser_commands_unittest.cc:367: // Tab no longer at default
zoom hence Actual size should be enabled.
nit: lowercase 'a' in actual ?

[sarka, 2015-01-28 14:35:36]

The CQ bit was checked by a.sarkar.arun@gmail.com

[commit-bot: I haz the power, 2015-01-28 14:36:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/678343002/200001

[commit-bot: I haz the power, 2015-01-28 14:39:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-28 14:39:18]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sarka, 2015-02-22 03:20:35]

The CQ bit was checked by a.sarkar.arun@gmail.com

[commit-bot: I haz the power, 2015-02-22 03:21:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/678343002/240001

[commit-bot: I haz the power, 2015-02-22 03:24:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-22 03:24:42]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[sarka, 2015-02-23 01:57:29]

a.sarkar.arun@gmail.com changed reviewers:
+ jam@chromium.org

[sarka, 2015-02-23 01:58:50]

a.sarkar.arun@gmail.com changed reviewers:
+ avi@chromium.org
- jam@chromium.org

[sarka, 2015-02-23 01:59:23]

On 2015/02/22 03:24:42, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   ios_dbg_simulator on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
Added content reviewer 
@avi, PTAL

Thanks

[Avi (use Gerrit), 2015-02-23 04:51:55]

not lgtm

Am I missing something? The naming is completely wrong here.

https://codereview.chromium.org/678343002/diff/240001/chrome/browser/browser_...
File chrome/browser/browser_commands_unittest.cc (right):

https://codereview.chromium.org/678343002/diff/240001/chrome/browser/browser_...
chrome/browser/browser_commands_unittest.cc:23: typedef
BrowserWithTestWindowTest BrowserCommandsTest;
Blank line above this one.

https://codereview.chromium.org/678343002/diff/240001/chrome/browser/browser_...
chrome/browser/browser_commands_unittest.cc:350: content::WebContents* contents
= tab_strip_model->GetWebContentsAt(0);
You have a using statement above, so you shouldn't need the "content::".

https://codereview.chromium.org/678343002/diff/240001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/678343002/diff/240001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands.cc:572: }
This rename appears wrong. "CanZoomIn" makes sense; if the zoom percent isn't
the maximum zoom percent, we can zoom further. "IsAtMaximumZoom" is completely
wrong. It's _backwards_, returning true only when the value for the zoom percent
is *not* at the maximum zoom.

https://codereview.chromium.org/678343002/diff/240001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands.cc:579: }
This rename appears wrong. "CanZoomOut" makes sense; if the zoom percent isn't
the minimum zoom percent, we can zoom further. "IsAtMinimumZoom" is completely
wrong. It's _backwards_, returning true only when the value for the zoom percent
is *not* at the minimum zoom.

[sarka, 2015-02-23 05:35:40]

On 2015/02/23 04:51:55, Avi wrote:
> not lgtm
> 
> Am I missing something? The naming is completely wrong here.
> 
Honestly after the uploading the first patch , I changed the method name from
ActualSize to IsAtDefaultZoom which made sense. However I may have not paid
enough attention while renaming the other two methods. Now it does.
Thanks for pointing it out.

I would upload another set.
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/browser_...
> File chrome/browser/browser_commands_unittest.cc (right):
> 
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/browser_...
> chrome/browser/browser_commands_unittest.cc:23: typedef
> BrowserWithTestWindowTest BrowserCommandsTest;
> Blank line above this one.
> 
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/browser_...
> chrome/browser/browser_commands_unittest.cc:350: content::WebContents*
contents
> = tab_strip_model->GetWebContentsAt(0);
> You have a using statement above, so you shouldn't need the "content::".
> 
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/ui/brows...
> File chrome/browser/ui/browser_commands.cc (right):
> 
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/ui/brows...
> chrome/browser/ui/browser_commands.cc:572: }
> This rename appears wrong. "CanZoomIn" makes sense; if the zoom percent isn't
> the maximum zoom percent, we can zoom further. "IsAtMaximumZoom" is completely
> wrong. It's _backwards_, returning true only when the value for the zoom
percent
> is *not* at the maximum zoom.
> 
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/ui/brows...
> chrome/browser/ui/browser_commands.cc:579: }
> This rename appears wrong. "CanZoomOut" makes sense; if the zoom percent isn't
> the minimum zoom percent, we can zoom further. "IsAtMinimumZoom" is completely
> wrong. It's _backwards_, returning true only when the value for the zoom
percent
> is *not* at the minimum zoom.

[sarka, 2015-02-24 02:13:04]

On 2015/02/23 05:35:40, sarka wrote:
> On 2015/02/23 04:51:55, Avi wrote:
> > not lgtm
> > 
> > Am I missing something? The naming is completely wrong here.
> > 
> Honestly after the uploading the first patch , I changed the method name from
> ActualSize to IsAtDefaultZoom which made sense. However I may have not paid
> enough attention while renaming the other two methods. Now it does.
> Thanks for pointing it out.
> 
> I would upload another set.
> >
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/browser_...
> > File chrome/browser/browser_commands_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/browser_...
> > chrome/browser/browser_commands_unittest.cc:23: typedef
> > BrowserWithTestWindowTest BrowserCommandsTest;
> > Blank line above this one.
> > 
> >
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/browser_...
> > chrome/browser/browser_commands_unittest.cc:350: content::WebContents*
> contents
> > = tab_strip_model->GetWebContentsAt(0);
> > You have a using statement above, so you shouldn't need the "content::".
> > 
> >
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/ui/brows...
> > File chrome/browser/ui/browser_commands.cc (right):
> > 
> >
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/ui/brows...
> > chrome/browser/ui/browser_commands.cc:572: }
> > This rename appears wrong. "CanZoomIn" makes sense; if the zoom percent
isn't
> > the maximum zoom percent, we can zoom further. "IsAtMaximumZoom" is
completely
> > wrong. It's _backwards_, returning true only when the value for the zoom
> percent
> > is *not* at the maximum zoom.
> > 
> >
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/ui/brows...
> > chrome/browser/ui/browser_commands.cc:579: }
> > This rename appears wrong. "CanZoomOut" makes sense; if the zoom percent
isn't
> > the minimum zoom percent, we can zoom further. "IsAtMinimumZoom" is
completely
> > wrong. It's _backwards_, returning true only when the value for the zoom
> percent
> > is *not* at the minimum zoom.

PTAL.
Thanks

[Avi (use Gerrit), 2015-02-25 19:22:21]

On 2015/02/24 02:13:04, sarka wrote:
> On 2015/02/23 05:35:40, sarka wrote:
> > On 2015/02/23 04:51:55, Avi wrote:
> > > not lgtm
> > > 
> > > Am I missing something? The naming is completely wrong here.
> > > 
> > Honestly after the uploading the first patch , I changed the method name
from
> > ActualSize to IsAtDefaultZoom which made sense. However I may have not paid
> > enough attention while renaming the other two methods. Now it does.
> > Thanks for pointing it out.
> > 
> > I would upload another set.
> > >
> >
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/browser_...
> > > File chrome/browser/browser_commands_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/browser_...
> > > chrome/browser/browser_commands_unittest.cc:23: typedef
> > > BrowserWithTestWindowTest BrowserCommandsTest;
> > > Blank line above this one.
> > > 
> > >
> >
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/browser_...
> > > chrome/browser/browser_commands_unittest.cc:350: content::WebContents*
> > contents
> > > = tab_strip_model->GetWebContentsAt(0);
> > > You have a using statement above, so you shouldn't need the "content::".
> > > 
> > >
> >
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/ui/brows...
> > > File chrome/browser/ui/browser_commands.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/ui/brows...
> > > chrome/browser/ui/browser_commands.cc:572: }
> > > This rename appears wrong. "CanZoomIn" makes sense; if the zoom percent
> isn't
> > > the maximum zoom percent, we can zoom further. "IsAtMaximumZoom" is
> completely
> > > wrong. It's _backwards_, returning true only when the value for the zoom
> > percent
> > > is *not* at the maximum zoom.
> > > 
> > >
> >
>
https://codereview.chromium.org/678343002/diff/240001/chrome/browser/ui/brows...
> > > chrome/browser/ui/browser_commands.cc:579: }
> > > This rename appears wrong. "CanZoomOut" makes sense; if the zoom percent
> isn't
> > > the minimum zoom percent, we can zoom further. "IsAtMinimumZoom" is
> completely
> > > wrong. It's _backwards_, returning true only when the value for the zoom
> > percent
> > > is *not* at the minimum zoom.
> 
> PTAL.
> Thanks

Thanks!

LGTM now.

[sarka, 2015-02-26 03:19:04]

The CQ bit was checked by a.sarkar.arun@gmail.com

[sarka, 2015-02-26 03:19:05]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
wjmaclean@chromium.org, tfarina@chromium.org, jeremy@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/678343002/#ps260001
(title: "Renamed Zoom Update commands")

[commit-bot: I haz the power, 2015-02-26 03:20:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/678343002/260001

[commit-bot: I haz the power, 2015-02-26 05:39:25]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-02-26 05:40:12]

Patchset 14 (id:??) landed as
https://crrev.com/daadc7150bd4955f19af36c67d104e68fef5d5fe
Cr-Commit-Position: refs/heads/master@{#318191}

[Alexei Svitkine (slow), 2015-02-26 14:26:35]

https://codereview.chromium.org/678343002/diff/260001/chrome/browser/browser_...
File chrome/browser/browser_commands_unittest.cc (right):

https://codereview.chromium.org/678343002/diff/260001/chrome/browser/browser_...
chrome/browser/browser_commands_unittest.cc:242: WebContents* firstTab =
tab_strip_model->GetWebContentsAt(0);
Hmm, not sure at which point this slipped through - but actually code in this
file should be using hacker_style, not camelCase.

Can you fix in a follow-up CL? Thanks.

[sarka, 2015-02-26 15:37:22]

https://codereview.chromium.org/678343002/diff/120001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/678343002/diff/120001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands.cc:604: contents->GetMaximumZoomPercent() +
1;
On 2014/11/24 15:46:57, sky wrote:
> Why the +1 here?

If I remember correct contents->GetMaximumZoomPercent() returned 499 when I had
initially uploaded the patch. Hence +1

https://codereview.chromium.org/678343002/diff/260001/chrome/browser/browser_...
File chrome/browser/browser_commands_unittest.cc (right):

https://codereview.chromium.org/678343002/diff/260001/chrome/browser/browser_...
chrome/browser/browser_commands_unittest.cc:242: WebContents* firstTab =
tab_strip_model->GetWebContentsAt(0);
On 2015/02/26 14:26:35, Alexei Svitkine wrote:
> Hmm, not sure at which point this slipped through - but actually code in this
> file should be using hacker_style, not camelCase.
> 
> Can you fix in a follow-up CL? Thanks.

My bad, I updated the names in the last CL. The names in the older patch looked
silly. Will upload a followup CL.
Thanks