Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2888693003:
Remove the usage of BrowserThread::FILE in the shell_integration* files (Closed)
|
Created:
3 years, 7 months ago by Patrick Monette Modified:
3 years, 5 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove the usage of BrowserThread::FILE in the shell_integration* files
The usage of BrowserThread::FILE is deprecated in favor of the
post_task.h API.
BUG=689520
Review-Url: https://codereview.chromium.org/2888693003
Cr-Commit-Position: refs/heads/master@{#476129}
Committed: https://chromium.googlesource.com/chromium/src/+/b18379f369eec8d994cca85d1133f3fac5818a4a
Patch Set 1 #Patch Set 2 : stuff #Patch Set 3 : Rebase on ProtocolHandlerRegistry change #Patch Set 4 : Again #
Total comments: 4
Patch Set 5 : Add SequenceChecker and make all calls to SetAsDefault share a sequence #
Total comments: 16
Patch Set 6 : Stuff #
Total comments: 11
Patch Set 7 : Gab comments #Patch Set 8 : Added a comment and fix typo #
Messages
Total messages: 52 (33 generated)
The CQ bit was checked by pmonette@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_...)
The CQ bit was checked by pmonette@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_...)
The CQ bit was checked by pmonette@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pmonette@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_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by pmonette@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.
pmonette@chromium.org changed reviewers: + thestig@chromium.org
PTAL
lgtm https://codereview.chromium.org/2888693003/diff/60001/chrome/browser/shell_in... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/2888693003/diff/60001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:7: #include "base//task_scheduler/post_task.h" Extra / https://codereview.chromium.org/2888693003/diff/60001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (left): https://codereview.chromium.org/2888693003/diff/60001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:330: DCHECK_CURRENTLY_ON(BrowserThread::FILE); Looks like you wrote this code. If you are ok without any DCHECKs to help enforce threading requirements, ok.
pmonette@chromium.org changed reviewers: + gab@chromium.org
gab@ PTAL at shell_integration.cc https://codereview.chromium.org/2888693003/diff/60001/chrome/browser/shell_in... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/2888693003/diff/60001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:7: #include "base//task_scheduler/post_task.h" On 2017/05/19 23:23:13, Lei Zhang wrote: > Extra / Done. https://codereview.chromium.org/2888693003/diff/60001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (left): https://codereview.chromium.org/2888693003/diff/60001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:330: DCHECK_CURRENTLY_ON(BrowserThread::FILE); On 2017/05/19 23:23:13, Lei Zhang wrote: > Looks like you wrote this code. If you are ok without any DCHECKs to help > enforce threading requirements, ok. Your comment made me realize that there is actually a threading requirement for the deletion of the OpenSystemSettingsHelper. I added a SequenceChecker.
https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:140: base::CreateSequencedTaskRunnerWithTraits({base::MayBlock()}) Why does this need to be called on a sequence which isn't re-used? Does it use SequencedTaskRunnerHandle or something? If there are multiple check default calls, shouldn't they be sequenced between themselves? And with SetAsDefault calls as well? https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:184: // SequencedTaskRunner when crbug.com/552633 is fixed. Feels like we might as well just sequence all attempts to SetAsDefault() on all platforms. Feels wrong to SetAsDefault() in parallel on any platform. https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:185: static base::SingleThreadTaskRunner* task_runner = nullptr; Make this a scoped_refptr<> assign the result of base::Create...() directly into it. Will avoid the whole AddRef() dance. https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:197: #endif // defined(OS_WIN) #else? https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:199: #endif // !defined(OS_WIN) #endif // defined(OS_WIN) ^ don't add '!', keep it the same as initial statement even in the "else" step https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.h:178: scoped_refptr<base::SequencedTaskRunner> GetSetDefaultTaskRunner(); "GetSet" sounds weird (took me a while to see this was "Get" "SetDefault") maybe GetTaskRunnerForSetAsDefault()? https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.h:180: // Checks whether Chrome is the default web client. Always called on the a "the a" https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:750: return; Can we remove this check while you're here, we only support Win7+ now
gab@ Another look? https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:140: base::CreateSequencedTaskRunnerWithTraits({base::MayBlock()}) On 2017/05/26 18:44:05, gab wrote: > Why does this need to be called on a sequence which isn't re-used? Does it use > SequencedTaskRunnerHandle or something? > > If there are multiple check default calls, shouldn't they be sequenced between > themselves? And with SetAsDefault calls as well? I decided to go with sequencing everything https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:184: // SequencedTaskRunner when crbug.com/552633 is fixed. On 2017/05/26 18:44:05, gab wrote: > Feels like we might as well just sequence all attempts to SetAsDefault() on all > platforms. Feels wrong to SetAsDefault() in parallel on any platform. Sounds good. https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:185: static base::SingleThreadTaskRunner* task_runner = nullptr; On 2017/05/26 18:44:05, gab wrote: > Make this a scoped_refptr<> assign the result of base::Create...() directly into > it. Will avoid the whole AddRef() dance. Done. https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:197: #endif // defined(OS_WIN) On 2017/05/26 18:44:05, gab wrote: > #else? Done. https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:199: #endif // !defined(OS_WIN) On 2017/05/26 18:44:05, gab wrote: > #endif // defined(OS_WIN) > > ^ don't add '!', keep it the same as initial statement even in the > "else" step Done. https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.h:178: scoped_refptr<base::SequencedTaskRunner> GetSetDefaultTaskRunner(); On 2017/05/26 18:44:06, gab wrote: > "GetSet" sounds weird (took me a while to see this was "Get" "SetDefault") > > maybe GetTaskRunnerForSetAsDefault()? Good suggestion, but I used GetTaskRunner() since it is used for the CheckIsDefault() operationt too. https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.h:180: // Checks whether Chrome is the default web client. Always called on the a On 2017/05/26 18:44:06, gab wrote: > "the a" Done. https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/2888693003/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration_win.cc:750: return; On 2017/05/26 18:44:06, gab wrote: > Can we remove this check while you're here, we only support Win7+ now Will do in another CL
gab@chromium.org changed reviewers: + benwells@chromium.org
Should also have benwells@ look at chrome/browser/custom_handlers/* I think https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:177: static scoped_refptr<base::SequencedTaskRunner>* task_runner = nullptr; I think this can just be a static scoped_refptr<base::SequencedTaskRunner> task_runner; without being a scoped_refptr* https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:185: task_runner = new scoped_refptr<base::SequencedTaskRunner>( and then here you can forgo this new https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... chrome/browser/shell_integration.h:179: // the worker. It's not just operations of this DefaultProtocolClientWorker, it's for all DefaultProtocolClientWorker (and if that's not required then don't make it static) https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... chrome/browser/shell_integration.h:180: scoped_refptr<base::SequencedTaskRunner> GetTaskRunner(); static
lgtm
PTAL I moved some code to a utility function https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:177: static scoped_refptr<base::SequencedTaskRunner>* task_runner = nullptr; On 2017/05/30 19:22:21, gab wrote: > I think this can just be a > > static scoped_refptr<base::SequencedTaskRunner> task_runner; > > without being a scoped_refptr* As per offline discussion, keeping this as a pointer since static variables must be POD. https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:185: task_runner = new scoped_refptr<base::SequencedTaskRunner>( On 2017/05/30 19:22:21, gab wrote: > and then here you can forgo this new Acknowledged. https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... chrome/browser/shell_integration.h:179: // the worker. On 2017/05/30 19:22:21, gab wrote: > It's not just operations of this DefaultProtocolClientWorker, it's for all > DefaultProtocolClientWorker (and if that's not required then don't make it > static) Done. https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... chrome/browser/shell_integration.h:180: scoped_refptr<base::SequencedTaskRunner> GetTaskRunner(); On 2017/05/30 19:22:21, gab wrote: > static Done.
lgtm
Thanks!
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, benwells@chromium.org Link to the patchset: https://codereview.chromium.org/2888693003/#ps120001 (title: "Gab 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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, gab@chromium.org, benwells@chromium.org Link to the patchset: https://codereview.chromium.org/2888693003/#ps140001 (title: "Added a comment and fix typo")
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pmonette@chromium.org
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": 140001, "attempt_start_ts": 1496277770898340,
"parent_rev": "6d095ec7a71eb72e2ad9992d1ca3080f5726f6bf", "commit_rev":
"b18379f369eec8d994cca85d1133f3fac5818a4a"}
Message was sent while issue was closed.
Description was changed from ========== Remove the usage of BrowserThread::FILE in the shell_integration* files The usage of BrowserThread::FILE is deprecated in favor of the post_task.h API. BUG=689520 ========== to ========== Remove the usage of BrowserThread::FILE in the shell_integration* files The usage of BrowserThread::FILE is deprecated in favor of the post_task.h API. BUG=689520 Review-Url: https://codereview.chromium.org/2888693003 Cr-Commit-Position: refs/heads/master@{#476129} Committed: https://chromium.googlesource.com/chromium/src/+/b18379f369eec8d994cca85d1133... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/b18379f369eec8d994cca85d1133...
Message was sent while issue was closed.
robliao@chromium.org changed reviewers: + robliao@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:177: static scoped_refptr<base::SequencedTaskRunner>* task_runner = nullptr; On 2017/05/31 16:13:57, Patrick Monette wrote: > On 2017/05/30 19:22:21, gab wrote: > > I think this can just be a > > > > static scoped_refptr<base::SequencedTaskRunner> task_runner; > > > > without being a scoped_refptr* > > As per offline discussion, keeping this as a pointer since static variables must > be POD. Wait, why must function static variables be POD? Are we just attempting to avoid destruction here?
Message was sent while issue was closed.
https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:177: static scoped_refptr<base::SequencedTaskRunner>* task_runner = nullptr; On 2017/07/14 21:13:13, robliao wrote: > On 2017/05/31 16:13:57, Patrick Monette wrote: > > On 2017/05/30 19:22:21, gab wrote: > > > I think this can just be a > > > > > > static scoped_refptr<base::SequencedTaskRunner> task_runner; > > > > > > without being a scoped_refptr* > > > > As per offline discussion, keeping this as a pointer since static variables > must > > be POD. > > Wait, why must function static variables be POD? Are we just attempting to avoid > destruction here? Prescribed by the style guide. https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
Message was sent while issue was closed.
https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/2888693003/diff/100001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:177: static scoped_refptr<base::SequencedTaskRunner>* task_runner = nullptr; On 2017/07/14 21:25:34, Patrick Monette wrote: > On 2017/07/14 21:13:13, robliao wrote: > > On 2017/05/31 16:13:57, Patrick Monette wrote: > > > On 2017/05/30 19:22:21, gab wrote: > > > > I think this can just be a > > > > > > > > static scoped_refptr<base::SequencedTaskRunner> task_runner; > > > > > > > > without being a scoped_refptr* > > > > > > As per offline discussion, keeping this as a pointer since static variables > > must > > > be POD. > > > > Wait, why must function static variables be POD? Are we just attempting to > avoid > > destruction here? > > Prescribed by the style guide. > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables Got it. I thought that only applied at the global scope. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
