[Offline Pages] Expanding event logger to be used by harness.

components/offline_pages/offline_event_logger.h

 // Copyright 2016 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 COMPONENTS_OFFLINE_PAGES_OFFLINE_EVENT_LOGGER_H_
 #define COMPONENTS_OFFLINE_PAGES_OFFLINE_EVENT_LOGGER_H_
 
 #include <deque>
 #include <string>
 #include <vector>
 
 #include "base/macros.h"
 
+#define OFFLINE_LOG_TAG \
+  std::string(__FILE__) + "(" + std::to_string(__LINE__) + ") :"
+
 namespace offline_pages {
 
 // Maximum number of recorded Logs to keep track of at any moment.
 static const size_t kMaxLogCount = 50;
 
 // Facilitates the logging of events. Subclasses should create methods that
 // call RecordActivity to write into the log. |SetIsLogging|, |GetLogs|, and
 // |Clear| are called from the chrome://offline-internals page.
 //
 // Logging should be done by calling the corresponding subclass methods when
 // a loggable event occurs (i.e. when status has changed for a save request
 // or when an offlined page has been accessed/saved).
 //
 // This log only keeps track of the last |kMaxLogCount| events.
 class OfflineEventLogger {
  public:
+  class Client {

[chili, 2016/11/25 00:00:07]

So... if I understand it correctly, CustomLog and the Client allows a way for
classes to put the logged events elsewhere?  If the only purpose is to write to
a file, I don't see why we can't have an open file handle passed in?

[romax, 2016/11/30 00:15:10]

Yes that was another option.
The goal is 'we need to write logs to a file'. And this could happen on either
Java or C++ side. Then the other side will need a method to call in order to
have all the logs on the same side for consistency. Currently they're all on
Java side.
I don't really have a strong opinion where it should be, can you please
elaborate the advantage of passing file handle to c++?
Also I think it might end up hitting some async issues but I'm not 100% sure...
(Even for this patch there might be async issues)

[chili, 2016/11/30 04:56:39]

Keeping the file handle on Java sounds/looks ok to me.  I think the main
potential difference is:

Right now if I understand right - 
- Java calls C++ log
- C++ log calls Custom log
- Custom log calls into Java
- Java writes to file, returns to C++ call stack
- finish log call
This would occur with every single log message

If the file handle is on C++, then Java would pass in file handle one time, and
we have -
- Java calls C++ log
- C++ log writes to file
- finish log call

Having heard before that JNI boundary stuff is harder to test, having file
writes on C++ side may be easier?

[dougarnett, 2016/11/30 18:05:25]

It is a good point about hopping back and forth over JNI. 
This class is the one non-eval class in this CL though so I think I prefer not
having a file handle in this class (interface seems better). Perhaps an open
file/stream could be held in the eval native code though? I'm not sure if that
is critical for this iteration.

[romax, 2016/12/01 01:47:52]

Yes JNI is hard to test.
The current logging process should look like:
1. Log on Java
- Java calls EvalBridge.log()
- EvalBridge.log() writes to file.
- finish.

2. Log on C++
- C++ log calls Custom log
- Custom log calls into Java
- Java writes to file, returns to C++ call stack
- finish.

If we do the writing on C++ side (custom log writes to file?)
1. Log on Java
- Java calls C++ log
- C++ log calls Custom log
- Custom log writes to file.
- finish.

2. Log on C++
- C++ log calls custom log
- Custom log writes to file.
- finish.

These two ways should look like mirror images in some sense...
Oh I see what's missing... So in this patch, I'm not posting the Java logs to
the log pool (so Java logs would not be shown in internals page). So Java logs
wouldn't call C++ log methods.

However I think it's a good thing to consider how the logger can be used through
our code. Maybe we should combine OPM and RC into one and have the single
service own the logger and policy controller...

Regarding Doug's comment, I think the idea behind bridges are keeping them as
simple bridge, doing some translation and calling. So I'm not sure if it's a
good idea to keep the handle in the bridge.

[chili, 2016/12/01 21:07:04]

I see.

As I mentioned, I'm fine with the CL as is. In this particular case, I still
think it might be worth it (in future) to ensure that the written log file is
consistent with what our internal logger is keeping track of so we don't
accidentally log more things on Java side expecting it to show up in internal
page logs.

I'm also generally supportive of doing log writing on C++ side. There exists
some intention to also have our items work on other OS. I think it might be
better in the long-term maintenance perspective to not have potentially
OS-dependent log impls for something relatively common as writing to file.

+   public:
+    virtual void CustomLog(const std::string& message);

[chili, 2016/11/30 04:56:40]

nit - should this also have = 0?

[romax, 2016/12/01 01:47:52]

Done.

+  };
+
   OfflineEventLogger();
 
   ~OfflineEventLogger();
 
   // Clears the recorded activities.
   void Clear();
 
   // Turns logging on/off.
   void SetIsLogging(bool is_logging);
 
   // Returns whether we are currently logging.
   bool GetIsLogging();
 
   // Dumps the current activity list into |records|.
   void GetLogs(std::vector<std::string>* records);
 
- protected:
+  // Sets client.
+  void SetClient(Client* client);
+
   // Write the activity into the cycling log if we are currently logging.
   void RecordActivity(const std::string& activity);
 
  private:
   // Recorded offline page activities.
   std::deque<std::string> activities_;
 
   // Whether we are currently recording logs or not.
   bool is_logging_;
+
+  // Not owned.
+  Client* client_;
 };
 }  // namespace offline_pages
 
 #endif  // COMPONENTS_OFFLINE_PAGES_OFFLINE_EVENT_LOGGER_H_