Offer user to send feedback from profile error dialog

sql/connection.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef SQL_CONNECTION_H_
 #define SQL_CONNECTION_H_
 
 #include <stddef.h>
 #include <stdint.h>
 #include <map>
 #include <memory>
 #include <set>
 #include <string>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
+#include "sql/db_diagnostic_map.h"

[michaeln, 2016/07/09 03:00:38]

probably overkill to add a new file for using a std::map<> alias, unless there's
a compelling reason not i think you can put it in this

[Scott Hess - ex-Googler, 2016/07/10 05:16:52]

Honestly, I'm not sure I see why this is in sql/ or sql:: at all, nor can I see
why it's a map.  Is there some long-term plan which justifies any of those
decisions?  Why not just have the various clients pass along a string which they
keep appending key:value data to and be done with it?

I'll admit, figuring out where to put the shared code to generate the diagnostic
info is a tough one.  As a sql/OWNER member, I'd rather that such code wasn't
arbitrarily pushed into here simply because you couldn't figure out a better
place to put it.  But I can see how that might happen.  Having happened, I'd
rather it were as simple as possible, and not uncommented and named as if it's a
general-purpose method which just anyone should be using.

[afakhry, 2016/07/11 16:47:46]

The reasons behind my decision to put it there were the following:
- I was following the same convention in "sql/init_status.h" which is only there
to include an enum used by the clients of sqlite rather than sqlite itself.
   - I could have just added the DatabaseDiagnosticMap there in
"sql/init_status.h" , but it felt even more awkward, since DatabaseDiagnosticMap
is actually used by sql::Connection inside sql/ and the file name will be
misleading.

- I thought of declaring DatabaseDiagnosticMap in connection.h, but I wasn't
sure if I wanted clients to include "connection.h" whenever they needed this
map.

The reasons behind choosing a map:
- It keeps the keys sorted, which is a nice thing to have if we decide to expand
the list of diagnostic information we add, or integrate this data with the
feedback App system information window, which contains sorted key-value pair of
information.
- It makes the code shorter and cleaner; just map["key"] = value;
- It is also easier for clients to query for certain diagnostics key (might be
useful in the future).

That being said, I'm open for alternatives/suggestions if they're found to be
better than what's mentioned above.

[Scott Hess - ex-Googler, 2016/07/13 01:18:46]

sql/init_status.h is also really client code which really probably shouldn't be
in sql/.
This seems completely arbitrary, why should the lexicographic ordering of the
keys be the best way to sort them?  As opposed to some other ordering like "The
order things ran in"?
result.append("key: ").append(value).append("\n");

I don't think making diagnostic code shorter and cleaner should be a primary
factor.  I mean, we shouldn't make it long and horrible, but I don't think
you're making an order-of-magnitude improvement in this case.
I think that you shouldn't design a general-purpose diagnostics-querying engine
in an ad-hoc manner.  If you want to build such a system, you should probably
put up a design doc and post it around to see what people want to dump into it.
What are the downsides of a std::string blob?  Not the "Maybe someday someone
might want to do something different" downsides, what are the downsides for the
problem currently in front of you?

[afakhry, 2016/07/13 19:57:59]

Nothing other than the reason I stated above. I'm now using string as suggested.

 #include "sql/sql_export.h"
 
 struct sqlite3;
 struct sqlite3_stmt;
 
 namespace base {
 class FilePath;
 class HistogramBase;
 }
 
@@ skipping 199 matching lines @@
   // results into |messages|.  Returns the success of running the
   // statement - per the SQLite documentation, if no errors are found the
   // call should succeed, and a single value "ok" should be in messages.
   bool FullIntegrityCheck(std::vector<std::string>* messages);
 
   // Runs "PRAGMA quick_check" and, unlike the FullIntegrityCheck method,
   // interprets the results returning true if the the statement executes
   // without error and results in a single "ok" value.
   bool QuickIntegrityCheck() WARN_UNUSED_RESULT;
 
+  sql::DatabaseDiagnosticMap GetDiagnosticMap();
+
   // Initialization ------------------------------------------------------------
 
   // Initializes the SQL connection for the given file, returning true if the
   // file could be opened. You can call this or OpenInMemory.
   bool Open(const base::FilePath& path) WARN_UNUSED_RESULT;
 
   // Initializes the SQL connection for a temporary in-memory database. There
   // will be no associated file on disk, and the initial database will be
   // empty. You can call this or Open.
   bool OpenInMemory() WARN_UNUSED_RESULT;
@@ skipping 545 matching lines @@
 
   // Stores the dump provider object when db is open.
   std::unique_ptr<ConnectionMemoryDumpProvider> memory_dump_provider_;
 
   DISALLOW_COPY_AND_ASSIGN(Connection);
 };
 
 }  // namespace sql
 
 #endif  // SQL_CONNECTION_H_