Restored Pre-Harmony Bubble title margin metrics

Restored Pre-Harmony Bubble title margin metrics

Added INSETS_BUBBLE_TITLE metric to distinguish from INSETS_DIALOG_TITLE

BUG=711012
BUG=705907
Review-Url: https://codereview.chromium.org/2819633002
Cr-Commit-Position: refs/heads/master@{#465635}
Committed: https://chromium.googlesource.com/chromium/src/+/5d0f09e42f23950e3937e36e5bdc3a384f732330

[kylix_rd, 2017-04-17 18:54:37]

Description was changed from

==========
Restored Pre-Harmony Bubble title margin metrics

Added INSETS_BUBBLE_TITLE metric to distinguish from INSETS_DIALOG_TITLE

BUG=711012
==========

to

==========
Restored Pre-Harmony Bubble title margin metrics

Added INSETS_BUBBLE_TITLE metric to distinguish from INSETS_DIALOG_TITLE

BUG=711012
==========

[kylix_rd, 2017-04-17 18:54:37]

kylixrd@chromium.org changed reviewers:
+ pkasting@chromium.org, tapted@chromium.org

[kylix_rd, 2017-04-17 18:57:49]

I'll add the appropriate owners once I can get some guidance on using the
BrowserDialogTest framework in interactive_ui_tests. For the tests I've added,
it is working.

[Peter Kasting, 2017-04-17 19:53:36]

LGTM

Further cleanup is possible (e.g. renaming INSETS_PANEL to
INSETS_DIALOG_CONTENTS and in general nuking the word "panel" from the codebase;
combining the CONTENTS/TITLE insets pairs into singles and having callers
compute one from the other, so they can't get out of sync), but doesn't have to
happen in this CL.

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:18: #include
"chrome/browser/ui/views/harmony/chrome_layout_provider.h"
Nit: Can remove this now?

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc
(right):

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:144: void
SetUpOnMainThread() override;
Nit: Combine these two override blocks, since you no longer subclass this class
directly:

  // DialogBrowserTest:
  void SetUpOnMainThread() override;
  void ShowDialog(const std::string& name) override;

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:481:
local_credentials.push_back(base::MakeUnique<autofill::PasswordForm>(form));
Nit: Having to construct on the static and then copy-construct on the heap seems
really lame.  Can we add an n-arg constructor to PasswordForm so this can become
something like this?:

  std::vector<std::unique_ptr<autofill::PasswordForm>> local_credentials = {
    base::MakeUnique<autofill::PasswordForm>(...),
    base::MakeUnique<autofill::PasswordForm>(...),
  };

It seems like there are other places in this file that could benefit too.

This could be done as a separate change.

[tapted, 2017-04-18 05:26:34]

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
File
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc
(right):

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:358:
: public SupportsTestDialog<ManagePasswordsBubbleViewTest> {
Note this file isn't compiled on Mac (but will be soon - see
https://codereview.chromium.org/2808823002/

And in fact ManagePasswordsBubbleViewTest::view() is not correct for MacViews
(and won't be called by this test, and in fact view() returns a
ManagePasswordsIcon which has "TODO(estade): remove this class from Cocoa as it
has been from Views.")

Basically, I don't think we need to inherit from ManagePasswordsBubbleViewTest
-- ManagePasswordsBubbleViewTest doesn't need to exist at all since ::view() is
never called except on Mac+Cocoa.

That's all a verbose way of saying we're not gaining any code-reuse by putting
this file in manage_passwords_bubble_view_interactive_uitest.cc rather than a
new file like manage_passwords_bubble_view_browsertest.cc which I think is the
correct thing to do.

manage_passwords_bubble_view_browsertest.cc probably should have existed long
ago -- some of the tests above don't need to be interactive_ui_tests - one that
does (the one using ui_test_utils::ClickOnView) is disabled for being flaky and
the tests that are doing things like

// Bubble should not be focused by default.
EXPECT_FALSE(ManagePasswordsBubbleView::manage_password_bubble()->GetFocusManager()->GetFocusedView());

are already broken on desktop linux since focus is asynchronous there (i.e. even
if the bubble _did_ take focus it won't happen until after a delay on Linux).

But that's a lot of Yaks. I think we just need a new file --
manage_passwords_bubble_view_browsertest.cc -- that has


#include "chrome/browser/ui/passwords/manage_passwords_test.h"
#include "chrome/browser/ui/test/test_browser_dialog.h"

class ManagePasswordsBubbleDialogViewTest : public
SupportsTestDialog<ManagePasswordsTest> {

 /* stuff below */



that new file --  manage_passwords_bubble_view_browsertest.cc -- won't run yet
on Mac, but we can move it after https://codereview.chromium.org/2808823002/
lands)

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:375:
EXPECT_TRUE(IsBubbleShowing());
I wouldn't bother copying the implementation of this since the machinery for
DialogBrowserTest will fail if a new Widget isn't Show()n, before ShowDialog()
returns.

[tapted, 2017-04-18 07:32:55]

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
File
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc
(right):

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:72:
ManagePasswordsIconView* view() override {
I made https://codereview.chromium.org/2818383004/ to remove this

[kylix_rd, 2017-04-18 16:05:18]

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:18: #include
"chrome/browser/ui/views/harmony/chrome_layout_provider.h"
On 2017/04/17 19:53:36, Peter Kasting wrote:
> Nit: Can remove this now?

Yes. Done.

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
File
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc
(right):

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:72:
ManagePasswordsIconView* view() override {
On 2017/04/18 07:32:54, tapted wrote:
> I made https://codereview.chromium.org/2818383004/ to remove this

Acknowledged.

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:358:
: public SupportsTestDialog<ManagePasswordsBubbleViewTest> {
On 2017/04/18 05:26:34, tapted wrote:
> Note this file isn't compiled on Mac (but will be soon - see
> https://codereview.chromium.org/2808823002/
> 
> And in fact ManagePasswordsBubbleViewTest::view() is not correct for MacViews
> (and won't be called by this test, and in fact view() returns a
> ManagePasswordsIcon which has "TODO(estade): remove this class from Cocoa as
it
> has been from Views.")
> 
> Basically, I don't think we need to inherit from ManagePasswordsBubbleViewTest
> -- ManagePasswordsBubbleViewTest doesn't need to exist at all since ::view()
is
> never called except on Mac+Cocoa.
> 
> That's all a verbose way of saying we're not gaining any code-reuse by putting
> this file in manage_passwords_bubble_view_interactive_uitest.cc rather than a
> new file like manage_passwords_bubble_view_browsertest.cc which I think is the
> correct thing to do.
> 
> manage_passwords_bubble_view_browsertest.cc probably should have existed long
> ago -- some of the tests above don't need to be interactive_ui_tests - one
that
> does (the one using ui_test_utils::ClickOnView) is disabled for being flaky
and
> the tests that are doing things like
> 
> // Bubble should not be focused by default.
>
EXPECT_FALSE(ManagePasswordsBubbleView::manage_password_bubble()->GetFocusManager()->GetFocusedView());
> 
> are already broken on desktop linux since focus is asynchronous there (i.e.
even
> if the bubble _did_ take focus it won't happen until after a delay on Linux).
> 
> But that's a lot of Yaks. I think we just need a new file --
> manage_passwords_bubble_view_browsertest.cc -- that has
> 
> 
> #include "chrome/browser/ui/passwords/manage_passwords_test.h"
> #include "chrome/browser/ui/test/test_browser_dialog.h"
> 
> class ManagePasswordsBubbleDialogViewTest : public
> SupportsTestDialog<ManagePasswordsTest> {
> 
>  /* stuff below */
> 
> 
> 
> that new file --  manage_passwords_bubble_view_browsertest.cc -- won't run yet
> on Mac, but we can move it after https://codereview.chromium.org/2808823002/
> lands)

I've created a new *_browsertest.cc which only contains the
ManagePasswordsBubbleDialogViewTest for now. We can look at moving whatever
tests are appropriate from the *_interactive_uitest.cc file.

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc:375:
EXPECT_TRUE(IsBubbleShowing());
On 2017/04/18 05:26:34, tapted wrote:
> I wouldn't bother copying the implementation of this since the machinery for
> DialogBrowserTest will fail if a new Widget isn't Show()n, before ShowDialog()
> returns.
> 

Acknowledged.

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc
(right):

https://codereview.chromium.org/2819633002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:481:
local_credentials.push_back(base::MakeUnique<autofill::PasswordForm>(form));
On 2017/04/17 19:53:36, Peter Kasting wrote:
> Nit: Having to construct on the static and then copy-construct on the heap
seems
> really lame.  Can we add an n-arg constructor to PasswordForm so this can
become
> something like this?:
> 
>   std::vector<std::unique_ptr<autofill::PasswordForm>> local_credentials = {
>     base::MakeUnique<autofill::PasswordForm>(...),
>     base::MakeUnique<autofill::PasswordForm>(...),
>   };
> 
> It seems like there are other places in this file that could benefit too.

That makes sense. I do, however, approach it from the angle that this code is
clearly working since it based on existing code within this same file. In a
"Hippocratic" sense, I'm not doing any harm with this code.

> This could be done as a separate change.

I'll add it to the list :)

[kylix_rd, 2017-04-18 18:50:24]

Description was changed from

==========
Restored Pre-Harmony Bubble title margin metrics

Added INSETS_BUBBLE_TITLE metric to distinguish from INSETS_DIALOG_TITLE

BUG=711012
==========

to

==========
Restored Pre-Harmony Bubble title margin metrics

Added INSETS_BUBBLE_TITLE metric to distinguish from INSETS_DIALOG_TITLE

BUG=711012
BUG=705907
==========

[kylix_rd, 2017-04-18 18:57:26]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 18:58:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 19:19:42]

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

[commit-bot: I haz the power, 2017-04-18 19:19:43]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kylix_rd, 2017-04-18 19:26:28]

Patchset #4 (id:60001) has been deleted

[kylix_rd, 2017-04-18 19:26:38]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 19:27:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 20:31:10]

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

[commit-bot: I haz the power, 2017-04-18 20:31:11]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-04-19 00:25:03]

lgtm with the following.

https://codereview.chromium.org/2818383004/ is in the CQ now (tree is closed
though). Note there won't be a patch conflict, so we should coordinate to make
sue we don't accidentally land a compile error :)

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
File
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc
(right):

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc:10:
#include "chrome/browser/ui/views/frame/browser_view.h"
I think some of these can be removed (this line and the next 2 unless i've
missed something).

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc:20:
// Remove once https://codereview.chromium.org/2818383004/ lands
in the CQ now... :)

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc
(right):

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:470: //
DialogBrowserTest methods for interactive dialog invocation
nit: full stop at end

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:472: GURL
origin("https://example.com");
EXPECT_EQ("PopupAutoSigninPrompt", name); ?

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:486:
ASSERT_TRUE(controller()->current_account_chooser());
optional: EXPECT_TRUE? (I tend to avoid `ASSERT_FOO` outside of the *TEST_F
methods since all it does is `return;` early and, e.g., it can only be used in
methods that return `void`).

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view_browsertest.cc (right):

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view_browsertest.cc:5: #include
"session_crashed_bubble_view.h"
needs the full path

[kylix_rd, 2017-04-19 14:38:14]

kylixrd@chromium.org changed reviewers:
+ vabr@chromium.org

[kylix_rd, 2017-04-19 14:53:39]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 14:54:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kylix_rd, 2017-04-19 14:55:37]

Addressed the feedback. Added vabr@. 

vabr@, please review the changes in the c/b/ui/views/passwords folder.

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
File
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc
(right):

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc:10:
#include "chrome/browser/ui/views/frame/browser_view.h"
On 2017/04/19 00:25:03, tapted wrote:
> I think some of these can be removed (this line and the next 2 unless i've
> missed something).

Done.

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc:20:
// Remove once https://codereview.chromium.org/2818383004/ lands
On 2017/04/19 00:25:03, tapted wrote:
> in the CQ now... :)

Done.

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc
(right):

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:470: //
DialogBrowserTest methods for interactive dialog invocation
On 2017/04/19 00:25:03, tapted wrote:
> nit: full stop at end

Done.

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:472: GURL
origin("https://example.com");
On 2017/04/19 00:25:03, tapted wrote:
> EXPECT_EQ("PopupAutoSigninPrompt", name); ?

Done.

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:486:
ASSERT_TRUE(controller()->current_account_chooser());
On 2017/04/19 00:25:03, tapted wrote:
> optional: EXPECT_TRUE? (I tend to avoid `ASSERT_FOO` outside of the *TEST_F
> methods since all it does is `return;` early and, e.g., it can only be used in
> methods that return `void`).

I was following the existing pattern among all the other test methods.

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view_browsertest.cc (right):

https://codereview.chromium.org/2819633002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view_browsertest.cc:5: #include
"session_crashed_bubble_view.h"
On 2017/04/19 00:25:03, tapted wrote:
> needs the full path

Done.

[vabr (Chromium), 2017-04-19 15:07:30]

c/b/ui/views/passwords LGTM,

thanks for fixing this for us and for adding the tests!

Vaclav

https://codereview.chromium.org/2819633002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc
(right):

https://codereview.chromium.org/2819633002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:483:
form.display_name = base::ASCIIToUTF16("Peter Pen");
typo? Pen -> Pan :)
(If this is intentional, that's fine as well.)

[kylix_rd, 2017-04-19 15:28:48]

The CQ bit was checked by kylixrd@chromium.org

[kylix_rd, 2017-04-19 15:28:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, tapted@chromium.org, vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2819633002/#ps120001
(title: "Fixed a typo")

[kylix_rd, 2017-04-19 15:29:02]

https://codereview.chromium.org/2819633002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc
(right):

https://codereview.chromium.org/2819633002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc:483:
form.display_name = base::ASCIIToUTF16("Peter Pen");
On 2017/04/19 15:07:29, vabr (Chromium) wrote:
> typo? Pen -> Pan :)
> (If this is intentional, that's fine as well.)

Maybe? It was copied from some of the existing code in the file. I've fixed them
all...

[commit-bot: I haz the power, 2017-04-19 15:29:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 16:45:18]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492615728006580,
"parent_rev": "a79078c894d66ddf908750e1b0f1a749a9b0fb01", "commit_rev":
"5d0f09e42f23950e3937e36e5bdc3a384f732330"}

[commit-bot: I haz the power, 2017-04-19 16:45:25]

Description was changed from

==========
Restored Pre-Harmony Bubble title margin metrics

Added INSETS_BUBBLE_TITLE metric to distinguish from INSETS_DIALOG_TITLE

BUG=711012
BUG=705907
==========

to

==========
Restored Pre-Harmony Bubble title margin metrics

Added INSETS_BUBBLE_TITLE metric to distinguish from INSETS_DIALOG_TITLE

BUG=711012
BUG=705907

Review-Url: https://codereview.chromium.org/2819633002
Cr-Commit-Position: refs/heads/master@{#465635}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d0f09e42f23950e3937e36e5bdc...
==========

[commit-bot: I haz the power, 2017-04-19 16:45:26]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/5d0f09e42f23950e3937e36e5bdc...