Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1080673004:
Remove network load flags not used in net/ (Closed)
|
Created:
5 years, 8 months ago by g.mehndiratt Modified:
5 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, Randy Smith (Not in Mondays) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove network load flags not used in net/
Removed LOAD_FROM_CACHE_IF_OFFLINE flag and related test cases.
BUG=426442
Committed: https://crrev.com/5c8771e8daf16680b63ee4ce8acafcbd23ea325e
Cr-Commit-Position: refs/heads/master@{#327015}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Remove network load flags not used in net #
Total comments: 4
Patch Set 3 : Review comments incorporated. #
Total comments: 2
Patch Set 4 : Patch on latest code because of trybot errors. #Patch Set 5 : Removed errors due to missing changes. #Patch Set 6 : Removed errors due to missing changes. #
Messages
Total messages: 38 (14 generated)
g.mehndiratt@samsung.com changed reviewers: + mmenke@chromium.org
Please review the patch.
Thanks for doing this! [+rdsmith]: FYI. https://codereview.chromium.org/1080673004/diff/1/AUTHORS File AUTHORS (right): https://codereview.chromium.org/1080673004/diff/1/AUTHORS#newcode579 AUTHORS:579: Gitanshu Mehndiratta <g.mehndiratt@samsung.com> Should put your name in alphabetical order, by first name (The last couple names aren't, but the first 500+ are). https://codereview.chromium.org/1080673004/diff/1/net/http/http_cache_transac... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1080673004/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.cc:1134: } This entire block of code is related to the load flag experiment - it should be removed, too (Include the RecordOfflineStatus method and OFFLINE_STATUS enum). Since we're removing a histogram, that should be marked obsolete, too (In tools/metrics/histograms/histograms.xml, locate "HttpCache.OfflineStatus", and add: <obsolete> Deprecated 4/2015. </obsolete>
Review comments incorporated. https://codereview.chromium.org/1080673004/diff/1/AUTHORS File AUTHORS (right): https://codereview.chromium.org/1080673004/diff/1/AUTHORS#newcode579 AUTHORS:579: Gitanshu Mehndiratta <g.mehndiratt@samsung.com> On 2015/04/23 13:16:45, mmenke wrote: > Should put your name in alphabetical order, by first name (The last couple names > aren't, but the first 500+ are). Acknowledged. https://codereview.chromium.org/1080673004/diff/1/net/http/http_cache_transac... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1080673004/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.cc:1134: } On 2015/04/23 13:16:45, mmenke wrote: > This entire block of code is related to the load flag experiment - it should be > removed, too (Include the RecordOfflineStatus method and OFFLINE_STATUS enum). > > Since we're removing a histogram, that should be marked obsolete, too (In > tools/metrics/histograms/histograms.xml, locate "HttpCache.OfflineStatus", and > add: > <obsolete> > Deprecated 4/2015. > </obsolete> Other points are accepted. but if i will remove RecordOfflineStatus, this function is also called in same file with RecordOfflineStatus(effective_load_flags_, OFFLINE_STATUS_FRESH_CACHE); which records another flag. do i need to remove this line also? i think i got it...because we are removing HttpCache.OfflineStatus flag itself it makes sense not to record offlineStatus from any where. Please just confirm for removal of above line. https://codereview.chromium.org/1080673004/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.cc:1134: } On 2015/04/23 13:16:45, mmenke wrote: > This entire block of code is related to the load flag experiment - it should be > removed, too (Include the RecordOfflineStatus method and OFFLINE_STATUS enum). > > Since we're removing a histogram, that should be marked obsolete, too (In > tools/metrics/histograms/histograms.xml, locate "HttpCache.OfflineStatus", and > add: > <obsolete> > Deprecated 4/2015. > </obsolete> Acknowledged.
Thanks for doing this! Just one more quick comment. You'll also need someone from tools/metrics/OWNERS to sign off (Just pick one, add them as a reviewer to this CL, and post a message here asking them to review histograms.xml). https://codereview.chromium.org/1080673004/diff/1/net/http/http_cache_transac... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1080673004/diff/1/net/http/http_cache_transac... net/http/http_cache_transaction.cc:1134: } On 2015/04/23 14:26:35, g.mehndiratt wrote: > On 2015/04/23 13:16:45, mmenke wrote: > > This entire block of code is related to the load flag experiment - it should > be > > removed, too (Include the RecordOfflineStatus method and OFFLINE_STATUS enum). > > > > Since we're removing a histogram, that should be marked obsolete, too (In > > tools/metrics/histograms/histograms.xml, locate "HttpCache.OfflineStatus", and > > add: > > <obsolete> > > Deprecated 4/2015. > > </obsolete> > > > Other points are accepted. but if i will remove RecordOfflineStatus, this > function is also called in same file with > RecordOfflineStatus(effective_load_flags_, OFFLINE_STATUS_FRESH_CACHE); > which records another flag. do i need to remove this line also? > > i think i got it...because we are removing HttpCache.OfflineStatus flag itself > it makes sense not to record offlineStatus from any where. Please just confirm > for removal of above line. Correct, that line isn't needed anymore, either. https://codereview.chromium.org/1080673004/diff/20001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1080673004/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:203: }; You can remove this entire enum.
Whoops; sorry about LOAD_FROM_CACHE_IF_OFFLINE. I removed the code that used that a while back, and forgot to remove the flag.
g.mehndiratt@samsung.com changed reviewers: + mpearson@chromium.org
Added Mr. mpearson to review.
On 2015/04/24 04:30:29, g.mehndiratt wrote: Please review histogram.xml
On 2015/04/23 15:37:17, rdsmith wrote: That's fine i am removing the code where it was missed.
https://codereview.chromium.org/1080673004/diff/20001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1080673004/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:203: }; On 2015/04/23 14:41:28, mmenke (Out 4-27 to 5-1) wrote: > You can remove this entire enum. It's possible you're making the changes now, just wanted to make sure you saw this comment.
Review comments incorporated.Please review. https://codereview.chromium.org/1080673004/diff/20001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (right): https://codereview.chromium.org/1080673004/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:203: }; On 2015/04/24 12:14:01, mmenke (Out 4-27 to 5-1) wrote: Acknowledged. https://codereview.chromium.org/1080673004/diff/20001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:203: }; On 2015/04/23 14:41:28, mmenke (Out 4-27 to 5-1) wrote: Acknowledged.
LGTM! Sorry, thought I signed off last time.
histograms.xml lgtm
b.kelemen@samsung.com changed reviewers: + b.kelemen@samsung.com
https://codereview.chromium.org/1080673004/diff/40001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/1080673004/diff/40001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1129: return SetupEntryForRead(); Are you sure you want to remove this? This seem to be a non-dead code that might be quite useful.
https://codereview.chromium.org/1080673004/diff/40001/net/http/http_cache_tra... File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/1080673004/diff/40001/net/http/http_cache_tra... net/http/http_cache_transaction.cc:1129: return SetupEntryForRead(); On 2015/04/24 20:54:55, kbalazs wrote: > Are you sure you want to remove this? This seem to be a non-dead code that might > be quite useful. It's only set when LOAD_FROM_CACHE_IF_OFFLINE is set, and the flag is no longer being set anywhere, so it is currently dead code. Basically the problem is that we can't just show stale content without some sort of notice that we're doing so, and after talking with the UI guys, a load from cache button won out over just showing a stale content and some sort of infobar warning the page was stale. Thanks for the driveby comment, though - extra eyes on code are never a bad thing.
On 2015/04/24 21:02:52, mmenke (Out 4-27 to 5-1) wrote: > https://codereview.chromium.org/1080673004/diff/40001/net/http/http_cache_tra... > File net/http/http_cache_transaction.cc (left): > > https://codereview.chromium.org/1080673004/diff/40001/net/http/http_cache_tra... > net/http/http_cache_transaction.cc:1129: return SetupEntryForRead(); > On 2015/04/24 20:54:55, kbalazs wrote: > > Are you sure you want to remove this? This seem to be a non-dead code that > might > > be quite useful. > > It's only set when LOAD_FROM_CACHE_IF_OFFLINE is set, and the flag is no longer > being set anywhere, so it is currently dead code. > > Basically the problem is that we can't just show stale content without some sort > of notice that we're doing so, and after talking with the UI guys, a load from > cache button won out over just showing a stale content and some sort of infobar > warning the page was stale. > > Thanks for the driveby comment, though - extra eyes on code are never a bad > thing. I see, thanks.
The CQ bit was checked by g.mehndiratt@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1080673004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by g.mehndiratt@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1080673004/#ps60001 (title: "Patch on latest code because of trybot errors.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1080673004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by g.mehndiratt@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1080673004/#ps80001 (title: "Removed errors due to missing changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1080673004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by g.mehndiratt@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1080673004/#ps100001 (title: "Removed errors due to missing changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1080673004/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5c8771e8daf16680b63ee4ce8acafcbd23ea325e Cr-Commit-Position: refs/heads/master@{#327015}
Message was sent while issue was closed.
Patchset #7 (id:120001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
