Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1969463002:
Add a save page request to the request queue (and test it). (Closed)
|
Created:
4 years, 7 months ago by Pete Williamson Modified:
4 years, 7 months ago CC:
chromium-reviews, fgorski+watch_chromium.org, romax+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a save page request to the request queue (and test it).
We already have code to catch the press on a menu item for saving
a page offline in the background. The call arrives at the request
coordinator. This change will then place the request onto the request
queue.
BUG=610521
patch from issue 1956633002 at patchset 20001 (http://crrev.com/1956633002#ps20001)
Committed: https://crrev.com/c8a55c9469c3642d822ef07b638bc6d1e2c95fd4
Cr-Commit-Position: refs/heads/master@{#393107}
Patch Set 1 #Patch Set 2 : Remove files added in error. #
Total comments: 35
Patch Set 3 : CR feedback per DougArnett #Patch Set 4 : CR feedback per FGorski #
Total comments: 8
Patch Set 5 : More CR feedback from FGorski #
Messages
Total messages: 19 (6 generated)
petewil@chromium.org changed reviewers: + dougarnett@chromium.org, fgorski@chromium.org
Hooks up the menu click to the request queue. Scheduler will be next changelist.
just nits so far https://codereview.chromium.org/1969463002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc (right): https://codereview.chromium.org/1969463002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 16 https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:31: DVLOG(0) << "URL is " << url << " " << __FUNCTION__; maybe not submit? or the @@@@@ one below https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:62: // The queue is also owned by this RequestCoordinator. nit - unless I'm missing something, seems like all three of these members should have comments that read the same - whether "takes over ownership" or "owned by"
https://codereview.chromium.org/1969463002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc (right): https://codereview.chromium.org/1969463002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/05/10 20:23:34, dougarnett wrote: > 16 Done. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:31: DVLOG(0) << "URL is " << url << " " << __FUNCTION__; On 2016/05/10 20:23:34, dougarnett wrote: > maybe not submit? or the @@@@@ one below Changed both to DVLOG(2). Both of these will help as we get our wireframe up and working. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:62: // The queue is also owned by this RequestCoordinator. On 2016/05/10 20:23:34, dougarnett wrote: > nit - unless I'm missing something, seems like all three of these members should > have comments that read the same - whether "takes over ownership" or "owned by" Sure. I was trying to avoid boring the reader by varying my wording, but it is probably better for the reader if I am consistent than not boring.
https://codereview.chromium.org/1969463002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc (right): https://codereview.chromium.org/1969463002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc:7: #include <memory> Is this used? https://codereview.chromium.org/1969463002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc:15: // constants here This does not seem needed. How about removing it? https://codereview.chromium.org/1969463002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc:23: void SetUp() override; Is this needed? How about removing it if it is not? https://codereview.chromium.org/1969463002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc:35: RequestCoordinatorFactory* factory(RequestCoordinatorFactory::GetInstance()); please use assignment, it is confusing and I couldn't find this type of ptr initialization in Chromium https://codereview.chromium.org/1969463002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc:40: EXPECT_NE(nullptr, coordinator); you can also verify that calling multiple times gives you the same coordinator. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:38: id++, url, client_id, base::Time::Now()); nit align Also, please add a TODO to use something like base::Clock. This will help us when testing this class. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:43: base::Unretained(this))); base::Unretained should not be used in production. You should add a weak_ptr_factory_ to the coordinator and use weak pointer here. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:44: // TODO: Do I need to persist the request in case the add fails? Adding a request to the queue is meant to persist it. Perhaps the right thing to do is to update the interface, to use a callback for SavePageLater, which will be able to send a result like that. This is something to consider as an alternative and definitely doesn't have to be solved for this patch. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:22: class RequestQueue; since you are adding the header above, you don't need this. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:62: // The queue is also owned by this RequestCoordinator. On 2016/05/10 20:23:34, dougarnett wrote: > nit - unless I'm missing something, seems like all three of these members should > have comments that read the same - whether "takes over ownership" or "owned by" Good point. I suggest: // Request queue. Used to <what it does>. Owned. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:72: RequestQueue::GetRequestsResult result, nit: align properly https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:80: RequestCoordinator::ProcessingDoneCallback callback = now that 84-90 block has grown, and regardless what is done in terms of extracting it, this code (80-83) feels like it should be closer to line 91. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:101: std::move(policy), std::move(factory), std::move(queue)); I know this code only shows up twice 84-90 and 95-101, but I think it is good time to consider extracting it. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:113: EXPECT_EQ(1U, last_requests().size()); 1UL?
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1969463002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1969463002/60001
Address CR feedback per FGorski https://codereview.chromium.org/1969463002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc (right): https://codereview.chromium.org/1969463002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc:7: #include <memory> On 2016/05/10 21:18:12, fgorski wrote: > Is this used? Removed (leftover from using smart pointers) https://codereview.chromium.org/1969463002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc:15: // constants here On 2016/05/10 21:18:12, fgorski wrote: > This does not seem needed. How about removing it? Done. https://codereview.chromium.org/1969463002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc:23: void SetUp() override; On 2016/05/10 21:18:12, fgorski wrote: > Is this needed? How about removing it if it is not? Yes, used below for the RenderViewHostTestHarness setup. https://codereview.chromium.org/1969463002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc:35: RequestCoordinatorFactory* factory(RequestCoordinatorFactory::GetInstance()); On 2016/05/10 21:18:12, fgorski wrote: > please use assignment, it is confusing and I couldn't find this type of ptr > initialization in Chromium Done. https://codereview.chromium.org/1969463002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc:40: EXPECT_NE(nullptr, coordinator); On 2016/05/10 21:18:12, fgorski wrote: > you can also verify that calling multiple times gives you the same coordinator. Done. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:38: id++, url, client_id, base::Time::Now()); On 2016/05/10 21:18:12, fgorski wrote: > nit align > > Also, please add a TODO to use something like base::Clock. > This will help us when testing this class. Done. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:43: base::Unretained(this))); On 2016/05/10 21:18:12, fgorski wrote: > base::Unretained should not be used in production. > > You should add a weak_ptr_factory_ to the coordinator and use weak pointer here. Done. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:44: // TODO: Do I need to persist the request in case the add fails? On 2016/05/10 21:18:12, fgorski wrote: > Adding a request to the queue is meant to persist it. > Perhaps the right thing to do is to update the interface, to use a callback for > SavePageLater, which will be able to send a result like that. > > This is something to consider as an alternative and definitely doesn't have to > be solved for this patch. Noted. We'll solve this with a later patch (and I'll leave the TODO in to remind us). https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:22: class RequestQueue; On 2016/05/10 21:18:12, fgorski wrote: > since you are adding the header above, you don't need this. Done. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:62: // The queue is also owned by this RequestCoordinator. On 2016/05/10 21:18:12, fgorski wrote: > On 2016/05/10 20:23:34, dougarnett wrote: > > nit - unless I'm missing something, seems like all three of these members > should > > have comments that read the same - whether "takes over ownership" or "owned > by" > > Good point. I suggest: > // Request queue. Used to <what it does>. Owned. Done. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:72: RequestQueue::GetRequestsResult result, On 2016/05/10 21:18:12, fgorski wrote: > nit: align properly Done. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:80: RequestCoordinator::ProcessingDoneCallback callback = On 2016/05/10 21:18:12, fgorski wrote: > now that 84-90 block has grown, and regardless what is done in terms of > extracting it, this code (80-83) feels like it should be closer to line 91. Done. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:101: std::move(policy), std::move(factory), std::move(queue)); On 2016/05/10 21:18:12, fgorski wrote: > I know this code only shows up twice 84-90 and 95-101, but I think it is good > time to consider extracting it. Done. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:113: EXPECT_EQ(1U, last_requests().size()); On 2016/05/10 21:18:12, fgorski wrote: > 1UL? Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with comments. I'd still remove that setup call in test, as it is not needed. https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1969463002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:44: // TODO: Do I need to persist the request in case the add fails? On 2016/05/10 22:51:57, Pete Williamson wrote: > On 2016/05/10 21:18:12, fgorski wrote: > > Adding a request to the queue is meant to persist it. > > Perhaps the right thing to do is to update the interface, to use a callback > for > > SavePageLater, which will be able to send a result like that. > > > > This is something to consider as an alternative and definitely doesn't have to > > be solved for this patch. > > Noted. We'll solve this with a later patch (and I'll leave the TODO in to > remind us). Acknowledged. https://codereview.chromium.org/1969463002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc (right): https://codereview.chromium.org/1969463002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc:26: content::RenderViewHostTestHarness::SetUp(); About removal of setup. This is exactly what I implied: RenderViewHostTestHarness is in your inheritance chain and happens without redefining SetUp(). Hence this method does not contribute, I suggest removing it. https://codereview.chromium.org/1969463002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc:33: factory->GetForBrowserContext(browser_context()); Nice. I missed that. https://codereview.chromium.org/1969463002/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1969463002/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:27: public base::SupportsWeakPtr<RequestCoordinator> { nit: alignment. Unless you ran git cl format and this is the output of course. https://codereview.chromium.org/1969463002/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/1969463002/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:82: nit: remove extra empty line
https://codereview.chromium.org/1969463002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc (right): https://codereview.chromium.org/1969463002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc:26: content::RenderViewHostTestHarness::SetUp(); On 2016/05/11 19:45:15, fgorski wrote: > About removal of setup. > This is exactly what I implied: RenderViewHostTestHarness is in your inheritance > chain and happens without redefining SetUp(). > > Hence this method does not contribute, I suggest removing it. Done. https://codereview.chromium.org/1969463002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory_unittest.cc:33: factory->GetForBrowserContext(browser_context()); On 2016/05/11 19:45:15, fgorski wrote: > Nice. I missed that. Acknowledged. https://codereview.chromium.org/1969463002/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1969463002/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:27: public base::SupportsWeakPtr<RequestCoordinator> { On 2016/05/11 19:45:15, fgorski wrote: > nit: alignment. > > Unless you ran git cl format and this is the output of course. Done. https://codereview.chromium.org/1969463002/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/1969463002/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:82: On 2016/05/11 19:45:15, fgorski wrote: > nit: remove extra empty line Done.
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougarnett@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/1969463002/#ps80001 (title: "More CR feedback from FGorski")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1969463002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1969463002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add a save page request to the request queue (and test it). We already have code to catch the press on a menu item for saving a page offline in the background. The call arrives at the request coordinator. This change will then place the request onto the request queue. BUG=610521 patch from issue 1956633002 at patchset 20001 (http://crrev.com/1956633002#ps20001) ========== to ========== Add a save page request to the request queue (and test it). We already have code to catch the press on a menu item for saving a page offline in the background. The call arrives at the request coordinator. This change will then place the request onto the request queue. BUG=610521 patch from issue 1956633002 at patchset 20001 (http://crrev.com/1956633002#ps20001) Committed: https://crrev.com/c8a55c9469c3642d822ef07b638bc6d1e2c95fd4 Cr-Commit-Position: refs/heads/master@{#393107} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c8a55c9469c3642d822ef07b638bc6d1e2c95fd4 Cr-Commit-Position: refs/heads/master@{#393107} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
