Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
310913002:
Issue 32919: Update the Zoom NSMenuItems (Zoom-In/Zoom-out/Actual-Size) when the zoom state changes. (Closed)
|
Created:
6 years, 6 months ago by sarka Modified:
6 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionUpdate the Zoom NSMenuItems (Zoom-In/Zoom-out/Actual-Size) when the zoom state changes.
The fix basically works in the following way. Look at Zoom Observer overridden methods which is implemented in Browser.cc. When the zoom state changes send relevant messages to Command_Updater to enable / disable Zoom Menu Items.
BUG=32919
TEST=Open a NTP , load a url. Zoom-In max and the Zoom-In menu item should be disabled likewise zoom-out max and the zoom-out menu should be disabled.
When a NTP or a page is loaded by default the zoom scale is 100% hence the Actual size menu item should be disabled.
R=ben@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287401
Patch Set 1 #
Total comments: 5
Patch Set 2 : Unit tests for Issue:32919 #
Total comments: 2
Patch Set 3 : Cross-platform unittest for Issue 32919 #
Total comments: 4
Patch Set 4 : Fixed Lint errors in zoom_controller_unittest.cc, fixed indent in browser_commands.cc #Patch Set 5 : Updated patch with inline namespace in zoom_controller_unittest #Patch Set 6 : Modified zoom_controller_unittests to pass try jobs on bots. #
Total comments: 2
Patch Set 7 : Removed an un-used protected variable from ZoomCommandsUnitTest class #Patch Set 8 : Uploading a clean patch after rebase #
Total comments: 6
Patch Set 9 : Removed unused decls and extra empty lines #Patch Set 10 : Uses ZoomController::GetZoomPercent since WebContents::GetZoomPercent doesn't exist any more #Patch Set 11 : Moved the unit tests to browser_commands_unittest since its independent of ZoomController #
Total comments: 2
Patch Set 12 : Reverted zoom_controller_unittest.cc #
Total comments: 2
Patch Set 13 : Fixed Nit #
Messages
Total messages: 58 (0 generated)
Please start reviewing this patch for defect 32919. Thanks
https://codereview.chromium.org/310913002/diff/1/chrome/browser/ui/browser_co... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/310913002/diff/1/chrome/browser/ui/browser_co... chrome/browser/ui/browser_commands.cc:962: bool CanZoomIn(content::WebContents *contents) { nit: WebContents* contents here, and in the subsequent 2 functions. https://codereview.chromium.org/310913002/diff/1/chrome/browser/ui/browser_co... chrome/browser/ui/browser_commands.cc:965: &dummy) == contents->GetMaximumZoomPercent() + 1 ? false : true; I'd write these as: return contents->GetZoomPercent(&dummy, &dummy) != contents->GetMaximumZoomPercent() + 1; (note also the 4-space indent for wrapped lines).
On 2014/06/05 15:45:03, Ben Goodger (Google) wrote: > https://codereview.chromium.org/310913002/diff/1/chrome/browser/ui/browser_co... > File chrome/browser/ui/browser_commands.cc (right): > > https://codereview.chromium.org/310913002/diff/1/chrome/browser/ui/browser_co... > chrome/browser/ui/browser_commands.cc:962: bool CanZoomIn(content::WebContents > *contents) { > nit: WebContents* contents here, and in the subsequent 2 functions. > > https://codereview.chromium.org/310913002/diff/1/chrome/browser/ui/browser_co... > chrome/browser/ui/browser_commands.cc:965: &dummy) == > contents->GetMaximumZoomPercent() + 1 ? false : true; > I'd write these as: > > return contents->GetZoomPercent(&dummy, &dummy) != > contents->GetMaximumZoomPercent() + 1; > > (note also the 4-space indent for wrapped lines). Can you also write a unit test? (browser_command_controller_unittest.cc)
Included unit tests associated with this Fix. Tests for Max-Zoom in, max Zoom-out and Actual size. The test specifically looks for whether the commands IDC_ZOOM_PLUS, IDC_ZOOM_MINUS, IDC_ZOOM_NORMAL are enabled or disabled based on zoom state for a tab. https://codereview.chromium.org/310913002/diff/1/chrome/browser/ui/browser_co... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/310913002/diff/1/chrome/browser/ui/browser_co... chrome/browser/ui/browser_commands.cc:962: bool CanZoomIn(content::WebContents *contents) { On 2014/06/05 15:45:03, Ben Goodger (Google) wrote: > nit: WebContents* contents here, and in the subsequent 2 functions. Done. https://codereview.chromium.org/310913002/diff/1/chrome/browser/ui/browser_co... chrome/browser/ui/browser_commands.cc:962: bool CanZoomIn(content::WebContents *contents) { On 2014/06/05 15:45:03, Ben Goodger (Google) wrote: > nit: WebContents* contents here, and in the subsequent 2 functions. Done. https://codereview.chromium.org/310913002/diff/1/chrome/browser/ui/browser_co... chrome/browser/ui/browser_commands.cc:965: &dummy) == contents->GetMaximumZoomPercent() + 1 ? false : true; On 2014/06/05 15:45:03, Ben Goodger (Google) wrote: > I'd write these as: > > return contents->GetZoomPercent(&dummy, &dummy) != > contents->GetMaximumZoomPercent() + 1; > > (note also the 4-space indent for wrapped lines). Done.
Finally, have you submitted patches to chromium before? If not, can you add your info to src/AUTHORS? https://codereview.chromium.org/310913002/diff/20001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm (right): https://codereview.chromium.org/310913002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm:137: IN_PROC_BROWSER_TEST_F(ZoomDecorationTest, MaxZoomIn) { these tests are cross platform... therefore should not be a .mm file. Also, is it possible to write these as unit tests? unit tests are way faster than browser tests. https://codereview.chromium.org/310913002/diff/20001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm:145: content::RunAllPendingInMessageLoop(); do you need this line in all your tests?
On 2014/06/05 21:19:02, Ben Goodger (Google) wrote: > Finally, have you submitted patches to chromium before? If not, can you add your > info to src/AUTHORS? Added name to AUTHORS > > https://codereview.chromium.org/310913002/diff/20001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm > (right): > > https://codereview.chromium.org/310913002/diff/20001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm:137: > IN_PROC_BROWSER_TEST_F(ZoomDecorationTest, MaxZoomIn) { > these tests are cross platform... therefore should not be a .mm file. > > Also, is it possible to write these as unit tests? unit tests are way faster > than browser tests. > Added a cross-platform unittest for the patch > https://codereview.chromium.org/310913002/diff/20001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm:145: > content::RunAllPendingInMessageLoop(); > do you need this line in all your tests?
Added a cross-platform unit test for Zoom commands Please start reviewing the patch. Thanks
Including more reviewers
On 2014/06/11 15:11:04, eka508 wrote: > Including more reviewers Ben is probably the right reviewer for this.
lgtm with following nit: https://codereview.chromium.org/310913002/diff/40001/chrome/browser/ui/zoom/z... File chrome/browser/ui/zoom/zoom_controller_unittest.cc (right): https://codereview.chromium.org/310913002/diff/40001/chrome/browser/ui/zoom/z... chrome/browser/ui/zoom/zoom_controller_unittest.cc:31: using content::SiteInstance; nit: prevailing style in chromium is to avoid doing this unless you are having significant trouble keeping within the 80 col limit. (significant trouble doesn't mean line wrapping a single statement, it means you're having trouble fitting broken lines onto a single line).
Please make the first line of your CL description the same as the CL title. Otherwise, it will look like your cl is "Summary of changes". Deferring to Ben's review otherwise % drive-by nit below. https://codereview.chromium.org/310913002/diff/40001/chrome/browser/ui/browse... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/310913002/diff/40001/chrome/browser/ui/browse... chrome/browser/ui/browser_commands.cc:580: contents->GetMaximumZoomPercent() + 1; Nit: indent is wrong, same below.
On 2014/06/16 17:41:46, Alexei Svitkine wrote: > Please make the first line of your CL description the same as the CL title. > Otherwise, it will look like your cl is "Summary of changes". > > Deferring to Ben's review otherwise % drive-by nit below. > > https://codereview.chromium.org/310913002/diff/40001/chrome/browser/ui/browse... > File chrome/browser/ui/browser_commands.cc (right): > > https://codereview.chromium.org/310913002/diff/40001/chrome/browser/ui/browse... > chrome/browser/ui/browser_commands.cc:580: contents->GetMaximumZoomPercent() + > 1; > Nit: indent is wrong, same below. Isn't it 4-space indent for wrapped lines? I made these changes based on Ben's first comments.
On 2014/06/16 18:03:26, eka508 wrote: > On 2014/06/16 17:41:46, Alexei Svitkine wrote: > > Please make the first line of your CL description the same as the CL title. > > Otherwise, it will look like your cl is "Summary of changes". Done > > Deferring to Ben's review otherwise % drive-by nit below. > > > > > https://codereview.chromium.org/310913002/diff/40001/chrome/browser/ui/browse... > > File chrome/browser/ui/browser_commands.cc (right): > > > > > https://codereview.chromium.org/310913002/diff/40001/chrome/browser/ui/browse... > > chrome/browser/ui/browser_commands.cc:580: contents->GetMaximumZoomPercent() + > > 1; > > Nit: indent is wrong, same below. > > Isn't it 4-space indent for wrapped lines? I made these changes based on Ben's > first comments.
On 2014/06/16 18:04:42, eka508 wrote: > On 2014/06/16 18:03:26, eka508 wrote: > > On 2014/06/16 17:41:46, Alexei Svitkine wrote: > > > Please make the first line of your CL description the same as the CL title. > > > Otherwise, it will look like your cl is "Summary of changes". > Done > > > Deferring to Ben's review otherwise % drive-by nit below. > > > > > > > > > https://codereview.chromium.org/310913002/diff/40001/chrome/browser/ui/browse... > > > File chrome/browser/ui/browser_commands.cc (right): > > > > > > > > > https://codereview.chromium.org/310913002/diff/40001/chrome/browser/ui/browse... > > > chrome/browser/ui/browser_commands.cc:580: contents->GetMaximumZoomPercent() > + > > > 1; > > > Nit: indent is wrong, same below. > > Please ignore the last comment. My bad, will upload another patch. Thanks
Fixed indent in browser_commands.cc and lint errors in zoom_controller_unittest.cc Please review Thanks https://codereview.chromium.org/310913002/diff/40001/chrome/browser/ui/zoom/z... File chrome/browser/ui/zoom/zoom_controller_unittest.cc (right): https://codereview.chromium.org/310913002/diff/40001/chrome/browser/ui/zoom/z... chrome/browser/ui/zoom/zoom_controller_unittest.cc:31: using content::SiteInstance; On 2014/06/16 17:37:34, Ben Goodger (Google) wrote: > nit: prevailing style in chromium is to avoid doing this unless you are having > significant trouble keeping within the 80 col limit. (significant trouble > doesn't mean line wrapping a single statement, it means you're having trouble > fitting broken lines onto a single line). Hi Ben, Did you mean to comment about a particular line in this file or just something more general.
https://codereview.chromium.org/310913002/diff/40001/chrome/browser/ui/zoom/z... File chrome/browser/ui/zoom/zoom_controller_unittest.cc (right): https://codereview.chromium.org/310913002/diff/40001/chrome/browser/ui/zoom/z... chrome/browser/ui/zoom/zoom_controller_unittest.cc:31: using content::SiteInstance; On 2014/06/16 20:25:57, eka508 wrote: > On 2014/06/16 17:37:34, Ben Goodger (Google) wrote: > > nit: prevailing style in chromium is to avoid doing this unless you are having > > significant trouble keeping within the 80 col limit. (significant trouble > > doesn't mean line wrapping a single statement, it means you're having trouble > > fitting broken lines onto a single line). > > Hi Ben, Did you mean to comment about a particular line in this file or just > something more general. I think Ben is suggesting to remove the "using" statements and just prefix things with their namespace in the file itself.
On 2014/06/17 19:44:02, Alexei Svitkine wrote: > https://codereview.chromium.org/310913002/diff/40001/chrome/browser/ui/zoom/z... > File chrome/browser/ui/zoom/zoom_controller_unittest.cc (right): > > https://codereview.chromium.org/310913002/diff/40001/chrome/browser/ui/zoom/z... > chrome/browser/ui/zoom/zoom_controller_unittest.cc:31: using > content::SiteInstance; > On 2014/06/16 20:25:57, eka508 wrote: > > On 2014/06/16 17:37:34, Ben Goodger (Google) wrote: > > > nit: prevailing style in chromium is to avoid doing this unless you are > having > > > significant trouble keeping within the 80 col limit. (significant trouble > > > doesn't mean line wrapping a single statement, it means you're having > trouble > > > fitting broken lines onto a single line). > > > > Hi Ben, Did you mean to comment about a particular line in this file or just > > something more general. > > I think Ben is suggesting to remove the "using" statements and just prefix > things with their namespace in the file itself. Done.
lgtm
The CQ bit was checked by a.sarkar.arun@gmail.com
The CQ bit was checked by a.sarkar.arun@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.sarkar.arun@gmail.com/310913002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
On 2014/06/19 19:44:34, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > android_clang_dbg on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) > android_dbg on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) Looks like OnMaxZoomIn unit test has failed. Working on a fix
Modified unit_tests to pass try jobs. PTAL. Thanks
LGTM % comments https://codereview.chromium.org/310913002/diff/100001/chrome/browser/ui/zoom/... File chrome/browser/ui/zoom/zoom_controller_unittest.cc (right): https://codereview.chromium.org/310913002/diff/100001/chrome/browser/ui/zoom/... chrome/browser/ui/zoom/zoom_controller_unittest.cc:88: TestZoomObserver zoom_observer_; Nit: Protected should be above private: Also, add an empty line between the two. https://codereview.chromium.org/310913002/diff/100001/chrome/browser/ui/zoom/... chrome/browser/ui/zoom/zoom_controller_unittest.cc:104: // TODO: Figure out why Zoom-In is not disabled after Max-zoom is reached. TODO's should have a username behind them. (e.g. TODO(a.sarkar.arun@gmail.com): ... )
PTAL Thanks
LGTM In the future, you should answer each comments directly through the web UI. Given that I think this is good to go, I'll check the cq bit for you.
The CQ bit was checked by asvitkine@chromium.org
On 2014/07/30 20:41:20, Alexei Svitkine wrote: > LGTM > > In the future, you should answer each comments directly through the web UI. > > Given that I think this is good to go, I'll check the cq bit for you. Thanks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.sarkar.arun@gmail.com/310913002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/35523) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/40552) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/35544) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
On 2014/07/30 20:41:20, Alexei Svitkine wrote: > LGTM > > In the future, you should answer each comments directly through the web UI. > > Given that I think this is good to go, I'll check the cq bit for you. Uploaded a clean patch. PTAL Thanks
https://codereview.chromium.org/310913002/diff/140001/chrome/browser/ui/zoom/... File chrome/browser/ui/zoom/zoom_controller_unittest.cc (right): https://codereview.chromium.org/310913002/diff/140001/chrome/browser/ui/zoom/... chrome/browser/ui/zoom/zoom_controller_unittest.cc:249: for (int i = 0; i < 7; ++i) { Please add a comment explaining the purpose of this loop.
lgtm https://codereview.chromium.org/310913002/diff/140001/chrome/browser/ui/zoom/... File chrome/browser/ui/zoom/zoom_controller_unittest.cc (right): https://codereview.chromium.org/310913002/diff/140001/chrome/browser/ui/zoom/... chrome/browser/ui/zoom/zoom_controller_unittest.cc:33: using content::WebContentsTester; Do you need these using decls - I thought you removed them in a previous patchset. https://codereview.chromium.org/310913002/diff/140001/chrome/browser/ui/zoom/... chrome/browser/ui/zoom/zoom_controller_unittest.cc:158: Nit: Remove empty line.
lgtm lgtm https://codereview.chromium.org/310913002/diff/140001/chrome/browser/ui/zoom/... File chrome/browser/ui/zoom/zoom_controller_unittest.cc (right): https://codereview.chromium.org/310913002/diff/140001/chrome/browser/ui/zoom/... chrome/browser/ui/zoom/zoom_controller_unittest.cc:33: using content::WebContentsTester; Do you need these using decls - I thought you removed them in a previous patchset. https://codereview.chromium.org/310913002/diff/140001/chrome/browser/ui/zoom/... chrome/browser/ui/zoom/zoom_controller_unittest.cc:158: Nit: Remove empty line.
PTAL Thanks https://codereview.chromium.org/310913002/diff/140001/chrome/browser/ui/zoom/... File chrome/browser/ui/zoom/zoom_controller_unittest.cc (right): https://codereview.chromium.org/310913002/diff/140001/chrome/browser/ui/zoom/... chrome/browser/ui/zoom/zoom_controller_unittest.cc:158: On 2014/07/31 15:43:33, Alexei Svitkine wrote: > Nit: Remove empty line. Done. https://codereview.chromium.org/310913002/diff/140001/chrome/browser/ui/zoom/... chrome/browser/ui/zoom/zoom_controller_unittest.cc:249: for (int i = 0; i < 7; ++i) { On 2014/07/31 15:41:56, Alexei Svitkine wrote: > Please add a comment explaining the purpose of this loop. Done.
https://codereview.chromium.org/310913002/diff/140001/chrome/browser/ui/zoom/... File chrome/browser/ui/zoom/zoom_controller_unittest.cc (right): https://codereview.chromium.org/310913002/diff/140001/chrome/browser/ui/zoom/... chrome/browser/ui/zoom/zoom_controller_unittest.cc:33: using content::WebContentsTester; On 2014/07/31 15:43:33, Alexei Svitkine wrote: > Do you need these using decls - I thought you removed them in a previous > patchset. My bad. Removing this patch. Thanks
On 2014/07/31 15:43:34, Alexei Svitkine wrote: > lgtm > > lgtm > > https://codereview.chromium.org/310913002/diff/140001/chrome/browser/ui/zoom/... > File chrome/browser/ui/zoom/zoom_controller_unittest.cc (right): > > https://codereview.chromium.org/310913002/diff/140001/chrome/browser/ui/zoom/... > chrome/browser/ui/zoom/zoom_controller_unittest.cc:33: using > content::WebContentsTester; > Do you need these using decls - I thought you removed them in a previous > patchset. > Done > https://codereview.chromium.org/310913002/diff/140001/chrome/browser/ui/zoom/... > chrome/browser/ui/zoom/zoom_controller_unittest.cc:158: > Nit: Remove empty line. Done. PTAL Thanks
lgtm
On 2014/07/31 19:26:35, Alexei Svitkine wrote: > lgtm I think I might have to re-do this! Apparently WebContents doesn't have GetZoomPercent any more. Thanks
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/36051) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/41084) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
On 2014/07/31 19:26:35, Alexei Svitkine wrote: > lgtm Uploaded a patch that uses ZoomController::GetZoomPercent. Hopefully try jobs wouldn't throw error. PTAL. Thanks
lgtm
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
On 2014/08/01 17:00:20, Alexei Svitkine wrote: > lgtm Sorry for all these trouble. Made the unit tests conditional since it fails on Android. Aren't the scripts suppose to pick the bots based on the files changed? Also moved the unit browser_commands_unittest since it has nothing to do with zoom_controller_unittest.cc. PTAL
https://codereview.chromium.org/310913002/diff/220001/chrome/browser/browser_... File chrome/browser/browser_commands_unittest.cc (right): https://codereview.chromium.org/310913002/diff/220001/chrome/browser/browser_... chrome/browser/browser_commands_unittest.cc:234: #if !defined(OS_ANDROID) I don't think this ifdef is needed - the test right above this also uses tab strip model. My guess is this file is not used on Android. So just remove the ifdef/comment. https://codereview.chromium.org/310913002/diff/220001/chrome/browser/ui/zoom/... File chrome/browser/ui/zoom/zoom_controller_unittest.cc (right): https://codereview.chromium.org/310913002/diff/220001/chrome/browser/ui/zoom/... chrome/browser/ui/zoom/zoom_controller_unittest.cc:24: #include "content/public/browser/web_contents.h" Can you revert these changes to this file? Right now your CL is adding a bunch of includes with no code changes to this file.
On 2014/08/04 15:49:15, Alexei Svitkine wrote: > https://codereview.chromium.org/310913002/diff/220001/chrome/browser/browser_... > File chrome/browser/browser_commands_unittest.cc (right): > > https://codereview.chromium.org/310913002/diff/220001/chrome/browser/browser_... > chrome/browser/browser_commands_unittest.cc:234: #if !defined(OS_ANDROID) > I don't think this ifdef is needed - the test right above this also uses tab > strip model. My guess is this file is not used on Android. So just remove the > ifdef/comment. > Done. But Android keeps throwing run time errors ? Aren't bots suppose to skip those? > https://codereview.chromium.org/310913002/diff/220001/chrome/browser/ui/zoom/... > File chrome/browser/ui/zoom/zoom_controller_unittest.cc (right): > > https://codereview.chromium.org/310913002/diff/220001/chrome/browser/ui/zoom/... > chrome/browser/ui/zoom/zoom_controller_unittest.cc:24: #include > "content/public/browser/web_contents.h" > Can you revert these changes to this file? Right now your CL is adding a bunch > of includes with no code changes to this file. Done. PTAL
https://codereview.chromium.org/310913002/diff/240001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/310913002/diff/240001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:1133: // Update the zoom commands when an active tab is selected Nit: Add a . at the end of the sentence. https://codereview.chromium.org/310913002/diff/240001/chrome/browser/ui/zoom/... File chrome/browser/ui/zoom/zoom_controller_unittest.cc (right): https://codereview.chromium.org/310913002/diff/240001/chrome/browser/ui/zoom/... chrome/browser/ui/zoom/zoom_controller_unittest.cc:24: #include "content/public/browser/web_contents.h" I still see a bunch of new includes in the diff for this file. I think you can remove this file entirely from your CL.
On 2014/08/04 16:29:54, Alexei Svitkine wrote: > https://codereview.chromium.org/310913002/diff/240001/chrome/browser/ui/brows... > File chrome/browser/ui/browser_command_controller.cc (right): > > https://codereview.chromium.org/310913002/diff/240001/chrome/browser/ui/brows... > chrome/browser/ui/browser_command_controller.cc:1133: // Update the zoom > commands when an active tab is selected > Nit: Add a . at the end of the sentence. > Done. > https://codereview.chromium.org/310913002/diff/240001/chrome/browser/ui/zoom/... > File chrome/browser/ui/zoom/zoom_controller_unittest.cc (right): > > https://codereview.chromium.org/310913002/diff/240001/chrome/browser/ui/zoom/... > chrome/browser/ui/zoom/zoom_controller_unittest.cc:24: #include > "content/public/browser/web_contents.h" > I still see a bunch of new includes in the diff for this file. > > I think you can remove this file entirely from your CL. Done. PTAL
lgtm
Message was sent while issue was closed.
Change committed as 287401 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
