Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2843062:
Header Cleanup2: Remove another include of sqlite_utils.h that is not needed. (Closed)
|
Created:
10 years, 5 months ago by tfarina Modified:
9 years, 7 months ago Reviewers:
Scott Hess - ex-Googler CC:
chromium-reviews, ben+cc_chromium.org Base URL:
git://git.chromium.org/chromium.git Visibility:
Public. |
DescriptionHeader Cleanup2: Remove another include of sqlite_utils.h that is not needed.
BUG=None
TEST=trybots
Signed-off-by: Thiago Farina <tfarina@chromium.org>;
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52837
Patch Set 1 #
Total comments: 3
Patch Set 2 : revert some changes #
Messages
Total messages: 14 (0 generated)
Hi Scott, may you have a look?
The login_database.cc change looks good, but I don't think I agree with the other change. http://codereview.chromium.org/2843062/diff/1/3 File chrome/browser/task_manager_resource_providers.cc (right): http://codereview.chromium.org/2843062/diff/1/3#newcode13 chrome/browser/task_manager_resource_providers.cc:13: #endif I should rather continue to use a common header than distribute a block like this to all the files currently include sqlite_utils.h.
http://codereview.chromium.org/2843062/diff/1/3 File chrome/browser/task_manager_resource_providers.cc (right): http://codereview.chromium.org/2843062/diff/1/3#newcode13 chrome/browser/task_manager_resource_providers.cc:13: #endif On 2010/07/17 17:59:39, shess wrote: > I should rather continue to use a common header than distribute a block like > this to all the files currently include sqlite_utils.h. Sure, but this file is using nothing of sqlite_utils.h. It's is using just the sqlite3_memory_usage.
http://codereview.chromium.org/2843062/diff/1/3 File chrome/browser/task_manager_resource_providers.cc (right): http://codereview.chromium.org/2843062/diff/1/3#newcode13 chrome/browser/task_manager_resource_providers.cc:13: #endif On 2010/07/17 18:03:33, tfarina wrote: > On 2010/07/17 17:59:39, shess wrote: > > I should rather continue to use a common header than distribute a block like > > this to all the files currently include sqlite_utils.h. > > Sure, but this file is using nothing of sqlite_utils.h. It's is using just the > sqlite3_memory_usage. But my concern wasn't related to whether it was using any of the things added by sqlite_utils.h. My concern was around sprinkling specific porting knowledge around the codebase. [Yes, I see another couple places this idiom is used, and I don't like those, either. At some point someone will screw it up and their notice will be an opaque link error, or worse, something which links but uses distinct versions of the same symbol.] Is there a cleanup bug or something to provide these changes with context? Is the goal to get rid of sqlite_utils.h entirely, or something else? I notice that the app/sql/*.cc files don't have the USE_SYSTEM_SQLITE at all. They carefully keep the sqlite3.h include within the implementation files, presumably to keep clients from depending specifically on sqlite. Maybe someone who did most of that could provide some light on this. If the long-term goal is to have everything using app/sql/ and not be specifically sqlite dependent, it might make sense to have app/sql/util.h export something like size_t sql::MemoryUsedBytes();. But if there should be a distinction between the generic stuff in app/sql/ and the specific stuff in chrome/, then chrome/common/sqlite3.h might be in order.
On 2010/07/17 18:18:19, shess wrote: > http://codereview.chromium.org/2843062/diff/1/3 > File chrome/browser/task_manager_resource_providers.cc (right): > > http://codereview.chromium.org/2843062/diff/1/3#newcode13 > chrome/browser/task_manager_resource_providers.cc:13: #endif > On 2010/07/17 18:03:33, tfarina wrote: > > On 2010/07/17 17:59:39, shess wrote: > > > I should rather continue to use a common header than distribute a block like > > > this to all the files currently include sqlite_utils.h. > > > > Sure, but this file is using nothing of sqlite_utils.h. It's is using just the > > sqlite3_memory_usage. > > But my concern wasn't related to whether it was using any of the things added by > sqlite_utils.h. My concern was around sprinkling specific porting knowledge > around the codebase. [Yes, I see another couple places this idiom is used, and > I don't like those, either. At some point someone will screw it up and their > notice will be an opaque link error, or worse, something which links but uses > distinct versions of the same symbol.] > > Is there a cleanup bug or something to provide these changes with context? Is > the goal to get rid of sqlite_utils.h entirely, or something else? Not that I know. But I would like to get rid of these duplicated API if possible. I don't like to have two versions of code, doing the same thing for the same purpose. >I notice > that the app/sql/*.cc files don't have the USE_SYSTEM_SQLITE at all. I think that is a mistake. Maybe Tony and Evan Martin knows more about this. > If the long-term goal is to have everything using app/sql/ and not be > specifically sqlite dependent, it might make sense to have app/sql/util.h export > something like size_t sql::MemoryUsedBytes();. But if there should be a > distinction between the generic stuff in app/sql/ and the specific stuff in > chrome/, then chrome/common/sqlite3.h might be in order. Why have another util file to talk with sqlite? If everything we need is already need is provided by the app/sql? Would you like me to close this issue, since you don't agree with the changes?
On 2010/07/17 18:29:18, tfarina wrote: > Why have another util file to talk with sqlite? If everything we need is already > need is provided by the app/sql? s/we need is already need is/is already provided.
On 2010/07/17 18:29:18, tfarina wrote: > On 2010/07/17 18:18:19, shess wrote: > >I notice > > that the app/sql/*.cc files don't have the USE_SYSTEM_SQLITE at all. > I think that is a mistake. Maybe Tony and Evan Martin knows more about this. Probably would shed some light. > > If the long-term goal is to have everything using app/sql/ and not be > > specifically sqlite dependent, it might make sense to have app/sql/util.h > export > > something like size_t sql::MemoryUsedBytes();. But if there should be a > > distinction between the generic stuff in app/sql/ and the specific stuff in > > chrome/, then chrome/common/sqlite3.h might be in order. > > Why have another util file to talk with sqlite? If everything we need is already > need is provided by the app/sql? It depends on what the goal is. Getting the amount of memory used by the database system is probably intrinsically not portable between backends, so calling the sqlite3 function directly is appropriate, in which case my complaint is with spreading the include-file boilerplate all over the place. But if the goal is to funnel everything possible through app/sql/, then new API needs to be added to expose global information like this. There is probably someone who could directly state whether exposing new stuff in app/sql/ would be appropriate. I'd say brettw. > Would you like me to close this issue, since you don't agree with the changes? Up to you. I agree with the removal of the dead include line, I just don't agree with adding new references to USE_SYSTEM_SQLITE. I would support a change which pushed just that part of sqlite_util.h to chrome/common/sqlite3.h, and modified the other references to include that file.
On 2010/07/17 18:36:46, shess wrote: > > Would you like me to close this issue, since you don't agree with the changes? > > Up to you. I agree with the removal of the dead include line, I just don't > agree with adding new references to USE_SYSTEM_SQLITE. I would support a change > which pushed just that part of sqlite_util.h to chrome/common/sqlite3.h, and > modified the other references to include that file. You mean by doing the work of '#if defined(USE_SYSTEM_SQLITE)' in chrome/common/sqlite3.h and fix the other places to include it instead?
On 2010/07/17 18:36:46, shess wrote: > > Would you like me to close this issue, since you don't agree with the changes? > > Up to you. I agree with the removal of the dead include line OK, I reverted the offending changed. :)
On 2010/07/17 18:50:28, tfarina wrote: > On 2010/07/17 18:36:46, shess wrote: > > > Would you like me to close this issue, since you don't agree with the > changes? > > > > Up to you. I agree with the removal of the dead include line > OK, I reverted the offending changed. :) /s/changed/change.
Yes, I think that pulling that 5-line stanza out and sharing it would be an improvement over replicating it to a half-dozen files. Though the app/sql/ files obviously couldn't include it. -scott On Sat, Jul 17, 2010 at 11:43 AM, <tfarina@chromium.org> wrote: > On 2010/07/17 18:36:46, shess wrote: >> >> > Would you like me to close this issue, since you don't agree with the > > changes? > >> Up to you. I agree with the removal of the dead include line, I just >> don't >> agree with adding new references to USE_SYSTEM_SQLITE. I would support a > > change >> >> which pushed just that part of sqlite_util.h to chrome/common/sqlite3.h, >> and >> modified the other references to include that file. > > You mean by doing the work of '#if defined(USE_SYSTEM_SQLITE)' in > chrome/common/sqlite3.h and fix the other places to include it instead? > > > http://codereview.chromium.org/2843062/show >
Earlier LGTM on this part still stands.
On Sat, Jul 17, 2010 at 9:15 PM, <shess@chromium.org> wrote: > Earlier LGTM on this part still stands. > Thanks Scott, committing now... -- A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? -Thiago
On 2010/07/18 00:13:36, shess wrote: > Yes, I think that pulling that 5-line stanza out and sharing it would > be an improvement over replicating it to a half-dozen files. Though > the app/sql/ files obviously couldn't include it. > Sure, I will prepare a patch for that. My idea is to add the sqlite3.h in the app/sql/ directory so it can be shared by chrome/common/, webkit/, and chrome/renderer/. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
