Issue 116 - Document the libadblockplus API

include/AdblockPlus/JsEngine.h

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 29 matching lines @@
   class Context;
   template<class T> class Handle;
   typedef Handle<Value>(*InvocationCallback)(const Arguments &args);
 }
 
 namespace AdblockPlus
 {
   class JsEngine;
   typedef std::tr1::shared_ptr<JsEngine> JsEnginePtr;
 
+  /**
+   * JavaScript engine used by `FilterEngine`, uses v8.

[Wladimir Palant, 2014/08/28 17:24:40]

"uses v8" => "wraps v8"?

[Felix Dahlke, 2014/08/28 22:56:41]

Done.

+   */
   class JsEngine : public std::tr1::enable_shared_from_this<JsEngine>
   {
     friend class JsValue;
     friend class JsContext;
 
   public:
+    /**
+     * Event callback function.
+     */
     typedef std::tr1::function<void(JsValueList& params)> EventCallback;
+
+    /**
+     * Maps events to callback functions.
+     */
     typedef std::map<std::string, EventCallback> EventMap;
 
+    /**
+     * Creates a new JavaScript engine instance.
+     * @param appInfo AppInfo Information about the app, used for update checks.

[Wladimir Palant, 2014/08/28 17:24:40]

"AppInfo" is unnecessary here?

[Felix Dahlke, 2014/08/28 22:56:41]

Done.

+     * @return New `JsEngine` instance.
+     */
     static JsEnginePtr New(const AppInfo& appInfo = AppInfo());
+
+    /**
+     * Registers the callback function for an event.
+     * @param eventName Name of the event.

[Wladimir Palant, 2014/08/28 17:24:40]

Here and below, why not just "Event name" and "Event parameters"?

[Felix Dahlke, 2014/08/28 22:56:41]

Done.

+     * @param callback Event callback function.
+     */
     void SetEventCallback(const std::string& eventName, EventCallback callback);
+
+    /**
+     * Removes the callback function for an event.
+     * @param eventName Name of the event.
+     */
     void RemoveEventCallback(const std::string& eventName);
+
+    /**
+     * Triggers an event.
+     * @param eventName Name of the event.
+     * @param params Parameters for the event.
+     */
     void TriggerEvent(const std::string& eventName, JsValueList& params);
+
+    /**
+     * Evaluates a JavaScript expression.
+     * @param source JavaScript expression to evalute.

[Wladimir Palant, 2014/08/28 17:24:40]

"evaluate"

[Felix Dahlke, 2014/08/28 22:56:41]

Done.

+     * @param filename Optional file name for the expression, used in error
+     *        messages.
+     * @return Result of the evaluated expression.
+     */
     JsValuePtr Evaluate(const std::string& source,
         const std::string& filename = "");
+
+    /**
+     * Initiates a garbage collection.

[Wladimir Palant, 2014/08/28 17:24:40]

I don't think you can use an article here...

[Felix Dahlke, 2014/08/28 22:56:41]

I do, here's one example found in the wild:
http://msdn.microsoft.com/en-us/library/s5zscb2d%28v=vs.85%29.aspx

+     */
     void Gc();
+
+    //@{
+    /**
+     * Creates a new JavaScript value.
+     * @param val Value to convert.
+     * @return New `JsValue` instance.
+     */
     JsValuePtr NewValue(const std::string& val);
     JsValuePtr NewValue(int64_t val);
     JsValuePtr NewValue(bool val);
     inline JsValuePtr NewValue(const char* val)
     {
       return NewValue(std::string(val));
     }
     inline JsValuePtr NewValue(int val)
     {
       return NewValue(static_cast<int64_t>(val));
     }
 #ifdef __APPLE__
     inline JsValuePtr NewValue(long val)
     {
       return NewValue(static_cast<int64_t>(val));
     }
 #endif
+    //@}
+
+    /**
+     * Creates a new JavaScript object.
+     * @return New `JsValue` instance.
+     */
     JsValuePtr NewObject();
+
+    /**
+     * Creates a new JavaScript callback.

[Wladimir Palant, 2014/08/28 17:24:40]

The thing about documentation is: it should be helpful ;)

You probably want to note that you create the callback so that you can pass it
to JavaScript functions and let them call your C++ code. You can also note that
the callback will get v8::Arguments as parameter - and that it can retrieve the
JsEngine instance from these using the FromArguments() method. And pointing to
NewCallback() in the FromArguments() documentation isn't wrong either of course
- this method will work *only* for callbacks created this way.

[Felix Dahlke, 2014/08/28 22:56:41]

Done.

+     * @param callback Callback to convert.
+     * @return New `JsValue` instance.
+     */
     JsValuePtr NewCallback(v8::InvocationCallback callback);
+
+    /**
+     * Returns a `JsEngine` instance contained in a `v8::Arguments` object.
+     * @param arguments `v8::Arguments` object containing the `JsEngine`
+     *        instance.
+     * @return `JsEngine` instance.
+     */
     static JsEnginePtr FromArguments(const v8::Arguments& arguments);
+
+    /**
+     * Converts v8 arguments to `JsValue` objects.
+     * @param arguments `v8::Arguments` object containing the arguments to
+     *        convert.
+     * @return List of arguments converted to `JsValue` objects.
+     */
     JsValueList ConvertArguments(const v8::Arguments& arguments);
 
+    /**
+     * @see SetFileSystem().
+     */
     FileSystemPtr GetFileSystem();
+
+    /**
+     * Sets the `FileSystem` implementation used for all file I/O.
+     * Setting this is optional, the engine will use a `DefaultFileSystem`
+     * instance by default, which should normally be sufficient.

[Wladimir Palant, 2014/08/28 17:24:40]

"normally be sufficient"? "might be sufficient" is more like it :)

[Felix Dahlke, 2014/08/28 22:56:41]

Done.

+     * @param The `FileSystem` instance to use.
+     */
     void SetFileSystem(FileSystemPtr val);
+
+    /**
+     * @see SetWebRequest().
+     */
     WebRequestPtr GetWebRequest();
+
+    /**
+     * Sets the `WebRequest` implementation used for XMLHttpRequests.
+     * Setting this is optional, the engine will use a `DefaultWebRequest`
+     * instance by default, which should normally be sufficient.
+     * @param The `WebRequest` instance to use.
+     */
     void SetWebRequest(WebRequestPtr val);
+
+    /**
+     * @see SetLogSystem().
+     */
     LogSystemPtr GetLogSystem();
+
+    /**
+     * Sets the `LogSystem` implementation used for logging (e.g. console.log).

[Wladimir Palant, 2014/08/28 17:24:40]

"e.g. to handle console.log() calls from JavaScript"

[Felix Dahlke, 2014/08/28 22:56:41]

Done.

+     * Setting this is optional, the engine will use a `DefaultLogSystem`
+     * instance by default, which should normally be sufficient.
+     * @param The `FileSystem` instance to use.

[Wladimir Palant, 2014/08/28 17:24:40]

FileSystem => LogSystem?

[Felix Dahlke, 2014/08/28 22:56:41]

Done.

+     */
     void SetLogSystem(LogSystemPtr val);
 
   private:
     JsEngine();
 
     FileSystemPtr fileSystem;
     WebRequestPtr webRequest;
     LogSystemPtr logSystem;
     v8::Isolate* isolate;
     V8ValueHolder<v8::Context> context;
     EventMap eventCallbacks;
   };
 }
 
 #endif