Issue 116 - Document the libadblockplus API

include/AdblockPlus/FilterEngine.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 14 matching lines @@
 #include <AdblockPlus/JsEngine.h>
 #include <AdblockPlus/JsValue.h>
 
 #include "tr1_memory.h"
 
 namespace AdblockPlus
 {
   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.

+   * Wrapper for an Adblock Plus filter object.
    */
   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};
 
+    /**
+     * Retrieves the type of this filter.
+     * @return Type of this filter.
+     */
     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.

+     * Checks whether this filter has been added to the list of custom filters.
+     * @return `true` if this filter has been added.
      */
     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.

+     * Adds this filter to the list of custom filters.
      */
     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.

+     * Removes this filter from the list of custom filters.
      */
     void RemoveFromList();
 
     bool operator==(const Filter& filter) const;
 
+    /**
+     * Creates a wrapper for an existing JavaScript filter object.
+     * Normally you shouldn't call this directly, but use
+     * FilterEngine::GetFilter() instead.
+     * @param value JavaScript filter object.
+     */
     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.

+     * Checks if this subscription has been added to the list of subscriptions.
+     * @return `true` if this subscription has been added.
      */
     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.

+     * Adds this subscription to the list of subscriptions.
      */
     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.

+     * Removes this subscription from the list of subscriptions.
      */
     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.

+     * Updates this subscription, i.e.\ retrieves the current filters from the
      * subscription URL.
      */
     void UpdateFilters();
 
     /**
-     * Checks if this subscriptions is currently being updated.
-     * @return `true` if this subscriptions is currently being updated.
+     * Checks if the subscription is currently being updated.
+     * @return `true` if the subscription is currently being updated.
      */
     bool IsUpdating();
 
     bool operator==(const Subscription& subscription) const;
 
+    /**
+     * Creates a wrapper for an existing JavaScript subscription object.
+     * Normally you shouldn't call this directly, but use
+     * FilterEngine::GetSubscription() instead.
+     * @param value JavaScript subscription object.
+     */
     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.
+   * 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.

+     * Callback type invoked when an update is available.
+     * The parameter is optional error message.
      */
     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.

+     * Callback type invoked when the filters change.
+     * The first parameter is the action event code (see
+     * [FilterNotifier.triggerListeners](https://adblockplus.org/jsdoc/adblockplus/symbols/FilterNotifier.html#.triggerListeners)
+     * for the full list).
+     * The second parameter is the filter/subscription object affected, if any.
      */
     typedef std::tr1::function<void(const std::string&, const JsValuePtr)> FilterChangeCallback;
 
+    /**
+     * Constructor.
+     * @param jsEngine `JsEngine` instance used to run JavaScript code
+     *        internally.
+     */
     explicit FilterEngine(JsEnginePtr jsEngine);
+
+    /**
+     * Retrieves the `JsEngine` instance associated with this `FilterEngine`
+     * instance.
+     */
     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.

+     * @return New `Filter` instance.
      */
     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.

+     * @return New `Subscription` instance.
      */
     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.

+     * Retrieves the list of custom filters.
+     * @return List of custom filters.
      */
     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.

+     * Retrieves all subscriptions.
+     * @return List of subscriptions.
      */
     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.
+     * @param documentUrls Chain of documents requesting the resource, starting
+     *        with the current resource's parent frame, ending with the
+     *        top-level frame.
+     *        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 a `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.
+     * Retrieves CSS selectors for all element hiding filters active on the
+     * supplied domain.
      * @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.

+     * Extracts the host from a URL.
      * @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.

+     * `FilterEngine` will automatically check for updates in regular intervals,
+     * so applications should only call this when the user triggers an update
+     * check manually.
+     * @param callback Optional callback to invoke when the update check is
+     *        finished. The string parameter will be empty when the update check
+     *        succeeded, or contain an error message if it failed.
+     *        Note that the callback will be invoked after every update check -
+     *        to react to updates being available, register a callback for the
+     *        "updateAvailable" event (see JsEngine::SetEventCallback()).
      */
     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.

+     * @param callback Callback to invoke.
      */
     void SetFilterChangeCallback(FilterChangeCallback callback);
 
     /**
      * Removes the callback invoked when the filters change.
      */
     void RemoveFilterChangeCallback();
 
     /**
      * Compares two version strings in
@@ skipping 16 matching lines @@
     void InitDone(JsValueList& params);
     FilterPtr CheckFilterMatch(const std::string& url,
                                const std::string& contentType,
                                const std::string& documentUrl) const;
     void UpdateCheckDone(const std::string& eventName, UpdaterCallback callback, JsValueList& params);
     void FilterChanged(FilterChangeCallback callback, JsValueList& params);
   };
 }
 
 #endif