Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1914333002:
Adding a BUILD file for both gyp and gn for the new background_offlining (Closed)
|
Created:
4 years, 7 months ago by Pete Williamson Modified:
4 years, 7 months ago CC:
chromium-reviews, dewittj+watch_chromium.org, dimich+watch_chromium.org, fgorski+watch_chromium.org, petewil+watch_chromium.org, romax+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding a BUILD file for both gyp and gn for the new background_offlining
feature
BUG=607628
Committed: https://crrev.com/0b4d366ada5f05de6c9fce8ab505dab89e7813ab
Cr-Commit-Position: refs/heads/master@{#391009}
Patch Set 1 #Patch Set 2 : fix gypi to remove files we don't have yet. #Patch Set 3 : Remove commented out lines in the build file. #
Total comments: 6
Patch Set 4 : CR changes per DougArnett #
Total comments: 11
Patch Set 5 : CR feedback per jochen and fgorski. #Patch Set 6 : CR fixes per fgorski #Patch Set 7 : Add .h files to BUILD.gn. #
Total comments: 12
Patch Set 8 : More CR feedback per fgorski. #
Dependent Patchsets: Messages
Total messages: 43 (17 generated)
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/1914333002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1914333002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 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/1914333002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1914333002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
petewil@chromium.org changed reviewers: + dougarnett@chromium.org
DougArnett: Please review everything, when you are satisfied, I will add reviewers for the higher level build files.
Just few minor comments. Looks good to add more reviewers. https://codereview.chromium.org/1914333002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1914333002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:6: nit - drop this empty line https://codereview.chromium.org/1914333002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:27: // Starts processing of one or more queued save page later requests. might be good to include rest of .h comment wrt contract in calling callback if returns true https://codereview.chromium.org/1914333002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:30: return true; false until/unless this will call callback
petewil@chromium.org changed reviewers: + jochen@chromium.org
jochen: Please review changes to c/b/BUILD.gn and offline_pages.gypi. Thanks! https://codereview.chromium.org/1914333002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1914333002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:6: On 2016/04/27 19:15:35, dougarnett wrote: > nit - drop this empty line There is supposed to be a blank line here. Normal practice is to include the associated .h file first, and other headers after a blank line. The practice is meant to assure that each header can be standalone, and there are no include order dependencies. https://codereview.chromium.org/1914333002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:27: // Starts processing of one or more queued save page later requests. On 2016/04/27 19:15:35, dougarnett wrote: > might be good to include rest of .h comment wrt contract in calling callback if > returns true Done. https://codereview.chromium.org/1914333002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:30: return true; On 2016/04/27 19:15:35, dougarnett wrote: > false until/unless this will call callback Done.
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... File components/offline_pages.gypi (right): https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... components/offline_pages.gypi:60: # GN: //components/offline_pages/background:background_offliner is this a duplicate? https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... File components/offline_pages/background/BUILD.gn (right): https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... components/offline_pages/background/BUILD.gn:9: # GYP: //components/offline_pages/offline_pages.gypi:background_offlining a) gyp says background_offliner b) I see value in having the default simply say background https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... components/offline_pages/background/BUILD.gn:17: "//components/bookmarks/browser", not needed I think https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.cc:7: #include "components/offline_pages/background/scheduler.h" needed?
can you please file a tracking bug? also, what's the testing story here? https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... File components/offline_pages/background/BUILD.gn (right): https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... components/offline_pages/background/BUILD.gn:12: "request_coordinator.cc", why are the sources of the gyp and gn target different?
On 2016/04/28 15:39:06, jochen wrote: > can you please file a tracking bug? > > also, what's the testing story here? > > https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... > File components/offline_pages/background/BUILD.gn (right): > > https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... > components/offline_pages/background/BUILD.gn:12: "request_coordinator.cc", > why are the sources of the gyp and gn target different? More details on the feature behind this change (sorry, should have included that before) go/chrome-background-offlining Tracking bug: crbug.com/607628 Tests: Plan is to add more .cc stub files next, then to add some testing stub files in the following changelist. crbug.com/607629 is tracking the tests.
https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... File components/offline_pages.gypi (right): https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... components/offline_pages.gypi:60: # GN: //components/offline_pages/background:background_offliner On 2016/04/28 11:54:58, fgorski wrote: > is this a duplicate? Yes. Duplicate code removed. https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... File components/offline_pages/background/BUILD.gn (right): https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... components/offline_pages/background/BUILD.gn:9: # GYP: //components/offline_pages/offline_pages.gypi:background_offlining On 2016/04/28 11:54:59, fgorski wrote: > a) gyp says background_offliner > b) I see value in having the default simply say background Changed to background_offliner to be consistent. While our directory is just "background", it seems insufficiently specific for a target name. https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... components/offline_pages/background/BUILD.gn:12: "request_coordinator.cc", On 2016/04/28 15:39:06, jochen wrote: > why are the sources of the gyp and gn target different? Looking elsewhere, it seems that the .gypi files mention both .h files and .cc files, but BUILD.gn files mention only .cc files. https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... components/offline_pages/background/BUILD.gn:17: "//components/bookmarks/browser", On 2016/04/28 11:54:58, fgorski wrote: > not needed I think Done. https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.cc:7: #include "components/offline_pages/background/scheduler.h" On 2016/04/28 11:54:59, fgorski wrote: > needed? Done.
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/1914333002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1914333002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... File components/offline_pages/background/BUILD.gn (right): https://codereview.chromium.org/1914333002/diff/60001/components/offline_page... components/offline_pages/background/BUILD.gn:12: "request_coordinator.cc", On 2016/04/28 20:22:03, Pete Williamson wrote: > On 2016/04/28 15:39:06, jochen wrote: > > why are the sources of the gyp and gn target different? > > Looking elsewhere, it seems that the .gypi files mention both .h files and .cc > files, but BUILD.gn files mention only .cc files. After asking around, it looks like I picked a bad example build.gn file to look at. Added the .h files.
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/1914333002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1914333002/120001
lgtm with comments. https://codereview.chromium.org/1914333002/diff/120001/components/offline_pag... File components/offline_pages.gypi (right): https://codereview.chromium.org/1914333002/diff/120001/components/offline_pag... components/offline_pages.gypi:54: 'offline_pages/background/request_coordinator.h', nit: cc goes first this is meant to be sorted. https://codereview.chromium.org/1914333002/diff/120001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1914333002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:7: // TODO: include file with SavePageRequest definition. I have that in my patch. https://codereview.chromium.org/1914333002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:20: // Queues |request| to later load and save when system conditions allow. don't have to repeat the documentation. https://codereview.chromium.org/1914333002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:27: // a callback. If processing was already active, returns false. ditto https://codereview.chromium.org/1914333002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:33: // Stops the current request processing if active. ditto https://codereview.chromium.org/1914333002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:35: nit: remove empty line
Addressed FGorski's issues. https://codereview.chromium.org/1914333002/diff/120001/components/offline_pag... File components/offline_pages.gypi (right): https://codereview.chromium.org/1914333002/diff/120001/components/offline_pag... components/offline_pages.gypi:54: 'offline_pages/background/request_coordinator.h', On 2016/04/28 22:18:19, fgorski wrote: > nit: cc goes first > this is meant to be sorted. Done. https://codereview.chromium.org/1914333002/diff/120001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1914333002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:7: // TODO: include file with SavePageRequest definition. On 2016/04/28 22:18:19, fgorski wrote: > I have that in my patch. OK, I added your name to the TODO to clear it when you land your patch. https://codereview.chromium.org/1914333002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:20: // Queues |request| to later load and save when system conditions allow. On 2016/04/28 22:18:19, fgorski wrote: > don't have to repeat the documentation. Done. https://codereview.chromium.org/1914333002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:27: // a callback. If processing was already active, returns false. On 2016/04/28 22:18:19, fgorski wrote: > ditto Done. https://codereview.chromium.org/1914333002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:33: // Stops the current request processing if active. On 2016/04/28 22:18:19, fgorski wrote: > ditto Done. https://codereview.chromium.org/1914333002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:35: On 2016/04/28 22:18:19, fgorski wrote: > nit: remove empty line Done.
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/1914333002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1914333002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/04/28 23:52:26, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. Pete, please link it to the bug you opened before you check this in. Jochen, as for tests, we haven't built the functionality to test yet, we just want to put the files in place now.
Description was changed from ========== Adding a BUILD file for both gyp and gn for the new background_offlining feature BUG= ========== to ========== Adding a BUILD file for both gyp and gn for the new background_offlining feature BUG=607628 ==========
On 2016/04/29 04:59:35, fgorski wrote: > On 2016/04/28 23:52:26, commit-bot: I haz the power wrote: > > Dry run: This issue passed the CQ dry run. > > Pete, please link it to the bug you opened before you check this in. > > Jochen, as for tests, we haven't built the functionality to test yet, we just > want to put the files in place now. done, bug number added.
lgtm
lgtm
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/1914333002/#ps140001 (title: "More CR feedback per fgorski.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1914333002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1914333002/140001
Message was sent while issue was closed.
Description was changed from ========== Adding a BUILD file for both gyp and gn for the new background_offlining feature BUG=607628 ========== to ========== Adding a BUILD file for both gyp and gn for the new background_offlining feature BUG=607628 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Adding a BUILD file for both gyp and gn for the new background_offlining feature BUG=607628 ========== to ========== Adding a BUILD file for both gyp and gn for the new background_offlining feature BUG=607628 Committed: https://crrev.com/0b4d366ada5f05de6c9fce8ab505dab89e7813ab Cr-Commit-Position: refs/heads/master@{#391009} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/0b4d366ada5f05de6c9fce8ab505dab89e7813ab Cr-Commit-Position: refs/heads/master@{#391009} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
