Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2978843002:
[Extensions][Task Scheduler] Migrate ExecuteCodeFunction (Closed)
|
Created:
3 years, 5 months ago by Devlin Modified:
3 years, 5 months ago Reviewers:
karandeepb CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Extensions][Task Scheduler] Migrate ExecuteCodeFunction
Migrate ExecuteCodeFunction away from BrowserThread::PostTask to post a
task to the FILE thread and instead use base::PostTask with MayBlock
traits. Also avoid a double post and instead just use PostTask*AndReply.
BUG=689520
Review-Url: https://codereview.chromium.org/2978843002
Cr-Commit-Position: refs/heads/master@{#486611}
Committed: https://chromium.googlesource.com/chromium/src/+/498f93b1609fe139bd5fa863006afe627583de57
Patch Set 1 #
Total comments: 10
Patch Set 2 : Karan's #Patch Set 3 : . #
Messages
Total messages: 21 (14 generated)
The CQ bit was checked by rdevlin.cronin@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: This issue passed the CQ dry run.
rdevlin.cronin@chromium.org changed reviewers: + karandeepb@chromium.org
karandeepb@, can you take a look?
Also, the CL description makes it seems as if we are migrating away from using BrowserThread::PostTask. Clarify that we are migrating away from using FILE thread https://codereview.chromium.org/2978843002/diff/1/extensions/browser/api/exec... File extensions/browser/api/execute_code_function.cc (right): https://codereview.chromium.org/2978843002/diff/1/extensions/browser/api/exec... extensions/browser/api/execute_code_function.cc:55: file_url_ = net::FilePathToFileURL(resource_.GetFilePath()); It seems to me that net::FilePathToFileURL does not need to be executed on a sequence with FILE IO. Also, it seems that currently file_url_ is modified on file thread and used on UI thread (Not sure if other constraints make this thread safe). Maybe it would be a good idea to initialize file_url_ on UI thread and skip the hop to file thread if might_require_localization is false. Though this should probably be done separately. (Also, didn't really dive into the code so may be wrong) https://codereview.chromium.org/2978843002/diff/1/extensions/browser/api/exec... extensions/browser/api/execute_code_function.cc:225: base::PostTaskWithTraitsAndReplyWithResult( Changing to a base::PostTask instead of a sequenced task runner, would cause us to lost the ordering between the different posted tasks. (which we have currently using the file thread). Is this still correct? https://codereview.chromium.org/2978843002/diff/1/extensions/browser/api/exec... extensions/browser/api/execute_code_function.cc:226: FROM_HERE, {base::MayBlock()}, It would be nice to be explicit about the shutdown behavior and task priority. https://codereview.chromium.org/2978843002/diff/1/extensions/browser/api/exec... File extensions/browser/api/execute_code_function.h (right): https://codereview.chromium.org/2978843002/diff/1/extensions/browser/api/exec... extensions/browser/api/execute_code_function.h:87: void GetFileURLAndMaybeLocalizeOnBackgroundThread( Why "Background"? Maybe GetFileURLAndMaybeLocalizeOnFileSequence?
https://codereview.chromium.org/2978843002/diff/1/extensions/browser/api/exec... File extensions/browser/api/execute_code_function.cc (right): https://codereview.chromium.org/2978843002/diff/1/extensions/browser/api/exec... extensions/browser/api/execute_code_function.cc:55: file_url_ = net::FilePathToFileURL(resource_.GetFilePath()); On 2017/07/13 01:47:48, karandeepb wrote: > It seems to me that net::FilePathToFileURL does not need to be executed on a > sequence with FILE IO. Also, it seems that currently file_url_ is modified on > file thread and used on UI thread (Not sure if other constraints make this > thread safe). > > Maybe it would be a good idea to initialize file_url_ on UI thread and skip the > hop to file thread if might_require_localization is false. Though this should > probably be done separately. (Also, didn't really dive into the code so may be > wrong) Good find! I agree that this would be better done separately (to keep this patch targeted and allow reverting of only one if something goes wrong), but I've added a TODO. https://codereview.chromium.org/2978843002/diff/1/extensions/browser/api/exec... extensions/browser/api/execute_code_function.cc:225: base::PostTaskWithTraitsAndReplyWithResult( On 2017/07/13 01:47:48, karandeepb wrote: > Changing to a base::PostTask instead of a sequenced task runner, would cause us > to lost the ordering between the different posted tasks. (which we have > currently using the file thread). Is this still correct? Yep, this shouldn't be a problem. This function only does one thing at a time, so even though there are multiple threads involved, it should only be active on a single one, which means we don't care about the sequence between posted tasks. https://codereview.chromium.org/2978843002/diff/1/extensions/browser/api/exec... extensions/browser/api/execute_code_function.cc:226: FROM_HERE, {base::MayBlock()}, On 2017/07/13 01:47:48, karandeepb wrote: > It would be nice to be explicit about the shutdown behavior and task priority. Done for shutdown behavior. For priority, it's difficult to say since we can't really judge the importance of this task. I'd rather leave it up to the task scheduler to find the inherited priority than set an explicit one here. https://codereview.chromium.org/2978843002/diff/1/extensions/browser/api/exec... File extensions/browser/api/execute_code_function.h (right): https://codereview.chromium.org/2978843002/diff/1/extensions/browser/api/exec... extensions/browser/api/execute_code_function.h:87: void GetFileURLAndMaybeLocalizeOnBackgroundThread( On 2017/07/13 01:47:48, karandeepb wrote: > Why "Background"? > > Maybe GetFileURLAndMaybeLocalizeOnFileSequence? "Background" is just the idea of a background thread (i.e., not UI). FileSequence isn't quite right, I think, because it implies a file thread (which is what we're moving away from), and these aren't run on a specific sequence. I've also seen "InBackground", or we can just remove the suffix entirely... wdyt?
LGTM but please also consider updating the CL description. My comment from previous review- "Also, the CL description makes it seems as if we are migrating away from using BrowserThread::PostTask. Clarify that we are migrating away from using FILE thread" https://codereview.chromium.org/2978843002/diff/1/extensions/browser/api/exec... File extensions/browser/api/execute_code_function.h (right): https://codereview.chromium.org/2978843002/diff/1/extensions/browser/api/exec... extensions/browser/api/execute_code_function.h:87: void GetFileURLAndMaybeLocalizeOnBackgroundThread( On 2017/07/13 18:35:09, Devlin wrote: > On 2017/07/13 01:47:48, karandeepb wrote: > > Why "Background"? > > > > Maybe GetFileURLAndMaybeLocalizeOnFileSequence? > > "Background" is just the idea of a background thread (i.e., not UI). > FileSequence isn't quite right, I think, because it implies a file thread (which > is what we're moving away from), and these aren't run on a specific sequence. > I've also seen "InBackground", or we can just remove the suffix entirely... > wdyt? Background seems to tell me that this is run on a sequence with background priority which may not be true. However if this is also used similarly at other places in the codebase, then this sg.
Description was changed from ========== [Extensions][Task Scheduler] Migrate ExecuteCodeFunction Migrate ExecuteCodeFunction away from BrowserThread::PostTask and instead use base::PostTask. Also avoid a double post and instead just use PostTask*AndReply. BUG=689520 ========== to ========== [Extensions][Task Scheduler] Migrate ExecuteCodeFunction Migrate ExecuteCodeFunction away from BrowserThread::PostTask to post a task to the FILE thread and instead use base::PostTask with MayBlock traits. Also avoid a double post and instead just use PostTask*AndReply. BUG=689520 ==========
The CQ bit was checked by rdevlin.cronin@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/07/13 20:18:48, karandeepb wrote: > please also consider updating the CL description. My comment from > previous review- > "Also, the CL description makes it seems as if we are migrating away from using > BrowserThread::PostTask. Clarify that we are migrating away from using FILE > thread" Whoops, thanks for the reminder. I've updated the CL description to make this more clear. Though, note, we *are* migrating away from BrowserThread::PostTask (it's now deprecated in favor of base::PostTask) - but I've added a bit more detail to include the fact that we're also migrating away from FILE thread usage. https://codereview.chromium.org/2978843002/diff/1/extensions/browser/api/exec... File extensions/browser/api/execute_code_function.h (right): https://codereview.chromium.org/2978843002/diff/1/extensions/browser/api/exec... extensions/browser/api/execute_code_function.h:87: void GetFileURLAndMaybeLocalizeOnBackgroundThread( On 2017/07/13 20:18:48, karandeepb wrote: > On 2017/07/13 18:35:09, Devlin wrote: > > On 2017/07/13 01:47:48, karandeepb wrote: > > > Why "Background"? > > > > > > Maybe GetFileURLAndMaybeLocalizeOnFileSequence? > > > > "Background" is just the idea of a background thread (i.e., not UI). > > FileSequence isn't quite right, I think, because it implies a file thread > (which > > is what we're moving away from), and these aren't run on a specific sequence. > > I've also seen "InBackground", or we can just remove the suffix entirely... > > wdyt? > > Background seems to tell me that this is run on a sequence with background > priority which may not be true. However if this is also used similarly at other > places in the codebase, then this sg. Done.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from karandeepb@chromium.org Link to the patchset: https://codereview.chromium.org/2978843002/#ps40001 (title: ".")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1499993604479080,
"parent_rev": "4796382af1831ac42afcae653206c8508040aa53", "commit_rev":
"498f93b1609fe139bd5fa863006afe627583de57"}
Message was sent while issue was closed.
Description was changed from ========== [Extensions][Task Scheduler] Migrate ExecuteCodeFunction Migrate ExecuteCodeFunction away from BrowserThread::PostTask to post a task to the FILE thread and instead use base::PostTask with MayBlock traits. Also avoid a double post and instead just use PostTask*AndReply. BUG=689520 ========== to ========== [Extensions][Task Scheduler] Migrate ExecuteCodeFunction Migrate ExecuteCodeFunction away from BrowserThread::PostTask to post a task to the FILE thread and instead use base::PostTask with MayBlock traits. Also avoid a double post and instead just use PostTask*AndReply. BUG=689520 Review-Url: https://codereview.chromium.org/2978843002 Cr-Commit-Position: refs/heads/master@{#486611} Committed: https://chromium.googlesource.com/chromium/src/+/498f93b1609fe139bd5fa863006a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/498f93b1609fe139bd5fa863006a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
