Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737283002:
xdg-settings: add secondary check in GetIsDefaultWebClient (Closed)
|
Created:
3 years, 9 months ago by Tom (Use chromium acct) Modified:
3 years, 9 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionxdg-settings: add secondary check in GetIsDefaultWebClient
This CL fixes an issue where Chrome would display a banner saying that it
was not the default browser when it actually was.
The cause of this was 'xdg-settings check default-web-browser
google-chrome.desktop' returning 'no' when Chrome might have been the
default (see further explanation in the CL comments).
BUG=578888
R=thestig@chromium.org
Review-Url: https://codereview.chromium.org/2737283002
Cr-Commit-Position: refs/heads/master@{#456224}
Committed: https://chromium.googlesource.com/chromium/src/+/dece487a37276f1043d8092acd01a761ec99c17e
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address thestig@'s comments #
Total comments: 1
Messages
Total messages: 16 (7 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== xdg-settings: add secondary check in GetIsDefaultWebClient BUG=578888 ========== to ========== xdg-settings: add secondary check in GetIsDefaultWebClient This CL fixes an issue where Chrome would display a banner saying that it was not the default browser when it actually was. The cause of this was 'xdg-settings check default-web-browser google-chrome.desktop' returning 'no' when Chrome might have been the default (see further explanation in the CL comments). BUG=578888 R=thestig@chromium.org ==========
thomasanderson@google.com changed reviewers: + thestig@chromium.org
thestig@ ptal
thestig@chromium.org changed reviewers: + mdm@chromium.org
Looks ok, but +mdm who wrote xdg-settings and may be able to comment on the "odd case". https://codereview.chromium.org/2737283002/diff/20001/chrome/browser/shell_in... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/2737283002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:200: // xdg-settings failed: we can't determine or set the default browser. Move this comment down to line 239? https://codereview.chromium.org/2737283002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:211: DefaultWebClientState GetIsDefaultWebClient(const std::string& protocol) { Can we remove this to just GetDefaultWebClient() while we are here? https://codereview.chromium.org/2737283002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:221: } else if (base::StartsWith(xdg_is_default, "no", No need for else-if after a return. https://codereview.chromium.org/2737283002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:228: // (crbug.com/578888). Add https:// so code search can auto-linkify.
https://codereview.chromium.org/2737283002/diff/20001/chrome/browser/shell_in... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/2737283002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:200: // xdg-settings failed: we can't determine or set the default browser. On 2017/03/10 02:04:45, Lei Zhang (super slow) wrote: > Move this comment down to line 239? Done. https://codereview.chromium.org/2737283002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:211: DefaultWebClientState GetIsDefaultWebClient(const std::string& protocol) { On 2017/03/10 02:04:45, Lei Zhang (super slow) wrote: > Can we remove this to just GetDefaultWebClient() while we are here? Done. https://codereview.chromium.org/2737283002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:221: } else if (base::StartsWith(xdg_is_default, "no", On 2017/03/10 02:04:45, Lei Zhang (super slow) wrote: > No need for else-if after a return. So you mean to change it to just 'if'? Done. https://codereview.chromium.org/2737283002/diff/20001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:228: // (crbug.com/578888). On 2017/03/10 02:04:45, Lei Zhang (super slow) wrote: > Add https:// so code search can auto-linkify. Done.
lgtm
The CQ bit was checked by thomasanderson@google.com
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": 40001, "attempt_start_ts": 1489187624577800,
"parent_rev": "449ca301d5722d443b57ab5fd5db36a57b9f821a", "commit_rev":
"dece487a37276f1043d8092acd01a761ec99c17e"}
Message was sent while issue was closed.
Description was changed from ========== xdg-settings: add secondary check in GetIsDefaultWebClient This CL fixes an issue where Chrome would display a banner saying that it was not the default browser when it actually was. The cause of this was 'xdg-settings check default-web-browser google-chrome.desktop' returning 'no' when Chrome might have been the default (see further explanation in the CL comments). BUG=578888 R=thestig@chromium.org ========== to ========== xdg-settings: add secondary check in GetIsDefaultWebClient This CL fixes an issue where Chrome would display a banner saying that it was not the default browser when it actually was. The cause of this was 'xdg-settings check default-web-browser google-chrome.desktop' returning 'no' when Chrome might have been the default (see further explanation in the CL comments). BUG=578888 R=thestig@chromium.org Review-Url: https://codereview.chromium.org/2737283002 Cr-Commit-Position: refs/heads/master@{#456224} Committed: https://chromium.googlesource.com/chromium/src/+/dece487a37276f1043d8092acd01... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/dece487a37276f1043d8092acd01...
Message was sent while issue was closed.
https://codereview.chromium.org/2737283002/diff/40001/chrome/browser/shell_in... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/2737283002/diff/40001/chrome/browser/shell_in... chrome/browser/shell_integration_linux.cc:221: // An output of "no" does not necessarily mean Chrom[e,ium] is not the So, you're sort of defeating the purpose of having a separate "check" feature in xdg-settings, compared to only having set/get and then comparing the result of get inside the caller. The "check" feature is specifically there because in some cases the different ways in which a browser may be listed as the default can disagree, and we want to show the bar if any of them aren't us. For instance, it may be that clicking on a http:// link in an application would launch us, but double clicking on a local .html file would launch a different browser. We want to show the bar, in that case. It would be better to figure out *why* xdg-settings on the user's desktop is saying that we aren't completely the default, keeping in mind that on modern distributions, the version of xdg-settings we run is generally the one packaged with the distribution (and thus may have distro patches), even though we ship our own as well.
Message was sent while issue was closed.
On 2017/03/11 03:05:04, Mike Mammarella wrote: > https://codereview.chromium.org/2737283002/diff/40001/chrome/browser/shell_in... > File chrome/browser/shell_integration_linux.cc (right): > > https://codereview.chromium.org/2737283002/diff/40001/chrome/browser/shell_in... > chrome/browser/shell_integration_linux.cc:221: // An output of "no" does not > necessarily mean Chrom[e,ium] is not the > So, you're sort of defeating the purpose of having a separate "check" feature in > xdg-settings, compared to only having set/get and then comparing the result of > get inside the caller. The "check" feature is specifically there because in some > cases the different ways in which a browser may be listed as the default can > disagree, and we want to show the bar if any of them aren't us. For instance, it > may be that clicking on a http:// link in an application would launch us, but > double clicking on a local .html file would launch a different browser. We want > to show the bar, in that case. > > It would be better to figure out *why* xdg-settings on the user's desktop is > saying that we aren't completely the default, keeping in mind that on modern > distributions, the version of xdg-settings we run is generally the one packaged > with the distribution (and thus may have distro patches), even though we ship > our own as well. So... is there some plan to fix this? As-is, this CL basically breaks a number of use cases. I don't work on Chromium any more, but I'm still sad to see parts of it I worked on get broken. :(
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:40001) has been created in https://codereview.chromium.org/2751233002/ by thomasanderson@google.com. The reason for reverting is: mdm@ says this is the wrong approach. |
