Fix to hide Edit Items button when there are no items in history

Fix to hide Edit Items button when there are no items in history


BUG=37978
TEST=Verify that history page with zero items should not display Edit Items button

[Patrick Dubroy, 2011-10-04 11:40:27]

Thanks for the patch.

The changes look fine to me, but the changes to history2.js will be unnecessary
once http://codereview.chromium.org/8079010/ lands. Also, we will soon be making
history2 the default.

[naveenbobbili, 2011-10-04 11:56:53]

On 2011/10/04 11:40:27, dubroy wrote:
> Thanks for the patch.
> 
> The changes look fine to me, but the changes to history2.js will be
unnecessary
> once http://codereview.chromium.org/8079010/ lands. Also, we will soon be
making
> history2 the default.

Do you want me to add these changes only to history.js and upload the patch?

[naveenbobbili, 2011-10-04 12:03:18]

Uploaded patch with only history.js changes.

[naveenbobbili, 2011-10-05 04:43:06]

On 2011/10/04 12:03:18, naveenbobbili wrote:
> Uploaded patch with only history.js changes.

Hi,
Can you please review this patch. I have removed history2 changes.

Regards,
Naveen

[NaveenBobbili (Motorola), 2011-10-10 04:40:45]

On 2011/10/05 04:43:06, naveenbobbili wrote:
> On 2011/10/04 12:03:18, naveenbobbili wrote:
> > Uploaded patch with only history.js changes.
> 
> Hi,
> Can you please review this patch. I have removed history2 changes.
> 
> Regards,
> Naveen

These changes are working fine and pending review. Please review this
changelist.

[NaveenBobbili (Motorola), 2011-10-13 08:27:31]

Please review this issue.

[James Hawkins, 2011-10-15 20:39:42]

http://codereview.chromium.org/8118001/diff/3002/chrome/browser/resources/his...
File chrome/browser/resources/history.js (right):

http://codereview.chromium.org/8118001/diff/3002/chrome/browser/resources/his...
chrome/browser/resources/history.js:479: this.editButtonTd_.hidden = true;
Inconsistent. The line directly above this uses self instead of this. Personally
I think the line above should be changed s/self/this/.

http://codereview.chromium.org/8118001/diff/3002/chrome/browser/resources/his...
chrome/browser/resources/history.js:587: // Hide the Edit Button if we are not
in edit mode and
Don't use 'we' in comments; it's ambiguous.

http://codereview.chromium.org/8118001/diff/3002/chrome/browser/resources/his...
chrome/browser/resources/history.js:587: // Hide the Edit Button if we are not
in edit mode and
nit: Fill up the 80 cols when writing comments instead of wrapping randomly.

http://codereview.chromium.org/8118001/diff/3002/chrome/browser/resources/his...
chrome/browser/resources/history.js:589: if (this.model_.getSize() <= 0 &&
!this.model_.getEditMode())
When can getSize() return a number < 0?

http://codereview.chromium.org/8118001/diff/3002/chrome/browser/resources/his...
chrome/browser/resources/history.js:589: if (this.model_.getSize() <= 0 &&
!this.model_.getEditMode())
this.editButtonTd_.hidden = this.model_.getSize()...;

[NaveenBobbili (Motorola), 2011-10-17 06:18:28]

Thanks for posting your comments. I have uploaded a new patch set after
addressing your comments.

[James Hawkins, 2011-10-17 17:09:15]

On 2011/10/17 06:18:28, NaveenBobbili (Motorola) wrote:
> Thanks for posting your comments. I have uploaded a new patch set after
> addressing your comments.

See my previous comment on your other CL about using 'Done'.

[Evan Stade, 2011-10-17 22:18:21]

slight rewrite of cl description:

history: hide "edit items" button when there are no items to display

aside from that LGTM.

[NaveenBobbili (Motorola), 2011-10-18 04:12:43]

http://codereview.chromium.org/8118001/diff/3002/chrome/browser/resources/his...
File chrome/browser/resources/history.js (right):

http://codereview.chromium.org/8118001/diff/3002/chrome/browser/resources/his...
chrome/browser/resources/history.js:479: this.editButtonTd_.hidden = true;
On 2011/10/15 20:39:43, James Hawkins wrote:
> Inconsistent. The line directly above this uses self instead of this.
Personally
> I think the line above should be changed s/self/this/.

Done.

http://codereview.chromium.org/8118001/diff/3002/chrome/browser/resources/his...
chrome/browser/resources/history.js:587: // Hide the Edit Button if we are not
in edit mode and
On 2011/10/15 20:39:43, James Hawkins wrote:
> Don't use 'we' in comments; it's ambiguous.

Done.

http://codereview.chromium.org/8118001/diff/3002/chrome/browser/resources/his...
chrome/browser/resources/history.js:587: // Hide the Edit Button if we are not
in edit mode and
On 2011/10/15 20:39:43, James Hawkins wrote:
> nit: Fill up the 80 cols when writing comments instead of wrapping randomly.

Done.

http://codereview.chromium.org/8118001/diff/3002/chrome/browser/resources/his...
chrome/browser/resources/history.js:589: if (this.model_.getSize() <= 0 &&
!this.model_.getEditMode())
On 2011/10/15 20:39:43, James Hawkins wrote:
> When can getSize() return a number < 0?

Done.

http://codereview.chromium.org/8118001/diff/3002/chrome/browser/resources/his...
chrome/browser/resources/history.js:589: if (this.model_.getSize() <= 0 &&
!this.model_.getEditMode())
On 2011/10/15 20:39:43, James Hawkins wrote:
> this.editButtonTd_.hidden = this.model_.getSize()...;

Done.

[NaveenBobbili (Motorola), 2011-10-18 04:33:43]

On 2011/10/17 22:18:21, Evan Stade wrote:
> slight rewrite of cl description:
> 
> history: hide "edit items" button when there are no items to display
> 
> aside from that LGTM.

Initially I was able to create reviews with my alias account
naveenbobbili@motorola.com. Due to internal motorola policy and new migrations
that took place I had to switch over and submit reviews using only my primary
account which is qghc36@motorola.com which is preventing me from editing this
issue. So I am creating a new review request after addressing all the review
comments with qghc36@motorola.com. I will have to switch over all my pending
review requests also. Sorry for the inconvienience. I have raised a new request
for the same issue at http://codereview.chromium.org/8334010.