Issue 116 - Document the libadblockplus API

include/AdblockPlus/FilterEngine.h

Index: include/AdblockPlus/FilterEngine.h
===================================================================
--- a/include/AdblockPlus/FilterEngine.h
+++ b/include/AdblockPlus/FilterEngine.h
@@ -31,67 +31,266 @@
 {
   class FilterEngine;
 
+  /**
+   * Wrapper for a filter object.

[Wladimir Palant, 2014/08/29 18:18:50]

"Adblock Plus filter object"? Just to make it clear that this is ABP
functionality, as opposed to V8 for example.

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+   */
   class Filter : public JsValue,
                  public std::tr1::enable_shared_from_this<Filter>
   {
   public:
+    /**
+     * Filter types, see https://adblockplus.org/en/filters.
+     */
     enum Type {TYPE_BLOCKING, TYPE_EXCEPTION,
                TYPE_ELEMHIDE, TYPE_ELEMHIDE_EXCEPTION,
                TYPE_COMMENT, TYPE_INVALID};
 
     Type GetType();

[Wladimir Palant, 2014/08/29 18:18:50]

I think this method should be documented ;)

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+
+    /**
+     * Checks whether this filter has been added to its subscription.
+     * @return `true` if this filter has been added to its subscription.

[Wladimir Palant, 2014/08/29 18:18:50]

No, "Checks whether this filter has been added to the list of custom filters."
Filters in subscriptions aren't considered here.

[Felix Dahlke, 2014/09/01 17:00:15]

Done, seems I misinterpreted FilterStorage.addFilter here.

+     */
     bool IsListed();
+
+    /**
+     * Adds this filter to its subscription.

[Wladimir Palant, 2014/08/29 18:18:50]

No, "Adds this filter to user's custom filters."

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+     */
     void AddToList();
+
+    /**
+     * Removes this filter from its subscription.

[Wladimir Palant, 2014/08/29 18:18:50]

No, "Removes this filter from user's custom filters."

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+     */
     void RemoveFromList();
+
     bool operator==(const Filter& filter) const;
 
     Filter(JsValuePtr value);

[Wladimir Palant, 2014/08/29 18:18:50]

Want to document this? "Creates a wrapper for a JavaScript filter object
instance. Normally you won't call this directly but use
FilterEngine::GetFilter() instead."

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

   };
 
+  /**
+   * Wrapper for a subscription object.
+   */
   class Subscription : public JsValue,
                        public std::tr1::enable_shared_from_this<Subscription>
   {
   public:
+    /**
+     * Checks if this subscription has been added to the known subscriptions.
+     * @return `true` if this subscription has been added to the known
+     *         subscriptions.

[Wladimir Palant, 2014/08/29 18:18:50]

What are "known subscriptions"? "Checks if the subscription has been added to
user's list of subscriptions"?

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+     */
     bool IsListed();
+
+    /**
+     * Adds this subscription to the known subscriptions.

[Wladimir Palant, 2014/08/29 18:18:50]

"Adds this subscription to user's list of subscriptions"?

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+     */
     void AddToList();
+
+    /**
+     * Removes this subscription from the known subscriptions.

[Wladimir Palant, 2014/08/29 18:18:50]

"Removes this subscription from user's list of subscriptions"?

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+     */
     void RemoveFromList();
+
+    /**
+     * Updates this subscription, i.e.\ retrieves the latest version from the

[Wladimir Palant, 2014/08/29 18:18:50]

"latest version" => "current filters"?

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+     * subscription URL.
+     */
     void UpdateFilters();
+
+    /**
+     * Checks if this subscriptions is currently being updated.
+     * @return `true` if this subscriptions is currently being updated.
+     */
     bool IsUpdating();
+
     bool operator==(const Subscription& subscription) const;
 
     Subscription(JsValuePtr value);

[Wladimir Palant, 2014/08/29 18:18:50]

Want to document this? "Creates a wrapper for a JavaScript subscription object
instance. Normally you won't call this directly but use
FilterEngine::GetSubscription() instead."

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

   };
 
+  /**
+   * Shared smart pointer to a `Filter` instance.
+   */
   typedef std::tr1::shared_ptr<Filter> FilterPtr;
+
+  /**
+   * Shared smart pointer to a `Subscription` instance.
+   */
   typedef std::tr1::shared_ptr<Subscription> SubscriptionPtr;
 
+  /**
+   * Main component of Libadblockplus.
+   * It handles:
+   * - Filter management and matching.
+   * - Subscription management and synchronization.
+   * - Update checks for the application.
+   */
   class FilterEngine
   {
   public:
+    /**
+     * Callback invoked when an update is available.

[Wladimir Palant, 2014/08/29 18:18:50]

"Callback" => "Type of callback"?

Want to note that the parameter here is an optional error message?

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+     */
     typedef std::tr1::function<void(const std::string&)> UpdaterCallback;
+
+    /**
+     * Callback invoked when the filters change.

[Wladimir Palant, 2014/08/29 18:18:50]

"Callback" => "Type of callback"?

Want to note that the parameters here are the change type and the
Filter/Subscription object affected if any?

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+     */
     typedef std::tr1::function<void(const std::string&, const JsValuePtr)> FilterChangeCallback;
 
     explicit FilterEngine(JsEnginePtr jsEngine);
     JsEnginePtr GetJsEngine() const { return jsEngine; }

[Wladimir Palant, 2014/08/29 18:18:50]

I guess you plan to document these two functions later?

[Felix Dahlke, 2014/09/01 17:00:15]

Documented them now. They seemed rather obvious to me so I left them out, but on
second thought they don't seem that obvious.

+
+    /**
+     * Checks if this is the first run of the application.

[Wladimir Palant, 2014/08/29 18:18:50]

Want to note that this is determined by the presence of a filter database? This
flag exists to indicate that the filters/subscriptions need to be initialized.

[Felix Dahlke, 2014/09/01 17:00:15]

Libadblockplus does initialise filter lists internally when first run, so this
seems like a technical detail, and a bit of a leaky abstraction to me. That a
client might want to initialise some additional filter lists (like we do with
acceptable ads) when the filter engine is running for the first time seems clear
enough.

[Wladimir Palant, 2014/09/01 17:36:56]

True, I forgot that this logic moved from the application into the library a
while ago. So at this point the flag is mostly there to show the first-run page,
I guess additional documentation is indeed unnecessary.

+     * @return `true` if the application is running for the first time.
+     */
     bool IsFirstRun() const;
+
+    /**
+     * Retrieves a filter object from its text representation.
+     * @param text Text representation of the filter,
+     *        see https://adblockplus.org/en/filters.
+     * @return New filter object if the filter wasn't known, otherwise the
+     *         existing one.

[Wladimir Palant, 2014/08/29 18:18:50]

This is pretty misleading. This function will always create a new wrapper but
the underlying JavaScript object can be the same for multiple wrappers. This is
covered by the overloaded == operator in Filter. So IMHO we shouldn't document
the implementation details here, just say that a filter object is returned.

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+     */
     FilterPtr GetFilter(const std::string& text);
+
+    /**
+     * Retrieves a subscription object for the supplied URL.
+     * @param url Subscription URL.
+     * @return New subscription object if the subscription wasn't known,
+     *         otherwise the existing one.

[Wladimir Palant, 2014/08/29 18:18:50]

Same as above, IMHO we should just say that a subscription object is returned.

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+     */
     SubscriptionPtr GetSubscription(const std::string& url);
+
+    /**
+     * Retrieves all known filters.
+     * @return List of known filters.

[Wladimir Palant, 2014/08/29 18:18:50]

What are "known filters"? "Retrieves user's custom filters"?

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+     */
     std::vector<FilterPtr> GetListedFilters() const;
+
+    /**
+     * Retrieves all known subscriptions.
+     * @return List of known subscriptions.

[Wladimir Palant, 2014/08/29 18:18:50]

What are "known subscriptions"? "Retrieves user's list of subscriptions"?

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+     */
     std::vector<SubscriptionPtr> GetListedSubscriptions() const;
+
+    /**
+     * Retrieves all recommended subscriptions.
+     * This will parse all subscriptions listed in `subscriptions.xml`.

[Wladimir Palant, 2014/08/29 18:18:50]

Why document implementation details? There is no subscriptions.xml in
libadblockplus, only a JavaScript structure created from it - but nobody should
care.

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+     * @return List of recommended subscriptions.
+     */
     std::vector<SubscriptionPtr> FetchAvailableSubscriptions() const;
+
+    /**
+     * Checks if any active filter matches the supplied URL.
+     * @param url URL to match.
+     * @param contentType Content type of the requested resource, possible
+     *        values are:
+     *        - OTHER
+     *        - SCRIPT
+     *        - IMAGE
+     *        - STYLESHEET
+     *        - OBJECT
+     *        - SUBDOCUMENT
+     *        - DOCUMENT
+     *        - XMLHTTPREQUEST
+     *        - OBJECT_SUBREQUEST
+     *        - FONT
+     *        - MEDIA
+     * @param documentUrl URL of the document requesting the resource.
+     *        Note that there will be more than one document if frames are
+     *        involved, see
+     *        Matches(const std::string&, const std::string&, const std::vector<std::string>&) const.
+     * @return Matching filter, or `null` if there was no match.

[Wladimir Palant, 2014/08/29 18:18:50]

Strictly speaking, is it really null? It's an empty FilterPtr instance, will it
equal null?

[Felix Dahlke, 2014/09/01 17:00:15]

Not strictly speaking, it's a shared_ptr instance pointing to 0. operator bool
etc. work as expected from a null pointer, and I think even a null comparison
would work.

Think we want to be more elaborate here? Something like "or a `FilterPtr`
pointing to `null` if there was no match."? IMHO it should be obvious enough as
it is though.

[Wladimir Palant, 2014/09/01 17:36:56]

I am not really sure, that's why I asked. You don't really distinguish between
instances and pointers to them elsewhere so speaking about null here should be
fine...

[Felix Dahlke, 2014/09/02 05:24:51]

I do think so. Note that:

1. There is no "null" in C++, there is NULL, 0 and nullptr - so "null" describes
a concept rather than a concrete value - a concept that applies to smart
pointers as well as raw pointers.
2. The return type cannot take the value NULL, so it's not ambiguous. The
closest thing to NULL this function can return is a shared_ptr to NULL.

+     */
     FilterPtr Matches(const std::string& url,
         const std::string& contentType,
         const std::string& documentUrl) const;
+
+    /**
+     * Checks if any active filter matches the supplied URL.
+     * @param url URL to match.
+     * @param contentType Content type of the requested resource, possible
+     *        values are:
+     *        - OTHER
+     *        - SCRIPT
+     *        - IMAGE
+     *        - STYLESHEET
+     *        - OBJECT
+     *        - SUBDOCUMENT
+     *        - DOCUMENT
+     *        - XMLHTTPREQUEST
+     *        - OBJECT_SUBREQUEST
+     *        - FONT
+     *        - MEDIA
+     * @param documentUrls Chain of documents requesting the resource.

[Wladimir Palant, 2014/08/29 18:18:50]

In which order? "starting with the top-level frame, followed by its child frame,
and so on until the frame actually responsible for the request."

[Felix Dahlke, 2014/09/01 17:00:15]

I've put it slightly differently - we actually start with the parent frame here
and end with the top-level frame.

+     *        This will typically be the frame structure. If the application is
+     *        not capable of identifying the frame structure, e.g. because it is
+     *        a proxy, it can be approximated using `ReferrerMapping`.
+     * @return Matching filter, or `null` if there was no match.
+     */
     FilterPtr Matches(const std::string& url,
         const std::string& contentType,
         const std::vector<std::string>& documentUrls) const;
+
+    /**
+     * Retrieves CSS selectors for all active element hiding filters.
+     * @param domain Domain to retrieve CSS selectors for.

[Wladimir Palant, 2014/08/29 18:18:50]

What is the relevance of the domain parameter? This isn't clear from the
documentation. "Retrieves CSS selectors for all element hiding filters that
apply to a particular domain"?

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+     * @return List of CSS selectors.
+     */
     std::vector<std::string> GetElementHidingSelectors(const std::string& domain) const;
+
+    /**
+     * Retrieves a preference value.
+     * @param pref Preference name.
+     * @return Preference value, or `null` if it doesn't exist.
+     */
     JsValuePtr GetPref(const std::string& pref) const;
+
+    /**
+     * Sets a preference value.
+     * @param pref Preference name.
+     * @param value New value of the preference.
+     */
     void SetPref(const std::string& pref, JsValuePtr value);
+
+    /**
+     * Extracts the host from an URL.

[Wladimir Palant, 2014/08/29 18:18:50]

"from a URL" please.

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+     * @param url URL to extract the host from.
+     * @return Extracted host.
+     */
     std::string GetHostFromURL(const std::string& url);
+
+    /**
+     * Forces an immediate update check.
+     * @param Optional callback to invoke when updates are available.

[Wladimir Palant, 2014/08/29 18:18:50]

Parameter name is missing here.

The callback will be called once the update check is done, not when the updates
are available. It is also important to note that the callback will either get ""
as parameter (success) or an error message.

Note that "success" here merely means that the check was performed correctly. If
an update is actually available then the "updateAvailable" event will be
triggered - same as for automatic update checks.

Probably worth mentioning that the application doesn't need to poll this method
- automatic update checks are always running, this method is only for the user
checking for updates manually.

[Felix Dahlke, 2014/09/01 17:00:15]

Done.

+     */
     void ForceUpdateCheck(UpdaterCallback callback = 0);
+
+    /**
+     * Sets the callback invoked when the filters change.
+     * @param The callback to invoke.

[Wladimir Palant, 2014/08/29 18:18:50]

Parameter name is missing here.

Want to note that currently only one callback can exist?

[Felix Dahlke, 2014/09/01 17:00:15]

Added the parameter. Seems obvious to me that there can be only one - it's
"set", not "add" after all.

+     */
     void SetFilterChangeCallback(FilterChangeCallback callback);
+
+    /**
+     * Removes the callback invoked when the filters change.
+     */
     void RemoveFilterChangeCallback();
+
+    /**
+     * Compares two version strings in
+     * [Mozilla toolkit version format](https://developer.mozilla.org/en/docs/Toolkit_version_format).
+     * @param v1 First version string.
+     * @param v2 Second version string.
+     * @return
+     *         - `0` if `v1` and `v2` are identical.
+     *         - A negative number if `v1` is less than `v2`.
+     *         - A positive number if `v1` is greater than `v2`.
+     */
     int CompareVersions(const std::string& v1, const std::string& v2);
 
   private: