New issue
Advanced search Search tips

Issue 897841 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 24
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug


Show other hotlists

Hotlists containing this issue:
Downloads-Home-Rework


Sign in to add a comment

[Downloads Home] Storage total size doesn't take into account offline pages

Project Member Reported by shaktisahu@chromium.org, Oct 22

Issue description

This happens as OfflineItem#receivedBytes is always set to zero.
 
Let's expand this bug to make sure all OfflineItem fields that should be set are set (in both CreateOfflineItem calls) and tested for that (in offline_item_conversions_unittest.cc).

I confirmed that OfflineItem::operator== checks for all of them but there are two issues: 
1) Checking for equality of the expectation and the result will only reveal differences (missed fields in the copy) if they are actually set in the base object sent to CreateOfflineItem.
2) There's no test that verifies that operator== actually covers all fields so if another one is added it might slip through.

OfflineItem has these fields (alphabetically sorted, here and for all following lists):
allow_metered
completion_time
creation_time
description
externally_removed
fail_state
file_path
filter
id
is_accelerated
is_dangerous
is_off_the_record
is_openable
is_resumable
is_suggested
is_transient
last_accessed_time
mime_type
original_url
page_url
pending_state
progress
promote_origin
received_bytes
refresh_visuals
state
time_remaining_ms
title
total_size_bytes


Currently OfflineItemConversionsTest.OfflinePageItemConversion only tests for:
creation_time
externally_removed
file_path
filter
id
is_openable
is_suggested
last_accessed_time
mime_type
page_url
progress
state
title
total_size_bytes


While OfflineItemConversionsTest.SavePageRequestConversion only tests for:
creation_time
file_path
filter
id
is_suggested
last_accessed_time
mime_type
page_url
progress
state
title

Maybe not all fields make sense for both conversions but we should test all the ones that do.
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9b1829c578c6680cfedba88bcbffee23508a0249

commit 9b1829c578c6680cfedba88bcbffee23508a0249
Author: Shakti Sahu <shaktisahu@chromium.org>
Date: Tue Oct 23 21:40:10 2018

Offline pages : Fixed received_bytes for OfflineItem conversion

Fixed the received_bytes field for offline pages to OfflineItem conversion.
Also added all the missing fields to the conversion unit tests.

Bug:  897841 
Change-Id: I823a84bc36a9d07c2f955f756c91412d22fa59ae
Reviewed-on: https://chromium-review.googlesource.com/c/1294404
Commit-Queue: Shakti Sahu <shaktisahu@chromium.org>
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602110}
[modify] https://crrev.com/9b1829c578c6680cfedba88bcbffee23508a0249/components/offline_pages/core/downloads/download_ui_adapter_unittest.cc
[modify] https://crrev.com/9b1829c578c6680cfedba88bcbffee23508a0249/components/offline_pages/core/downloads/offline_item_conversions.cc
[modify] https://crrev.com/9b1829c578c6680cfedba88bcbffee23508a0249/components/offline_pages/core/downloads/offline_item_conversions_unittest.cc

Labels: Merge-Request-71 OS-Android
Project Member

Comment 4 by sheriffbot@chromium.org, Oct 24

Labels: -Merge-Request-71 Hotlist-Merge-Approved Merge-Approved-71
Your change meets the bar and is auto-approved for M71. Please go ahead and merge the CL to branch 3578 manually. Please contact milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: Fixed (was: Started)
Labels: -Merge-Approved-71 Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/f5f81dbd070c6ed4bffc19b477cb5a6cc3595cc7

Commit: f5f81dbd070c6ed4bffc19b477cb5a6cc3595cc7
Author: shaktisahu@chromium.org
Commiter: shaktisahu@chromium.org
Date: 2018-10-24 22:02:34 +0000 UTC

Offline pages : Fixed received_bytes for OfflineItem conversion

Fixed the received_bytes field for offline pages to OfflineItem conversion.
Also added all the missing fields to the conversion unit tests.

Bug:  897841 
Change-Id: I823a84bc36a9d07c2f955f756c91412d22fa59ae
Reviewed-on: https://chromium-review.googlesource.com/c/1294404
Commit-Queue: Shakti Sahu <shaktisahu@chromium.org>
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#602110}(cherry picked from commit 9b1829c578c6680cfedba88bcbffee23508a0249)
Reviewed-on: https://chromium-review.googlesource.com/c/1298084
Cr-Commit-Position: refs/branch-heads/3578@{#307}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 24

Labels: merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f5f81dbd070c6ed4bffc19b477cb5a6cc3595cc7

commit f5f81dbd070c6ed4bffc19b477cb5a6cc3595cc7
Author: Shakti Sahu <shaktisahu@chromium.org>
Date: Wed Oct 24 22:02:34 2018

Offline pages : Fixed received_bytes for OfflineItem conversion

Fixed the received_bytes field for offline pages to OfflineItem conversion.
Also added all the missing fields to the conversion unit tests.

Bug:  897841 
Change-Id: I823a84bc36a9d07c2f955f756c91412d22fa59ae
Reviewed-on: https://chromium-review.googlesource.com/c/1294404
Commit-Queue: Shakti Sahu <shaktisahu@chromium.org>
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#602110}(cherry picked from commit 9b1829c578c6680cfedba88bcbffee23508a0249)
Reviewed-on: https://chromium-review.googlesource.com/c/1298084
Cr-Commit-Position: refs/branch-heads/3578@{#307}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/f5f81dbd070c6ed4bffc19b477cb5a6cc3595cc7/components/offline_pages/core/downloads/download_ui_adapter_unittest.cc
[modify] https://crrev.com/f5f81dbd070c6ed4bffc19b477cb5a6cc3595cc7/components/offline_pages/core/downloads/offline_item_conversions.cc
[modify] https://crrev.com/f5f81dbd070c6ed4bffc19b477cb5a6cc3595cc7/components/offline_pages/core/downloads/offline_item_conversions_unittest.cc

Sign in to add a comment