Clicking on scroll bar in timing mode should get handled by scrollbar only.

Clicking on scroll bar in timing mode should get handled by scrollbar only.

On clicking scroll bar of 'timing mode', thread times window is shifting to
right side and user is unable to scroll down. We are assuming that every hit
in timing mode is on the canvas area itself and moving to setting the interest
range value including that the range right is being selected. Do the interest
range actions only if the mousedown hit point is in the canvas rect.

BUG=549

[r.kasibhatla, 2014-09-11 07:01:23]

PTAL.

[vivekg, 2014-09-11 14:29:33]

https://codereview.appspot.com/138330043/diff/20001/trace_viewer/tracing/timi...
File trace_viewer/tracing/timing_tool.html (right):

https://codereview.appspot.com/138330043/diff/20001/trace_viewer/tracing/timi...
trace_viewer/tracing/timing_tool.html:48: onBeginTiming: function(e) {
I think it would be better if we do the bound checking before we compute the
snappedPosition.

[nduca, 2014-09-11 22:41:32]

https://codereview.appspot.com/138330043/diff/20001/trace_viewer/tracing/timi...
File trace_viewer/tracing/timing_tool.html (right):

https://codereview.appspot.com/138330043/diff/20001/trace_viewer/tracing/timi...
trace_viewer/tracing/timing_tool.html:48: onBeginTiming: function(e) {
agreed. even better would be a is a helper isPointInsideBlah: function(clientX,
clientY) and this just calls that.

https://codereview.appspot.com/138330043/diff/20001/trace_viewer/tracing/timi...
trace_viewer/tracing/timing_tool.html:104: var canvasRect =
this.viewport_.modelTrackContainer.canvas.getBoundingClientRect();
do you really need the canvas? can you use modelTrackContainer's bounding client
rect?

[r.kasibhatla, 2014-09-12 05:23:07]

PTAL.

https://codereview.appspot.com/138330043/diff/20001/trace_viewer/tracing/timi...
File trace_viewer/tracing/timing_tool.html (right):

https://codereview.appspot.com/138330043/diff/20001/trace_viewer/tracing/timi...
trace_viewer/tracing/timing_tool.html:48: onBeginTiming: function(e) {
On 2014/09/11 14:29:33, vivekg wrote:
> I think it would be better if we do the bound checking before we compute the
> snappedPosition.

Done.

https://codereview.appspot.com/138330043/diff/20001/trace_viewer/tracing/timi...
trace_viewer/tracing/timing_tool.html:48: onBeginTiming: function(e) {
On 2014/09/11 22:41:31, nduca wrote:
> agreed. even better would be a is a helper isPointInsideBlah:
function(clientX,
> clientY) and this just calls that.

Done.

https://codereview.appspot.com/138330043/diff/20001/trace_viewer/tracing/timi...
trace_viewer/tracing/timing_tool.html:104: var canvasRect =
this.viewport_.modelTrackContainer.canvas.getBoundingClientRect();
On 2014/09/11 22:41:31, nduca wrote:
> do you really need the canvas? can you use modelTrackContainer's bounding
client
> rect?
We cannot use the bounding rect of modelTrackContainer as the scrollbar is
attached to the rect of modelTrackContainer.
Using the modelTrackContainer bounding rect, mousedown event on scrollbar will
highlight canvas. Hence, we should use canvas bounding rect only.

[r.kasibhatla, 2014-09-16 03:31:36]

On 2014/09/12 05:23:07, r.kasibhatla wrote:
> PTAL.
> 
>
https://codereview.appspot.com/138330043/diff/20001/trace_viewer/tracing/timi...
> File trace_viewer/tracing/timing_tool.html (right):
> 
>
https://codereview.appspot.com/138330043/diff/20001/trace_viewer/tracing/timi...
> trace_viewer/tracing/timing_tool.html:48: onBeginTiming: function(e) {
> On 2014/09/11 14:29:33, vivekg wrote:
> > I think it would be better if we do the bound checking before we compute the
> > snappedPosition.
> 
> Done.
> 
>
https://codereview.appspot.com/138330043/diff/20001/trace_viewer/tracing/timi...
> trace_viewer/tracing/timing_tool.html:48: onBeginTiming: function(e) {
> On 2014/09/11 22:41:31, nduca wrote:
> > agreed. even better would be a is a helper isPointInsideBlah:
> function(clientX,
> > clientY) and this just calls that.
> 
> Done.
> 
>
https://codereview.appspot.com/138330043/diff/20001/trace_viewer/tracing/timi...
> trace_viewer/tracing/timing_tool.html:104: var canvasRect =
> this.viewport_.modelTrackContainer.canvas.getBoundingClientRect();
> On 2014/09/11 22:41:31, nduca wrote:
> > do you really need the canvas? can you use modelTrackContainer's bounding
> client
> > rect?
> We cannot use the bounding rect of modelTrackContainer as the scrollbar is
> attached to the rect of modelTrackContainer.
> Using the modelTrackContainer bounding rect, mousedown event on scrollbar will
> highlight canvas. Hence, we should use canvas bounding rect only.

Friendly ping... :)

[dsinclair, 2014-09-18 00:33:22]

https://codereview.appspot.com/138330043/diff/80001/trace_viewer/tracing/timi...
File trace_viewer/tracing/timing_tool.html (right):

https://codereview.appspot.com/138330043/diff/80001/trace_viewer/tracing/timi...
trace_viewer/tracing/timing_tool.html:106: }
This is missing a final return statement.

Can you invert the outside if and have it return immediately. Would make this
easier to read.

[r.kasibhatla, 2014-09-18 04:13:37]

PTAL. Thanks!

https://codereview.appspot.com/138330043/diff/80001/trace_viewer/tracing/timi...
File trace_viewer/tracing/timing_tool.html (right):

https://codereview.appspot.com/138330043/diff/80001/trace_viewer/tracing/timi...
trace_viewer/tracing/timing_tool.html:106: }
On 2014/09/18 00:33:21, dsinclair wrote:
> This is missing a final return statement.
> 
> Can you invert the outside if and have it return immediately. Would make this
> easier to read.

Done.

[dsinclair, 2014-09-18 13:19:29]

This is good to me. Can you turn it into a pull request against the github repo?