New issue
Advanced search Search tips

Issue 686285 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: 2017-11-10
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug
Launch-M-Target: 64-Beta

Blocked on:
issue 774563


Show other hotlists

Hotlists containing this issue:
HarmonyFutureP1s


Sign in to add a comment

Harmony - update infobars

Project Member Reported by pkasting@chromium.org, Jan 27 2017

Issue description

Needs mock.  Still, I can make some changes based on the general Harmony spec, so I'll do those.  In parallel, Alan, if you can create the relevant mock, that'd be fantastic.

Attached is the current trunk appearance on Windows with --secondary-ui-md.
 
default browser infobar screenshot.png
90.3 KB View Download

Comment 1 Deleted

Screenshot of my current checkout.  Not visible: close button size and hover region have been reduced from 24 to 16 to match spec.

Questions I need answered:
* How tall should the bar be?  It's not a dialog, and it's not immediately obvious to me how much padding there should be in it.
* Are the font and icon sizes used here correct?  Font I can probably answer myself with more research, icon I don't know about.
default browser infobar screenshot.png
153 KB View Download
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d62cf13204608ddc4d54a2dc5bbb10abb9fdb0f4

commit d62cf13204608ddc4d54a2dc5bbb10abb9fdb0f4
Author: pkasting <pkasting@chromium.org>
Date: Tue Jan 31 06:56:35 2017

Move some class-scope constants to file-scope in the lone subclass using them.

This will make future changes clearer when these are replaced by runtime-
computed values.

This also changes the layout constants from const to constexpr, so I can make
the new file-scope constants constexpr.  Of course, that won't be needed after
the change above, but it's a reasonable change to make anyway, so going ahead.

BUG= 686285 
TEST=none

Review-Url: https://codereview.chromium.org/2669503002
Cr-Commit-Position: refs/heads/master@{#447202}

[modify] https://crrev.com/d62cf13204608ddc4d54a2dc5bbb10abb9fdb0f4/chrome/browser/ui/views/infobars/confirm_infobar.cc
[modify] https://crrev.com/d62cf13204608ddc4d54a2dc5bbb10abb9fdb0f4/chrome/browser/ui/views/infobars/infobar_view.cc
[modify] https://crrev.com/d62cf13204608ddc4d54a2dc5bbb10abb9fdb0f4/chrome/browser/ui/views/infobars/infobar_view.h
[modify] https://crrev.com/d62cf13204608ddc4d54a2dc5bbb10abb9fdb0f4/ui/views/layout/layout_constants.h

Project Member

Comment 4 by bugdroid1@chromium.org, Feb 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f3ae9f99eb8b04497ec2778d256102a61848eaec

commit f3ae9f99eb8b04497ec2778d256102a61848eaec
Author: pkasting <pkasting@chromium.org>
Date: Tue Feb 07 20:38:31 2017

Position close buttons in bubbles based on a layout delegate constant.

This replaces hardcoded constants in bubble_frame_view.cc with one plumbed from
the layout delegate.  This allows other types of controls to also use this
constant.

BUG= 686285 
TEST=none

Review-Url: https://codereview.chromium.org/2666803005
Cr-Commit-Position: refs/heads/master@{#448723}

[modify] https://crrev.com/f3ae9f99eb8b04497ec2778d256102a61848eaec/chrome/browser/ui/views/chrome_views_delegate.cc
[modify] https://crrev.com/f3ae9f99eb8b04497ec2778d256102a61848eaec/chrome/browser/ui/views/chrome_views_delegate.h
[modify] https://crrev.com/f3ae9f99eb8b04497ec2778d256102a61848eaec/chrome/browser/ui/views/harmony/harmony_layout_delegate.cc
[modify] https://crrev.com/f3ae9f99eb8b04497ec2778d256102a61848eaec/chrome/browser/ui/views/harmony/layout_delegate.cc
[modify] https://crrev.com/f3ae9f99eb8b04497ec2778d256102a61848eaec/chrome/browser/ui/views/harmony/layout_delegate.h
[modify] https://crrev.com/f3ae9f99eb8b04497ec2778d256102a61848eaec/ui/views/bubble/bubble_frame_view.cc
[modify] https://crrev.com/f3ae9f99eb8b04497ec2778d256102a61848eaec/ui/views/layout/layout_constants.h
[modify] https://crrev.com/f3ae9f99eb8b04497ec2778d256102a61848eaec/ui/views/views_delegate.cc
[modify] https://crrev.com/f3ae9f99eb8b04497ec2778d256102a61848eaec/ui/views/views_delegate.h

Summary: Harmony - update infobars (was: Harmony - update default browser infobar)
Labels: Launch-M-Target-64-Beta
NextAction: 2017-11-10
Owner: ----
Status: Available (was: Assigned)
Infobars should be harmonized like toasts.  We'll need to figure out where the close button should go relative to the edge of the window.  Hopefully some work to harmonize the base objects should get most of the subclasses, since I don't think most people add custom controls to their infobars.

Look at the find bar for how toasts are done (using margins on the controls).
The NextAction date has arrived: 2017-11-10

Comment 10 by bsep@chromium.org, Nov 14 2017

Owner: pkasting@chromium.org
Status: Assigned (was: Available)
Load balancing
Close button should be like on find bar: apparent position of 16x16 icon 16 DIP from end (actual distance less due to highlight expansion).  Similarly, the lead icon is 16 DIP from the front of the bar, and there's basically 16 DIP between every component.
Project Member

Comment 12 by bugdroid1@chromium.org, Dec 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cf49b7b795f1911a866582084c7532bc48f4952d

commit cf49b7b795f1911a866582084c7532bc48f4952d
Author: Peter Kasting <pkasting@chromium.org>
Date: Mon Dec 18 23:27:45 2017

Refactor dialog testing framework to allow arbitrary UI testing.

This introduces a new base class, TestBrowserUI, and reorganizes the existing
test code into a series of virtual functions so tests can provide the necessary
implementation for non-dialog cases.

This also adds a single consumer of this new base class, InfoBarUITest, to
verify it works.

Bug:  686285 
Change-Id: I2b5a7c86347f5042ac5218c9fa90e94f68adf577
Reviewed-on: https://chromium-review.googlesource.com/804953
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524855}
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/download/download_danger_prompt_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/extensions/bookmark_app_helper_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/infobars/infobars_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/password_manager/password_manager_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/permissions/permission_request_manager_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/ask_google_for_suggestions_dialog_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/autofill/save_card_bubble_controller_impl_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/collected_cookies_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/extensions/extension_installed_bubble_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/global_error/global_error_browsertest.cc
[rename] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/test/browser_ui_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/test/test_browser_dialog.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/test/test_browser_dialog.h
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/test/test_browser_dialog_mac.h
[add] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/test/test_browser_ui.cc
[add] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/test/test_browser_ui.h
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/update_chrome_dialog_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/accessibility/invert_bubble_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/bookmarks/bookmark_bubble_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/bookmarks/bookmark_editor_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/certificate_selector_dialog_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/chrome_cleaner_dialog_browsertest_win.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/chrome_cleaner_reboot_dialog_browsertest_win.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/desktop_capture/desktop_media_picker_views_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/device_chooser_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/extensions/extension_message_bubble_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/extensions/media_galleries_dialog_views_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/extensions/pwa_confirmation_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/extensions/request_file_system_dialog_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/external_protocol_dialog_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/feature_promos/bookmark_promo_dialog_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/feature_promos/incognito_window_promo_dialog_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/feature_promos/new_tab_promo_dialog_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/first_run_bubble_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/hung_renderer_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/importer/import_lock_dialog_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/page_info/page_info_bubble_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/passwords/password_dialog_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/payments/payment_request_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/profiles/forced_reauthentication_dialog_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/safe_browsing/password_reuse_modal_warning_dialog_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/session_crashed_bubble_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/settings_reset_prompt_dialog_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/toolbar/outdated_upgrade_bubble_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/try_chrome_dialog_win/try_chrome_dialog_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/browser/ui/views/uninstall_view_browsertest.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/test/BUILD.gn
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/chrome/test/base/browser_tests_main.cc
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/components/infobars/core/infobar_manager.h
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/docs/README.md
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/docs/testing/test_browser_dialog.md
[modify] https://crrev.com/cf49b7b795f1911a866582084c7532bc48f4952d/testing/buildbot/filters/mash.browser_tests.filter

Project Member

Comment 13 by bugdroid1@chromium.org, Jan 4 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/42e76a5dcaec9324e740e2094c5eda336ab90c19

commit 42e76a5dcaec9324e740e2094c5eda336ab90c19
Author: Peter Kasting <pkasting@chromium.org>
Date: Thu Jan 04 02:17:29 2018

Don't send empty strings for nonexistent GURLs.

addLinkRange expects non-nil URL pointers to point to nonempty URLs.  But
infobars may have empty link URLs, for example if they're going to custom-handle
the link click by doing something other than a simple navigation.  So pass nil
in those cases.

This fixes a DCHECK exposed by adding infobar UI tests (yay!).

BUG= 686285 
TEST=none

Change-Id: I59235a12ea85cc300725e36fc815b07f845ec23e
Reviewed-on: https://chromium-review.googlesource.com/849418
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526899}
[modify] https://crrev.com/42e76a5dcaec9324e740e2094c5eda336ab90c19/chrome/browser/ui/cocoa/infobars/confirm_infobar_controller.mm

Project Member

Comment 14 by bugdroid1@chromium.org, Jan 4 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/71e57eadc6d1f4566a78126f0dd9585ccc6ded61

commit 71e57eadc6d1f4566a78126f0dd9585ccc6ded61
Author: Peter Kasting <pkasting@chromium.org>
Date: Thu Jan 04 23:47:30 2018

Add UI tests for some infobars.

More are coming; these are just the infobars that don't require refactoring to
easily trigger.

BUG= 686285 
TEST=none

Change-Id: I0ff1a9ea2c9af062378172fd7cbd47cd3b75a485
Reviewed-on: https://chromium-review.googlesource.com/849700
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527152}
[modify] https://crrev.com/71e57eadc6d1f4566a78126f0dd9585ccc6ded61/chrome/browser/infobars/infobars_browsertest.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Jan 6 2018

Project Member

Comment 19 by bugdroid1@chromium.org, Jan 8 2018

Project Member

Comment 20 by bugdroid1@chromium.org, Jan 9 2018

Project Member

Comment 24 by bugdroid1@chromium.org, Jan 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/99dad7411abd4dc9e1f62a313db51f1efffdcb90

commit 99dad7411abd4dc9e1f62a313db51f1efffdcb90
Author: Peter Kasting <pkasting@chromium.org>
Date: Tue Jan 09 05:42:34 2018

Test OutdatedPluginInfoBarDelegate.

BUG= 686285 
TEST=none

Change-Id: I23eb1c6210d2dedcde0849c1b57436072715bf9b
Reviewed-on: https://chromium-review.googlesource.com/852901
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527908}
[modify] https://crrev.com/99dad7411abd4dc9e1f62a313db51f1efffdcb90/chrome/browser/infobars/infobars_browsertest.cc
[modify] https://crrev.com/99dad7411abd4dc9e1f62a313db51f1efffdcb90/chrome/browser/plugins/plugin_installer_observer.cc

Project Member

Comment 28 by bugdroid1@chromium.org, Jan 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ca4fea1aa33c30de86f7d3a238ad456bf82abac9

commit ca4fea1aa33c30de86f7d3a238ad456bf82abac9
Author: Peter Kasting <pkasting@chromium.org>
Date: Wed Jan 10 22:58:51 2018

Miscellaneous changes to make infobar ui tests slightly better.

Basically, show more strings more of the time.  This makes it more obvious what
each infobar does.

BUG= 686285 
TEST=none

Change-Id: I600f8b0cf38adce293f9637b673463a0079fc403
Reviewed-on: https://chromium-review.googlesource.com/857846
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528457}
[modify] https://crrev.com/ca4fea1aa33c30de86f7d3a238ad456bf82abac9/chrome/browser/infobars/infobars_browsertest.cc

Info dump for my own benefit:

App banner: 12x12 / white
Hung plugin: 14x14 / yellow; plugin icon A + crack
DevTools: No icon / yellow
Extension dev tools: No icon / yellow
Incognito connectability: No icon / white
Theme installed: 12x12 / white
NaCl: No icon / yellow
Pepper broker: 15x15 / yellow; plugin icon B
Outdated plugin: 15x15 / yellow; plugin icon B
Reload plugin: 14x14 / yellow; plugin icon A + crack
Plugin observer: 14x14 / yellow; plugin icon A + crack
File access disabled: No icon / yellow
Keystone promotion:
Collected cookies: 14x14 / white
Installation error: No icon / yellow
Alternate nav: 14x14 / white
Bad flags: No icon / yellow
Default browser: 14x14 / white
Google API keys: No icon / yellow
Obsolete system: No icon / yellow
Session crashed: 
Page info: 14x14 / white
Translate: 
Data reduction proxy preview: No icon / yellow
Automation: No icon / yellow
Blockedon: 774563
Project Member

Comment 31 by bugdroid1@chromium.org, Jan 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0e7a30e1cdfa6a3561ff6539474fa2761accba94

commit 0e7a30e1cdfa6a3561ff6539474fa2761accba94
Author: Peter Kasting <pkasting@chromium.org>
Date: Fri Jan 19 01:02:11 2018

Harmonize infobars, part 1: modify element spacing/padding.

This applies the Harmony "toast" spacing values, using margins (a la the find
bar).

BUG= 686285 
TEST=none

Change-Id: I6e60008ec44030661dabcd4d959229d76ee8f833
Reviewed-on: https://chromium-review.googlesource.com/875140
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530373}
[modify] https://crrev.com/0e7a30e1cdfa6a3561ff6539474fa2761accba94/chrome/browser/ui/views/infobars/confirm_infobar.cc
[modify] https://crrev.com/0e7a30e1cdfa6a3561ff6539474fa2761accba94/chrome/browser/ui/views/infobars/confirm_infobar.h
[modify] https://crrev.com/0e7a30e1cdfa6a3561ff6539474fa2761accba94/chrome/browser/ui/views/infobars/infobar_view.cc

Project Member

Comment 32 by bugdroid1@chromium.org, Jan 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b28c3fc1d28f28d14e7c79f005fb5a517fb97bbb

commit b28c3fc1d28f28d14e7c79f005fb5a517fb97bbb
Author: Peter Kasting <pkasting@chromium.org>
Date: Fri Jan 19 03:11:53 2018

Harmonize infobars, part 2: remove trailing periods.

The Harmony spec removes periods when a control has only one phrase or sentence.
This applies even when there is a trailing button, e.g. "Do something [OK]".

BUG= 686285 
TEST=none

Change-Id: I9de4da4cdf94c1e19d86e2c18877e51dc1344e0e
Reviewed-on: https://chromium-review.googlesource.com/875171
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Scott Little <sclittle@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530414}
[modify] https://crrev.com/b28c3fc1d28f28d14e7c79f005fb5a517fb97bbb/chrome/app/chromium_strings.grd
[modify] https://crrev.com/b28c3fc1d28f28d14e7c79f005fb5a517fb97bbb/chrome/app/generated_resources.grd
[modify] https://crrev.com/b28c3fc1d28f28d14e7c79f005fb5a517fb97bbb/chrome/app/google_chrome_strings.grd
[modify] https://crrev.com/b28c3fc1d28f28d14e7c79f005fb5a517fb97bbb/chrome/browser/extensions/crx_installer.cc
[modify] https://crrev.com/b28c3fc1d28f28d14e7c79f005fb5a517fb97bbb/chrome/browser/previews/previews_infobar_delegate.cc
[modify] https://crrev.com/b28c3fc1d28f28d14e7c79f005fb5a517fb97bbb/chrome/test/data/extensions/api_test/webstore_private/incorrect_manifest1.js
[modify] https://crrev.com/b28c3fc1d28f28d14e7c79f005fb5a517fb97bbb/chrome/test/data/extensions/api_test/webstore_private/incorrect_manifest2.js
[modify] https://crrev.com/b28c3fc1d28f28d14e7c79f005fb5a517fb97bbb/extensions/common/file_util_unittest.cc
[modify] https://crrev.com/b28c3fc1d28f28d14e7c79f005fb5a517fb97bbb/extensions/common/manifest_constants.cc
[modify] https://crrev.com/b28c3fc1d28f28d14e7c79f005fb5a517fb97bbb/extensions/strings/extensions_strings.grd

Project Member

Comment 33 by bugdroid1@chromium.org, Jan 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8d295a9e59666b54eefd2e382178aad07d6919fd

commit 8d295a9e59666b54eefd2e382178aad07d6919fd
Author: Peter Kasting <pkasting@chromium.org>
Date: Tue Jan 23 22:40:23 2018

Harmonize infobars part 3: remove infobar types.

This changes all infobars to a white background, instead of having some be
yellow.

BUG= 686285 
TEST=none
TBR=benwells, vasilii, droger

Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I5b393c27bba8e7ee103b0a3cdcedc0609e7748d5
Reviewed-on: https://chromium-review.googlesource.com/875259
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531373}
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/app/generated_resources.grd
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/android/search_permissions/search_geolocation_disclosure_infobar_delegate.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/android/search_permissions/search_geolocation_disclosure_infobar_delegate.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/banners/app_banner_infobar_delegate_desktop.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/banners/app_banner_infobar_delegate_desktop.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/extensions/api/debugger/extension_dev_tools_infobar.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/extensions/api/messaging/incognito_connectability_infobar_delegate.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/extensions/api/messaging/incognito_connectability_infobar_delegate.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/extensions/theme_installed_infobar_delegate.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/extensions/theme_installed_infobar_delegate.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/password_manager/generated_password_saved_infobar_delegate_android.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/password_manager/generated_password_saved_infobar_delegate_android.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/password_manager/password_manager_infobar_delegate_android.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/password_manager/password_manager_infobar_delegate_android.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/permissions/grouped_permission_infobar_delegate_android.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/ui/android/infobars/infobar_container_android.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/ui/cocoa/infobars/infobar_controller.mm
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/ui/cocoa/infobars/infobar_gradient_view.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/ui/cocoa/infobars/infobar_gradient_view.mm
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/ui/collected_cookies_infobar_delegate.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/ui/collected_cookies_infobar_delegate.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/ui/omnibox/alternate_nav_infobar_delegate.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/ui/omnibox/alternate_nav_infobar_delegate.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/ui/page_info/page_info_infobar_delegate.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/ui/page_info/page_info_infobar_delegate.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/ui/startup/default_browser_infobar_delegate.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/ui/startup/default_browser_infobar_delegate.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/ui/views/infobars/infobar_background.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/ui/views/infobars/infobar_background.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/chrome/browser/ui/views/infobars/infobar_view.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/components/autofill/core/browser/autofill_credit_card_filling_infobar_delegate_mobile.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/components/autofill/core/browser/autofill_credit_card_filling_infobar_delegate_mobile.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/components/infobars/core/infobar.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/components/infobars/core/infobar.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/components/infobars/core/infobar_delegate.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/components/infobars/core/infobar_delegate.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/components/translate/core/browser/translate_infobar_delegate.cc
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/components/translate/core/browser/translate_infobar_delegate.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.h
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.mm
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/ios/chrome/browser/upgrade/upgrade_center.mm
[modify] https://crrev.com/8d295a9e59666b54eefd2e382178aad07d6919fd/ios/chrome/browser/web/blocked_popup_tab_helper.mm

See attached: top is the mock, bottom is implementation

The chrome logo looks incorrect. Maybe we can recycle the following asset:
https://chrome-assets.googleplex.com/?search=200-logo_chrome.png&path=src/ui/webui/resources/images&platform=0&grouped=true&flagged=false&shared=1
Screen Shot 2018-02-09 at 3.14.44 PM.png
149 KB View Download
Project Member

Comment 35 by bugdroid1@chromium.org, Feb 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b5fd3adb1a74eb38e20e63a13d9db1dbb60ac066

commit b5fd3adb1a74eb38e20e63a13d9db1dbb60ac066
Author: Peter Kasting <pkasting@chromium.org>
Date: Tue Feb 20 23:46:48 2018

Add a "multiple infobars" browsertest.

This required changing the test framework to correctly verify "n new infobars"
instead of just one.

Along the way I also fixed some crashiness:
* Pass do-nothing callbacks instead of null ones, which fixes crashes while
  closing certain infobars.
* Handle null objects better, which fixes crashes during shutdown.

BUG= 686285 
TEST=none

Change-Id: Ie34f4a7ae97f5ff95040bec0500b7728f22568b1
Reviewed-on: https://chromium-review.googlesource.com/920582
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537935}
[modify] https://crrev.com/b5fd3adb1a74eb38e20e63a13d9db1dbb60ac066/chrome/browser/infobars/infobars_browsertest.cc

Project Member

Comment 36 by bugdroid1@chromium.org, Feb 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fd64994376ab15e8dd927b9395716fd4d7995e2a

commit fd64994376ab15e8dd927b9395716fd4d7995e2a
Author: Peter Kasting <pkasting@chromium.org>
Date: Thu Feb 22 00:37:36 2018

Refactor BubbleBorder to use the border-and-shadow code elsewhere.

Specifically, we want to draw infobars with the same bottom border and shadow.
This patch is a precursor to doing that.

BUG= 686285 
TEST=none

Change-Id: Ie539f737d3a1f80cbeae97b1ffa7462b27f4325e
Reviewed-on: https://chromium-review.googlesource.com/923341
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538280}
[modify] https://crrev.com/fd64994376ab15e8dd927b9395716fd4d7995e2a/ui/views/bubble/bubble_border.cc
[modify] https://crrev.com/fd64994376ab15e8dd927b9395716fd4d7995e2a/ui/views/bubble/bubble_border.h

New logo asset to replace the existing "product.icon" at 
https://chrome-assets.googleplex.com/?search=product.icon&platform=5&order=0&sort=asc&grouped=true&flagged=false
ic_chrome_logo_20dp.svg.zip
222 KB Download
Screen Shot 2018-03-08 at 5.56.39 PM.png
41.7 KB View Download
Project Member

Comment 38 by bugdroid1@chromium.org, Mar 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cfb3ca21bc9f62f0c6723eba8a726ba9decb4653

commit cfb3ca21bc9f62f0c6723eba8a726ba9decb4653
Author: Peter Kasting <pkasting@chromium.org>
Date: Wed Mar 28 23:26:47 2018

Harmonize infobars part 4, suitable-for-M66 edition:

* Increase icon size to 20x20
* Change dividers to match color of bubble border.  This also slightly affects
  the bookmark bar separator color (to make that consistent also).
* Change shadow to match bubbles (specifically, to use their drawing code).
* Remove arrows.  Does not remove arrow-drawing machinery.
* Now that there are no arrows and only a single color scheme, theme infobars
  (matches the detached bookmark bar).
* Now that infobars are themed, match the incognito color scheme (reverts an old
  CL that forced infobars to always use the non-incognito theme).

Most of the above changes are views-only; the only change that affects Mac is
eliminating arrows.

This is aimed at doing the minimal cleanup necessary to implement the above;
https://chromium-review.googlesource.com/c/chromium/src/+/940769 is a more
full-fledged implementation of this that rips out all the arrow-drawing
infrastructure and makes further improvements, but isn't suitable for merging
back.

BUG= 686285 

Change-Id: I4833c5d43912d779950190cf0e44090d7e65c3d3
Reviewed-on: https://chromium-review.googlesource.com/981776
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546653}
[modify] https://crrev.com/cfb3ca21bc9f62f0c6723eba8a726ba9decb4653/chrome/browser/themes/theme_service.cc
[modify] https://crrev.com/cfb3ca21bc9f62f0c6723eba8a726ba9decb4653/chrome/browser/ui/cocoa/infobars/infobar_container_cocoa.mm
[modify] https://crrev.com/cfb3ca21bc9f62f0c6723eba8a726ba9decb4653/chrome/browser/ui/cocoa/infobars/infobar_controller.mm
[modify] https://crrev.com/cfb3ca21bc9f62f0c6723eba8a726ba9decb4653/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/cfb3ca21bc9f62f0c6723eba8a726ba9decb4653/chrome/browser/ui/views/infobars/confirm_infobar.cc
[modify] https://crrev.com/cfb3ca21bc9f62f0c6723eba8a726ba9decb4653/chrome/browser/ui/views/infobars/infobar_background.cc
[modify] https://crrev.com/cfb3ca21bc9f62f0c6723eba8a726ba9decb4653/chrome/browser/ui/views/infobars/infobar_container_view.cc
[modify] https://crrev.com/cfb3ca21bc9f62f0c6723eba8a726ba9decb4653/chrome/browser/ui/views/infobars/infobar_container_view.h
[modify] https://crrev.com/cfb3ca21bc9f62f0c6723eba8a726ba9decb4653/chrome/browser/ui/views/infobars/infobar_view.cc
[modify] https://crrev.com/cfb3ca21bc9f62f0c6723eba8a726ba9decb4653/chrome/browser/ui/views/infobars/infobar_view.h
[modify] https://crrev.com/cfb3ca21bc9f62f0c6723eba8a726ba9decb4653/components/infobars/core/infobar.h
[modify] https://crrev.com/cfb3ca21bc9f62f0c6723eba8a726ba9decb4653/components/infobars/core/infobar_delegate.cc

Labels: Merge-Request-66
Requesting M66 merge for the commit in comment 38 -- basically this is the remainder of Harmony styling of infobars, at least on views.  Mac misses out on a few of the changes, since Mac infobars are not views but native Cocoa and we're not going to rewrite them before MacViews Browser.
Labels: TE-Verified-M67 TE-Verified-67.0.3383.0
Able to reproduce this issue on 67.0.3382.0 using Mac 13.03, Windows 10 and Ubuntu 14.04. Hence verifying the fix on latest canary 67.0.3383.0.

Observing increase in icon size and not seeing arrow. Attaching screenshots for reference.

As fix is working as expected adding Verified labels.

Thanks!
686285_67.0.3383.0.png
75.4 KB View Download
686285_M67.png
71.2 KB View Download
Project Member

Comment 41 by sheriffbot@chromium.org, Mar 29 2018

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: There is .grd file changes and we are only 18 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
(Reviewers, note that the only change to be merged here is the one in comment 38, which has no .grd changes.  Separately, bettes@ is trying to get me to make an icon change for M66, but that hasn't landed yet and I don't want to block on it.)
Labels: -Merge-Review-66 Merge-Approved-66
Approving merge to M66. Branch3359
Project Member

Comment 44 by bugdroid1@chromium.org, Mar 30 2018

Labels: -merge-approved-66 merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a7163c548f083996300e81297767895bb8510311

commit a7163c548f083996300e81297767895bb8510311
Author: Peter Kasting <pkasting@chromium.org>
Date: Fri Mar 30 23:52:23 2018

Harmonize infobars part 4, suitable-for-M66 edition:

* Increase icon size to 20x20
* Change dividers to match color of bubble border.  This also slightly affects
  the bookmark bar separator color (to make that consistent also).
* Change shadow to match bubbles (specifically, to use their drawing code).
* Remove arrows.  Does not remove arrow-drawing machinery.
* Now that there are no arrows and only a single color scheme, theme infobars
  (matches the detached bookmark bar).
* Now that infobars are themed, match the incognito color scheme (reverts an old
  CL that forced infobars to always use the non-incognito theme).

Most of the above changes are views-only; the only change that affects Mac is
eliminating arrows.

This is aimed at doing the minimal cleanup necessary to implement the above;
https://chromium-review.googlesource.com/c/chromium/src/+/940769 is a more
full-fledged implementation of this that rips out all the arrow-drawing
infrastructure and makes further improvements, but isn't suitable for merging
back.

BUG= 686285 

Change-Id: I4833c5d43912d779950190cf0e44090d7e65c3d3
Reviewed-on: https://chromium-review.googlesource.com/981776
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#546653}(cherry picked from commit cfb3ca21bc9f62f0c6723eba8a726ba9decb4653)
Reviewed-on: https://chromium-review.googlesource.com/989052
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#518}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/a7163c548f083996300e81297767895bb8510311/chrome/browser/themes/theme_service.cc
[modify] https://crrev.com/a7163c548f083996300e81297767895bb8510311/chrome/browser/ui/cocoa/infobars/infobar_container_cocoa.mm
[modify] https://crrev.com/a7163c548f083996300e81297767895bb8510311/chrome/browser/ui/cocoa/infobars/infobar_controller.mm
[modify] https://crrev.com/a7163c548f083996300e81297767895bb8510311/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/a7163c548f083996300e81297767895bb8510311/chrome/browser/ui/views/infobars/confirm_infobar.cc
[modify] https://crrev.com/a7163c548f083996300e81297767895bb8510311/chrome/browser/ui/views/infobars/infobar_background.cc
[modify] https://crrev.com/a7163c548f083996300e81297767895bb8510311/chrome/browser/ui/views/infobars/infobar_container_view.cc
[modify] https://crrev.com/a7163c548f083996300e81297767895bb8510311/chrome/browser/ui/views/infobars/infobar_container_view.h
[modify] https://crrev.com/a7163c548f083996300e81297767895bb8510311/chrome/browser/ui/views/infobars/infobar_view.cc
[modify] https://crrev.com/a7163c548f083996300e81297767895bb8510311/chrome/browser/ui/views/infobars/infobar_view.h
[modify] https://crrev.com/a7163c548f083996300e81297767895bb8510311/components/infobars/core/infobar.h
[modify] https://crrev.com/a7163c548f083996300e81297767895bb8510311/components/infobars/core/infobar_delegate.cc

Status: Fixed (was: Assigned)
Calling this fixed.  I don't think Alan is happy with the default browser infobar icon, but it's no worse than pre-Harmony at least.
Labels: TE-Verified-M66 TE-Verified-66.0.3381.0
Able to reproduce this issue on 66.0.3359.66 using Mac 13.03, Windows 10 and Ubuntu 14.04. Hence verifying the fix on latest beta 66.0.3359.81.

Observing increase in icon size and not seeing arrow. Attaching screenshots for reference.

As fix is working as expected adding Verified labels.

Thanks!
686285_M66.png
61.1 KB View Download
686285_66.0.3359.81.png
58.0 KB View Download

Sign in to add a comment