CalendarPicker display in correct format for minimum Value

CalendarPicker display in correct format for minimum Value

Offset for minimum value, 0001/01/01 returns 0, resulting in setContentOffset not setting required values.

R=keishi1, tkent
BUG=348461
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169312

[Habib Virji, 2014-03-11 17:20:07]

Value 0001/01/01 which is a min value for calendarPicker returns offset 0,
resulting in setContentOffset failing to set contentOffset creating overlap. 

Though the previous check appears to be useful when redrawing table again but it
creates the overlap for min value. 

Test, found only manual test for calendarPicker which does not test values.
Please suggest if any automated test can be added.

[tkent, 2014-03-11 22:49:32]

Please refer to files in LayoutTests/fast/forms/calendar-picker/.
Making a rendering test is easy for this change.  Also, we might be able to make
a unit test like calendar-picker-date-types.html.

[Habib Virji, 2014-03-12 22:08:21]

On 2014/03/11 22:49:32, tkent wrote:
> Please refer to files in LayoutTests/fast/forms/calendar-picker/.
> Making a rendering test is easy for this change.  Also, we might be able to
make
> a unit test like calendar-picker-date-types.html.

Thanks. Did add test but they seem to be not matching on try bot server. Any
suggestion.

[tkent, 2014-03-13 01:21:27]

> Thanks. Did add test but they seem to be not matching on try bot server. Any
> suggestion.

Typically local result and bot results don't match for pixel tests. So please
add the following line to LayoutTests/TestExpectations.

crbug.com/348461
fast/forms/calendar-picker/calendar-picker-appearance-minimum-date.html [
NeedsRebaseline ]

[Habib Virji, 2014-03-13 14:26:37]

On 2014/03/13 01:21:27, tkent wrote:
> > Thanks. Did add test but they seem to be not matching on try bot server. Any
> > suggestion.
> 
> Typically local result and bot results don't match for pixel tests. So please
> add the following line to LayoutTests/TestExpectations.
> 
> crbug.com/348461
> fast/forms/calendar-picker/calendar-picker-appearance-minimum-date.html [
> NeedsRebaseline ]

Thanks. Updated in testexpectations.

[tkent, 2014-03-14 07:59:57]

Keishi, would you take a look at this please?

[keishi, 2014-03-14 08:38:59]

Thanks for fixing this.

https://codereview.chromium.org/195443002/diff/60001/Source/web/resources/cal...
File Source/web/resources/calendarPicker.js (right):

https://codereview.chromium.org/195443002/diff/60001/Source/web/resources/cal...
Source/web/resources/calendarPicker.js:1524: if (this._contentOffset < 0)
The original code's intent is to return early if the new content offset value is
the same as the current one. content offset should be able to take negative
values (as long as it is more than the minimumContentOffset). 

This change should be reverted.

https://codereview.chromium.org/195443002/diff/60001/Source/web/resources/cal...
Source/web/resources/calendarPicker.js:1525: return;
I think I would separate out everything below this line into a separate method
into something like ScrollView.prototype._updateScrollContent.

Then call it inside attachTo method.

ScrollView.prototype.attachTo = function(parent, before) {
    View.prototype.attachTo.call(this, parent, before);
    this._updateScrollContent();
};

https://codereview.chromium.org/195443002/diff/60001/Source/web/resources/cal...
Source/web/resources/calendarPicker.js:1532:
this.delegate.scrollViewDidChangeContentOffset(this);
Keep this line in setContentOffset()

[Habib Virji, 2014-03-14 10:38:15]

Thanks keishi for reviewing.

I have updated code as per your suggestion.

https://codereview.chromium.org/195443002/diff/60001/Source/web/resources/cal...
File Source/web/resources/calendarPicker.js (right):

https://codereview.chromium.org/195443002/diff/60001/Source/web/resources/cal...
Source/web/resources/calendarPicker.js:1524: if (this._contentOffset < 0)
On 2014/03/14 08:38:59, keishi1 wrote:
> The original code's intent is to return early if the new content offset value
is
> the same as the current one. content offset should be able to take negative
> values (as long as it is more than the minimumContentOffset). 
> 
> This change should be reverted.

Done.

https://codereview.chromium.org/195443002/diff/60001/Source/web/resources/cal...
Source/web/resources/calendarPicker.js:1525: return;
On 2014/03/14 08:38:59, keishi1 wrote:
> I think I would separate out everything below this line into a separate method
> into something like ScrollView.prototype._updateScrollContent.
> 
> Then call it inside attachTo method.
> 
> ScrollView.prototype.attachTo = function(parent, before) {
>     View.prototype.attachTo.call(this, parent, before);
>     this._updateScrollContent();
> };

Done.

https://codereview.chromium.org/195443002/diff/60001/Source/web/resources/cal...
Source/web/resources/calendarPicker.js:1532:
this.delegate.scrollViewDidChangeContentOffset(this);
On 2014/03/14 08:38:59, keishi1 wrote:
> Keep this line in setContentOffset()

Done.

[keishi, 2014-03-14 10:51:06]

https://codereview.chromium.org/195443002/diff/80001/Source/web/resources/cal...
File Source/web/resources/calendarPicker.js (right):

https://codereview.chromium.org/195443002/diff/80001/Source/web/resources/cal...
Source/web/resources/calendarPicker.js:1525: var partitionChanged =
this._updateScrollContent(value);
Set this._contentOffset before calling _updateScrollContent so we can get rid of
the value param.

https://codereview.chromium.org/195443002/diff/80001/Source/web/resources/cal...
Source/web/resources/calendarPicker.js:1530:
this.delegate.scrollViewDidChangePartition(this);
This should be called inside _updateScrollContent. When we attach,
ListView.prototype.scrollViewDidChangePartition should be called.

[Habib Virji, 2014-03-14 11:18:36]

Thanks updated as suggested

https://codereview.chromium.org/195443002/diff/80001/Source/web/resources/cal...
File Source/web/resources/calendarPicker.js (right):

https://codereview.chromium.org/195443002/diff/80001/Source/web/resources/cal...
Source/web/resources/calendarPicker.js:1525: var partitionChanged =
this._updateScrollContent(value);
On 2014/03/14 10:51:07, keishi1 wrote:
> Set this._contentOffset before calling _updateScrollContent so we can get rid
of
> the value param.

Done.

https://codereview.chromium.org/195443002/diff/80001/Source/web/resources/cal...
Source/web/resources/calendarPicker.js:1530:
this.delegate.scrollViewDidChangePartition(this);
On 2014/03/14 10:51:07, keishi1 wrote:
> This should be called inside _updateScrollContent. When we attach,
> ListView.prototype.scrollViewDidChangePartition should be called.

Done.

[keishi, 2014-03-14 14:29:08]

Thanks. LGTM

[tkent, 2014-03-15 16:28:47]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-15 18:53:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/habib.virji@samsung.com/195443002/100001

[commit-bot: I haz the power, 2014-03-15 19:03:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-15 19:03:58]

Try jobs failed on following builders:
  tryserver.blink on blink_presubmit

[tkent, 2014-03-16 00:05:29]

lgtm

[commit-bot: I haz the power, 2014-03-16 00:05:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/habib.virji@samsung.com/195443002/100001

[commit-bot: I haz the power, 2014-03-16 22:41:53]

Change committed as 169312