220925 2021-01-25 07:35:58 -0800 [webkit-patch] Allow user to opt out of browser for viewing the diff 2022-03-01 01:51:13 -0800 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 angelos webkit-unassigned aakash_jain ap ddkilzer ews-watchlist fujii glenn jbedard sam slewis webkit-bug-importer oldest_to_newest 1722071 0 angelos 2021-01-25 07:35:58 -0800 [webkit-patch] Allow user to opt out of browser for viewing the diff 1722072 1 418293 angelos 2021-01-25 07:39:03 -0800 Created attachment 418293 Patch 1722086 2 418293 jbedard 2021-01-25 08:51:23 -0800 Comment on attachment 418293 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418293&action=review > Tools/Scripts/webkitpy/common/system/user.py:175 > + return False Not against this in principle...is the rationale that the user does not want to see the diff (even though they have a browser) or that the user does not have a browser installed? If it's the latter, what does this function actually do? 1722171 3 angelos 2021-01-25 11:42:47 -0800 (In reply to Jonathan Bedard from comment #2) > Comment on attachment 418293 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=418293&action=review > > > Tools/Scripts/webkitpy/common/system/user.py:175 > > + return False > > Not against this in principle...is the rationale that the user does not want > to see the diff (even though they have a browser) or that the user does not > have a browser installed? If it's the latter, what does this function > actually do? The motivating use case is that the user does want to see the diff, except not in a browser. This may be the case when they're connected over ssh to a remote box that has a text-mode browser installed for other reasons, but the diff is not really viewable in the text-mode browser. 1722213 4 418293 sam 2021-01-25 12:54:13 -0800 Comment on attachment 418293 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418293&action=review > Tools/Scripts/webkitpy/common/system/user.py:174 > + if os.environ.get("WEBKIT_PATCH_NO_BROWSER"): This seems reasonably, but I think we would want this to have also be configurable via an argument and documented in the help text, as an environment variable is not very discoverable. 1722251 5 ap 2021-01-25 13:56:20 -0800 > The motivating use case is that the user does want to see the diff, except > not in a browser. This may be the case when they're connected over ssh to a > remote box that has a text-mode browser installed for other reasons, but the > diff is not really viewable in the text-mode browser. Is there a way to detect this automatically? There probably is one on macOS at least, although I'm do not know how exactly. The design of this function is that webbrowser.get() raises an exception when it's not available, so this is probably where the check for WEBKIT_PATCH_NO_BROWSER needs to go, if there is no automatic solution. What is the failure behavior that you are currently observing? 1722272 6 angelos 2021-01-25 14:17:16 -0800 (In reply to Alexey Proskuryakov from comment #5) > > The motivating use case is that the user does want to see the diff, except > > not in a browser. This may be the case when they're connected over ssh to a > > remote box that has a text-mode browser installed for other reasons, but the > > diff is not really viewable in the text-mode browser. > > Is there a way to detect this automatically? I'm not sure what 'this' refers to here. > There probably is one on macOS > at least, although I'm do not know how exactly. The design of this function > is that webbrowser.get() raises an exception when it's not available, so > this is probably where the check for WEBKIT_PATCH_NO_BROWSER needs to go, if > there is no automatic solution. Not sure if I understand correctly; the reason I placed the check at the top of the function was to do what the name of the variable does, i.e. not use a browser regardless of if one is available or not. I should add that this also helps in the case where the user doesn't want to hunt for the new diff tab in their open browser windows but would rather see the diff "synchronously", i.e. in the terminal they're working in, in a pager. > What is the failure behavior that you are currently observing? The failure is that if any text-mode browser is installed on the machine that I've ssh'd in to, then running webkit-patch will show me the diff in that browser, however the diff is not readable in a text-mode browser. 1722274 7 angelos 2021-01-25 14:20:24 -0800 (In reply to Sam Weinig from comment #4) > Comment on attachment 418293 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=418293&action=review > > > Tools/Scripts/webkitpy/common/system/user.py:174 > > + if os.environ.get("WEBKIT_PATCH_NO_BROWSER"): > > This seems reasonably, but I think we would want this to have also be > configurable via an argument and documented in the help text, as an > environment variable is not very discoverable. The discoverability is a good point. TBH adding an option for this felt a little heavy-weight and mentioning the environment variable in the help text seems even less subtle. Yet I imagine in most cases the user would want to set the environment variable rather than pass in a flag for every invocation. So if we want to make this discoverable, sounds like this is the way to go. 1722383 8 ap 2021-01-25 19:50:36 -0800 > I'm not sure what 'this' refers to here. Running in a session where the browser cannot display diffs. I imagine that it's pretty limited set of configurations that you'd need to detect. I guess another consideration is that the function that you are patching is more generic than just diff viewing. Wouldn't this also prevent viewing test results, and do we want that? 1722384 9 ap 2021-01-25 19:51:11 -0800 To be clear, I don't feel very strongly about this. 1722440 10 angelos 2021-01-26 03:05:56 -0800 (In reply to Alexey Proskuryakov from comment #8) > > I'm not sure what 'this' refers to here. > > Running in a session where the browser cannot display diffs. I imagine that > it's pretty limited set of configurations that you'd need to detect. I have no idea; also, I wonder if with some text-mode browsers the diff might be something a person can understand, but still hard to make out, in which case we'd need to make a judgement call. > I guess another consideration is that the function that you are patching is > more generic than just diff viewing. Wouldn't this also prevent viewing test > results, and do we want that? AFAICS, this patch will only affect diff viewing. can_open_url is used in exactly two places. The first when deciding how to show the diff, the other is in open_url. open_url is used in various places, but it only uses the result of can_open_url to print a warning and then proceeds to unconditionally try and open the url in a browser: def open_url(self, url): if not self.can_open_url(): _log.warn("Failed to open %s" % url) webbrowser.open(url) So I think this should be pretty safe ATM. Though perhaps I should rename the variable to WEBKIT_PATCH_PREFER_PAGER and only check it in ConfirmDiff._show_pretty_diff. 1722459 11 418397 angelos 2021-01-26 05:01:49 -0800 Created attachment 418397 Patch 1722460 12 angelos 2021-01-26 05:02:24 -0800 (In reply to Angelos Oikonomopoulos from comment #10) [...] > So I think this should be pretty safe ATM. Though perhaps I should rename > the variable to WEBKIT_PATCH_PREFER_PAGER and only check it in > ConfirmDiff._show_pretty_diff. Submitted a new patch with this change. 1722515 13 angelos 2021-01-26 08:06:04 -0800 (In reply to Angelos Oikonomopoulos from comment #7) > (In reply to Sam Weinig from comment #4) > > Comment on attachment 418293 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=418293&action=review > > > > > Tools/Scripts/webkitpy/common/system/user.py:174 > > > + if os.environ.get("WEBKIT_PATCH_NO_BROWSER"): > > > > This seems reasonably, but I think we would want this to have also be > > configurable via an argument and documented in the help text, as an > > environment variable is not very discoverable. > > The discoverability is a good point. TBH adding an option for this felt a > little heavy-weight and mentioning the environment variable in the help text > seems even less subtle. Yet I imagine in most cases the user would want to > set the environment variable rather than pass in a flag for every > invocation. So if we want to make this discoverable, sounds like this is the > way to go. Not sure how to best handle this. AFAIU adding this flag and propagating it to ConfirmDiff seems like a pretty invasive change for something this trivial. I guuess Ican look into that, but is it worth the trade off in added complexity? 1724237 14 webkit-bug-importer 2021-02-01 07:36:16 -0800 <rdar://problem/73828826> 1732475 15 angelos 2021-02-23 02:58:20 -0800 Ping. 1846662 16 418397 jbedard 2022-02-28 09:28:59 -0800 Comment on attachment 418397 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418397&action=review > Tools/Scripts/webkitpy/tool/steps/confirmdiff.py:54 > + if os.environ.get('WEBKIT_PATCH_PREFER_PAGER') or not self._tool.user.can_open_url(): r+ing this for now, but this will be more relevant in git-webkit, where we'll probably do this with a git config option. 1846713 17 ews-feeder 2022-02-28 11:20:11 -0800 Committed r290608 (247881@main): <https://commits.webkit.org/247881@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418397. 1846955 18 angelos 2022-03-01 01:51:13 -0800 (In reply to Jonathan Bedard from comment #16) > Comment on attachment 418397 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=418397&action=review > > > Tools/Scripts/webkitpy/tool/steps/confirmdiff.py:54 > > + if os.environ.get('WEBKIT_PATCH_PREFER_PAGER') or not self._tool.user.can_open_url(): > > r+ing this for now, but this will be more relevant in git-webkit, where > we'll probably do this with a git config option. I'll submit a patch as soon as I can start using git-webkit then ;-) 418293 2021-01-25 07:39:03 -0800 2021-01-26 05:01:42 -0800 Patch bug-220925-20210125153901.patch text/plain 1381 angelos U3VidmVyc2lvbiBSZXZpc2lvbjogMjcxNzg4CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggOWU0ODRiYWRhZWEzYTBjYzFhNjBlZGM5ZjNkMjYwNzQ5 NWNjYjgyZi4uOTUyYTU1NTgwMzIyNzBiNDM3Y2QwOTcxY2M2MmRlN2EwY2U2Y2JlMSAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2 IEBACisyMDIxLTAxLTI1ICBBbmdlbG9zIE9pa29ub21vcG91bG9zICA8YW5nZWxvc0BpZ2FsaWEu Y29tPgorCisgICAgICAgIFt3ZWJraXQtcGF0Y2hdIEFsbG93IHVzZXIgdG8gb3B0IG91dCBvZiBi cm93c2VyIGZvciB2aWV3aW5nIHRoZSBkaWZmCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0yMjA5MjUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBUaGUgdXNlciBtYXkgbGVnaXRpbWF0ZWx5IGhhdmUgYSBicm93 c2VyIGluc3RhbGxlZCB5ZXQgbm90IHdhbnQgdG8KKyAgICAgICAgdXNlIGl0IHRvIHZpZXcgdGhl IGRpZmYuCisKKyAgICAgICAgKiBTY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9zeXN0ZW0vdXNlci5w eToKKyAgICAgICAgKFVzZXIuY2FuX29wZW5fdXJsKToKKwogMjAyMS0wMS0yNCAgU2ltb24gRnJh c2VyICA8c2ltb24uZnJhc2VyQGFwcGxlLmNvbT4KIAogICAgICAgICBbaU9TIFdLMl0gdGhldmVy Z2UuY29tIC0gcnViYmVyIGJhbmQgc2Nyb2xsaW5nIGF0IHRoZSB0b3Agb2YgdGhlIHBhZ2UgY2F1 c2VzIGFuIGFicnVwdCBqdW1wCmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2Nv bW1vbi9zeXN0ZW0vdXNlci5weSBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL3N5c3Rl bS91c2VyLnB5CmluZGV4IDE0N2MxMDM1NTc0NzU1OTExMzMzMWExYjgyNjllYTI3NjRmODg1ZDcu LmYxZjdjMzFhNzMxN2JhODZkYjQ4YWVlZTdmMmE0ODI0MDU4NzE3ZjkgMTAwNjQ0Ci0tLSBhL1Rv b2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL3N5c3RlbS91c2VyLnB5CisrKyBiL1Rvb2xzL1Nj cmlwdHMvd2Via2l0cHkvY29tbW9uL3N5c3RlbS91c2VyLnB5CkBAIC0xNzEsNiArMTcxLDggQEAg Y2xhc3MgVXNlcihvYmplY3QpOgogICAgICAgICByZXR1cm4gcmVzcG9uc2UubG93ZXIoKSA9PSAn eScKIAogICAgIGRlZiBjYW5fb3Blbl91cmwoc2VsZik6CisgICAgICAgIGlmIG9zLmVudmlyb24u Z2V0KCJXRUJLSVRfUEFUQ0hfTk9fQlJPV1NFUiIpOgorICAgICAgICAgICAgcmV0dXJuIEZhbHNl CiAgICAgICAgIHRyeToKICAgICAgICAgICAgIHdlYmJyb3dzZXIuZ2V0KCkKICAgICAgICAgICAg IHJldHVybiBUcnVlCg== 418397 2021-01-26 05:01:49 -0800 2022-02-28 11:20:14 -0800 Patch bug-220925-20210126130148.patch text/plain 1673 angelos U3VidmVyc2lvbiBSZXZpc2lvbjogMjcxNzg4CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggOWU0ODRiYWRhZWEzYTBjYzFhNjBlZGM5ZjNkMjYwNzQ5 NWNjYjgyZi4uOTUyYTU1NTgwMzIyNzBiNDM3Y2QwOTcxY2M2MmRlN2EwY2U2Y2JlMSAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2 IEBACisyMDIxLTAxLTI1ICBBbmdlbG9zIE9pa29ub21vcG91bG9zICA8YW5nZWxvc0BpZ2FsaWEu Y29tPgorCisgICAgICAgIFt3ZWJraXQtcGF0Y2hdIEFsbG93IHVzZXIgdG8gb3B0IG91dCBvZiBi cm93c2VyIGZvciB2aWV3aW5nIHRoZSBkaWZmCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0yMjA5MjUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBUaGUgdXNlciBtYXkgbGVnaXRpbWF0ZWx5IGhhdmUgYSBicm93 c2VyIGluc3RhbGxlZCB5ZXQgbm90IHdhbnQgdG8KKyAgICAgICAgdXNlIGl0IHRvIHZpZXcgdGhl IGRpZmYuCisKKyAgICAgICAgKiBTY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9zeXN0ZW0vdXNlci5w eToKKyAgICAgICAgKFVzZXIuY2FuX29wZW5fdXJsKToKKwogMjAyMS0wMS0yNCAgU2ltb24gRnJh c2VyICA8c2ltb24uZnJhc2VyQGFwcGxlLmNvbT4KIAogICAgICAgICBbaU9TIFdLMl0gdGhldmVy Z2UuY29tIC0gcnViYmVyIGJhbmQgc2Nyb2xsaW5nIGF0IHRoZSB0b3Agb2YgdGhlIHBhZ2UgY2F1 c2VzIGFuIGFicnVwdCBqdW1wCmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L3Rv b2wvc3RlcHMvY29uZmlybWRpZmYucHkgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L3Rvb2wvc3Rl cHMvY29uZmlybWRpZmYucHkKaW5kZXggYWYzNDg5ZDNlNjkyZTU4MGIxMzVlZTE0YTc4YTIwMDQz ODA5OGQzNS4uOWI3YWQzZjllZWQ1ZTY4NDk0MGZhMDI1YTQyMDA5MTIzNmI2ZDY5YiAxMDA2NDQK LS0tIGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS90b29sL3N0ZXBzL2NvbmZpcm1kaWZmLnB5Cisr KyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvdG9vbC9zdGVwcy9jb25maXJtZGlmZi5weQpAQCAt MjYsNiArMjYsNyBAQAogIyAoSU5DTFVESU5HIE5FR0xJR0VOQ0UgT1IgT1RIRVJXSVNFKSBBUklT SU5HIElOIEFOWSBXQVkgT1VUIE9GIFRIRSBVU0UKICMgT0YgVEhJUyBTT0ZUV0FSRSwgRVZFTiBJ RiBBRFZJU0VEIE9GIFRIRSBQT1NTSUJJTElUWSBPRiBTVUNIIERBTUFHRS4KIAoraW1wb3J0IG9z CiBpbXBvcnQgc3lzCiAKIGZyb20gd2Via2l0cHkudG9vbC5zdGVwcy5hYnN0cmFjdHN0ZXAgaW1w b3J0IEFic3RyYWN0U3RlcApAQCAtNTAsNyArNTEsNyBAQCBjbGFzcyBDb25maXJtRGlmZihBYnN0 cmFjdFN0ZXApOgogICAgICAgICBdCiAKICAgICBkZWYgX3Nob3dfcHJldHR5X2RpZmYoc2VsZiwg ZGlmZik6Ci0gICAgICAgIGlmIG5vdCBzZWxmLl90b29sLnVzZXIuY2FuX29wZW5fdXJsKCk6Cisg ICAgICAgIGlmIG9zLmVudmlyb24uZ2V0KCdXRUJLSVRfUEFUQ0hfUFJFRkVSX1BBR0VSJykgb3Ig bm90IHNlbGYuX3Rvb2wudXNlci5jYW5fb3Blbl91cmwoKToKICAgICAgICAgICAgIHJldHVybiBO b25lCiAKICAgICAgICAgdHJ5Ogo=