Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2217083002:
Add sequential dispatching for InitializationRunner (Closed)
DescriptionAdd sequential dispatching for InitializationRunner
This CL adds a new way do dispatch tasks at the beginning of the application.
Before the tasks were run after an arbitrary delay. Now the tasks can be run
sequentially with a small delay between each.
It also deprecate the previous method.
BUG=227553
Committed: https://crrev.com/61f96f5bf492c26145a1682ac42420c261eb1f23
Cr-Commit-Position: refs/heads/master@{#421171}
Patch Set 1 #Patch Set 2 : Add test #
Total comments: 1
Patch Set 3 : Fix tests #
Total comments: 4
Patch Set 4 : Reorder header #Patch Set 5 : Revision #Patch Set 6 : gyp #
Total comments: 18
Patch Set 7 : Add comments #
Total comments: 8
Patch Set 8 : Add first block delay #Patch Set 9 : comment #
Total comments: 2
Patch Set 10 : Delays exposed for testing only #
Total comments: 5
Patch Set 11 : Address comment #Patch Set 12 : Fix tests #
Total comments: 6
Patch Set 13 : Addressing comments #
Total comments: 19
Patch Set 14 : Comments #
Messages
Total messages: 54 (26 generated)
gambard@chromium.org changed reviewers: + rohitrao@chromium.org
PTAL. https://codereview.chromium.org/2217083002/diff/20001/ios/chrome/app/deferred... File ios/chrome/app/deferred_initialization_runner.mm (right): https://codereview.chromium.org/2217083002/diff/20001/ios/chrome/app/deferred... ios/chrome/app/deferred_initialization_runner.mm:14: const NSTimeInterval kDelayBetweenBlock = 0.2; There are 18 current calls to this class ranging from 0.1 to 6 seconds. This will execute the calls from 0.2 to 3.6 seconds.
https://codereview.chromium.org/2217083002/diff/40001/ios/chrome/app/deferred... File ios/chrome/app/deferred_initialization_runner.mm (right): https://codereview.chromium.org/2217083002/diff/40001/ios/chrome/app/deferred... ios/chrome/app/deferred_initialization_runner.mm:83: BOOL isBlockScheduled; Should this be _isBlockScheduled? https://codereview.chromium.org/2217083002/diff/40001/ios/chrome/app/deferred... ios/chrome/app/deferred_initialization_runner.mm:143: [self scheduleNextBlock]; Is there any chance that retaining |self| could cause issues? For example, if we somehow managed to trigger shutdown before all of the blocks had finished executing? (I'm guessing not, because DeferredInitializationRunning is already a singleton, so retaining self can't make it live any longer than it already does.)
Thanks, PTAL. https://codereview.chromium.org/2217083002/diff/40001/ios/chrome/app/deferred... File ios/chrome/app/deferred_initialization_runner.mm (right): https://codereview.chromium.org/2217083002/diff/40001/ios/chrome/app/deferred... ios/chrome/app/deferred_initialization_runner.mm:83: BOOL isBlockScheduled; On 2016/08/08 14:07:23, rohitrao wrote: > Should this be _isBlockScheduled? Done. https://codereview.chromium.org/2217083002/diff/40001/ios/chrome/app/deferred... ios/chrome/app/deferred_initialization_runner.mm:143: [self scheduleNextBlock]; On 2016/08/08 14:07:23, rohitrao wrote: > Is there any chance that retaining |self| could cause issues? For example, if > we somehow managed to trigger shutdown before all of the blocks had finished > executing? > > (I'm guessing not, because DeferredInitializationRunning is already a singleton, > so retaining self can't make it live any longer than it already does.) As it was a singleton I thought it would be ok to use a strong reference. I have changed it to a weak self.
pkl@chromium.org changed reviewers: + pkl@chromium.org
https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.h (right): https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:32: // Adds |block| to a block queue containing all block not run. The blocks in the s/all block/all blocks/ https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:33: // queue are runned sequentially with a small delay between each block. s/runned/ran/ https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:54: // Time interval between two blocks. Default value is 200ms. Is this @property really part of the API or exists just for testing? If it is just for facilitating testing, I would rather make this private and unit tests can access it via categories. https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.mm (right): https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:133: NSString* nextBlockName = [_blocksNameQueue firstObject]; suggestion: Since -firstObject returns nil if NSArray is empty, don't need to test for [_blocksNameQueue count]. Just call -firstObject and early return value is nil. https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner_unittest.mm (right): https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:47: // Test. It would be helpful to explain the expected result, i.e. "both blocks should be executed and no additional blocks are queued on the runner." https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:64: runner.delayBetweenBlocks = 1; This will cause this unit test to take at least 1 second to run. Are there any ways to avoid this? As it stands now, this leaves the sharedInstance of DeferredInitializationRunner in a state of 1 second delay for the rest of this unit test run. What is the intent of this test that is different from TestRunBlockSequentially? https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:93: __block BOOL blockFinished = NO; You should set runner.delayBetweenBlocks = 0.01 here to control the test environment. https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:124: // Test. Explain expected results, e.g. "Block should never be executed because it was cancelled before it started running." https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:141: // Test. Explain expected results, e.g. "runBlock was executed only once, thus blockRunCount has a value of 1."
Thanks, PTAL. https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.h (right): https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:32: // Adds |block| to a block queue containing all block not run. The blocks in the On 2016/08/08 23:41:53, pklpkl wrote: > s/all block/all blocks/ Done. https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:33: // queue are runned sequentially with a small delay between each block. On 2016/08/08 23:41:53, pklpkl wrote: > s/runned/ran/ Done. https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:54: // Time interval between two blocks. Default value is 200ms. On 2016/08/08 23:41:53, pklpkl wrote: > Is this @property really part of the API or exists just for testing? If it is > just for facilitating testing, I would rather make this private and unit tests > can access it via categories. For now it is used for testing but I think it is relevant as part of the public API. https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.mm (right): https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:133: NSString* nextBlockName = [_blocksNameQueue firstObject]; On 2016/08/08 23:41:53, pklpkl wrote: > suggestion: Since -firstObject returns nil if NSArray is empty, don't need to > test for [_blocksNameQueue count]. Just call -firstObject and early return value > is nil. Done. https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner_unittest.mm (right): https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:47: // Test. On 2016/08/08 23:41:54, pklpkl wrote: > It would be helpful to explain the expected result, i.e. "both blocks should be > executed and no additional blocks are queued on the runner." Yes, I have adapted the old tests but I forget to add comments. Done. https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:64: runner.delayBetweenBlocks = 1; On 2016/08/08 23:41:54, pklpkl wrote: > This will cause this unit test to take at least 1 second to run. Are there any > ways to avoid this? > As it stands now, this leaves the sharedInstance of DeferredInitializationRunner > in a state of 1 second delay for the rest of this unit test run. > > What is the intent of this test that is different from TestRunBlockSequentially? No. The 1s delay is to ensure the next block won't be run immediately. It will be run inside the test function with |runBlockIfNecessary|. https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:93: __block BOOL blockFinished = NO; On 2016/08/08 23:41:53, pklpkl wrote: > You should set runner.delayBetweenBlocks = 0.01 here to control the test > environment. Done. https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:124: // Test. On 2016/08/08 23:41:53, pklpkl wrote: > Explain expected results, e.g. "Block should never be executed because it was > cancelled before it started running." Done. https://codereview.chromium.org/2217083002/diff/100001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:141: // Test. On 2016/08/08 23:41:54, pklpkl wrote: > Explain expected results, e.g. "runBlock was executed only once, thus > blockRunCount has a value of 1." Done.
lgtm https://codereview.chromium.org/2217083002/diff/120001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner_unittest.mm (right): https://codereview.chromium.org/2217083002/diff/120001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:94: // Tests that a block is not executed when canceld and it is removed from the s/canceld/cancelled/ https://codereview.chromium.org/2217083002/diff/120001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:95: // remaining block list. s/block/blocks/ https://codereview.chromium.org/2217083002/diff/120001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:117: // Tests that a canceled block will do nothing when run by name. s/canceled/cancelled/ https://codereview.chromium.org/2217083002/diff/120001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:134: // Test: expect false, the block should never be executed because it was The "expect false, " part seems unnecessary.
Thanks. Following our discussion, PTAL. https://codereview.chromium.org/2217083002/diff/120001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner_unittest.mm (right): https://codereview.chromium.org/2217083002/diff/120001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:94: // Tests that a block is not executed when canceld and it is removed from the On 2016/08/09 13:22:57, pklpkl wrote: > s/canceld/cancelled/ Done. https://codereview.chromium.org/2217083002/diff/120001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:95: // remaining block list. On 2016/08/09 13:22:56, pklpkl wrote: > s/block/blocks/ Done. https://codereview.chromium.org/2217083002/diff/120001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:117: // Tests that a canceled block will do nothing when run by name. On 2016/08/09 13:22:56, pklpkl wrote: > s/canceled/cancelled/ Done. https://codereview.chromium.org/2217083002/diff/120001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:134: // Test: expect false, the block should never be executed because it was On 2016/08/09 13:22:57, pklpkl wrote: > The "expect false, " part seems unnecessary. It tests that the block has not been run after being cancelled, even if it is called by name.
https://codereview.chromium.org/2217083002/diff/160001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.h (right): https://codereview.chromium.org/2217083002/diff/160001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:58: @property(nonatomic) NSTimeInterval delayBeforeFirstBlocks; Shouldn't this be singular, i.e. delayBeforeFirstBlock? I think you should mention here that the value for this must be set *before* the first call to -enqueueBlockNamed:. Once enqueue is called, setting this value has no effect. Since DeferredInitializationRunner is a singleton class, this makes the first call to +sharedInstance the one that gets the "first block" delay. This API feels a bit "strange". Is it really necessary to expose -delayBetweenBlocks and -delayBeforeFirstBlock as part of this API? Is it really being used (other than for unit tests)?
Patchset #10 (id:180001) has been deleted
PTAL. https://codereview.chromium.org/2217083002/diff/160001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.h (right): https://codereview.chromium.org/2217083002/diff/160001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:58: @property(nonatomic) NSTimeInterval delayBeforeFirstBlocks; On 2016/08/19 23:48:20, pklpkl wrote: > Shouldn't this be singular, i.e. delayBeforeFirstBlock? > > I think you should mention here that the value for this must be set *before* the > first call to -enqueueBlockNamed:. Once enqueue is called, setting this value > has no effect. Since DeferredInitializationRunner is a singleton class, this > makes the first call to +sharedInstance the one that gets the "first block" > delay. This API feels a bit "strange". > > Is it really necessary to expose -delayBetweenBlocks and -delayBeforeFirstBlock > as part of this API? Is it really being used (other than for unit tests)? You are right. This makes little sense as public API. I have put it into category for testing only.
ping. I have modified it relatively heavily since last lgtm.
LGTM, with comments and a question about possible dup declarations. https://codereview.chromium.org/2217083002/diff/200001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.h (right): https://codereview.chromium.org/2217083002/diff/200001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:62: // called before the first call to |enqueueBlockNamed|. I would change the start starting with "Default value is..." to the following: To override default value of 3s, set this property before the first call to |-enqueueBlockNamed:block:|. https://codereview.chromium.org/2217083002/diff/200001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.mm (right): https://codereview.chromium.org/2217083002/diff/200001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:91: // Time interval between two blocks. Default value is 200ms. Isn't this and the next property already declared in the .h file?
Thanks. https://codereview.chromium.org/2217083002/diff/200001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.h (right): https://codereview.chromium.org/2217083002/diff/200001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:62: // called before the first call to |enqueueBlockNamed|. On 2016/09/20 04:42:56, pklpkl wrote: > I would change the start starting with "Default value is..." to the following: > > To override default value of 3s, set this property before the first call to > |-enqueueBlockNamed:block:|. Done. https://codereview.chromium.org/2217083002/diff/200001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.mm (right): https://codereview.chromium.org/2217083002/diff/200001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:91: // Time interval between two blocks. Default value is 200ms. On 2016/09/20 04:42:56, pklpkl wrote: > Isn't this and the next property already declared in the .h file? Yes but I consider that all that is exposed for testing should be duplicated because if the tests don't use it, it can be removed. Are we doing differently?
The CQ bit was checked by gambard@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
https://codereview.chromium.org/2217083002/diff/200001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.mm (right): https://codereview.chromium.org/2217083002/diff/200001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:91: // Time interval between two blocks. Default value is 200ms. On 2016/09/22 14:00:25, gambard wrote: > On 2016/09/20 04:42:56, pklpkl wrote: > > Isn't this and the next property already declared in the .h file? > > Yes but I consider that all that is exposed for testing should be duplicated > because if the tests don't use it, it can be removed. > Are we doing differently? I think in other places we locally declare them in the test .mm files.
The CQ bit was checked by gambard@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/22 14:17:09, pklpkl wrote: > https://codereview.chromium.org/2217083002/diff/200001/ios/chrome/app/deferre... > File ios/chrome/app/deferred_initialization_runner.mm (right): > > https://codereview.chromium.org/2217083002/diff/200001/ios/chrome/app/deferre... > ios/chrome/app/deferred_initialization_runner.mm:91: // Time interval between > two blocks. Default value is 200ms. > On 2016/09/22 14:00:25, gambard wrote: > > On 2016/09/20 04:42:56, pklpkl wrote: > > > Isn't this and the next property already declared in the .h file? > > > > Yes but I consider that all that is exposed for testing should be duplicated > > because if the tests don't use it, it can be removed. > > Are we doing differently? > > I think in other places we locally declare them in the test .mm files. We do both and I think it is clearer this way.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by gambard@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkl@chromium.org Link to the patchset: https://codereview.chromium.org/2217083002/#ps240001 (title: "Fix tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
gambard@chromium.org changed reviewers: + jif@chromium.org
+jif for committer lgtm
lgtm with nits https://codereview.chromium.org/2217083002/diff/240001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.h (right): https://codereview.chromium.org/2217083002/diff/240001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:21: // Deprecated. // Deprecated, use |enqueueBlockNamed:block:| instead. https://codereview.chromium.org/2217083002/diff/240001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.mm (right): https://codereview.chromium.org/2217083002/diff/240001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:126: DCHECK(name); I'm afraid of |enqueueBlockNamed:| being called while |scheduleNextBlockWithDelay:| is running on an other thread: DCHECK([NSThread isMainThread]); https://codereview.chromium.org/2217083002/diff/240001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner_unittest.mm (right): https://codereview.chromium.org/2217083002/diff/240001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:55: // Tests that runBlockIfNecessary do not execute the block if it has already s/do/does/ ?
Thanks. https://codereview.chromium.org/2217083002/diff/240001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.h (right): https://codereview.chromium.org/2217083002/diff/240001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:21: // Deprecated. On 2016/09/26 09:12:47, jif wrote: > // Deprecated, use |enqueueBlockNamed:block:| instead. Done. https://codereview.chromium.org/2217083002/diff/240001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.mm (right): https://codereview.chromium.org/2217083002/diff/240001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:126: DCHECK(name); On 2016/09/26 09:12:47, jif wrote: > I'm afraid of |enqueueBlockNamed:| being called while > |scheduleNextBlockWithDelay:| is running on an other thread: > > DCHECK([NSThread isMainThread]); Done. https://codereview.chromium.org/2217083002/diff/240001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner_unittest.mm (right): https://codereview.chromium.org/2217083002/diff/240001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner_unittest.mm:55: // Tests that runBlockIfNecessary do not execute the block if it has already On 2016/09/26 09:12:47, jif wrote: > s/do/does/ ? Done.
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org, pkl@chromium.org Link to the patchset: https://codereview.chromium.org/2217083002/#ps260001 (title: "Addressing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
gambard@chromium.org changed reviewers: + sdefresne@chromium.org
+sdefresne for owner lgtm
https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.h (right): https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:34: // If the queue is empty |block| is run after a small delay. nit: I would remove this sentence as I found it confusing (and it is obvious from the previous one, if all blocks are run sequentially after a small delay). Or maybe rephrase as: // Stores |block| under |name| to a queue of blocks to run. All blocks are run // sequentially with a small delay before the first block and between each // successive block. If a block is already registered under |name|, it is // replaced with |block| unless it has already been run. https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:59: @property(nonatomic) NSTimeInterval delayBetweenBlocks; @property(nonatomic, assign) ... https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:63: @property(nonatomic) NSTimeInterval delayBeforeFirstBlock; @property(nonatomic, assign) ... https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.mm (right): https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:19: base::scoped_nsprotocol<ProceduralBlock> _runBlock; Can you change this to base::mac::ScopedBlock<ProceduralBlock>? https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:72: [[DeferredInitializationRunner sharedInstance] cancelBlockNamed:_name]; I think you should remove this line as the object is already removed from the _blocksNameQueue at line 160. Then, it means that the _name ivar of DeferredInitializationBlock can be removed (as it is no longer used). You'll need to add the following at line 180: [self cancelBlockNamed:name]; https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:126: DCHECK(name); DCHECK(!name.isEmpty); https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:141: _isBlockScheduled = NO; DCHECK([NSThread isMainThread]); https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:167: // Safety check in case this function is called with a nanosecond or DCHECK(!name.isEmpty); https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:180: [[_runBlocks objectForKey:name] run]; DCHECK(name); DCHECK(!name.isEmpty); https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:185: [_blocksNameQueue removeObject:name]; DCHECK(!name.isEmpty);
I mis-read the code and though it was not inline with the documentation (though code/documentation is a bit complex). lgtm after offline discussion, but can you add DCHECK([NSThread isMainThread]) to ensure that there are no access from multiple threads?
Thanks. https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.h (right): https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:34: // If the queue is empty |block| is run after a small delay. On 2016/09/26 15:12:29, sdefresne wrote: > nit: I would remove this sentence as I found it confusing (and it is obvious > from the previous one, if all blocks are run sequentially after a small delay). > > Or maybe rephrase as: > > // Stores |block| under |name| to a queue of blocks to run. All blocks are run > // sequentially with a small delay before the first block and between each > // successive block. If a block is already registered under |name|, it is > // replaced with |block| unless it has already been run. Nice rephrasing, thanks! https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:59: @property(nonatomic) NSTimeInterval delayBetweenBlocks; On 2016/09/26 15:12:29, sdefresne wrote: > @property(nonatomic, assign) ... Done. https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.h:63: @property(nonatomic) NSTimeInterval delayBeforeFirstBlock; On 2016/09/26 15:12:29, sdefresne wrote: > @property(nonatomic, assign) ... Done. https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... File ios/chrome/app/deferred_initialization_runner.mm (right): https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:19: base::scoped_nsprotocol<ProceduralBlock> _runBlock; On 2016/09/26 15:12:29, sdefresne wrote: > Can you change this to base::mac::ScopedBlock<ProceduralBlock>? Done. https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:126: DCHECK(name); On 2016/09/26 15:12:29, sdefresne wrote: > DCHECK(!name.isEmpty); Empty name is OK. https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:141: _isBlockScheduled = NO; On 2016/09/26 15:12:29, sdefresne wrote: > DCHECK([NSThread isMainThread]); Done. https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:167: // Safety check in case this function is called with a nanosecond or On 2016/09/26 15:12:29, sdefresne wrote: > DCHECK(!name.isEmpty); Empty name is OK. https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:180: [[_runBlocks objectForKey:name] run]; On 2016/09/26 15:12:29, sdefresne wrote: > DCHECK(name); > DCHECK(!name.isEmpty); Done. https://codereview.chromium.org/2217083002/diff/260001/ios/chrome/app/deferre... ios/chrome/app/deferred_initialization_runner.mm:185: [_blocksNameQueue removeObject:name]; On 2016/09/26 15:12:29, sdefresne wrote: > DCHECK(!name.isEmpty); Empty name is OK.
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org, pkl@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2217083002/#ps280001 (title: "Comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #14 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Add sequential dispatching for InitializationRunner This CL adds a new way do dispatch tasks at the beginning of the application. Before the tasks were run after an arbitrary delay. Now the tasks can be run sequentially with a small delay between each. It also deprecate the previous method. BUG=227553 ========== to ========== Add sequential dispatching for InitializationRunner This CL adds a new way do dispatch tasks at the beginning of the application. Before the tasks were run after an arbitrary delay. Now the tasks can be run sequentially with a small delay between each. It also deprecate the previous method. BUG=227553 Committed: https://crrev.com/61f96f5bf492c26145a1682ac42420c261eb1f23 Cr-Commit-Position: refs/heads/master@{#421171} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/61f96f5bf492c26145a1682ac42420c261eb1f23 Cr-Commit-Position: refs/heads/master@{#421171} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
