Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1896513002:
Fix registerProtocolHandler implementation on Windows 8 and 10. (Closed)
|
Created:
4 years, 8 months ago by Patrick Monette Modified:
4 years, 8 months ago CC:
chromium-reviews, grt+watch_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix registerProtocolHandler implementation on Windows 8 and 10.
On Windows 8, now make sure the required registry key exist so that
Chrome is always given as an option when poping the intent picker.
On Windows 10, now shows the system settings to the user to set the
default handler for a given protocol.
Also fixed a small UI issue.
BUG=562671
Committed: https://crrev.com/0c40087ad10f2a1f412d2dec25d8ff8946043644
Cr-Commit-Position: refs/heads/master@{#388609}
Patch Set 1 : #
Total comments: 14
Patch Set 2 : Round of comment #1 #
Total comments: 7
Patch Set 3 : grt comments #
Total comments: 2
Patch Set 4 : Fix nit #Patch Set 5 : Rebase #Patch Set 6 : Fixed test compilation #Patch Set 7 : Fix clang compilation #
Messages
Total messages: 36 (17 generated)
Patchset #1 (id:1) has been deleted
pmonette@chromium.org changed reviewers: + grt@chromium.org
PTAL
coolness https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:716: bool LaunchDefaultAppsSettingsModernDialog(const wchar_t* target) { how about having this take the protocol rather than a target? it can take care of the mapping of http and mailto to specific targets. https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:1851: if (base::win::GetVersion() == base::win::VERSION_WIN8) { nit: omit braces https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:1953: } break; is this what git cl format likes? https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:1961: base::StringPrintf(kSystemSettingsDefaultAppsFormat, L"Browser") how about using GetTargetForDefaultAppsSettings(L"http") for this? as a benefit, kSystemSettingsDefaultAppsFormat can be moved into the function. https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2037: if (!CreateKeyForProtocol(protocol.c_str())) { is this doing the same thing that the ScopedUserProtocolEntry does? can you generalize that and use it here? https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2055: ScopedUserProtocolEntry user_protocol_entry; as-is, this is specifically for http, so it isn't needed here, unless it's suitable to be generalized as per my comment above. https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... chrome/installer/util/shell_util.h:411: enum SetDefaultInteractiveMode { wdyt of InteractiveSetDefaultMode so that this doesn't sound like it's going to run off and do something (SetFoo)? the method below could be GetInteractiveSetDefaultMode, which sounds a little better to my eyes than GetSetFoo.
PTAnL https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:716: bool LaunchDefaultAppsSettingsModernDialog(const wchar_t* target) { On 2016/04/15 18:47:26, grt wrote: > how about having this take the protocol rather than a target? it can take care > of the mapping of http and mailto to specific targets. Done. https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:1851: if (base::win::GetVersion() == base::win::VERSION_WIN8) { On 2016/04/15 18:47:26, grt wrote: > nit: omit braces Done. https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:1953: } break; On 2016/04/15 18:47:26, grt wrote: > is this what git cl format likes? Yes. https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:1961: base::StringPrintf(kSystemSettingsDefaultAppsFormat, L"Browser") On 2016/04/15 18:47:26, grt wrote: > how about using GetTargetForDefaultAppsSettings(L"http") for this? as a benefit, > kSystemSettingsDefaultAppsFormat can be moved into the function. Done. https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2037: if (!CreateKeyForProtocol(protocol.c_str())) { On 2016/04/15 18:47:26, grt wrote: > is this doing the same thing that the ScopedUserProtocolEntry does? can you > generalize that and use it here? Good point! I generalized it for all protocols so I can use it for the win8 path here. For the win10 path, the better scope is the OpenSystemSettingsHelper so I moved it there. https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:2055: ScopedUserProtocolEntry user_protocol_entry; On 2016/04/15 18:47:26, grt wrote: > as-is, this is specifically for http, so it isn't needed here, unless it's > suitable to be generalized as per my comment above. Done. https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/1896513002/diff/20001/chrome/installer/util/s... chrome/installer/util/shell_util.h:411: enum SetDefaultInteractiveMode { On 2016/04/15 18:47:26, grt wrote: > wdyt of InteractiveSetDefaultMode so that this doesn't sound like it's going to > run off and do something (SetFoo)? the method below could be > GetInteractiveSetDefaultMode, which sounds a little better to my eyes than > GetSetFoo. I agree. Done.
https://codereview.chromium.org/1896513002/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1896513002/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:321: ScopedUserProtocolEntry scoped_user_protocol_entry_; nit: a comment here would be helpful to understand why this is here. on second thought, does this need to be here in shell_integration? can it be done inside Show*SystemUI for both the Win8 and Win10+ cases? https://codereview.chromium.org/1896513002/diff/40001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/1896513002/diff/40001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:718: if (base::EqualsCaseInsensitiveASCII(protocol, L"http")) { nit: omit braces for single-line conditionals https://codereview.chromium.org/1896513002/diff/40001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:720: } else if (base::EqualsCaseInsensitiveASCII(protocol, L"mailto")) { nit: omit "else" when the "if" block "return"s: if (foo) return blorf; if (bar) return splurm;
https://codereview.chromium.org/1896513002/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1896513002/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:321: ScopedUserProtocolEntry scoped_user_protocol_entry_; On 2016/04/18 19:00:56, grt wrote: > nit: a comment here would be helpful to understand why this is here. > > on second thought, does this need to be here in shell_integration? can it be > done inside Show*SystemUI for both the Win8 and Win10+ cases? as per face-to-face discussion, i understand this is needed here because the cleanup must happen in the helper's dtor. i think this is okay. are we now in a world where ShellUtil::ShowMakeChromeDefaultSystemUI and ShowMakeChromeDefaultProtocolClientSystemUI should never be called directly on Win10 since we always want the benefits of the helper? can we get rid of all other callers of these two functions and paint them with colours too painful to see so that noone ever calls them again (preferring the functions in shell_integrtion)?
Description was changed from ========== Fix registerProtocolHandler implementation on Windows 8 and 10. On Windows 8, now make sure the required registry key exist so that Chrome is always given as an option when poping the intent picker. On Windows 10, now shows the system settings to the user to set the default handler for a given protocol. Also fixes a small UI issue. BUG=562671 ========== to ========== Fix registerProtocolHandler implementation on Windows 8 and 10. On Windows 8, now make sure the required registry key exist so that Chrome is always given as an option when poping the intent picker. On Windows 10, now shows the system settings to the user to set the default handler for a given protocol. Also fixed a small UI issue. BUG=562671 ==========
pmonette@chromium.org changed reviewers: + sky@chromium.org
sky@ PTAL. Mostly need owners approval for shell_integration.cc only. https://codereview.chromium.org/1896513002/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1896513002/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:321: ScopedUserProtocolEntry scoped_user_protocol_entry_; On 2016/04/19 02:03:46, grt wrote: > On 2016/04/18 19:00:56, grt wrote: > > nit: a comment here would be helpful to understand why this is here. > > > > on second thought, does this need to be here in shell_integration? can it be > > done inside Show*SystemUI for both the Win8 and Win10+ cases? > > as per face-to-face discussion, i understand this is needed here because the > cleanup must happen in the helper's dtor. i think this is okay. > > are we now in a world where ShellUtil::ShowMakeChromeDefaultSystemUI and > ShowMakeChromeDefaultProtocolClientSystemUI should never be called directly on > Win10 since we always want the benefits of the helper? can we get rid of all > other callers of these two functions and paint them with colours too painful to > see so that noone ever calls them again (preferring the functions in > shell_integrtion)? I added a comment. I think shell util is due for a refactoring. I'll do it in another CL. https://codereview.chromium.org/1896513002/diff/40001/chrome/installer/util/s... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/1896513002/diff/40001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:718: if (base::EqualsCaseInsensitiveASCII(protocol, L"http")) { On 2016/04/18 19:00:56, grt wrote: > nit: omit braces for single-line conditionals Done. https://codereview.chromium.org/1896513002/diff/40001/chrome/installer/util/s... chrome/installer/util/shell_util.cc:720: } else if (base::EqualsCaseInsensitiveASCII(protocol, L"mailto")) { On 2016/04/18 19:00:56, grt wrote: > nit: omit "else" when the "if" block "return"s: > if (foo) > return blorf; > if (bar) > return splurm; Nice choice of variable name there. Done.
and grt@ PTanL
shell_integration LGTM
lgtm https://codereview.chromium.org/1896513002/diff/60001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1896513002/diff/60001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:321: // This is needed to make sure that Windows display an entry for the protocol nit: "displays"
Thanks! https://codereview.chromium.org/1896513002/diff/60001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/1896513002/diff/60001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:321: // This is needed to make sure that Windows display an entry for the protocol On 2016/04/20 12:37:52, grt wrote: > nit: "displays" Done.
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1896513002/#ps80001 (title: "Fix nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1896513002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1896513002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1896513002/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1896513002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1896513002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1896513002/#ps120001 (title: "Fixed test compilation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1896513002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1896513002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1896513002/#ps140001 (title: "Fix clang compilation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1896513002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1896513002/140001
Message was sent while issue was closed.
Description was changed from ========== Fix registerProtocolHandler implementation on Windows 8 and 10. On Windows 8, now make sure the required registry key exist so that Chrome is always given as an option when poping the intent picker. On Windows 10, now shows the system settings to the user to set the default handler for a given protocol. Also fixed a small UI issue. BUG=562671 ========== to ========== Fix registerProtocolHandler implementation on Windows 8 and 10. On Windows 8, now make sure the required registry key exist so that Chrome is always given as an option when poping the intent picker. On Windows 10, now shows the system settings to the user to set the default handler for a given protocol. Also fixed a small UI issue. BUG=562671 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Fix registerProtocolHandler implementation on Windows 8 and 10. On Windows 8, now make sure the required registry key exist so that Chrome is always given as an option when poping the intent picker. On Windows 10, now shows the system settings to the user to set the default handler for a given protocol. Also fixed a small UI issue. BUG=562671 ========== to ========== Fix registerProtocolHandler implementation on Windows 8 and 10. On Windows 8, now make sure the required registry key exist so that Chrome is always given as an option when poping the intent picker. On Windows 10, now shows the system settings to the user to set the default handler for a given protocol. Also fixed a small UI issue. BUG=562671 Committed: https://crrev.com/0c40087ad10f2a1f412d2dec25d8ff8946043644 Cr-Commit-Position: refs/heads/master@{#388609} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0c40087ad10f2a1f412d2dec25d8ff8946043644 Cr-Commit-Position: refs/heads/master@{#388609} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
