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 27 matching lines @@
   class Isolate;
   class Value;
   class Context;
   template<class T> class Handle;
   typedef Handle<Value>(*InvocationCallback)(const Arguments &args);
 }
 
 namespace AdblockPlus
 {
   class JsEngine;
+
+  /**
+   * Shared smart pointer to a `JsEngine` instance.
+   */
   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.

+   * JavaScript engine used by `FilterEngine`, wraps v8.
    */
   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.

+     * @param appInfo Information about the app.
      * @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 eventName Event name. Note that this can be any string - it's a
+     *        general purpose event handling mechanism.
      * @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.
+     * @param eventName Event name.
      */
     void RemoveEventCallback(const std::string& eventName);
 
     /**
      * Triggers an event.
-     * @param eventName Name of the event.
-     * @param params Parameters for the event.
+     * @param eventName Event name.
+     * @param params Event parameters.
      */
     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 source JavaScript expression to evaluate.
      * @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);
@@ skipping 15 matching lines @@
 #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.
+     * Creates a JavaScript function that invokes a C++ callback.
+     * @param callback C++ callback to invoke. The callback receives a
+     *        `v8::Arguments` object and can use `FromArguments()` to retrieve
+     *        the current `JsEngine`.
      * @return New `JsValue` instance.
      */
     JsValuePtr NewCallback(v8::InvocationCallback callback);
 
     /**
      * Returns a `JsEngine` instance contained in a `v8::Arguments` object.
+     * Use this in callbacks created via `NewCallback()` to retrieve the current
+     * `JsEngine`.
      * @param arguments `v8::Arguments` object containing the `JsEngine`
      *        instance.
-     * @return `JsEngine` instance.
+     * @return `JsEngine` instance from `v8::Arguments`.
      */
     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().
+     * @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.

+     * instance by default, which might be sufficient.
      * @param The `FileSystem` instance to use.
      */
     void SetFileSystem(FileSystemPtr val);
 
     /**
-     * @see SetWebRequest().
+     * @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.
+     * instance by default, which might be sufficient.
      * @param The `WebRequest` instance to use.
      */
     void SetWebRequest(WebRequestPtr val);
 
     /**
-     * @see SetLogSystem().
+     * @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.

+     * Sets the `LogSystem` implementation used for logging (e.g. to handle
+     * `console.log()` calls from JavaScript).
      * 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.

+     * instance by default, which might be sufficient.
+     * @param The `LogSystem` instance to use.
      */
     void SetLogSystem(LogSystemPtr val);
 
   private:
     JsEngine();
 
     FileSystemPtr fileSystem;
     WebRequestPtr webRequest;
     LogSystemPtr logSystem;
     v8::Isolate* isolate;
     V8ValueHolder<v8::Context> context;
     EventMap eventCallbacks;
   };
 }
 
 #endif