Bug 220925 – [webkit-patch] Allow user to opt out of browser for viewing the diff

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Angelos Oikonomopoulos]

[webkit-patch] Allow user to opt out of browser for viewing the diff

[Angelos Oikonomopoulos]

Created attachment 418293 [details]
Patch

[Jonathan Bedard]

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?

[Angelos Oikonomopoulos]

(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.

[Sam Weinig]

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.

[Alexey Proskuryakov]

> 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?

[Angelos Oikonomopoulos]

(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.

[Angelos Oikonomopoulos]

(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.

[Alexey Proskuryakov]

> 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?

[Alexey Proskuryakov]

To be clear, I don't feel very strongly about this.

[Angelos Oikonomopoulos]

(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.

[Angelos Oikonomopoulos]

Created attachment 418397 [details]
Patch

[Angelos Oikonomopoulos]

(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.

[Angelos Oikonomopoulos]

(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?

[Radar WebKit Bug Importer]

<rdar://problem/73828826>

[Angelos Oikonomopoulos]

Ping.

[Jonathan Bedard]

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.

[EWS]

Committed r290608 (247881@main): <https://commits.webkit.org/247881@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418397 [details].

[Angelos Oikonomopoulos]

(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 ;-)