Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2498503002:
Http Bad: Add icons to the http warning message (Closed)
|
Created:
4 years, 1 month ago by lshang Modified:
4 years ago CC:
chromium-reviews, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, agrieve+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHttp Bad: Add icons to the http warning message
For http sites, show gfx::VectorIconId::LOCATION_BAR_HTTP. For broken
https, show gfx::VectorIconId::LOCATION_BAR_HTTPS_INVALID icon.
BUG=662298, 662297
Committed: https://crrev.com/9c5dad4bc57d0d53a43bf5fabea8ff3a64a650e4
Cr-Commit-Position: refs/heads/master@{#435195}
Patch Set 1 #
Total comments: 6
Patch Set 2 : change icon name and color #Patch Set 3 : rebase #Patch Set 4 : change color after confirm #
Total comments: 4
Patch Set 5 : address #Patch Set 6 : indent comment #Patch Set 7 : fix compile error #
Total comments: 9
Patch Set 8 : address #
Total comments: 10
Patch Set 9 : address comments #Patch Set 10 : rebase #Dependent Patchsets: Messages
Total messages: 82 (63 generated)
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Http Bad: Add icon and "Learn more" label to http warning message add icon and learn more BUG= ========== to ========== Http Bad: Add icon and "Learn more" label to http warning message Add icons and "Learn more" sublabel to the http warning message. Also changed the orientation of label and sublabel on Android to make them displayed in one line. BUG=662298, 662297 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:90001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Http Bad: Add icon and "Learn more" label to http warning message Add icons and "Learn more" sublabel to the http warning message. Also changed the orientation of label and sublabel on Android to make them displayed in one line. BUG=662298, 662297 ========== to ========== Http Bad: Add icons to the http warning message For http sites, show omnibox_info icon. For broken https, show omnibox_https_invalid icon. BUG=662298, 662297 ==========
lshang@chromium.org changed reviewers: + mathp@chromium.org
+mathp@ for overall review of this CL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
vabr@chromium.org changed reviewers: + vabr@chromium.org
To spare you waiting for owners approvals after Mathieu reviews: components/password_manager/core/browser/password_autofill_manager.cc LGTM. I have not looked at the rest of the CL, though. Cheers, Vaclav https://codereview.chromium.org/2498503002/diff/110001/components/password_ma... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/2498503002/diff/110001/components/password_ma... components/password_manager/core/browser/password_autofill_manager.cc:233: "", icon_str, nit: Please use std::string() instead of "". The former is faster and makes the type clearer.
https://codereview.chromium.org/2498503002/diff/110001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2498503002/diff/110001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:579: std::string icon_str; I think there is enough here to create an anonymous namespace function that returns a Suggestion. https://codereview.chromium.org/2498503002/diff/110001/components/resources/a... File components/resources/autofill_scaled_resources.grdp (right): https://codereview.chromium.org/2498503002/diff/110001/components/resources/a... components/resources/autofill_scaled_resources.grdp:8: <structure type="chrome_scaled_image" name="IDR_AUTOFILL_HTTP_WARNING" file="autofill/omnibox_info.png" /> why does it have "omnibox" in the name?
https://codereview.chromium.org/2498503002/diff/110001/components/resources/a... File components/resources/autofill_scaled_resources.grdp (right): https://codereview.chromium.org/2498503002/diff/110001/components/resources/a... components/resources/autofill_scaled_resources.grdp:8: <structure type="chrome_scaled_image" name="IDR_AUTOFILL_HTTP_WARNING" file="autofill/omnibox_info.png" /> On 2016/11/16 04:53:09, Mathieu Perreault wrote: > why does it have "omnibox" in the name? Cause it is the same icon used in the omnibox to indicate security connections. On desktops these icons are in the format of vector icons, so I copied the png format icons from Android resources to use for autofill. I'm happy to change for another name for them:-)
On 2016/11/16 05:16:15, lshang wrote: > https://codereview.chromium.org/2498503002/diff/110001/components/resources/a... > File components/resources/autofill_scaled_resources.grdp (right): > > https://codereview.chromium.org/2498503002/diff/110001/components/resources/a... > components/resources/autofill_scaled_resources.grdp:8: <structure > type="chrome_scaled_image" name="IDR_AUTOFILL_HTTP_WARNING" > file="autofill/omnibox_info.png" /> > On 2016/11/16 04:53:09, Mathieu Perreault wrote: > > why does it have "omnibox" in the name? > > Cause it is the same icon used in the omnibox to indicate security connections. > On desktops these icons are in the format of vector icons, so I copied the png > format icons from Android resources to use for autofill. I'm happy to change for > another name for them:-) Yes please, let's have a better name like https_invalid_icon or something specific.
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Patchset #2 (id:130001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/16 05:23:05, Mathieu Perreault wrote: > On 2016/11/16 05:16:15, lshang wrote: > > > https://codereview.chromium.org/2498503002/diff/110001/components/resources/a... > > File components/resources/autofill_scaled_resources.grdp (right): > > > > > https://codereview.chromium.org/2498503002/diff/110001/components/resources/a... > > components/resources/autofill_scaled_resources.grdp:8: <structure > > type="chrome_scaled_image" name="IDR_AUTOFILL_HTTP_WARNING" > > file="autofill/omnibox_info.png" /> > > On 2016/11/16 04:53:09, Mathieu Perreault wrote: > > > why does it have "omnibox" in the name? > > > > Cause it is the same icon used in the omnibox to indicate security > connections. > > On desktops these icons are in the format of vector icons, so I copied the png > > format icons from Android resources to use for autofill. I'm happy to change > for > > another name for them:-) > > Yes please, let's have a better name like https_invalid_icon or something > specific. Done https://codereview.chromium.org/2498503002/diff/110001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2498503002/diff/110001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:579: std::string icon_str; On 2016/11/16 04:53:09, Mathieu Perreault wrote: > I think there is enough here to create an anonymous namespace function that > returns a Suggestion. Done. https://codereview.chromium.org/2498503002/diff/110001/components/password_ma... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/2498503002/diff/110001/components/password_ma... components/password_manager/core/browser/password_autofill_manager.cc:233: "", icon_str, On 2016/11/15 17:46:04, vabr (travelling until 21 Nov) wrote: > nit: Please use std::string() instead of "". The former is faster and makes the > type clearer. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Patchset #3 (id:170001) has been deleted
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mathp@chromium.org changed reviewers: + estade@chromium.org
Hi Liu, I'm sorry for the delay I was travelling! lgtm % these nits. Also adding estade@ for ui/autofill OWNERS https://codereview.chromium.org/2498503002/diff/210001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2498503002/diff/210001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:48: {"http", IDR_AUTOFILL_HTTP_WARNING}, Let's be a little more specific? httpWarning? https://codereview.chromium.org/2498503002/diff/210001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2498503002/diff/210001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:186: Suggestion CreateHttpWarningMessageSuggestionItem(const GURL& source_url) { Add a function comment detailing the conditions under which each type of warning message is created.
You should not need to add either of these new resources. They already exist as vector icons, info_outline.icon and warning.icon. As a general rule we should not be adding new png assets to Chrome. See https://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui
Patchset #5 (id:230001) has been deleted
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lshang@chromium.org changed reviewers: + tedchoc@chromium.org
Also +tedchoc@ for chrome/browser/android/resource_id.h PTAL thanks! On 2016/11/22 16:41:43, Evan Stade wrote: > You should not need to add either of these new resources. They already exist as > vector icons, info_outline.icon and warning.icon. > > As a general rule we should not be adding new png assets to Chrome. See > https://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui Done. Use VectorIconId on desktop. Thanks for reminding! https://codereview.chromium.org/2498503002/diff/210001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2498503002/diff/210001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:48: {"http", IDR_AUTOFILL_HTTP_WARNING}, On 2016/11/22 14:20:32, Mathieu Perreault wrote: > Let's be a little more specific? httpWarning? Done. https://codereview.chromium.org/2498503002/diff/210001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2498503002/diff/210001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:186: Suggestion CreateHttpWarningMessageSuggestionItem(const GURL& source_url) { On 2016/11/22 14:20:32, Mathieu Perreault wrote: > Add a function comment detailing the conditions under which each type of warning > message is created. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/11/23 09:58:27, lshang wrote: > Also +tedchoc@ for chrome/browser/android/resource_id.h > > PTAL thanks! > > On 2016/11/22 16:41:43, Evan Stade wrote: > > You should not need to add either of these new resources. They already exist > as > > vector icons, info_outline.icon and warning.icon. > > > > As a general rule we should not be adding new png assets to Chrome. See > > > https://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui > > Done. Use VectorIconId on desktop. Thanks for reminding! > > https://codereview.chromium.org/2498503002/diff/210001/chrome/browser/ui/auto... > File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): > > https://codereview.chromium.org/2498503002/diff/210001/chrome/browser/ui/auto... > chrome/browser/ui/autofill/autofill_popup_layout_model.cc:48: {"http", > IDR_AUTOFILL_HTTP_WARNING}, > On 2016/11/22 14:20:32, Mathieu Perreault wrote: > > Let's be a little more specific? httpWarning? > > Done. > > https://codereview.chromium.org/2498503002/diff/210001/components/autofill/co... > File components/autofill/core/browser/autofill_manager.cc (right): > > https://codereview.chromium.org/2498503002/diff/210001/components/autofill/co... > components/autofill/core/browser/autofill_manager.cc:186: Suggestion > CreateHttpWarningMessageSuggestionItem(const GURL& source_url) { > On 2016/11/22 14:20:32, Mathieu Perreault wrote: > > Add a function comment detailing the conditions under which each type of > warning > > message is created. > > Done. chrome/browser/android/resource_id.h - lgtm
Description was changed from ========== Http Bad: Add icons to the http warning message For http sites, show omnibox_info icon. For broken https, show omnibox_https_invalid icon. BUG=662298, 662297 ========== to ========== Http Bad: Add icons to the http warning message For http sites, show gfx::VectorIconId::LOCATION_BAR_HTTP. For broken https, show gfx::VectorIconId::LOCATION_BAR_HTTPS_INVALID icon. BUG=662298, 662297 ==========
https://codereview.chromium.org/2498503002/diff/290001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2498503002/diff/290001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:203: std::vector<autofill::Suggestion> suggestions = delegate_->GetSuggestions(); you're copying the entire list of suggestions --- GetSuggestions needs to return a const ref. https://codereview.chromium.org/2498503002/diff/290001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:213: } else if (icon_str == base::ASCIIToUTF16("httpsInvalid")) { nit: no else after return. (x3 in this function) https://codereview.chromium.org/2498503002/diff/290001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:218: NOTREACHED(); nit: no need to explicitly handle this case. Just DCHECK the second check (httpsInvalid equality) instead of making it a conditional. https://codereview.chromium.org/2498503002/diff/290001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2498503002/diff/290001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:200: l10n_util::GetStringUTF8(IDS_AUTOFILL_CREDIT_CARD_HTTP_WARNING_MESSAGE), nit: the documentation for Suggestion says this constructor is for tests only. Can you construct it in a way more similar to the code you deleted further down?
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
https://codereview.chromium.org/2498503002/diff/290001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2498503002/diff/290001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:203: std::vector<autofill::Suggestion> suggestions = delegate_->GetSuggestions(); On 2016/11/28 16:12:02, Evan Stade wrote: > you're copying the entire list of suggestions --- GetSuggestions needs to return > a const ref. This file is doing this in several places. I'd recommend addressing this in a follow-up CL? https://codereview.chromium.org/2498503002/diff/290001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:213: } else if (icon_str == base::ASCIIToUTF16("httpsInvalid")) { On 2016/11/28 16:12:02, Evan Stade wrote: > nit: no else after return. (x3 in this function) Done. https://codereview.chromium.org/2498503002/diff/290001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:218: NOTREACHED(); On 2016/11/28 16:12:02, Evan Stade wrote: > nit: no need to explicitly handle this case. Just DCHECK the second check > (httpsInvalid equality) instead of making it a conditional. Done. Good Point! https://codereview.chromium.org/2498503002/diff/290001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2498503002/diff/290001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:200: l10n_util::GetStringUTF8(IDS_AUTOFILL_CREDIT_CARD_HTTP_WARNING_MESSAGE), On 2016/11/28 16:12:02, Evan Stade wrote: > nit: the documentation for Suggestion says this constructor is for tests only. > Can you construct it in a way more similar to the code you deleted further down? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm https://codereview.chromium.org/2498503002/diff/290001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2498503002/diff/290001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:203: std::vector<autofill::Suggestion> suggestions = delegate_->GetSuggestions(); On 2016/11/29 10:07:35, lshang wrote: > On 2016/11/28 16:12:02, Evan Stade wrote: > > you're copying the entire list of suggestions --- GetSuggestions needs to > return > > a const ref. > > This file is doing this in several places. I'd recommend addressing this in a > follow-up CL? It's unfortunate that the code is set up like this already, but you're adding an n^2 operation. But sure, if it's easier to do in a follow up, by all means. I just didn't think it would take all that long (you can use a LazyInstance for no-ops or make it const* instead of const&). https://codereview.chromium.org/2498503002/diff/310001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2498503002/diff/310001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:129: int icon_width = GetIconImage(row).width(); nit: inline below https://codereview.chromium.org/2498503002/diff/310001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:204: base::string16 icon_str = suggestions[index].icon; const ref https://codereview.chromium.org/2498503002/diff/310001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:214: DCHECK(icon_str == base::ASCIIToUTF16("httpsInvalid")); nit: DCHECK_EQ https://codereview.chromium.org/2498503002/diff/310001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:222: return *(ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(icon_id)); I believe you have an extra set of parens here https://codereview.chromium.org/2498503002/diff/310001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2498503002/diff/310001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:203: cc_field_http_warning_suggestion.icon = base::UTF8ToUTF16(icon_str); nit: I would rearrange this so that you don't need icon_str (i.e. create the Suggestion before the block that determines the string) also you can use ASCIIToUTF16
Thanks all! https://codereview.chromium.org/2498503002/diff/310001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2498503002/diff/310001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:129: int icon_width = GetIconImage(row).width(); On 2016/11/29 15:23:32, Evan Stade wrote: > nit: inline below Done. https://codereview.chromium.org/2498503002/diff/310001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:204: base::string16 icon_str = suggestions[index].icon; On 2016/11/29 15:23:32, Evan Stade wrote: > const ref Done. https://codereview.chromium.org/2498503002/diff/310001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:214: DCHECK(icon_str == base::ASCIIToUTF16("httpsInvalid")); On 2016/11/29 15:23:32, Evan Stade wrote: > nit: DCHECK_EQ Done. https://codereview.chromium.org/2498503002/diff/310001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:222: return *(ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(icon_id)); On 2016/11/29 15:23:32, Evan Stade wrote: > I believe you have an extra set of parens here Done. https://codereview.chromium.org/2498503002/diff/310001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2498503002/diff/310001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:203: cc_field_http_warning_suggestion.icon = base::UTF8ToUTF16(icon_str); On 2016/11/29 15:23:32, Evan Stade wrote: > nit: I would rearrange this so that you don't need icon_str (i.e. create the > Suggestion before the block that determines the string) > > also you can use ASCIIToUTF16 Done.
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, mathp@chromium.org, tedchoc@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2498503002/#ps350001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by lshang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 350001, "attempt_start_ts": 1480495245611370,
"parent_rev": "0bc2df10b3252411dd30644906774151569f8fcb", "commit_rev":
"12bc4a88451b8cb2631e594505b07f5cbd50c5d0"}
Message was sent while issue was closed.
Description was changed from ========== Http Bad: Add icons to the http warning message For http sites, show gfx::VectorIconId::LOCATION_BAR_HTTP. For broken https, show gfx::VectorIconId::LOCATION_BAR_HTTPS_INVALID icon. BUG=662298, 662297 ========== to ========== Http Bad: Add icons to the http warning message For http sites, show gfx::VectorIconId::LOCATION_BAR_HTTP. For broken https, show gfx::VectorIconId::LOCATION_BAR_HTTPS_INVALID icon. BUG=662298, 662297 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:350001)
Message was sent while issue was closed.
Description was changed from ========== Http Bad: Add icons to the http warning message For http sites, show gfx::VectorIconId::LOCATION_BAR_HTTP. For broken https, show gfx::VectorIconId::LOCATION_BAR_HTTPS_INVALID icon. BUG=662298, 662297 ========== to ========== Http Bad: Add icons to the http warning message For http sites, show gfx::VectorIconId::LOCATION_BAR_HTTP. For broken https, show gfx::VectorIconId::LOCATION_BAR_HTTPS_INVALID icon. BUG=662298, 662297 Committed: https://crrev.com/9c5dad4bc57d0d53a43bf5fabea8ff3a64a650e4 Cr-Commit-Position: refs/heads/master@{#435195} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/9c5dad4bc57d0d53a43bf5fabea8ff3a64a650e4 Cr-Commit-Position: refs/heads/master@{#435195} |
