Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2220573004:
Caption for the Subresource filter bubble prompt. (Closed)
DescriptionCaption for the Subresource filter bubble prompt.
Xib changes: additional row for explanation message added.
BUG=609747
Committed: https://crrev.com/893311650c65a738a6b9d1921a9d485772c37a72
Cr-Commit-Position: refs/heads/master@{#411885}
Patch Set 1 #Patch Set 2 : xib #
Total comments: 6
Patch Set 3 : message #
Total comments: 5
Patch Set 4 : utf16 #Patch Set 5 : utf16 #Patch Set 6 : utf16 tests #Patch Set 7 : fix mac #
Messages
Total messages: 41 (31 generated)
The CQ bit was checked by melandory@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 melandory@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.
melandory@chromium.org changed reviewers: + msw@chromium.org
PTAL (screenshots are attached to the bug). Thanks!
https://codereview.chromium.org/2220573004/diff/20001/chrome/app/nibs/Content... File chrome/app/nibs/ContentSubresourceFilter.xib (right): https://codereview.chromium.org/2220573004/diff/20001/chrome/app/nibs/Content... chrome/app/nibs/ContentSubresourceFilter.xib:71: <rect key="frame" x="8" y="68" width="282" height="14"/> You'll want a reviewer more familiar with Cocoa for this file. https://codereview.chromium.org/2220573004/diff/20001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2220573004/diff/20001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1183: l10n_util::GetStringUTF8(IDS_FILTERED_DECEPTIVE_CONTENT_PROMPT_TITLE)); Can you make this the title, and make IDS_FILTERED_DECEPTIVE_CONTENT_PROMPT_EXPLANATION simply a message within the bubble (instead of a title)? It's a bit odd to introduce a 'caption' above and bigger than the title. Is that done anywhere else? https://codereview.chromium.org/2220573004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/2220573004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:208: caption_label->SetMultiLine(false); Why not allow multi-line? Will this work alright for longer translations?
The CQ bit was checked by melandory@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.
Description was changed from ========== Caption for the Subresource filter bubble prompt. BUG=609747 ========== to ========== Caption for the Subresource filter bubble prompt. Xib changes: additional row for explanation message added. BUG=609747 ==========
https://codereview.chromium.org/2220573004/diff/20001/chrome/app/nibs/Content... File chrome/app/nibs/ContentSubresourceFilter.xib (right): https://codereview.chromium.org/2220573004/diff/20001/chrome/app/nibs/Content... chrome/app/nibs/ContentSubresourceFilter.xib:71: <rect key="frame" x="8" y="68" width="282" height="14"/> On 2016/08/09 19:46:54, msw wrote: > You'll want a reviewer more familiar with Cocoa for this file. Yep, I'll add OWNER for this file. https://codereview.chromium.org/2220573004/diff/20001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2220573004/diff/20001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1183: l10n_util::GetStringUTF8(IDS_FILTERED_DECEPTIVE_CONTENT_PROMPT_TITLE)); On 2016/08/09 19:46:54, msw wrote: > Can you make this the title, and make > IDS_FILTERED_DECEPTIVE_CONTENT_PROMPT_EXPLANATION simply a message within the > bubble (instead of a title)? It's a bit odd to introduce a 'caption' above and > bigger than the title. Is that done anywhere else? Talked with out UX, so we're fine having title and explanation with same font. So made the change. https://codereview.chromium.org/2220573004/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/2220573004/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:208: caption_label->SetMultiLine(false); On 2016/08/09 19:46:54, msw wrote: > Why not allow multi-line? Will this work alright for longer translations? Done.
melandory@chromium.org changed reviewers: + grt@chromium.org
grt@chromium.org: Please review changes in chrome/app/nibs/ContentSubresourceFilter.xib
lgtm with minor nits https://codereview.chromium.org/2220573004/diff/40001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/2220573004/diff/40001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:121: std::string message; nit: order directly after title? https://codereview.chromium.org/2220573004/diff/40001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:183: void set_message(const std::string& message) { nit: match accessor ordering to member decl ordering.
rubberstamp lgtm for ContentSubresourceFilter.xib. please reduce string conversions as mentioned below. https://codereview.chromium.org/2220573004/diff/40001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2220573004/diff/40001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1182: l10n_util::GetStringUTF8(IDS_FILTERED_DECEPTIVE_CONTENT_PROMPT_TITLE)); you're doing two string conversions here: UTF18->UTF8 when you fetch the resource, then UTF8->UTF16 when you use it. can you switch from std::string to base::string16 for the title and message so that these conversions aren't needed?
The CQ bit was checked by melandory@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...)
The CQ bit was checked by melandory@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_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 melandory@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.
https://codereview.chromium.org/2220573004/diff/40001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/2220573004/diff/40001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:121: std::string message; On 2016/08/10 17:30:54, msw wrote: > nit: order directly after title? Done. https://codereview.chromium.org/2220573004/diff/40001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:183: void set_message(const std::string& message) { On 2016/08/10 17:30:54, msw wrote: > nit: match accessor ordering to member decl ordering. Done.
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2220573004/#ps120001 (title: "fix mac")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Caption for the Subresource filter bubble prompt. Xib changes: additional row for explanation message added. BUG=609747 ========== to ========== Caption for the Subresource filter bubble prompt. Xib changes: additional row for explanation message added. BUG=609747 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Caption for the Subresource filter bubble prompt. Xib changes: additional row for explanation message added. BUG=609747 ========== to ========== Caption for the Subresource filter bubble prompt. Xib changes: additional row for explanation message added. BUG=609747 Committed: https://crrev.com/893311650c65a738a6b9d1921a9d485772c37a72 Cr-Commit-Position: refs/heads/master@{#411885} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/893311650c65a738a6b9d1921a9d485772c37a72 Cr-Commit-Position: refs/heads/master@{#411885} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
