Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2076253002:
Simplify the text in the external protocol confirmation dialog. (Closed)
DescriptionSimplify the text in the external protocol confirmation dialog.
The external protocol handler dialog currently presents users with
a very large amount of jargon-filled text. This CL simplifies the dialog
down to just a title containing the app name and the existing checkbox.
On Windows 7 and earlier, the registry is queried for an app path,
which is displayed in the dialog. This CL modifies the query to
retrieve a program name from the app path, falling back to just
the executable name if the retrieval doesn't succeed.
A new histogram to log the number of times users check the
checkbox in the dialog is also added.
BUG=601725
Committed: https://crrev.com/0c9a5065a8a7c4b760b79eb996548b1921ed05ea
Cr-Commit-Position: refs/heads/master@{#423664}
Patch Set 1 #Patch Set 2 : Convert app path to app name on Windows 7 and older #Patch Set 3 : Add metric for when the checkbox is checked #
Total comments: 1
Patch Set 4 : Remove querying for app name #Patch Set 5 : Update based on UI review #
Total comments: 10
Patch Set 6 : Address comments #
Total comments: 33
Patch Set 7 : Address comments #
Total comments: 7
Patch Set 8 : Fix ChromeOS compile #
Total comments: 3
Patch Set 9 : Address comments, remove period #
Total comments: 6
Patch Set 10 : Fix variables #Patch Set 11 : Const #Messages
Total messages: 64 (36 generated)
Description was changed from ========== Simplify the text in the external protocol confirmation dialog. The external protocol handler dialog currently presents users with a very large amount of jargon-filled text. This CL simplifies the dialog down to just a title and a single line message. BUG=601725 ========== to ========== Simplify the text in the external protocol confirmation dialog. The external protocol handler dialog currently presents users with a very large amount of jargon-filled text. This CL simplifies the dialog down to just a title containing the app name and the existing checkbox. On Windows 7 and earlier, the registry is queried for an app path, which is displayed in the dialog. This CL modifies the query to retrieve a program name from the app path, falling back to just the executable name if the retrieval doesn't succeed. A new histogram to log the number of times users check the checkbox in the dialog is also added. BUG=601725 ==========
mgiuca@chromium.org changed reviewers: + mgiuca@chromium.org
https://codereview.chromium.org/2076253002/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/2076253002/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:216: base::FileVersionInfo* info = No "base::". This class isn't in a namespace. (whyyyyy?)
I accidentally triggered this code review. So I won't consider it "out for review" just yet, but I'm happy to review it, so ping when ready.
dominickn@chromium.org changed reviewers: + isherman@chromium.org, msw@chromium.org, oshima@chromium.org
+msw: PTAL at chrome/browser/ui +isherman: PTAL at tools/ +oshima: PTAL at chrome/browser/chromeos Thanks!
dominickn@chromium.org changed reviewers: + meacer@chromium.org
+meacer: PTAL at chrome/browser/external_protocol. Thanks!
https://codereview.chromium.org/2076253002/diff/80001/chrome/browser/external... File chrome/browser/external_protocol/external_protocol_handler.cc (right): https://codereview.chromium.org/2076253002/diff/80001/chrome/browser/external... chrome/browser/external_protocol/external_protocol_handler.cc:135: "ExternalProtocolDialog.RememberCheckbox"; I think it'll be nicer if the UMA code was moved here instead of exposing this static: void ExternalProtocolHandler::RecordMetrics(bool selected) { UMA_HISTOGRAM_SPARSE_SLOWLY(kRememberCheckedHistogram, selected); }
https://codereview.chromium.org/2076253002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/external_protocol_dialog.cc (right): https://codereview.chromium.org/2076253002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/external_protocol_dialog.cc:86: UMA_HISTOGRAM_SPARSE_SLOWLY( UMA_HISTOGRAM_BOOLEAN is probably more appropriate. https://codereview.chromium.org/2076253002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/external_protocol_dialog.cc:103: message_box_view_->IsCheckBoxSelected()); nit: I'd recommend adding a wrapper function for emitting to this histogram, so that you don't get the macro's expanded code twice. (And also to reduce a bit of repeated code.) https://codereview.chromium.org/2076253002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2076253002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:17084: +<histogram name="ExternalProtocolDialog.RememberCheckbox" enum="BooleanChecked"> It looks like you're adding a new top-level group name, "ExternalProtocolDialog". Could you please check whether there is already an existing group name that would make sense for this metric? If not, I'd encourage you to spend a bit of time thinking about whether this new name is general enough. Is there a more general, but still reasonably specific, name that would make sense for this and related metrics? (It might be that this is already the best name -- I'm just generally cautious about adding new top-level names, since they are added to what is effectively a global namespace.) https://codereview.chromium.org/2076253002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:17088: + to remember their choice of opening or not opening the specified app. Please document at what point this is logged, i.e. when the dialog is closed.
The CQ bit was checked by dominickn@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.
Thanks! https://codereview.chromium.org/2076253002/diff/80001/chrome/browser/external... File chrome/browser/external_protocol/external_protocol_handler.cc (right): https://codereview.chromium.org/2076253002/diff/80001/chrome/browser/external... chrome/browser/external_protocol/external_protocol_handler.cc:135: "ExternalProtocolDialog.RememberCheckbox"; On 2016/10/04 01:28:56, Mustafa Emre Acer wrote: > I think it'll be nicer if the UMA code was moved here instead of exposing this > static: > > void ExternalProtocolHandler::RecordMetrics(bool selected) { > UMA_HISTOGRAM_SPARSE_SLOWLY(kRememberCheckedHistogram, > selected); > } > Done. https://codereview.chromium.org/2076253002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/external_protocol_dialog.cc (right): https://codereview.chromium.org/2076253002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/external_protocol_dialog.cc:86: UMA_HISTOGRAM_SPARSE_SLOWLY( On 2016/10/04 01:34:45, Ilya Sherman wrote: > UMA_HISTOGRAM_BOOLEAN is probably more appropriate. Done. https://codereview.chromium.org/2076253002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/external_protocol_dialog.cc:103: message_box_view_->IsCheckBoxSelected()); On 2016/10/04 01:34:45, Ilya Sherman wrote: > nit: I'd recommend adding a wrapper function for emitting to this histogram, so > that you don't get the macro's expanded code twice. (And also to reduce a bit > of repeated code.) Done. https://codereview.chromium.org/2076253002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2076253002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:17084: +<histogram name="ExternalProtocolDialog.RememberCheckbox" enum="BooleanChecked"> On 2016/10/04 01:34:45, Ilya Sherman wrote: > It looks like you're adding a new top-level group name, > "ExternalProtocolDialog". Could you please check whether there is already an > existing group name that would make sense for this metric? If not, I'd > encourage you to spend a bit of time thinking about whether this new name is > general enough. Is there a more general, but still reasonably specific, name > that would make sense for this and related metrics? (It might be that this is > already the best name -- I'm just generally cautious about adding new top-level > names, since they are added to what is effectively a global namespace.) I changed the top level name to BrowserDialogs, since ExternalProtocolDialog is very specific. I couldn't find another top level name that seemed really appropriate (the closest was JsDialogs, but this isn't a JsDialog). https://codereview.chromium.org/2076253002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:17088: + to remember their choice of opening or not opening the specified app. On 2016/10/04 01:34:45, Ilya Sherman wrote: > Please document at what point this is logged, i.e. when the dialog is closed. Done.
On 2016/10/04 12:37:01, dominickn wrote: > Thanks! > > https://codereview.chromium.org/2076253002/diff/80001/chrome/browser/external... > File chrome/browser/external_protocol/external_protocol_handler.cc (right): > > https://codereview.chromium.org/2076253002/diff/80001/chrome/browser/external... > chrome/browser/external_protocol/external_protocol_handler.cc:135: > "ExternalProtocolDialog.RememberCheckbox"; > On 2016/10/04 01:28:56, Mustafa Emre Acer wrote: > > I think it'll be nicer if the UMA code was moved here instead of exposing this > > static: > > > > void ExternalProtocolHandler::RecordMetrics(bool selected) { > > UMA_HISTOGRAM_SPARSE_SLOWLY(kRememberCheckedHistogram, > > selected); > > } > > > > Done. > > https://codereview.chromium.org/2076253002/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/external_protocol_dialog.cc (right): > > https://codereview.chromium.org/2076253002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/external_protocol_dialog.cc:86: > UMA_HISTOGRAM_SPARSE_SLOWLY( > On 2016/10/04 01:34:45, Ilya Sherman wrote: > > UMA_HISTOGRAM_BOOLEAN is probably more appropriate. > > Done. > > https://codereview.chromium.org/2076253002/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/external_protocol_dialog.cc:103: > message_box_view_->IsCheckBoxSelected()); > On 2016/10/04 01:34:45, Ilya Sherman wrote: > > nit: I'd recommend adding a wrapper function for emitting to this histogram, > so > > that you don't get the macro's expanded code twice. (And also to reduce a bit > > of repeated code.) > > Done. > > https://codereview.chromium.org/2076253002/diff/80001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2076253002/diff/80001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:17084: +<histogram > name="ExternalProtocolDialog.RememberCheckbox" enum="BooleanChecked"> > On 2016/10/04 01:34:45, Ilya Sherman wrote: > > It looks like you're adding a new top-level group name, > > "ExternalProtocolDialog". Could you please check whether there is already an > > existing group name that would make sense for this metric? If not, I'd > > encourage you to spend a bit of time thinking about whether this new name is > > general enough. Is there a more general, but still reasonably specific, name > > that would make sense for this and related metrics? (It might be that this is > > already the best name -- I'm just generally cautious about adding new > top-level > > names, since they are added to what is effectively a global namespace.) > > I changed the top level name to BrowserDialogs, since ExternalProtocolDialog is > very specific. I couldn't find another top level name that seemed really > appropriate (the closest was JsDialogs, but this isn't a JsDialog). > > https://codereview.chromium.org/2076253002/diff/80001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:17088: + to remember their choice of > opening or not opening the specified app. > On 2016/10/04 01:34:45, Ilya Sherman wrote: > > Please document at what point this is logged, i.e. when the dialog is closed. > > Done. chrome/browser/external_protocol LGTM
(and forgot to send the comments) https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/externa... File chrome/browser/external_protocol/external_protocol_handler.h (right): https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/externa... chrome/browser/external_protocol/external_protocol_handler.h:49: static void RecordMetrics(bool selected); nit: Document |selected|? https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/externa... chrome/browser/external_protocol/external_protocol_handler.h:49: static void RecordMetrics(bool selected); Also, in general order of methods in the .cc file should follow the header. Feel free to ignore though, since this class is already out of order, unless you are willing to reorder the other methods :)
cros lgtm with one nit https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/external_protocol_dialog.cc (right): https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/external_protocol_dialog.cc:103: views::MessageBoxView::InitParams params(base::ASCIIToUTF16("")); nit: string16()
some minor comments for chrome/browser/ui https://codereview.chromium.org/2076253002/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (left): https://codereview.chromium.org/2076253002/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:11282: <message name="IDS_EXTERNAL_PROTOCOL_CANCEL_BUTTON_TEXT" desc="Button in External Protocol Dialog that cancels the application launch"> Can we remove this and just use IDS_CANCEL? https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... File chrome/browser/ui/external_protocol_dialog_delegate.cc (right): https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:7: #include <string> nit: remove if unused https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:9: #include "base/strings/utf_string_conversions.h" nit: remove if unused https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:10: #include "base/threading/thread.h" nit: remove if unused https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:11: #include "base/threading/thread_restrictions.h" nit: remove if unused https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:20: const size_t kMaxCommandSize = 256; 256 chars is rather long to put in a button; what does it look like? https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:27: } // namespace nit: blank line before https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:43: if (button == ui::DIALOG_BUTTON_OK) nit: curly braces (or put the cancel in the conditional) https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:46: else nit: no else after return https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:51: return base::ASCIIToUTF16(""); nit: return base::string16(); https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:65: const GURL& url, optional nit: fix wrapping; (fits on line above) https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:68: ExternalProtocolHandler::SetBlockState( optional nit: fix indent https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:77: const GURL& url, optional nit: fix wrapping; (fits on line above) https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:80: ExternalProtocolHandler::SetBlockState( optional nit: fix indent
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2076253002/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (left): https://codereview.chromium.org/2076253002/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:11282: <message name="IDS_EXTERNAL_PROTOCOL_CANCEL_BUTTON_TEXT" desc="Button in External Protocol Dialog that cancels the application launch"> On 2016/10/04 18:05:01, msw wrote: > Can we remove this and just use IDS_CANCEL? Done, though it now requires an include of components_strings.h. https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/external_protocol_dialog.cc (right): https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/external_protocol_dialog.cc:103: views::MessageBoxView::InitParams params(base::ASCIIToUTF16("")); On 2016/10/04 17:30:59, oshima wrote: > nit: string16() Done. https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/externa... File chrome/browser/external_protocol/external_protocol_handler.h (right): https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/externa... chrome/browser/external_protocol/external_protocol_handler.h:49: static void RecordMetrics(bool selected); On 2016/10/04 17:18:28, Mustafa Emre Acer wrote: > nit: Document |selected|? Done. https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... File chrome/browser/ui/external_protocol_dialog_delegate.cc (right): https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:7: #include <string> On 2016/10/04 18:05:01, msw wrote: > nit: remove if unused Done. https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:9: #include "base/strings/utf_string_conversions.h" On 2016/10/04 18:05:01, msw wrote: > nit: remove if unused Done. https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:10: #include "base/threading/thread.h" On 2016/10/04 18:05:01, msw wrote: > nit: remove if unused Done. https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:11: #include "base/threading/thread_restrictions.h" On 2016/10/04 18:05:01, msw wrote: > nit: remove if unused Done. https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:20: const size_t kMaxCommandSize = 256; On 2016/10/04 18:05:01, msw wrote: > 256 chars is rather long to put in a button; what does it look like? Good point, it looks pretty weird. Truncated to 64. https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:27: } // namespace On 2016/10/04 18:05:01, msw wrote: > nit: blank line before Done. https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:43: if (button == ui::DIALOG_BUTTON_OK) On 2016/10/04 18:05:01, msw wrote: > nit: curly braces (or put the cancel in the conditional) Done. https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:46: else On 2016/10/04 18:05:01, msw wrote: > nit: no else after return Done. https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:51: return base::ASCIIToUTF16(""); On 2016/10/04 18:05:02, msw wrote: > nit: return base::string16(); Done. https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:65: const GURL& url, On 2016/10/04 18:05:01, msw wrote: > optional nit: fix wrapping; (fits on line above) Done. https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:68: ExternalProtocolHandler::SetBlockState( On 2016/10/04 18:05:01, msw wrote: > optional nit: fix indent Done. https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:77: const GURL& url, On 2016/10/04 18:05:01, msw wrote: > optional nit: fix wrapping; (fits on line above) Done. https://codereview.chromium.org/2076253002/diff/100001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:80: ExternalProtocolHandler::SetBlockState( On 2016/10/04 18:05:01, msw wrote: > optional nit: fix indent 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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
c/b/ui lgtm with minor comments/questions https://codereview.chromium.org/2076253002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/external_protocol_dialog.mm (right): https://codereview.chromium.org/2076253002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/external_protocol_dialog.mm:69: IDS_EXTERNAL_PROTOCOL_OK_BUTTON_TEXT, appName)]; Should this ok button also be elided to 64 chars? It might be good to post a picture on the bug of what a long appName looks like here. https://codereview.chromium.org/2076253002/diff/120001/chrome/browser/ui/exte... File chrome/browser/ui/external_protocol_dialog_delegate.cc (right): https://codereview.chromium.org/2076253002/diff/120001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:7: #include "base/strings/utf_string_conversions.h" repeated nit: remove if unused (does indeed appear to be unused) https://codereview.chromium.org/2076253002/diff/120001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:17: const size_t kMaxCommandSize = 64: You might want to post a picture on the bug and check that everyone is alright with an okay button that has text: "Open Super Long Application Title String For My Super Cool Applicatio?" It's unlikely that many applications will have such long names, but it's good to see the worst possible form this button can take.
Thanks! https://codereview.chromium.org/2076253002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/external_protocol_dialog.mm (right): https://codereview.chromium.org/2076253002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/external_protocol_dialog.mm:69: IDS_EXTERNAL_PROTOCOL_OK_BUTTON_TEXT, appName)]; On 2016/10/04 23:46:28, msw wrote: > Should this ok button also be elided to 64 chars? It might be good to post a > picture on the bug of what a long appName looks like here. Mac has never had any eliding at all, which leads me to suspect that long app names aren't that much of a concern on that platform. Scrolling through my /Applications directory shows that the longest app name is just 23 chars. I think the Mac UI in general is pretty aggressive in preferring short app names so I'm leaning towards not having to do the ellision. Thoughts? :) https://codereview.chromium.org/2076253002/diff/120001/chrome/browser/ui/exte... File chrome/browser/ui/external_protocol_dialog_delegate.cc (right): https://codereview.chromium.org/2076253002/diff/120001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:7: #include "base/strings/utf_string_conversions.h" On 2016/10/04 23:46:28, msw wrote: > repeated nit: remove if unused (does indeed appear to be unused) Bah, sorry. I put it back in to test a super long app name and didn't remove it again. Done. https://codereview.chromium.org/2076253002/diff/120001/chrome/browser/ui/exte... chrome/browser/ui/external_protocol_dialog_delegate.cc:17: const size_t kMaxCommandSize = 64: On 2016/10/04 23:46:28, msw wrote: > You might want to post a picture on the bug and check that everyone is alright > with an okay button that has text: > "Open Super Long Application Title String For My Super Cool Applicatio?" > It's unlikely that many applications will have such long names, but it's good to > see the worst possible form this button can take. Done - https://bugs.chromium.org/p/chromium/issues/detail?id=601725#c62
still lgtm https://codereview.chromium.org/2076253002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/external_protocol_dialog.mm (right): https://codereview.chromium.org/2076253002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/external_protocol_dialog.mm:69: IDS_EXTERNAL_PROTOCOL_OK_BUTTON_TEXT, appName)]; On 2016/10/05 00:06:47, dominickn wrote: > On 2016/10/04 23:46:28, msw wrote: > > Should this ok button also be elided to 64 chars? It might be good to post a > > picture on the bug of what a long appName looks like here. > > Mac has never had any eliding at all, which leads me to suspect that long app > names aren't that much of a concern on that platform. Scrolling through my > /Applications directory shows that the longest app name is just 23 chars. > > I think the Mac UI in general is pretty aggressive in preferring short app names > so I'm leaning towards not having to do the ellision. Thoughts? :) Fine by me, but I'd defer to any reviewer more familiar with Mac.
mgiuca@chromium.org changed reviewers: - mgiuca@chromium.org
https://codereview.chromium.org/2076253002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2076253002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:4823: +<histogram name="BrowserDialogs.ExternalProtocol.RememberCheckbox" Hmm, "BrowserDialogs" is more general, but I'm not quite sure that it's quite right for the top-level name. If you were to componentize your feature, what is the component name that you would choose for it? That's often a good way to come up with a top-level UMA name as well =)
The CQ bit was checked by dominickn@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/2076253002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2076253002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:4823: +<histogram name="BrowserDialogs.ExternalProtocol.RememberCheckbox" On 2016/10/05 01:04:18, Ilya Sherman wrote: > Hmm, "BrowserDialogs" is more general, but I'm not quite sure that it's quite > right for the top-level name. If you were to componentize your feature, what is > the component name that you would choose for it? That's often a good way to > come up with a top-level UMA name as well =) To be honest, it would probably just be the "external protocol handler" if we componentized it. The code is pretty small and fairly specialised. The only thing you could really pull out is the dialog. I'm completely open to ideas though.
dominickn@chromium.org changed reviewers: + thestig@chromium.org
+thestig: PTAL at chrome/browser/shell_integration, thanks!
On 2016/10/05 04:48:24, dominickn wrote: > +thestig: PTAL at chrome/browser/shell_integration, thanks! LGTM, but ask chrome/browser/OWNERS next time? (There are specific entries for shell_integration_win*)
dominickn@chromium.org changed reviewers: + gab@chromium.org
So there are. I'll run it by one of them to make sure, thanks! :) +gab: PTAL at shell_integration_win.cc. This CL changes the format of the string returned from a registry lookup method for display to the user.
metrics LGTM (with optional name change) https://codereview.chromium.org/2076253002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2076253002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:4823: +<histogram name="BrowserDialogs.ExternalProtocol.RememberCheckbox" On 2016/10/05 04:47:41, dominickn wrote: > On 2016/10/05 01:04:18, Ilya Sherman wrote: > > Hmm, "BrowserDialogs" is more general, but I'm not quite sure that it's quite > > right for the top-level name. If you were to componentize your feature, what > is > > the component name that you would choose for it? That's often a good way to > > come up with a top-level UMA name as well =) > > To be honest, it would probably just be the "external protocol handler" if we > componentized it. The code is pretty small and fairly specialised. The only > thing you could really pull out is the dialog. I'm completely open to ideas > though. Okay. I guess I'd be inclined to file it under "Navigation" or something like that, but something like "BrowserDialogs" or "ExternalProtocolHandler" might be reasonable as well. I'll defer to your judgement as someone more familiar with the feature than I am. Thanks for thinking about it in some detail =)
https://codereview.chromium.org/2076253002/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/2076253002/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:204: base::ASCIIToUTF16(url.scheme() + "\\shell\\open\\command"); I'd say using the exe's name should be last resort. The Application's display name should be used instead. If I'm reading MSDN correctly this should be stored in the (default) registry value under "url.scheme()". https://msdn.microsoft.com/en-us/library/windows/desktop/cc144175(v=vs.85).aspx https://msdn.microsoft.com/en-us/library/windows/desktop/cc144171(v=vs.85).aspx If there is nothing there then I guess falling back to the program name is fair, but the display name should be preferred. (I also just realized that Chrome is doing this wrong, it registers ChromeHTML as a handler for every protocol it handles but ChromeHTML's display name is "Chrome HTML document" which results in the protocol binding for say MAILTO being bound to "Chrome HTML document". Checkout "Choose default apps by protocol" on Win10 for yourself) https://codereview.chromium.org/2076253002/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:214: return base::WideToUTF16(command_line.GetProgram().BaseName().value()); wstring == string16 on Windows so this conversion isn't necessary.
https://codereview.chromium.org/2076253002/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/2076253002/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:214: return base::WideToUTF16(command_line.GetProgram().BaseName().value()); On 2016/10/05 14:23:36, gab wrote: > wstring == string16 on Windows so this conversion isn't necessary. In fact, WideToUTF16() just returns its argument. I suppose this is an _win.cc file, so there's no need to try to be cross-platform.
The CQ bit was checked by dominickn@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 checked by dominickn@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 #10 (id:180001) has been deleted
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2076253002/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/2076253002/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:204: base::ASCIIToUTF16(url.scheme() + "\\shell\\open\\command"); On 2016/10/05 14:23:36, gab wrote: > I'd say using the exe's name should be last resort. > > The Application's display name should be used instead. > > If I'm reading MSDN correctly this should be stored in the (default) registry > value under "url.scheme()". > > https://msdn.microsoft.com/en-us/library/windows/desktop/cc144175(v=vs.85).aspx > https://msdn.microsoft.com/en-us/library/windows/desktop/cc144171(v=vs.85).aspx > > If there is nothing there then I guess falling back to the program name is fair, > but the display name should be preferred. > > (I also just realized that Chrome is doing this wrong, it registers ChromeHTML > as a handler for every protocol it handles but ChromeHTML's display name is > "Chrome HTML document" which results in the protocol binding for say MAILTO > being bound to "Chrome HTML document". Checkout "Choose default apps by > protocol" on Win10 for yourself) Looks like we should update ChromeHTML's display name at some point. I've made this first try to query the display name, and fall back on the exe name. In our testing, I seem to remember not running into many display names, which is why we went to the exe first. Doesn't hurt to try I suppose. https://codereview.chromium.org/2076253002/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:214: return base::WideToUTF16(command_line.GetProgram().BaseName().value()); On 2016/10/05 16:29:27, Lei Zhang wrote: > On 2016/10/05 14:23:36, gab wrote: > > wstring == string16 on Windows so this conversion isn't necessary. > > In fact, WideToUTF16() just returns its argument. I suppose this is an _win.cc > file, so there's no need to try to be cross-platform. 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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by dominickn@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.
shell_integration_win.cc lgtm https://codereview.chromium.org/2076253002/diff/160001/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/2076253002/diff/160001/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:204: base::ASCIIToUTF16(url.scheme() + "\\shell\\open\\command"); On 2016/10/06 00:56:38, dominickn wrote: > On 2016/10/05 14:23:36, gab wrote: > > I'd say using the exe's name should be last resort. > > > > The Application's display name should be used instead. > > > > If I'm reading MSDN correctly this should be stored in the (default) registry > > value under "url.scheme()". > > > > > https://msdn.microsoft.com/en-us/library/windows/desktop/cc144175(v=vs.85).aspx > > > https://msdn.microsoft.com/en-us/library/windows/desktop/cc144171(v=vs.85).aspx > > > > If there is nothing there then I guess falling back to the program name is > fair, > > but the display name should be preferred. > > > > (I also just realized that Chrome is doing this wrong, it registers ChromeHTML > > as a handler for every protocol it handles but ChromeHTML's display name is > > "Chrome HTML document" which results in the protocol binding for say MAILTO > > being bound to "Chrome HTML document". Checkout "Choose default apps by > > protocol" on Win10 for yourself) > > Looks like we should update ChromeHTML's display name at some point. http://crbug.com/653576 > I've made > this first try to query the display name, and fall back on the exe name. In our > testing, I seem to remember not running into many display names, which is why we > went to the exe first. Doesn't hurt to try I suppose.
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, meacer@chromium.org, msw@chromium.org, thestig@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2076253002/#ps220001 (title: "Const")
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 ========== Simplify the text in the external protocol confirmation dialog. The external protocol handler dialog currently presents users with a very large amount of jargon-filled text. This CL simplifies the dialog down to just a title containing the app name and the existing checkbox. On Windows 7 and earlier, the registry is queried for an app path, which is displayed in the dialog. This CL modifies the query to retrieve a program name from the app path, falling back to just the executable name if the retrieval doesn't succeed. A new histogram to log the number of times users check the checkbox in the dialog is also added. BUG=601725 ========== to ========== Simplify the text in the external protocol confirmation dialog. The external protocol handler dialog currently presents users with a very large amount of jargon-filled text. This CL simplifies the dialog down to just a title containing the app name and the existing checkbox. On Windows 7 and earlier, the registry is queried for an app path, which is displayed in the dialog. This CL modifies the query to retrieve a program name from the app path, falling back to just the executable name if the retrieval doesn't succeed. A new histogram to log the number of times users check the checkbox in the dialog is also added. BUG=601725 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Simplify the text in the external protocol confirmation dialog. The external protocol handler dialog currently presents users with a very large amount of jargon-filled text. This CL simplifies the dialog down to just a title containing the app name and the existing checkbox. On Windows 7 and earlier, the registry is queried for an app path, which is displayed in the dialog. This CL modifies the query to retrieve a program name from the app path, falling back to just the executable name if the retrieval doesn't succeed. A new histogram to log the number of times users check the checkbox in the dialog is also added. BUG=601725 ========== to ========== Simplify the text in the external protocol confirmation dialog. The external protocol handler dialog currently presents users with a very large amount of jargon-filled text. This CL simplifies the dialog down to just a title containing the app name and the existing checkbox. On Windows 7 and earlier, the registry is queried for an app path, which is displayed in the dialog. This CL modifies the query to retrieve a program name from the app path, falling back to just the executable name if the retrieval doesn't succeed. A new histogram to log the number of times users check the checkbox in the dialog is also added. BUG=601725 Committed: https://crrev.com/0c9a5065a8a7c4b760b79eb996548b1921ed05ea Cr-Commit-Position: refs/heads/master@{#423664} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/0c9a5065a8a7c4b760b79eb996548b1921ed05ea Cr-Commit-Position: refs/heads/master@{#423664} |
