Chromium extension API - written as part of course in Open Software Development at Reykjavik Univer…

Chromium extension API - written as part of course in Open Software Development at Reykjavik University - summer 2013

Authors:
Hans Petur Jonsson
Einar Tryggvi Leifsson

Instructor:
Joi Sigurdsson

Description:
The extension API returns the current time for perusal for Chrome extensions.

BUG=

[hpjonsson, 2013-06-27 13:08:42]

Sælir,

Hér er draft af Chromium extension API sem við settum saman til að nota með
Watchpocket Chrome viðbótinni.

Kveðja,
Hans Pétur

[hpjonsson, 2013-06-27 17:30:48]

Jæja, já.

[Jói, 2013-06-28 10:49:15]

Please see review comments below.  Overall looks good.

As an FYI to any Chromium folks who might see this:  This is for a school
project, not intended for check-in.

Cheers,
Jói

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/api/time/time_api.cc (right):

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2013

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.cc:4: #include
"chrome/browser/extensions/api/time/time_api.h"
We put a blank line after including our "own" header.

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.cc:28: std::strftime(buffer, 80,
format.c_str(), timeinfo);
80 -> arraysize(buffer)

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.cc:31: result.time_string = buffer;
indent -2

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.cc:41: }  // namespace extensions
Usually we put a blank line before closing the namespace.

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/api/time/time_api.h (right):

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2013

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.h:9: 
One blank line is enough.

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.h:11: namespace extensions {
Usually we put a blank line after this.

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.h:13: private:
indent -1

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.h:14: virtual
~TimeGetTimeFunction();
Not sure, but the destructor probably needs to be public. If it compiles like
this, fine.

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.h:17: };
indent -2

https://codereview.chromium.org/18063003/diff/7001/chrome/chrome_browser_exte...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/18063003/diff/7001/chrome/chrome_browser_exte...
chrome/chrome_browser_extensions.gypi:771:
'browser/extensions/api/time/time_api.h',
Indentation is off.

https://codereview.chromium.org/18063003/diff/7001/chrome/common/extensions/a...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/18063003/diff/7001/chrome/common/extensions/a...
chrome/common/extensions/api/_api_features.json:481: "time": {
Indent -2.

https://codereview.chromium.org/18063003/diff/7001/chrome/common/extensions/a...
File chrome/common/extensions/api/api.gyp (right):

https://codereview.chromium.org/18063003/diff/7001/chrome/common/extensions/a...
chrome/common/extensions/api/api.gyp:99: 'time.idl',
Indentation is off. Note, we use 2 spaces to indent, not tabs.

https://codereview.chromium.org/18063003/diff/7001/chrome/common/extensions/a...
File chrome/common/extensions/api/time.idl (right):

https://codereview.chromium.org/18063003/diff/7001/chrome/common/extensions/a...
chrome/common/extensions/api/time.idl:5: // Use the <code>chrome.time</code> API
to get the current time
Generally we don't use HTML or other markup in comments. By convention you would
sometimes put | around code or variable names, e.g. |chrome.time|.

https://codereview.chromium.org/18063003/diff/7001/chrome/test/data/prerender...
File chrome/test/data/prerender/prerender_excessive_memory.html (left):

https://codereview.chromium.org/18063003/diff/7001/chrome/test/data/prerender...
chrome/test/data/prerender/prerender_excessive_memory.html:1: <html>
Why did you delete this file?

[einartryggvi, 2013-06-28 10:57:49]

lgtm

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/api/time/time_api.h (right):

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.h:14: virtual
~TimeGetTimeFunction();
We got an error saying a virtual destructor has to be private, thought it was
weird though.

On 2013/06/28 10:49:15, Jói wrote:
> Not sure, but the destructor probably needs to be public. If it compiles like
> this, fine.

[hpjonsson, 2013-06-28 11:27:32]

https://codereview.chromium.org/18063003/diff/7001/chrome/test/data/prerender...
File chrome/test/data/prerender/prerender_excessive_memory.html (left):

https://codereview.chromium.org/18063003/diff/7001/chrome/test/data/prerender...
chrome/test/data/prerender/prerender_excessive_memory.html:1: <html>
It was AVG Virus protection that did it and it escaped our attention - thanks
Jói.
On 2013/06/28 10:49:15, Jói wrote:
> Why did you delete this file?

[hpjonsson, 2013-06-28 12:13:04]

Thanks Jói, I think we addressed all the action points so far?

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/api/time/time_api.cc (right):

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/06/28 10:49:15, Jói wrote:
> 2013

Done.

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.cc:4: #include
"chrome/browser/extensions/api/time/time_api.h"
On 2013/06/28 10:49:15, Jói wrote:
> We put a blank line after including our "own" header.

Done.

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.cc:28: std::strftime(buffer, 80,
format.c_str(), timeinfo);
On 2013/06/28 10:49:15, Jói wrote:
> 80 -> arraysize(buffer)

Done.

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.cc:31: result.time_string = buffer;
On 2013/06/28 10:49:15, Jói wrote:
> indent -2

Done.

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.cc:41: }  // namespace extensions
On 2013/06/28 10:49:15, Jói wrote:
> Usually we put a blank line before closing the namespace.

Done.

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/api/time/time_api.h (right):

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/06/28 10:49:15, Jói wrote:
> 2013

Done.

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.h:9: 
On 2013/06/28 10:49:15, Jói wrote:
> One blank line is enough.

Done.

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.h:11: namespace extensions {
On 2013/06/28 10:49:15, Jói wrote:
> Usually we put a blank line after this.

Done.

https://codereview.chromium.org/18063003/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.h:17: };
On 2013/06/28 10:49:15, Jói wrote:
> indent -2

Done.

https://codereview.chromium.org/18063003/diff/7001/chrome/chrome_browser_exte...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/18063003/diff/7001/chrome/chrome_browser_exte...
chrome/chrome_browser_extensions.gypi:771:
'browser/extensions/api/time/time_api.h',
On 2013/06/28 10:49:15, Jói wrote:
> Indentation is off.

Done.

https://codereview.chromium.org/18063003/diff/7001/chrome/common/extensions/a...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/18063003/diff/7001/chrome/common/extensions/a...
chrome/common/extensions/api/_api_features.json:481: "time": {
On 2013/06/28 10:49:15, Jói wrote:
> Indent -2.

Done.

https://codereview.chromium.org/18063003/diff/7001/chrome/common/extensions/a...
File chrome/common/extensions/api/api.gyp (right):

https://codereview.chromium.org/18063003/diff/7001/chrome/common/extensions/a...
chrome/common/extensions/api/api.gyp:99: 'time.idl',
On 2013/06/28 10:49:15, Jói wrote:
> Indentation is off. Note, we use 2 spaces to indent, not tabs.

Done.

https://codereview.chromium.org/18063003/diff/7001/chrome/common/extensions/a...
File chrome/common/extensions/api/time.idl (right):

https://codereview.chromium.org/18063003/diff/7001/chrome/common/extensions/a...
chrome/common/extensions/api/time.idl:5: // Use the <code>chrome.time</code> API
to get the current time
On 2013/06/28 10:49:15, Jói wrote:
> Generally we don't use HTML or other markup in comments. By convention you
would
> sometimes put | around code or variable names, e.g. |chrome.time|.

Done.

[Jói, 2013-06-28 12:17:15]

Could you upload a new version for me to take a look?  Thanks.

[hpjonsson, 2013-06-28 12:25:36]

On 2013/06/28 12:17:15, Jói wrote:
> Could you upload a new version for me to take a look?  Thanks.

Done.

[Jói, 2013-06-28 12:36:55]

Looks good, couple of minor nits.

https://codereview.chromium.org/18063003/diff/3002/chrome/browser/extensions/...
File chrome/browser/extensions/api/time/time_api.h (right):

https://codereview.chromium.org/18063003/diff/3002/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.h:20: }  // namespace extensions
nit: Usually one blank line before closing the namespace.

https://codereview.chromium.org/18063003/diff/3002/chrome/common/extensions/a...
File chrome/common/extensions/api/time.idl (right):

https://codereview.chromium.org/18063003/diff/3002/chrome/common/extensions/a...
chrome/common/extensions/api/time.idl:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
2013

[hpjonsson, 2013-06-28 13:38:10]

New version has been uploaded.

https://codereview.chromium.org/18063003/diff/3002/chrome/browser/extensions/...
File chrome/browser/extensions/api/time/time_api.h (right):

https://codereview.chromium.org/18063003/diff/3002/chrome/browser/extensions/...
chrome/browser/extensions/api/time/time_api.h:20: }  // namespace extensions
On 2013/06/28 12:36:55, Jói wrote:
> nit: Usually one blank line before closing the namespace.

Done.

https://codereview.chromium.org/18063003/diff/3002/chrome/common/extensions/a...
File chrome/common/extensions/api/time.idl (right):

https://codereview.chromium.org/18063003/diff/3002/chrome/common/extensions/a...
chrome/common/extensions/api/time.idl:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/06/28 12:36:55, Jói wrote:
> 2013

Done.