threads / rfc / 22097

RFC patchAdd support for the google-chrome web browser

Subject: [PATCH/RFC] Add support for the google-chrome web browser

## tl;dr

6 messages between Jan 5, 2010 and Jan 5, 2010. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Evan Farrer· Jan 5, 2010, 05:19 UTC · lore

Signed-off-by: Evan Farrer <Evan.Farrer@gmail.com>
---
 Documentation/git-web--browse.txt |    1 +
 git-web--browse.sh                |    6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

Show changes to 2 files +4 −3

Documentation/git-web--browse.txt, git-web--browse.sh

diff --git a/Documentation/git-web--browse.txt b/Documentation/git-web--browse.txt
index 278cf73..0994139 100644
--- a/Documentation/git-web--browse.txt
+++ b/Documentation/git-web--browse.txt
@@ -25,6 +25,7 @@ The following browsers (or commands) are currently supported:
 * links
 * lynx
 * dillo
+* google-chrome
 * open (this is the default under Mac OS X GUI)
 * start (this is the default under MinGW)
 
diff --git a/git-web--browse.sh b/git-web--browse.sh
index a578c3a..0664b9e 100755
--- a/git-web--browse.sh
+++ b/git-web--browse.sh
@@ -31,7 +31,7 @@ valid_custom_tool()
 
 valid_tool() {
 	case "$1" in
-		firefox | iceweasel | konqueror | w3m | links | lynx | dillo | open | start)
+		firefox | iceweasel | konqueror | w3m | links | lynx | dillo | google-chrome | open | start)
 			;; # happy
 		*)
 			valid_custom_tool "$1" || return 1
@@ -103,7 +103,7 @@ fi
 
 if test -z "$browser" ; then
     if test -n "$DISPLAY"; then
-	browser_candidates="firefox iceweasel konqueror w3m links lynx dillo"
+	browser_candidates="firefox iceweasel konqueror w3m links lynx dillo google-chrome"
 	if test "$KDE_FULL_SESSION" = "true"; then
 	    browser_candidates="konqueror $browser_candidates"
 	fi
@@ -162,7 +162,7 @@ case "$browser" in
 		;;
 	esac
 	;;
-    w3m|links|lynx|open)
+    w3m|links|lynx|google-chrome|open)
 	eval "$browser_path" "$@"
 	;;
     start)

-- 
1.6.6.78.gbd757c

Junio C Hamano· Jan 5, 2010, 05:40 UTC · re: Evan Farrer · lore

Re: [PATCH/RFC] Add support for the google-chrome web browser

Evan Farrer <evan.farrer@gmail.com> writes:

Show 15 quoted lines

> Signed-off-by: Evan Farrer <Evan.Farrer@gmail.com>
> ---
>  Documentation/git-web--browse.txt |    1 +
>  git-web--browse.sh                |    6 +++---
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-web--browse.txt b/Documentation/git-web--browse.txt
> index 278cf73..0994139 100644
> --- a/Documentation/git-web--browse.txt
> +++ b/Documentation/git-web--browse.txt
> @@ -25,6 +25,7 @@ The following browsers (or commands) are currently supported:
>  * links
>  * lynx
>  * dillo
> +* google-chrome

Hmmm, the support added by this patch for the chrome is, together with w3m, links, and open, to simply run the command with given arguments without any other magic.

Perhaps valid_custom_tool() function should be changed to simply return $1 if $browser_cmd is not found, like this:

	valid_custom_tool ()
        {
        	browser_cmd=$(git config "browser.$1.cmd")
                : ${browser_cmdn:=$1}
	}

Then we don't even have to add any specific support for "google-chrome" or anything that takes "$command $path..." and opens the documents.

Is there a downside in this approach?

Avery Pennarun· Jan 5, 2010, 17:20 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] Add support for the google-chrome web browser

On Tue, Jan 5, 2010 at 12:40 AM, Junio C Hamano <gitster@pobox.com> wrote:

> Then we don't even have to add any specific support for "google-chrome" or
> anything that takes "$command $path..." and opens the documents.
>
> Is there a downside in this approach?

If someone has another firefox-derived browser installed with a different name and tries to use it, this default wouldn't do the right "firefoxy" thing, and would instead fail strangely. On the other hand, right now it'll fail anyway, just not strangely.

So your proposed change would decrease the number of failures, but increase the strangeness of the remaining failures. Maybe that's the right thing to do though.

Avery

Junio C Hamano· Jan 5, 2010, 18:39 UTC · re: Avery Pennarun · lore

Re: [PATCH/RFC] Add support for the google-chrome web browser

Avery Pennarun <apenwarr@gmail.com> writes:

Show 10 quoted lines

> On Tue, Jan 5, 2010 at 12:40 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Then we don't even have to add any specific support for "google-chrome" or
>> anything that takes "$command $path..." and opens the documents.
>>
>> Is there a downside in this approach?
>
> If someone has another firefox-derived browser installed with a
> different name and tries to use it, this default wouldn't do the right
> "firefoxy" thing, and would instead fail strangely.  On the other
> hand, right now it'll fail anyway, just not strangely.

You probably didn't notice/understand why I singled out w3m/links/open and excluded firefox from the set. There is no question that the ones that need custom command line need custom support. But to support a new browser that takes a bog standard "command then args" command line, there is no reason to add cruft, every time somebody comes up with a new browser.

Avery Pennarun· Jan 5, 2010, 18:56 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] Add support for the google-chrome web browser

On Tue, Jan 5, 2010 at 1:39 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

> Avery Pennarun <apenwarr@gmail.com> writes:
>> On Tue, Jan 5, 2010 at 12:40 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Then we don't even have to add any specific support for "google-chrome" or
>>> anything that takes "$command $path..." and opens the documents.
>>>
>>> Is there a downside in this approach?
>>
>> If someone has another firefox-derived browser installed with a
>> different name and tries to use it, this default wouldn't do the right
>> "firefoxy" thing, and would instead fail strangely.  On the other
>> hand, right now it'll fail anyway, just not strangely.
>
> You probably didn't notice/understand why I singled out w3m/links/open and
> excluded firefox from the set.  There is no question that the ones that
> need custom command line need custom support.  But to support a new
> browser that takes a bog standard "command then args" command line, there
> is no reason to add cruft, every time somebody comes up with a new browser.

Yes, I'm probably missing something, that would be normal :)

My point is that, given a random browser name, you don't know whether it's an easy one *or* if it needs to work more like firefox.

The current behaviour will barf right away (I think) because it doesn't know. If it instead had a default case that just assumed non-firefox behaviour, then it would fail *strangely* (instead of predictably) on browsers that needed special workarounds, such as an as-yet-unknown firefox derivative.

Maybe this isn't important though.

Avery

Junio C Hamano· Jan 5, 2010, 19:56 UTC · re: Avery Pennarun · lore

Re: [PATCH/RFC] Add support for the google-chrome web browser

Avery Pennarun <apenwarr@gmail.com> writes:

Show 5 quoted lines

> The current behaviour will barf right away (I think) because it
> doesn't know.  If it instead had a default case that just assumed
> non-firefox behaviour, then it would fail *strangely* (instead of
> predictably) on browsers that needed special workarounds, such as an
> as-yet-unknown firefox derivative.

Ok, I can buy that.

I have a feeling that extensibility situation of this script is similar to that of mergetool/difftool. Perhaps a similar approach to refactor and then support browser-specific peculiarities by separate files, outlined in http://thread.gmane.org/gmane.comp.version-control.git/134906/focus=135006 might work well, don't you think?