Issue 116 - Document the libadblockplus API

Issue 116 - Document the libadblockplus API

[Felix Dahlke, 2014-08-28 16:29:14]

This is still WIP, but I'd still like to get some early feedback. I've
documented AppInfo and JsEngine so far, please review those.

I've tried to keep the documentation style similar to the JSDoc we have in the
Adblock Plus core code - I suppose the major difference is that I used periods
for consistency (we need to use periods for the brief when using autobrief).

Let me know if you think it's too verbose or something.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/Doxy...
File Doxyfile (right):

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/Doxy...
Doxyfile:1: # Doxyfile 1.8.6
This was generated automatically by Doxygen, so it's mostly default. I'll point
out the parts I changed below.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/Doxy...
Doxyfile:35: PROJECT_NAME           = "Libadblockplus"
This was changed, naturally.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/Doxy...
Doxyfile:61: OUTPUT_DIRECTORY       = build/documentation
This was changed. Note that Doxygen will fail if the "build" dir does not exist
- we might want to consider a toplevel directory instead.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/Doxy...
Doxyfile:172: JAVADOC_AUTOBRIEF      = YES
I've changed this to yes - that way our Doxygen and JSDoc comments can be fairly
close. One quirk with Doxygen is that the brief ends after the first period
followed by a space - so you'll see me escape spaces sometimes.

The alternative would be to have explicit @brief tags, do you think that'd be
better?

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/Doxy...
Doxyfile:211: TAB_SIZE               = 2
Changed this to comply with our coding style.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/Doxy...
Doxyfile:333: DISTRIBUTE_GROUP_DOC   = YES
I've enabled this to make it possible to give multiple functions the same
comment. The only example of this so far is JsEngine::NewValue.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/Doxy...
Doxyfile:746: INPUT                  = include
Only added the include directory here - I don't think we want to document any
non-public API.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/Doxy...
Doxyfile:772: RECURSIVE              = YES
Enabled this.

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

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/Doxy...
File Doxyfile (right):

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/Doxy...
Doxyfile:1: # Doxyfile 1.8.6
On 2014/08/28 16:29:14, Felix H. Dahlke wrote:
> This was generated automatically by Doxygen, so it's mostly default. I'll
point
> out the parts I changed below.

Do we really want the generic parts here? It isn't really maintainable this way
- I'd rather keep only the relevant settings and drop all the documentation
which duplicates content that can be easily found on the web. See for example
http://www.stack.nl/~dimitri/doxygen/manual/config.html#cfg_doxyfile_encoding -
|DOXYFILE_ENCODING = UTF-8| is the default value. We can still have our own
comments to explain why we changed some setting.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/Doxy...
Doxyfile:172: JAVADOC_AUTOBRIEF      = YES
On 2014/08/28 16:29:14, Felix H. Dahlke wrote:
> The alternative would be to have explicit @brief tags, do you think that'd be
> better?

No, specifying this explicitly causes significant and IMHO unnecessary overhead.
Taking the first sentence sounds like a good heuristic to me. From what I
remember, that's what JavaDoc actually does.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/Doxy...
Doxyfile:211: TAB_SIZE               = 2
On 2014/08/28 16:29:14, Felix H. Dahlke wrote:
> Changed this to comply with our coding style.

Should actually be irrelevant since we aren't supposed to use tabs per our
coding style :) - but won't hurt of course.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
File include/AdblockPlus/AppInfo.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/AppInfo.h:33: std::string id;
Didn't we want to remove this? This is the ID used by the platform to identify
the app, it makes fairly little sense in libadblockplus... The only place where
this is actually used seems to be the code integrating with the Firefox Add-ons
Manager.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/AppInfo.h:42: * Name of the app, e.g.\ _adblockplus_.
This should stress the fact that it is a codename, not the user-visible name.
Also, please don't give embedders the idea to actually use "adblockplus" here -
"mygreatapp" should be better.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/AppInfo.h:47: * Name of the platform, e.g.\ _msie64_.
"platform that the app is running on"

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
File include/AdblockPlus/JsEngine.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:51: * JavaScript engine used by `FilterEngine`,
uses v8.
"uses v8" => "wraps v8"?

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:71: * @param appInfo AppInfo Information about
the app, used for update checks.
"AppInfo" is unnecessary here?

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:78: * @param eventName Name of the event.
Here and below, why not just "Event name" and "Event parameters"?

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:98: * @param source JavaScript expression to
evalute.
"evaluate"

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:107: * Initiates a garbage collection.
I don't think you can use an article here...

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:143: * Creates a new JavaScript callback.
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.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:173: * instance by default, which should normally
be sufficient.
"normally be sufficient"? "might be sufficient" is more like it :)

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:197: * Sets the `LogSystem` implementation used
for logging (e.g. console.log).
"e.g. to handle console.log() calls from JavaScript"

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:200: * @param The `FileSystem` instance to use.
FileSystem => LogSystem?

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

New patch set. I'll add more documentation and update this review once I'm done
with the things I want to document for #116.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/Doxy...
File Doxyfile (right):

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/Doxy...
Doxyfile:1: # Doxyfile 1.8.6
On 2014/08/28 17:24:40, Wladimir Palant wrote:
> On 2014/08/28 16:29:14, Felix H. Dahlke wrote:
> > This was generated automatically by Doxygen, so it's mostly default. I'll
> point
> > out the parts I changed below.
> 
> Do we really want the generic parts here? It isn't really maintainable this
way
> - I'd rather keep only the relevant settings and drop all the documentation
> which duplicates content that can be easily found on the web. See for example
>
http://www.stack.nl/%7Edimitri/doxygen/manual/config.html#cfg_doxyfile_encoding
-
> |DOXYFILE_ENCODING = UTF-8| is the default value. We can still have our own
> comments to explain why we changed some setting.

Yes, good point, there is no way we can keep track of our changes like this.
Replaced this with a tiny file setting only the stuff we change. I don't see any
point in repeating documentation - every option can be looked up.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
File include/AdblockPlus/AppInfo.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/AppInfo.h:33: std::string id;
On 2014/08/28 17:24:40, Wladimir Palant wrote:
> Didn't we want to remove this? This is the ID used by the platform to identify
> the app, it makes fairly little sense in libadblockplus... The only place
where
> this is actually used seems to be the code integrating with the Firefox
Add-ons
> Manager.

Yup, came to the same conclusion, but I'd rather not mix such a change with
adding documentation. Created #1284 for that.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/AppInfo.h:42: * Name of the app, e.g.\ _adblockplus_.
On 2014/08/28 17:24:40, Wladimir Palant wrote:
> This should stress the fact that it is a codename, not the user-visible name.
> Also, please don't give embedders the idea to actually use "adblockplus" here
-
> "mygreatapp" should be better.

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/AppInfo.h:47: * Name of the platform, e.g.\ _msie64_.
On 2014/08/28 17:24:40, Wladimir Palant wrote:
> "platform that the app is running on"

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
File include/AdblockPlus/JsEngine.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:51: * JavaScript engine used by `FilterEngine`,
uses v8.
On 2014/08/28 17:24:40, Wladimir Palant wrote:
> "uses v8" => "wraps v8"?

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:71: * @param appInfo AppInfo Information about
the app, used for update checks.
On 2014/08/28 17:24:40, Wladimir Palant wrote:
> "AppInfo" is unnecessary here?

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:78: * @param eventName Name of the event.
On 2014/08/28 17:24:40, Wladimir Palant wrote:
> Here and below, why not just "Event name" and "Event parameters"?

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:98: * @param source JavaScript expression to
evalute.
On 2014/08/28 17:24:40, Wladimir Palant wrote:
> "evaluate"

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:107: * Initiates a garbage collection.
On 2014/08/28 17:24:40, Wladimir Palant wrote:
> I don't think you can use an article here...

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

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:143: * Creates a new JavaScript callback.
On 2014/08/28 17:24:40, Wladimir Palant wrote:
> 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.

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:173: * instance by default, which should normally
be sufficient.
On 2014/08/28 17:24:40, Wladimir Palant wrote:
> "normally be sufficient"? "might be sufficient" is more like it :)

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:197: * Sets the `LogSystem` implementation used
for logging (e.g. console.log).
On 2014/08/28 17:24:40, Wladimir Palant wrote:
> "e.g. to handle console.log() calls from JavaScript"

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5629499534213120/incl...
include/AdblockPlus/JsEngine.h:200: * @param The `FileSystem` instance to use.
On 2014/08/28 17:24:40, Wladimir Palant wrote:
> FileSystem => LogSystem?

Done.

[Wladimir Palant, 2014-08-29 06:23:49]

Looks good now.

[Felix Dahlke, 2014-08-29 13:18:27]

Documented JsValue and FilterEngine, more feedback please.

Please note that I'm being intentionally brief (leaving out articles etc. where
I think it's possible), but I still want all of this to be proper English, so
please nit pick if you notice something.

[Felix Dahlke, 2014-08-29 16:38:24]

Alright, now everything is documented, so this is not WIP anymore and officially
under review :)

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

Done reviewing Patch Set 3, this should keep you busy while I review the rest...

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
File include/AdblockPlus.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus.h:20: * This is the source code documentation of
Libadblockplus. See the README for
"README file"? Not sure how helpful this hint is however - if these are
pregenerated docs somewhere on the web then the link to the README file is
missing. If some developer generated the docs himself then he most likely read
the README file already.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
File include/AdblockPlus/FilterEngine.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:35: * Wrapper for a filter object.
"Adblock Plus filter object"? Just to make it clear that this is ABP
functionality, as opposed to V8 for example.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:48: Type GetType();
I think this method should be documented ;)

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:52: * @return `true` if this filter has been
added to its subscription.
No, "Checks whether this filter has been added to the list of custom filters."
Filters in subscriptions aren't considered here.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:57: * Adds this filter to its subscription.
No, "Adds this filter to user's custom filters."

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:62: * Removes this filter from its
subscription.
No, "Removes this filter from user's custom filters."

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:68: Filter(JsValuePtr value);
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."

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:81: *         subscriptions.
What are "known subscriptions"? "Checks if the subscription has been added to
user's list of subscriptions"?

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:86: * Adds this subscription to the known
subscriptions.
"Adds this subscription to user's list of subscriptions"?

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:91: * Removes this subscription from the
known subscriptions.
"Removes this subscription from user's list of subscriptions"?

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:96: * Updates this subscription, i.e.\
retrieves the latest version from the
"latest version" => "current filters"?

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:109: Subscription(JsValuePtr value);
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."

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:133: * Callback invoked when an update is
available.
"Callback" => "Type of callback"?

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

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:138: * Callback invoked when the filters
change.
"Callback" => "Type of callback"?

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

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:143: JsEnginePtr GetJsEngine() const { return
jsEngine; }
I guess you plan to document these two functions later?

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:146: * Checks if this is the first run of the
application.
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.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:156: *         existing one.
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.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:164: *         otherwise the existing one.
Same as above, IMHO we should just say that a subscription object is returned.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:170: * @return List of known filters.
What are "known filters"? "Retrieves user's custom filters"?

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:176: * @return List of known subscriptions.
What are "known subscriptions"? "Retrieves user's list of subscriptions"?

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:182: * This will parse all subscriptions
listed in `subscriptions.xml`.
Why document implementation details? There is no subscriptions.xml in
libadblockplus, only a JavaScript structure created from it - but nobody should
care.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:207: * @return Matching filter, or `null` if
there was no match.
Strictly speaking, is it really null? It's an empty FilterPtr instance, will it
equal null?

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:229: * @param documentUrls Chain of documents
requesting the resource.
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."

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:241: * @param domain Domain to retrieve CSS
selectors for.
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"?

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:261: * Extracts the host from an URL.
"from a URL" please.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:269: * @param Optional callback to invoke
when updates are available.
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.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:275: * @param The callback to invoke.
Parameter name is missing here.

Want to note that currently only one callback can exist?

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
File include/AdblockPlus/JsEngine.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/JsEngine.h:81: * Registers the callback function for an
event.
Want to note that currently only one callback can be registered per event?

Probably also a good idea to document the possible events here. Currently the
only event the application would register for directly is "updateAvailable". It
is called with the URL of the update download as the only parameter.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
File include/AdblockPlus/JsValue.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/JsValue.h:51: * Wrapper for JavaScript objects.
"JavaScript values"? This can wrap primitive values as well.

[Wladimir Palant, 2014-08-29 18:42:27]

Reviewed the newer changes as well now.

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
File include/AdblockPlus/DefaultFileSystem.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
include/AdblockPlus/DefaultFileSystem.h:33: * system's file system.
"All paths are considered relative to the base path or to the current work
directory if no base path is set."

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
include/AdblockPlus/DefaultFileSystem.h:46: void SetBasePath(const std::string&
path);
This method needs documenting, it isn't part of the interface.

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
File include/AdblockPlus/DefaultWebRequest.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
include/AdblockPlus/DefaultWebRequest.h:27: * on other systems. A dummy
implementation is used if libcurl is not
"other platforms"?

Also, "A dummy implementation always reporting failure"?

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
File include/AdblockPlus/ReferrerMapping.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
include/AdblockPlus/ReferrerMapping.h:31: * (see `BuildReferrerChain()`), which
approximates the frame structure.
see FilterEngine::Matches()?

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
include/AdblockPlus/ReferrerMapping.h:45: * Adds a new mapping from URL to
referrer.
"Records the referrer for a URL"?

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
include/AdblockPlus/ReferrerMapping.h:52: * Builds the referrer chain for the
supplied URL.
"Reconstructs the URLs of parent frames for a document URL"?

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
File include/AdblockPlus/WebRequest.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
include/AdblockPlus/WebRequest.h:41: * Status code of the response.
[Mozilla status
code](https://developer.mozilla.org/en/docs/Table_Of_Errors#Network_Errors)
indicating the network-level request status. This should be 0 (NS_OK) if the
request succeeded. Note that this should be NS_OK if the server responded with
an error code like "404 Not Found".

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
include/AdblockPlus/WebRequest.h:56: * HTTP status of the response.
" like 200 or 404"?

[Felix Dahlke, 2014-09-01 17:00:14]

Addressed all comments!

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
File include/AdblockPlus.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus.h:20: * This is the source code documentation of
Libadblockplus. See the README for
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> "README file"? Not sure how helpful this hint is however - if these are
> pregenerated docs somewhere on the web then the link to the README file is
> missing. If some developer generated the docs himself then he most likely read
> the README file already.

Yeah, it's for the web readable documentation. It does make sense to link to the
README, but I wasn't sure if it's a good idea to do that before we have
RhodeCode or something similar. But some link is better than none - I'm linking
to the README via hg.adblockplus.org now.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
File include/AdblockPlus/FilterEngine.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:35: * Wrapper for a filter object.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> "Adblock Plus filter object"? Just to make it clear that this is ABP
> functionality, as opposed to V8 for example.

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:48: Type GetType();
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> I think this method should be documented ;)

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:52: * @return `true` if this filter has been
added to its subscription.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> No, "Checks whether this filter has been added to the list of custom filters."
> Filters in subscriptions aren't considered here.

Done, seems I misinterpreted FilterStorage.addFilter here.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:57: * Adds this filter to its subscription.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> No, "Adds this filter to user's custom filters."

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:62: * Removes this filter from its
subscription.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> No, "Removes this filter from user's custom filters."

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:68: Filter(JsValuePtr value);
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> 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."

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:81: *         subscriptions.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> What are "known subscriptions"? "Checks if the subscription has been added to
> user's list of subscriptions"?

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:86: * Adds this subscription to the known
subscriptions.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> "Adds this subscription to user's list of subscriptions"?

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:91: * Removes this subscription from the
known subscriptions.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> "Removes this subscription from user's list of subscriptions"?

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:96: * Updates this subscription, i.e.\
retrieves the latest version from the
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> "latest version" => "current filters"?

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:109: Subscription(JsValuePtr value);
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> 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."

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:133: * Callback invoked when an update is
available.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> "Callback" => "Type of callback"?
> 
> Want to note that the parameter here is an optional error message?

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:138: * Callback invoked when the filters
change.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> "Callback" => "Type of callback"?
> 
> Want to note that the parameters here are the change type and the
> Filter/Subscription object affected if any?

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:143: JsEnginePtr GetJsEngine() const { return
jsEngine; }
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> I guess you plan to document these two functions later?

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

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:146: * Checks if this is the first run of the
application.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> 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.

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.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:156: *         existing one.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> 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.

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:164: *         otherwise the existing one.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> Same as above, IMHO we should just say that a subscription object is returned.

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:170: * @return List of known filters.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> What are "known filters"? "Retrieves user's custom filters"?

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:176: * @return List of known subscriptions.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> What are "known subscriptions"? "Retrieves user's list of subscriptions"?

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:182: * This will parse all subscriptions
listed in `subscriptions.xml`.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> Why document implementation details? There is no subscriptions.xml in
> libadblockplus, only a JavaScript structure created from it - but nobody
should
> care.

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:207: * @return Matching filter, or `null` if
there was no match.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> Strictly speaking, is it really null? It's an empty FilterPtr instance, will
it
> equal null?

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.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:229: * @param documentUrls Chain of documents
requesting the resource.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> 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."

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

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:241: * @param domain Domain to retrieve CSS
selectors for.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> 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"?

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:261: * Extracts the host from an URL.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> "from a URL" please.

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:269: * @param Optional callback to invoke
when updates are available.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> 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.

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:275: * @param The callback to invoke.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> Parameter name is missing here.
> 
> Want to note that currently only one callback can exist?

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

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
File include/AdblockPlus/DefaultFileSystem.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
include/AdblockPlus/DefaultFileSystem.h:33: * system's file system.
On 2014/08/29 18:42:27, Wladimir Palant wrote:
> "All paths are considered relative to the base path or to the current work
> directory if no base path is set."

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
include/AdblockPlus/DefaultFileSystem.h:46: void SetBasePath(const std::string&
path);
On 2014/08/29 18:42:27, Wladimir Palant wrote:
> This method needs documenting, it isn't part of the interface.

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
File include/AdblockPlus/DefaultWebRequest.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
include/AdblockPlus/DefaultWebRequest.h:27: * on other systems. A dummy
implementation is used if libcurl is not
On 2014/08/29 18:42:27, Wladimir Palant wrote:
> "other platforms"?
> 
> Also, "A dummy implementation always reporting failure"?

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
File include/AdblockPlus/ReferrerMapping.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
include/AdblockPlus/ReferrerMapping.h:31: * (see `BuildReferrerChain()`), which
approximates the frame structure.
On 2014/08/29 18:42:27, Wladimir Palant wrote:
> see FilterEngine::Matches()?

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
include/AdblockPlus/ReferrerMapping.h:45: * Adds a new mapping from URL to
referrer.
On 2014/08/29 18:42:27, Wladimir Palant wrote:
> "Records the referrer for a URL"?

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
include/AdblockPlus/ReferrerMapping.h:52: * Builds the referrer chain for the
supplied URL.
On 2014/08/29 18:42:27, Wladimir Palant wrote:
> "Reconstructs the URLs of parent frames for a document URL"?

I've changed the comment to include this, but I feel your take on it didn't make
it clear how this works - and how limited it is. I'd rather document the "how it
works" rather than the "what we use it for".

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
File include/AdblockPlus/WebRequest.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
include/AdblockPlus/WebRequest.h:41: * Status code of the response.
On 2014/08/29 18:42:27, Wladimir Palant wrote:
> [Mozilla status
> code](https://developer.mozilla.org/en/docs/Table_Of_Errors#Network_Errors)
> indicating the network-level request status. This should be 0 (NS_OK) if the
> request succeeded. Note that this should be NS_OK if the server responded with
> an error code like "404 Not Found".

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5718998062727168/incl...
include/AdblockPlus/WebRequest.h:56: * HTTP status of the response.
On 2014/08/29 18:42:27, Wladimir Palant wrote:
> " like 200 or 404"?

Done.

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

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
File include/AdblockPlus/FilterEngine.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:146: * Checks if this is the first run of the
application.
On 2014/09/01 17:00:15, Felix H. Dahlke wrote:
> Libadblockplus does initialise filter lists internally when first run,

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.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:207: * @return Matching filter, or `null` if
there was no match.
On 2014/09/01 17:00:15, Felix H. Dahlke wrote:
> 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.

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...

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
File include/AdblockPlus/JsEngine.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/JsEngine.h:81: * Registers the callback function for an
event.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> Want to note that currently only one callback can be registered per event?
> 
> Probably also a good idea to document the possible events here. Currently the
> only event the application would register for directly is "updateAvailable".
It
> is called with the URL of the update download as the only parameter.

While I see that "set" makes it fairly obvious that there can be only one event
handler, what about documenting available event types? These need to be
documented somewhere, and this looks like the logical place.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
File include/AdblockPlus/JsValue.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/JsValue.h:51: * Wrapper for JavaScript objects.
On 2014/08/29 18:18:50, Wladimir Palant wrote:
> "JavaScript values"? This can wrap primitive values as well.

This comment hasn't been addressed.

http://codereview.adblockplus.org/5728380594946048/diff/5068993417183232/incl...
File include/AdblockPlus.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5068993417183232/incl...
include/AdblockPlus.h:21: *
[README](https://hg.adblockplus.org/libadblockplus/file/default/README.md/)
Nit: Please remove trailing slash.

http://codereview.adblockplus.org/5728380594946048/diff/5068993417183232/incl...
File include/AdblockPlus/FilterEngine.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5068993417183232/incl...
include/AdblockPlus/FilterEngine.h:74: * Normally you shouldn't call this
directly, but use GetFilter() instead.
It's FilterEngine::GetFilter(), not Filter::GetFilter().

http://codereview.adblockplus.org/5728380594946048/diff/5068993417183232/incl...
include/AdblockPlus/FilterEngine.h:119: * Normally you shouldn't call this
directly, but use GetSubscription()
It's FilterEngine::GetSubscription(), not Subscription::GetSubscription().

[Felix Dahlke, 2014-09-02 05:24:50]

New patch set.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
File include/AdblockPlus/FilterEngine.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/FilterEngine.h:207: * @return Matching filter, or `null` if
there was no match.
On 2014/09/01 17:36:56, Wladimir Palant wrote:
> On 2014/09/01 17:00:15, Felix H. Dahlke wrote:
> > 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.
> 
> 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...

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.

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
File include/AdblockPlus/JsEngine.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/JsEngine.h:81: * Registers the callback function for an
event.
On 2014/09/01 17:36:56, Wladimir Palant wrote:
> On 2014/08/29 18:18:50, Wladimir Palant wrote:
> > Want to note that currently only one callback can be registered per event?
> > 
> > Probably also a good idea to document the possible events here. Currently
the
> > only event the application would register for directly is "updateAvailable".
> It
> > is called with the URL of the update download as the only parameter.
> 
> While I see that "set" makes it fairly obvious that there can be only one
event
> handler, what about documenting available event types? These need to be
> documented somewhere, and this looks like the logical place.

I don't think it's a good place actually. Logically, FilterEngine (or the JS
code it runs) throws the specific events. As far as JsEngine is concerned, this
is just a general purpose event handling mechanism. Made this a bit clearer in
the comment.

Documenting "updateAvailable" is easy - clients want to use it anyway, so we
should add a setter for it:

https://issues.adblockplus.org/ticket/1296

As for "init" and "updateCheckDone*" -> that's more tricky. We should probably
not document either, since Libadblockplus uses them internally - it will only
end in tears if clients interfere with that. I suggest we prefix them with "_":

https://issues.adblockplus.org/ticket/1297

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
File include/AdblockPlus/JsValue.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5738600293466112/incl...
include/AdblockPlus/JsValue.h:51: * Wrapper for JavaScript objects.
On 2014/09/01 17:36:56, Wladimir Palant wrote:
> On 2014/08/29 18:18:50, Wladimir Palant wrote:
> > "JavaScript values"? This can wrap primitive values as well.
> 
> This comment hasn't been addressed.

Oops, done.

http://codereview.adblockplus.org/5728380594946048/diff/5068993417183232/incl...
File include/AdblockPlus.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5068993417183232/incl...
include/AdblockPlus.h:21: *
[README](https://hg.adblockplus.org/libadblockplus/file/default/README.md/)
On 2014/09/01 17:36:56, Wladimir Palant wrote:
> Nit: Please remove trailing slash.

It's deliberate actually - for some reason Doxygen won't render a proper link
here unless I add the trailing slash. But it's probably unnecessarily magical
that way, I've replaced it with a plain HTML link.

http://codereview.adblockplus.org/5728380594946048/diff/5068993417183232/incl...
File include/AdblockPlus/FilterEngine.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5068993417183232/incl...
include/AdblockPlus/FilterEngine.h:74: * Normally you shouldn't call this
directly, but use GetFilter() instead.
On 2014/09/01 17:36:56, Wladimir Palant wrote:
> It's FilterEngine::GetFilter(), not Filter::GetFilter().

Done.

http://codereview.adblockplus.org/5728380594946048/diff/5068993417183232/incl...
include/AdblockPlus/FilterEngine.h:119: * Normally you shouldn't call this
directly, but use GetSubscription()
On 2014/09/01 17:36:56, Wladimir Palant wrote:
> It's FilterEngine::GetSubscription(), not Subscription::GetSubscription().

Done.

[Wladimir Palant, 2014-09-02 13:11:21]

http://codereview.adblockplus.org/5728380594946048/diff/5678807906254848/incl...
File include/AdblockPlus/FilterEngine.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5678807906254848/incl...
include/AdblockPlus/FilterEngine.h:112: * @return `true` if subscriptions are
currently being updated.
This is misleading now - the mistake was actually the plural form, should have
been "subscription". "Checks whether this subscription is currently being
updated" - this state is per-subscription.

[Felix Dahlke, 2014-09-02 13:24:05]

http://codereview.adblockplus.org/5728380594946048/diff/5678807906254848/incl...
File include/AdblockPlus/FilterEngine.h (right):

http://codereview.adblockplus.org/5728380594946048/diff/5678807906254848/incl...
include/AdblockPlus/FilterEngine.h:112: * @return `true` if subscriptions are
currently being updated.
On 2014/09/02 13:11:21, Wladimir Palant wrote:
> This is misleading now - the mistake was actually the plural form, should have
> been "subscription". "Checks whether this subscription is currently being
> updated" - this state is per-subscription.

Double ouch. Done.

[Felix Dahlke, 2014-09-02 14:19:33]

One more patch set: I've been writing libadblockplus/Libadblockplus
inconsistently, the official name should be lower case IMO, so I've changed
that. (We only use upper case for the module name now, which is fine since all
modules start with an upper case letter.)

[Wladimir Palant, 2014-09-02 14:52:01]

LGTM