Display annotated "blame" version of a file

[GoogleCodeExporter]

Annotate the lines of a file with the revision they came from using the blame 
algorithm.

JGit blame is not identical to git-core's blame algorithm. Its a close 
approximation, but there are some differences in how JGit tracks the scoreboard 
and what annotations it can find.

Use /+blame/revision/path as the URL.

Because annotation is slow, results might want to be cached, and the page might 
want to use AJAX to feed down hunks of blame as they are discovered by the 
algorithm. This would be like the incremental blame display visible in git 
gui's blame window.

Original issue reported on code.google.com by dborowitz@google.com on 11 Nov 2012 at 11:25

[GoogleCodeExporter]

I thought about this a bit over the holiday and got a bit frustrated by the 
fact that JGit has no way to cache common blame info between successive 
revisions of the same blob, let a lone across multiple requests. Obviously all 
blame information for deadbeef^:foo is identical to deadbeef:foo except for 
what was actually modified by foo. Caching one revision's worth of blame is an 
obvious optimization, but someone clicking back through the blame history one 
revision at a time would be recomputing huge chunks of identical blame data.

Fixing this, I think, would require substantial changes to JGit's blame 
internals.

Original comment by dborowitz@google.com on 26 Dec 2012 at 7:37

[GoogleCodeExporter]

Chromium wants this sooner rather than later. Incremental caching and AJAX 
support may not be blockers.

Original comment by dborowitz@google.com on 23 Jan 2014 at 9:50

[GoogleCodeExporter]

For our timeline, it would be wonderful to have the first pass on this around 
Valentine's day. We just need something to show to the other Chrome devs so we 
can focus on other aspects of the git migration. Is that at all feasible? If 
not, is there a different date you can give us that will let us put our minds 
and schedules at ease?

Original comment by aga...@chromium.org on 27 Jan 2014 at 7:29

[GoogleCodeExporter]

Got a rough version of this working today, but found a bug in upstream JGit's 
blame implementation that may make the results...unsatisfying:

$ ~/c/jgit/org.eclipse.jgit.pgm/target/jgit blame -L 25,35 
61dcc102242e00d3b0061405d5c909564ebbd819 
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
d2fa1fdb (Shawn O. Pearce 2012-06-05 08:49:52 -0700 25) import static 
org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_OTHER_REASON;
06cb1d25 (Dave Borowitz   2012-02-29 11:39:00 -0800 26) 
c545c090 (Shawn O. Pearce 2012-07-27 16:38:55 -0700 27) import 
com.google.common.base.Function;
e6298f72 (Shawn O. Pearce 2012-07-26 12:36:55 -0700 28) import 
com.google.common.base.Predicate;
         (                                          29) import com.google.common.base.Splitter;
0795c58a (Shawn Pearce    2013-02-24 15:13:27 -0800 30) import 
com.google.common.base.Strings;
         (                                          31) import com.google.common.collect.ArrayListMultimap;
         (                                          32) import com.google.common.collect.BiMap;
         (                                          33) import com.google.common.collect.HashBiMap;
5972e244 (Bruce Zu        2013-05-28 11:12:16 +0800 34) import 
com.google.common.collect.HashMultimap;

Original comment by dborowitz@google.com on 29 Jan 2014 at 1:32

[GoogleCodeExporter]

Submitted: 
https://gerrit-review.googlesource.com/#/q/project:gitiles+topic:blame,n,z

JGit bug still exists so results may not be totally accurate, but you can get 
some idea of the UI. Should be live on *.googlesource.com later this week.

Original comment by dborowitz@google.com on 30 Jan 2014 at 12:50

[GoogleCodeExporter]

This might be the upstream JGit bug:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=374382

Doesn't look like anybody has attempted to fix it, but at least there's a 
better description than "blame doesn't work sometimes."

Original comment by dborowitz@google.com on 3 Feb 2014 at 11:49

[GoogleCodeExporter]

Original comment by benhenry@chromium.org on 5 Feb 2014 at 6:33

[GoogleCodeExporter]

This is awesome.  I notice that the left column links on a blame page go to a 
blame from that revision.  Is this on purpose?  It might be more intuitive for 
the most prominent link to go to the actual revision, rather than the blame.  

For me, it's pretty rare that I'd want to redo the blame at the revision 
identified in the original blame.  Frequently I will need to do a blame on the 
parent of that revision though, if the identified commit was just a refactor or 
rename.

Original comment by ilevy@chromium.org on 7 Feb 2014 at 10:44

[GoogleCodeExporter]

ilevy: 
https://chromium.googlesource.com/chromium/src/+blame/e3a928b81366df875af873614f
0703f7733f0050/DEPS shows an example of blame links that take us to blame for 
those revisions on this file.  To get to the corresponding revision data, I 
simply scroll to the top of the page and then click the revision.

If each line took me to the basic revision info, reblaming on that revision in 
the same file would require going through the tree of files to find the file 
again, and then blame from there (painful).  Or changing the URL by hand (no 
good).

Comparing the number of clicks between the cases, the greater cost would be on 
those who are actively investigating blame data in a file going back previous 
revisions.  Also, the user has signaled their interest in blame data for that 
file at that revision by finding that file in the web interface already.  
Forcing people to do that over and over again as they go further and further 
back seems bad.

Original comment by cmp@chromium.org on 7 Feb 2014 at 10:54

[GoogleCodeExporter]

The decision to link to /+blame was somewhat arbitrary. I was on the fence 
between going to the ref (with the downsides cmp mentioned), the file contents, 
or the blame.

Original comment by dborowitz@google.com on 7 Feb 2014 at 11:13

[GoogleCodeExporter]

On a chromium-dev thread, szager suggested I post this here (see 
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/XUHR_iq5m1I/oq4iaji0
NI4J). Perhaps it's the same as to cmp's comment (I'm not sure). Also, the 
performance was slow (nearly 2 minutes to blame this file).

If needed, I'm happy to sit with anyone and show them the workflow that I use 
to track down code.

Just saw this update, nice to see that work is going on this tool. To try it 
out, I picked a recent example that git users couldn't track down and which we 
were investigating because of a bug. Let's say I want to look at 
RenderViewImpl::OnMessageReceived, and the 
"GetContentClient()->SetActiveURL(main_frame->document().url());" line near the 
top and see the cl which added it. Currently using ViewVC my steps are:
1) go to src.chromium.org/viewvc/, find the file
2) 2s later 
http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_view_imp
l.cc?revision=254479 loads, click on "show annotations"
3) 20s later the page loads, I see that line with a link 
http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_view_imp
l.cc?r1=163060&r2=163061&. It's a cl from me moving code into a namespace. 
However the link is helpful in that it has a side-by-side diff and the left 
side has a link to the previous revision before this change, so I click on that 
link.
4) I go through the above line 4 more times because that line changed by a 
bunch of people. The total network load time is 35s. There's a bug in viewvc 
when the change is before a file rename, since it tries to generate a url with 
the old filename and that fails. However that bug is easy to workaround by 
correcting the filename in the url, which I know since it's what I started 
with. I finally get to the original cl that added this: 
http://src.chromium.org/viewvc/chrome?revision=11337&view=revision

This process was more convoluted than normal, because that line had 5-6 changes 
to it. However even then, it takes me about a minute or two to do this tracking.

I tried to do this with the git tool.
1) I click "blame" on 
https://chromium.googlesource.com/chromium/src/+/master/content/renderer/render_
view_impl.cc
2) 108s later, I look for SetActiveURL and see that first change from me. It 
has a link on the left to 
https://chromium.googlesource.com/chromium/src/+blame/a80af12efcba198f4beda21c1d
88c63f04c4dc41/content/renderer/render_view_impl.cc
3) 60s later, it takes me to 
https://chromium.googlesource.com/chromium/src/+blame/a80af12efcba198f4beda21c1d
88c63f04c4dc41/content/renderer/render_view_impl.cc, i.e. the same link where I 
came from. At this point, I can't do anything.

Original comment by jam@chromium.org on 4 Mar 2014 at 12:20

[GoogleCodeExporter]

jam@ also wrote at the end of his message on chromium-dev:
> So it seems that the tool needs more work to find changes as it
> could only go one step backwards. Having it match viewvc to go
> back multiple changes is needed as this information is often
> crucial in figuring out why a piece of code is the way it is
> when refactoring.
> 
> Hopefully these steps are useful to the team working on this
> tool in figuring out how to get this sort of investigation to
> work.

Original comment by cmp@chromium.org on 4 Mar 2014 at 3:54

[GoogleCodeExporter]

pkasting@ wrote:

There are a couple problems here:
(1) Because git hashes aren't sequential in nature and the view here displays a 
"rounded" last modified time, we have displays like:

12507e3 dpranke@chromium.org - 1 year, 4 months ago  ...
1f7d4b9 torne@chromium.org - 1 year, 4 months ago  ...

It's not possible to glance at this and tell which change came last.  The 
easiest fix I can think for this would be to change the "1 year, 4 months ago" 
string to a timestamp:

12507e3 dpranke@chromium.org - 12 Dec 2012 18:30 PST  ...
1f7d4b9 torne@chromium.org - 19 Dec 2012 4:27 PST  ...

(2) Clicking on one of these blame lines sends you to an annotated version of 
the file as of that commit.  ViewVC's behavior, which is vastly more useful, 
takes you to the diff from the selected commit against the previous file 
version (from which you can get to the full file as of either version).  With 
this UI, I don't see a trivial mechanism to compute that diff.

To me both of these would be seriously detrimental to my workflow.  It would be 
nice to address both before the switch.

Original comment by cmp@chromium.org on 4 Mar 2014 at 3:56

[GoogleCodeExporter]

Re: pkasting's (2), there's some discussion here about whether the link should 
go to:
- the blame for the file at that revision (my comment)
- the diff for the file at that revision (pkasting's comment)
- the content of the file at that revision (ilevy's comment)

All I know is that the current UI doesn't make it easy to jump between these 
modes from each other.  It would be nice to have links at the top of each 
modes' page that allowed getting to the other modes easily.

For example: on 
https://chromium.googlesource.com/chromium/src/+blame/master/OWNERS, show:
[link]file history[/link] [link]raw[/link] [link]diff[/link] blame

And on https://chromium.googlesource.com/chromium/src/+/master/OWNERS, show:
[link]file history[/link] raw [link]diff[/link] [link]blame[/link]

etc

Dave, is that doable?

Original comment by cmp@chromium.org on 4 Mar 2014 at 4:02

[GoogleCodeExporter]

Thanks all for the concrete suggestions. I'll take a look at some other
tools, rethink the top bar, and get back to you.

Original comment by dborowitz@google.com on 4 Mar 2014 at 4:04

[GoogleCodeExporter]

Here's one more issue I found, from email:

"When trying to use gitiles' blame on the internal, pre-launch Chromium source 
tree, I found that the line height in the "blame" column is not identical to 
the line height in the "source" column, so that as you scroll down, the blame 
lines rapidly get out-of-sync with the source lines, and at the bottom of the 
page, you can see a discontinuity in the horizontal line that appears under the 
two columns.

"This makes blame pretty much useless, because it's not clear where you should 
click to actually see the blame for a particular line of code."

Also, regarding comment 14's three options:
* I don't see how "content of the file at that revision" is more useful than 
"blame of the file at that revision".  Showing the blame annotations is 
strictly more info, so it seems strictly more likely to be useful.  That 
suggests not doing option 3.
* That said, I'm not sure what use cases are addressed by showing the blame of 
the file at the chosen revision.  If you show the diff as I suggested, you can 
immediately distinguish between "yep, this is the change I was looking for" and 
"no, this just reordered comments/fixed a typo/fixed line endings/etc., I need 
to keep looking".  If you show the blame, you have to click through to the diff 
right away to determine this.  So it seems to me like showing the diff is much 
better.  Certainly while I've been doing hours of version control spelunking 
over the past few days (trying to assign owners for all 1000 Chrome 
command-line flags), I've wanted the diff every time.  But maybe I'm missing 
some use case.

Original comment by pkasting@chromium.org on 5 Mar 2014 at 11:26

[GoogleCodeExporter]

Peter, re: your comment, the current gitiles view doesn't have links at the top 
of the page to the other views of the file (so one can't easily go from blame 
to diff, diff to content, content to blame, etc).  I believe that will change 
and those links will get added, then more options open up.

Going on in your comment, you say the goal is to determine if this is revision 
that the user is looking for.  Once the UI makes it easy to jump between 
content/diff/blame for a file, I believe using "diff" for the default view 
should be the best choice.

For the sake of the bug and the record, I will document the essential steps we 
want to support as I'm aware of them:
1. find file in repo
2. blame on file
3. find line you want to know about and revision where line was changed
4. click on revision
5. see diff, this revision is not the right revision
6. go to blame view of this file at revision's parent
7. repeat from step 3 until step 5 finds a revision that is the right revision

Re: step 6: This feature is currently missing.  When viewing a file in 
content/diff/blame view, there should be a link to a previous revision for that 
file.  I know that we can edit the URL to do this, but we need the page to 
contain the link to consider this feature presented.  Expanding on comment 12, 
jam@ had commented that this feature was missing when he tried to use this tool.

Original comment by cmp@chromium.org on 12 Mar 2014 at 3:14

[GoogleCodeExporter]

For the record, the steps Chase outlines in c#17 are exactly what the gerrit 
and Chrome Infra teams agreed was the right way to proceed in a weekly meeting 
yesterday.

Original comment by benhe...@google.com on 12 Mar 2014 at 3:40

[GoogleCodeExporter]

FYI the bug in upstream JGit should soon be fixed:
https://git.eclipse.org/r/24916

Original comment by dborowitz@google.com on 13 Apr 2014 at 7:38

[GoogleCodeExporter]

https://gerrit-review.googlesource.com/56202 adds a bunch of links to the blame 
page.

I think it's pretty ugly, someone with a better eye for CSS feel free to take a 
look ;)

Original comment by dborowitz@google.com on 21 Apr 2014 at 10:43

[GoogleCodeExporter]

Added link to the parent blame view when viewing a single file diff:
https://gerrit-review.googlesource.com/56229

Original comment by dborowitz@google.com on 22 Apr 2014 at 12:25

[GoogleCodeExporter]

Great to see the improvements landing, thank you for incorporating them. I 
think these aren't pushed yet to the server on chromium.googlesource.com right? 
Please let me know when they are so I can test it, or if there's an internal 
canary server that works too.

Original comment by jam@chromium.org on 22 Apr 2014 at 2:55

[GoogleCodeExporter]

Not live on gogolesource.com, I wanted to get a round or two of feedback before 
going through that push process. Take a look at this change series, which you 
can fetch and run your own test server:
https://gerrit-review.googlesource.com/56229

Or, any Googlers, ping me on IM and I'll give you an internal test server URL.

Original comment by dborowitz@google.com on 22 Apr 2014 at 4:29

[GoogleCodeExporter]

I tried out the link you sent me over IM. I may be missing something, but I 
don't see how it's possible (and easy) to see previous blames of a piece of 
code. Please see my description in comment 11 and let me know how to do the 
equivalent.

Original comment by jam@chromium.org on 22 Apr 2014 at 4:45

[GoogleCodeExporter]

Did you hover over the revision and not get the "[commit] [diff] [blame]" popup?

Original comment by mmoss@chromium.org on 22 Apr 2014 at 4:54

[GoogleCodeExporter]

@mmoss: yep, I had. I clicked on "commit", and that takes me to the parent 
commit. Then I see a "parent    1b12cf346932031c6da0a9114b100824f466e80a[diff]". I 
clicked on "1b12cf346932031c6da0a9114b100824f466e80a", but then I have to find 
the path of the file I was interested in again (by clicking "content", then 
"renderer", then "render_view_impl.cc"). Note that this step doesn't have to be 
done with viewvc, i.e. i can blame the parent revision directly. Anyways, after 
I found the file, I clicked annotate and it showed a revision from 3 years ago. 
i.e. it appears to have skipped a bunch of changes to that one line.

Please see my example in comment 5, and reproduce the steps but on gitiles to 
show the starting cl.

Original comment by jam@chromium.org on 22 Apr 2014 at 5:07

[GoogleCodeExporter]

@jam: If you just want to step through previous blames, click "[blame]" instead 
of commit.

If you want to do blame -> diff -> blame -> diff, from the first blame page, 
click "[diff]" -> "[blame]" (next to the parent link) -> "[diff]" etc. That was 
my understanding of the process you outlined in #11.

Original comment by dborowitz@google.com on 22 Apr 2014 at 5:40

[GoogleCodeExporter]

By "blame -> diff -> blame -> diff" I mean "blame of C -> diff of C~1..C -> 
blame of C~1 -> diff of C~2..C~1 -> ..."

Original comment by dborowitz@google.com on 22 Apr 2014 at 5:43

[GoogleCodeExporter]

Yeah, I think the "blame -> diff (in the left-side hover popup) -> parent blame 
-> diff -> parent blame ..." looks very similar to your viewvc workflow (it 
actually avoids the extra file view at the end of your step #3, before the next 
annotate). I don't think you want the "commit" link at all for that, which 
would be like clicking the "Revision #" link at the top viewvc annotation page, 
which you don't do.

Original comment by mmoss@google.com on 22 Apr 2014 at 6:00

[GoogleCodeExporter]

@dborowitz @mmoss: can you try out my specific example from comment 5? Clicking 
"blame" or "commit" takes me to a different cl. I'm trying to figure out what 
originally added the " 
GetContentClient()->SetActiveURL(main_frame->document().url());" line, even as 
the method call changes namespaces or the way to get the url changes.

Original comment by jam@chromium.org on 22 Apr 2014 at 6:09

[GoogleCodeExporter]

Ok, I'm going to run through your example in #11.

1. /chromium/chromium/+blame/trunk/content/renderer/render_view_impl.
Starting at l.1071: 
GetContentClient()->SetActiveURL(main_frame->document().url());
2. Hover over 63135f4, click "[diff]"
=> 
/chromium/chromium/+/63135f455920bed0b9b9a4758df5ce067253d4b4%5E%21/content/rend
erer/render_view_impl.cc
3. Read the commit message; see this was a move.
4. Click "[blame]" just to the right of 
"parent f2e92bd3d3d580925574bf11c9374098dda88bb6 [diff]"
=> 
/chromium/chromium/+blame/f2e92bd3d3d580925574bf11c9374098dda88bb6/content/rende
rer/render_view_impl.cc
5. Find that line again, now l. 947, click "[diff]"
=> 
/chromium/chromium/+/a2470189f253b3bd17c37d2765b6f00f47ae4233%5E%21/content/rend
erer/render_view.cc
6. Read commit message, also not what you want (I think), click the parent 
blame link again.
7. Repeat (5) and (6) till you find the commit you want.

Did I misunderstand your example?

Original comment by dborowitz@google.com on 22 Apr 2014 at 6:19

[GoogleCodeExporter]

Aside:
I've also been toying with the idea of including either a one-line summary or 
the full commit message in the hover popup. This might make you able to skip 
the "[diff]" link in some cases, if you can tell from the commit message that 
it's not the commit you're looking for.

The reason I haven't done this yet is doing it naively (i.e. server side) might 
seriously bloat the page size, so it would be time to think about loading it 
dynamically with JS, etc.

Original comment by dborowitz@google.com on 22 Apr 2014 at 6:37

[GoogleCodeExporter]

@dborowitz: did you eventually get to 
http://src.chromium.org/viewvc/chrome?revision=11337&view=revision?

I'm starting at:
http://serval.mtv.corp.google.com:8080/chromium/chromium/+blame/trunk/content/re
nderer/render_view_impl.cc
-searching for 
"GetContentClient()->SetActiveURL(main_frame->document().url());" shows a link 
to 3d9c9d7 (not sure where you got 63135f4?). I click [diff] over the 3d9c9d7 
link. Then I click blame to the right of the parent link which takes me to 
http://serval.mtv.corp.google.com:8080/chromium/chromium/+blame/3d9c9d736f955cea
a1a1076cba10859875fd667f%5E/content/renderer/render_view_impl.cc
-when I search for SetActiveURL in the new page, it shows "a705fea 
stuartmorgan@chromium.org - 3 years, 7 months ago" which is not the second-last 
change to that line.

I think I'm not being clear in my request. Let's try this a different way: can 
you try the exact steps I outlined in step 5 using viewvc (with the links that 
I supplied), and see the code changes to that specific function call 
(SetActiveURL). Then show me how using gitiles goes through the exact same 
changes to that one line.

Original comment by jam@chromium.org on 22 Apr 2014 at 7:57

[GoogleCodeExporter]

Can you send a screenshot of what you see on that first page? I'm seeing 
63135f4, as attached. There are references to 3d9c9d7, but for me, the closest 
one is about 40 lines above.

Original comment by mmoss@google.com on 22 Apr 2014 at 8:10

Attachments:

• gitiles.png

[GoogleCodeExporter]

And, yes, I can eventually get to svn r11337 (it was like 5 or 6 parents up):

http://serval.mtv.corp.google.com:8080/chromium/chromium/+/638e5b1b75e252a81daa9
5b9a3c6502d6397f4e0%5E%21/chrome/renderer/render_view.cc

Original comment by mmoss@google.com on 22 Apr 2014 at 8:22

[GoogleCodeExporter]

I think I got the right answer with gitiles. I'll check with John when he gets 
back.

Original comment by brettw@chromium.org on 22 Apr 2014 at 8:24

[GoogleCodeExporter]

ha, Brett figured out the problem. It looks like a font issue. On Linux this 
works fine, but on Windows the font spacing is different. I'm attaching a 
screenshot to show you how it looks like on my machine. I would try out on 
Linux but I'm having hardware issues there.

Original comment by jam@chromium.org on 22 Apr 2014 at 8:58

Attachments:

• capture.png

[GoogleCodeExporter]

It seems the current layout loads the left column and then the right column and 
expects them to line up. I think there are two problems with this:

1) There are too many things that can cause variable line spacing, such that 
the two sides don'e line up properly. This is what John was seeing.

2) The intermediate load state is weird. The page loads slowly, and often 
you'll be staring at only the annotations (left column) with a blank right 
column. I was pretty confused about this the first time, I thought the file was 
empty or the page was broken. Subsequent times it still tripped me up briefly 
before I remembered what was going on and that I needed to wait longer.

I was watching John use it, and this weird intermediate state also tripped him 
up slightly.

I think the layout should be by row instead of by column, which would eliminate 
both of these issues.

Original comment by brettw@chromium.org on 22 Apr 2014 at 9:03

[GoogleCodeExporter]

Ok, I'm going to take that as a vote for the current changes wrt to links.

Getting the fonts right cross-browser is going to be a nightmare, especially 
since I have no access to Windows, but I already knew that :(

If anybody has pointers to docs/people about how fonts and line height work in 
<pre> blocks, please send them my way.

But really we need to convert to CodeMirror.

Original comment by dborowitz@google.com on 22 Apr 2014 at 9:04

[GoogleCodeExporter]

And don't take my last comment as a sign of despair. CodeMirror conversion 
should be pretty easy.

Original comment by dborowitz@google.com on 22 Apr 2014 at 9:10

[GoogleCodeExporter]

My opinion is that any design where you count on different blocks ending up 
with exactly the same line spacing isn't going to work very well, especially 
when you have stuff like italic floating around (which is normally a completely 
different font file or might be emulated, depending on platform).

Original comment by brettw@chromium.org on 22 Apr 2014 at 10:07

[GoogleCodeExporter]

I agree. Didn't mean it wasn't the easiest thing to get working in a pinch :)

CodeMirror will solve this problem. It allows annotating specific line numbers 
with arbitrary DOM nodes.

Original comment by dborowitz@google.com on 22 Apr 2014 at 10:12

[GoogleCodeExporter]

I haven't seen the new UI, but the description of "hover popup" above worries 
me.

Hover-based UIs are problematic for many reasons, including primarily that
(1) accessibility suffers for users who can't hover a mouse over a visible 
target (blind users, people who can't use mice)
(2) hover is not generally a good signal of user intent/attention (the majority 
of the time, the user doesn't care or necessarily even know where the cursor is)

Accordingly, we try hard to avoid them both in Chrome and in the Google qeb 
properties I'm aware of.

Consider instead other options, like a custom context menu or using additional 
links (e.g. "abc@123 (b c)" where "abc@123" is a link to the diff and "b" and 
"c" are links to the blame and commit).

Incidentally, base on the screenshot in comment 37, it looks like comment 13 
point (1) has still not been addressed.  This is fairly important when trying 
to trace down changes older than a few weeks, and hopefully wouldn't be 
ridiculously hard to implement.

Original comment by pkasting@chromium.org on 24 Apr 2014 at 12:26

[GoogleCodeExporter]

Thanks for reminding me about #13(1), I was planning on changing to absolute 
dates anyway.

As for hover menus, I'm aware of the general arguments against hover, but there 
are a few specific reasons why I went with them _for this application_:
1. There are already potentially dozens of links (one per line) visible on the 
page; tripling the number of links just seems really busy.
2. Horizontal space is valuable here: every pixel we add to the gutter is a 
pixel taken away from the source code.
3. The only link targets on this page are the blame lines, so a user indicates 
intent by moving the cursor over those, at which point they will discover the 
hover menu. (Also, I'm not sure how your suggestion of a custom context menu 
addresses discoverability?)
4. The links in the hover menu are just for convenience and don't strictly add 
functionality; their targets are reachable from the default non-hover link).

That said I'm not set on these, I would just like the discussion to focus on 
this specific application rather than general objections. And it's easy enough 
in the CSS for me to prepare some A/B demos for you guys.

(Also: "we try hard to avoid them both in Chrome and in the Google [w]eb 
properties I'm aware of"...has somebody told the Gmail team? ;)

Original comment by dborowitz@google.com on 24 Apr 2014 at 1:28

[GoogleCodeExporter]

Side-by-side (rough & buggy) comparison of hover/non-hover links:
http://serval.mtv.corp.google.com:8081/chromium/src/+blame/master/content/render
er/render_view_impl.cc
http://serval.mtv.corp.google.com:8082/chromium/src/+blame/master/content/render
er/render_view_impl.cc

Switched to absolute dates, could use a narrower format though.

Original comment by dborowitz@google.com on 24 Apr 2014 at 3:51

[GoogleCodeExporter]

I like the non-hover links better.

The name and date should not be blue.

Original comment by sop@google.com on 24 Apr 2014 at 4:06

[GoogleCodeExporter]

The name and date are blue because they're a link. One of the CSS bugs is that 
that link is covered by a transparent div.

I guess I can also kill [commit].

Would [b][d] be any better, or too short (for both understandability and 
clickability)?

Original comment by dborowitz@google.com on 24 Apr 2014 at 4:09

[GoogleCodeExporter]

Yeah, the date can be a lot shorter, and since it's mostly wanted for 
chronological ordering, maybe something ISO 8601-ish, like "2013-01-01 
18:00:00", which is fixed width and easy to compare numerically. As for the 
links, you could maybe shorten them to something like "com | dif | blm" without 
making it too confusing.

Original comment by mmoss@chromium.org on 24 Apr 2014 at 4:15

[GoogleCodeExporter]

+1 to removing hover: it doesn't work on touch devices and isn't discoverable. 
I'm not too concerned about the extra width, since developers are generally on 
a wide display. the two demo links above are a bit misleading because they 
didn't take up the full width of the page.

i assume these changes will fix the font issue on windows?

thanks!

Original comment by jam@chromium.org on 24 Apr 2014 at 4:31

[GoogleCodeExporter]

big problem I saw now: why doesn't chrome's find-in-page work? the page 
overrides ctrl+f with a dialog that technically works, but doesn't support 
things like find as you type, and has an extra step (press enter). once i 
search through that dialog, chrome's find works. but if i try chrome's find 
first, it doesn't.

Original comment by jam@chromium.org on 24 Apr 2014 at 4:51