Fix extension scrollbar regression bug, and add a regression test to

Fix extension scrollbar regression bug, and add a regression test to catch similar issues in the future.

This changes the extension_install_dialog_view permission strings layout to have a fixed width for the bullet, centered, and subtract that width from the width used to size the permission string label.

This also adds two browser tests: one to test that a scrollbar _does_ show for a very long set of permissions, and one for the specific regression case from the bug.

BUG=385570
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283968

[Chris Thompson, 2014-07-16 00:54:33]

finnur@ could you PTAL?

[Finnur, 2014-07-16 10:36:12]

Happy to see this come to fruition. Minor things...

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (left):

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:58:
namespace {
Why remove this?

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:396: }  //
namespace
Shouldn't this be moved to line 120 (of the file on the right) instead of
removed?

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
(right):

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:54: #include
"ui/views/window/dialog_delegate.h"
Looks like lots of includes were copied from .cc to .h but should be moved
instead (see comment in .h file first, though).

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:60: // Width
of bullet column in BulletedView.
nit: s/of/of the/

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:134: 0,  //
no fixed width
Nit: Not your problem, but can you captitalize the N and add a period at the
end.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:150: 0,  //
no fixed width
Same here.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.h (right):

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.h:53: #include
"ui/views/window/dialog_delegate.h"
Are you sure you need all these includes in the header file?

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.h:58: void
AddResourceIcon(const gfx::ImageSkia* skia_image, void* data);
Does this need to be in the .h file? I only see a reference to it in the .cc
file...

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.h:62:
base::string16 PrepareForDisplay(const base::string16& message,
Same here.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.h:65: //}  //
namespace
Remove?

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.h:90: const
views::ScrollView* scroll_view() const { return scroll_view_; }
nit: Document.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
File
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
(right):

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:57:
ExtensionInstallPrompt(web_contents),
Weird indentation (also colon should be at the beginning of line 57, not end of
56.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:71:
class ScrollbarTest : public ExtensionBrowserTest {
Thank you so much for adding a test for this...

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:151:
return scrollbar->visible();
Just...
return dialog->scroll_view()->vertical_scroll_bar()->visible();

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:166:
ASSERT_TRUE(IsScrollbarVisible()) << "Scrollbar was not visible";
nit: s/was/is/

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:180:
ASSERT_FALSE(IsScrollbarVisible()) << "Scrollbar was visible";
nit: s/was/is/

https://codereview.chromium.org/363523002/diff/120001/chrome/test/data/extens...
File chrome/test/data/extensions/permissions_scrollbar_regression/manifest.json
(right):

https://codereview.chromium.org/363523002/diff/120001/chrome/test/data/extens...
chrome/test/data/extensions/permissions_scrollbar_regression/manifest.json:1: {
Can you move this folder...
   permissions_scrollbar_regression
... into the subfolder install_prompt?

[Chris Thompson, 2014-07-16 20:13:29]

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (left):

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:58:
namespace {
On 2014/07/16 10:36:12, Finnur wrote:
> Why remove this?

Hmm, good point. Added back in for the "static" definitions in this file.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:396: }  //
namespace
On 2014/07/16 10:36:12, Finnur wrote:
> Shouldn't this be moved to line 120 (of the file on the right) instead of
> removed?

Done.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
(right):

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:54: #include
"ui/views/window/dialog_delegate.h"
On 2014/07/16 10:36:11, Finnur wrote:
> Looks like lots of includes were copied from .cc to .h but should be moved
> instead (see comment in .h file first, though).

Shuffled around includes and forward declarations. See reply to comment in .h
file.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:60: // Width
of bullet column in BulletedView.
On 2014/07/16 10:36:11, Finnur wrote:
> nit: s/of/of the/

Done.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:134: 0,  //
no fixed width
On 2014/07/16 10:36:11, Finnur wrote:
> Nit: Not your problem, but can you captitalize the N and add a period at the
> end.

Done.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:150: 0,  //
no fixed width
On 2014/07/16 10:36:11, Finnur wrote:
> Same here.

Done.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.h (right):

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.h:53: #include
"ui/views/window/dialog_delegate.h"
On 2014/07/16 10:36:12, Finnur wrote:
> Are you sure you need all these includes in the header file?

Forward declared as much as I could, and then threw cppclean at these files to
clean up unnecessary includes. I think it's now reduced to the minimum
compilable state.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.h:58: void
AddResourceIcon(const gfx::ImageSkia* skia_image, void* data);
On 2014/07/16 10:36:12, Finnur wrote:
> Does this need to be in the .h file? I only see a reference to it in the .cc
> file...

Missed this one. Removed.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.h:62:
base::string16 PrepareForDisplay(const base::string16& message,
On 2014/07/16 10:36:12, Finnur wrote:
> Same here.

Missed this one. Removed.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.h:65: //}  //
namespace
On 2014/07/16 10:36:12, Finnur wrote:
> Remove?

Missed this one. Removed.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view.h:90: const
views::ScrollView* scroll_view() const { return scroll_view_; }
On 2014/07/16 10:36:12, Finnur wrote:
> nit: Document.

Added a brief comment on what this does/is useful for.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
File
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
(right):

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:57:
ExtensionInstallPrompt(web_contents),
On 2014/07/16 10:36:12, Finnur wrote:
> Weird indentation (also colon should be at the beginning of line 57, not end
of
> 56.

Done.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:71:
class ScrollbarTest : public ExtensionBrowserTest {
On 2014/07/16 10:36:12, Finnur wrote:
> Thank you so much for adding a test for this...

Acknowledged. If I had more time I'd want to try porting some of the tests that
the Cocoa version of this has as well.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:151:
return scrollbar->visible();
On 2014/07/16 10:36:12, Finnur wrote:
> Just...
> return dialog->scroll_view()->vertical_scroll_bar()->visible();

Missed this when I changed it to return the boolean. Fixed.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:166:
ASSERT_TRUE(IsScrollbarVisible()) << "Scrollbar was not visible";
On 2014/07/16 10:36:12, Finnur wrote:
> nit: s/was/is/

Done.

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:180:
ASSERT_FALSE(IsScrollbarVisible()) << "Scrollbar was visible";
On 2014/07/16 10:36:12, Finnur wrote:
> nit: s/was/is/

Done.

https://codereview.chromium.org/363523002/diff/120001/chrome/test/data/extens...
File chrome/test/data/extensions/permissions_scrollbar_regression/manifest.json
(right):

https://codereview.chromium.org/363523002/diff/120001/chrome/test/data/extens...
chrome/test/data/extensions/permissions_scrollbar_regression/manifest.json:1: {
On 2014/07/16 10:36:12, Finnur wrote:
> Can you move this folder...
>    permissions_scrollbar_regression
> ... into the subfolder install_prompt?

Done.

[Finnur, 2014-07-17 10:12:14]

LGTM

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
File
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
(right):

https://codereview.chromium.org/363523002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:71:
class ScrollbarTest : public ExtensionBrowserTest {
That would be fantastic!

[Chris Thompson, 2014-07-17 19:56:07]

The CQ bit was checked by cthomp@chromium.org

[commit-bot: I haz the power, 2014-07-17 19:57:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cthomp@chromium.org/363523002/140001

[commit-bot: I haz the power, 2014-07-17 22:58:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-07-17 23:19:12]

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

[commit-bot: I haz the power, 2014-07-17 23:19:13]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Chris Thompson, 2014-07-18 00:30:31]

The CQ bit was checked by cthomp@chromium.org

[commit-bot: I haz the power, 2014-07-18 00:33:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cthomp@chromium.org/363523002/140001

[commit-bot: I haz the power, 2014-07-18 01:41:37]

Change committed as 283968