Improvements to the Google News extension

Improvements to the Google News extension.
 * Patch by anujb

BUG=None
TEST=None
TBR=kathyw

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66100

[kurrik.chromium, 2010-10-21 03:25:10]

Hi, some general feedback:

License headers should be added or changed in all files to match the Chromium
license header.  I've linked to some examples in my comments.

I believe images should be consistently included in the extension or Base64
encoded.  I left a note about how to refer to images by absolute URLs in
feed.js.

There's a lot of direct manipulation of styles.  I'd prefer to see this changed
to adding/removing classes where you can, but if that's not possible please pull
out as much of this into helper functions as you can - all the style
manipulation hurts readability IMO.

Please make sure that the feed bodies can't call chrome.* functions - this is
pretty important.

Other than that, some small style nits, but nothing else major.  When you update
the review I'll take another look through.

Thanks!

http://codereview.chromium.org/3681008/diff/1/14
File chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js
(right):

http://codereview.chromium.org/3681008/diff/1/14#newcode1
chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js:1: //
Copyright 2010 Google Inc. All Rights Reserved.
See note in options.js about header

http://codereview.chromium.org/3681008/diff/1/14#newcode14
chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js:14:
var feedUrl = 'http://news.google.com/news?output=rss';
This feed URL is repeated in options.js - I would prefer to see this put into
util.js as a constant.

http://codereview.chromium.org/3681008/diff/1/14#newcode98
chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js:98:
'http://www.gstatic.com/news/img/logo/iw_il/news.gif') {
This image should be included in the extension or converted to a data URL in
util.js

http://codereview.chromium.org/3681008/diff/1/14#newcode191
chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js:191:
"<img src='" + FACEBOOK_BUTTON + "'/></a>   ";
See my comments about these constants in util.js, but you could just host the
images inside the extension and get a fully-qualified URL to them using
chrome.extension.getURL

http://codereview.chromium.org/3681008/diff/1/14#newcode200
chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js:200:
// The story body is created as an iframe with a data: URL in order to
Does this really work?  Cool :)  

Does this prevent the body from making chrome.* API calls?  That would be my
biggest concern here.

http://codereview.chromium.org/3681008/diff/1/15
File
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js
(right):

http://codereview.chromium.org/3681008/diff/1/15#newcode1
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:1:
// Copyright 2010 Google Inc. All Rights Reserved.
This copyright should be modified to match the standard Chrome header.  See
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs...
for an example

http://codereview.chromium.org/3681008/diff/1/15#newcode31
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:31:
}
Minor style nit - function declarations in JS should end in semicolons - see
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...
for rationale.  Applies everywhere.

http://codereview.chromium.org/3681008/diff/1/15#newcode48
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:48:
req.open('GET', 'http://news.google.com/news?output=rss', true);
This URL is repeated in feed.js.  I would prefer to see this as a constant in
util.js

http://codereview.chromium.org/3681008/diff/1/15#newcode64
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:64:
var images = doc.getElementsByTagName('image');
If you just want the first image element, use var image =
doc.querySelector('image'); which will return the first matching element or
null. 

Then you can check if (image) { ... }

http://codereview.chromium.org/3681008/diff/1/15#newcode66
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:66:
var links = images[0].getElementsByTagName('link');
Same goes for querying links, generally I don't like using [0] ;)

http://codereview.chromium.org/3681008/diff/1/15#newcode88
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:88:
if (getLocalizedMessage('direction') == 'rtl') {
Consider just setting document.body.className = 'rtl' and then having a set of
styles like:

body.rtl .col1 { ... }
body.rtl .col2 { ... }

There's no real need to do all of this in JavaScript.

http://codereview.chromium.org/3681008/diff/1/15#newcode139
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:139:
$('top').innerHTML = getLocalizedMessage('1');
Are localized messages ever HTML?  If not, then I'd prefer innerText.  Also see
my note about getLocalizedMessage in util.js

http://codereview.chromium.org/3681008/diff/1/15#newcode251
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:251:
for (i = 0; i < tempCusTopics.length; i++) {
This loop could be replaced with keywords = keywords.concat(tempCusTopics);

http://codereview.chromium.org/3681008/diff/1/15#newcode254
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:254:
window.localStorage.setItem('keywords', keywords);
You read keywords from localstorage and split on ",", but then here you write
the array into localstorage - does this code work the second time around? 
Shouldn't you set keywords to keywords.join(',') ?

http://codereview.chromium.org/3681008/diff/1/15#newcode256
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:256:
window.localStorage.setItem('keywords', tempCusTopics);
Same here, should this be tempCusTopics.join(',') ?

http://codereview.chromium.org/3681008/diff/1/15#newcode300
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:300:
}
Should this function also disable the save button if checkCount is equal to 0?

http://codereview.chromium.org/3681008/diff/1/15#newcode437
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:437:
$('save_status').style.setProperty('-webkit-transition',
This fade in/out code seems like it is repeated in a few places - this would be
a good candidate to strip out into a helper function that you could pass a DOM
element or ID to.

http://codereview.chromium.org/3681008/diff/1/16
File chrome/common/extensions/docs/examples/extensions/news/javascript/util.js
(right):

http://codereview.chromium.org/3681008/diff/1/16#newcode1
chrome/common/extensions/docs/examples/extensions/news/javascript/util.js:1: //
Copyright 2010 Google Inc. All Rights Reserved.
See note in options.js about header

http://codereview.chromium.org/3681008/diff/1/16#newcode9
chrome/common/extensions/docs/examples/extensions/news/javascript/util.js:9: var
FACEBOOK_BUTTON =
Is there a reason these are b64 encoded when there are other icons included as
part of the extension?  I would like to be consistent about either making all
icons data URLs or all icons image files unless there is a good reason for the
split.

http://codereview.chromium.org/3681008/diff/1/16#newcode61
chrome/common/extensions/docs/examples/extensions/news/javascript/util.js:61:
function getLocalizedMessage(messageId) {
This only really saves 3 characters - I would prefer to use the real function
inline as it would be easier to understand what is happening

http://codereview.chromium.org/3681008/diff/1/18
File
chrome/common/extensions/docs/examples/extensions/news/views/background.html
(right):

http://codereview.chromium.org/3681008/diff/1/18#newcode1
chrome/common/extensions/docs/examples/extensions/news/views/background.html:1:
<!--
I believe that <!DOCTYPE html> needs to be the first line in the document. 
Could you move this comment below that, and add in the correct open source
license?  See
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs...
for an example

http://codereview.chromium.org/3681008/diff/1/19
File chrome/common/extensions/docs/examples/extensions/news/views/feed.html
(right):

http://codereview.chromium.org/3681008/diff/1/19#newcode26
chrome/common/extensions/docs/examples/extensions/news/views/feed.html:26:
openUrl = 'http://www.facebook.com/sharer.php?u=';
These URLs could be constants in feed.js

http://codereview.chromium.org/3681008/diff/1/20
File chrome/common/extensions/docs/examples/extensions/news/views/options.html
(right):

http://codereview.chromium.org/3681008/diff/1/20#newcode2
chrome/common/extensions/docs/examples/extensions/news/views/options.html:2:
@fileoverview This file serves as the option page of the extension for
Please see my note about headers in background.html

http://codereview.chromium.org/3681008/diff/1/20#newcode10
chrome/common/extensions/docs/examples/extensions/news/views/options.html:10:
<script type = "text/javascript" src = "/javascript/util.js"></script>
Minor nit - you don't need type="text/javascript" for HTML5 documents

[navneetg, 2010-10-25 10:55:51]

I have incorporated all the changes. Please review.

http://codereview.chromium.org/3681008/diff/1/14
File chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js
(right):

http://codereview.chromium.org/3681008/diff/1/14#newcode1
chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js:1: //
Copyright 2010 Google Inc. All Rights Reserved.
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> See note in options.js about header

Done.

http://codereview.chromium.org/3681008/diff/1/14#newcode14
chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js:14:
var feedUrl = 'http://news.google.com/news?output=rss';
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> This feed URL is repeated in options.js - I would prefer to see this put into
> util.js as a constant.

Done.

http://codereview.chromium.org/3681008/diff/1/14#newcode98
chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js:98:
'http://www.gstatic.com/news/img/logo/iw_il/news.gif') {
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> This image should be included in the extension or converted to a data URL in
> util.js

Reply: This is just a URL to check condition, not an image.
Done.

http://codereview.chromium.org/3681008/diff/1/14#newcode191
chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js:191:
"<img src='" + FACEBOOK_BUTTON + "'/></a>   ";
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> See my comments about these constants in util.js, but you could just host the
> images inside the extension and get a fully-qualified URL to them using
> chrome.extension.getURL

Done.

http://codereview.chromium.org/3681008/diff/1/14#newcode200
chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js:200:
// The story body is created as an iframe with a data: URL in order to
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> Does this really work?  Cool :)  
> 
> Does this prevent the body from making chrome.* API calls?  That would be my
> biggest concern here.

Reply: Actually i am not using any chrome.* API right now inside iframe.

http://codereview.chromium.org/3681008/diff/1/15
File
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js
(right):

http://codereview.chromium.org/3681008/diff/1/15#newcode1
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:1:
// Copyright 2010 Google Inc. All Rights Reserved.
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> This copyright should be modified to match the standard Chrome header.  See
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs...
> for an example

Done.

http://codereview.chromium.org/3681008/diff/1/15#newcode31
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:31:
}
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> Minor style nit - function declarations in JS should end in semicolons - see
>
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...
> for rationale.  Applies everywhere.

Reply: I have incoporated this change everywhere, but it shows error while
running 'gjslint' command.

http://codereview.chromium.org/3681008/diff/1/15#newcode48
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:48:
req.open('GET', 'http://news.google.com/news?output=rss', true);
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> This URL is repeated in feed.js.  I would prefer to see this as a constant in
> util.js

Done.

http://codereview.chromium.org/3681008/diff/1/15#newcode64
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:64:
var images = doc.getElementsByTagName('image');
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> If you just want the first image element, use var image =
> doc.querySelector('image'); which will return the first matching element or
> null. 
> 
> Then you can check if (image) { ... }

Done.

http://codereview.chromium.org/3681008/diff/1/15#newcode66
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:66:
var links = images[0].getElementsByTagName('link');
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> Same goes for querying links, generally I don't like using [0] ;)

Done.

http://codereview.chromium.org/3681008/diff/1/15#newcode88
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:88:
if (getLocalizedMessage('direction') == 'rtl') {
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> Consider just setting document.body.className = 'rtl' and then having a set of
> styles like:
> 
> body.rtl .col1 { ... }
> body.rtl .col2 { ... }
> 
> There's no real need to do all of this in JavaScript.

Done.

http://codereview.chromium.org/3681008/diff/1/15#newcode139
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:139:
$('top').innerHTML = getLocalizedMessage('1');
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> Are localized messages ever HTML?  If not, then I'd prefer innerText.  Also
see
> my note about getLocalizedMessage in util.js

Done.

http://codereview.chromium.org/3681008/diff/1/15#newcode251
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:251:
for (i = 0; i < tempCusTopics.length; i++) {
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> This loop could be replaced with keywords = keywords.concat(tempCusTopics);

Done.

http://codereview.chromium.org/3681008/diff/1/15#newcode254
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:254:
window.localStorage.setItem('keywords', keywords);
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> You read keywords from localstorage and split on ",", but then here you write
> the array into localstorage - does this code work the second time around? 
> Shouldn't you set keywords to keywords.join(',') ?

Reply: As the array converts into string by default, i thought of using it as
such. Is it best practice?

http://codereview.chromium.org/3681008/diff/1/15#newcode256
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:256:
window.localStorage.setItem('keywords', tempCusTopics);
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> Same here, should this be tempCusTopics.join(',') ?

Reply :Same as above.

http://codereview.chromium.org/3681008/diff/1/15#newcode300
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:300:
}
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> Should this function also disable the save button if checkCount is equal to 0?

Reply: 'enableSaveButton()' function only enables the 'save' button.
'manageCheckCount()' function disables 'save' button if checkCount is equal to
0.

http://codereview.chromium.org/3681008/diff/1/15#newcode437
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:437:
$('save_status').style.setProperty('-webkit-transition',
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> This fade in/out code seems like it is repeated in a few places - this would
be
> a good candidate to strip out into a helper function that you could pass a DOM
> element or ID to.

Done.

http://codereview.chromium.org/3681008/diff/1/16
File chrome/common/extensions/docs/examples/extensions/news/javascript/util.js
(right):

http://codereview.chromium.org/3681008/diff/1/16#newcode1
chrome/common/extensions/docs/examples/extensions/news/javascript/util.js:1: //
Copyright 2010 Google Inc. All Rights Reserved.
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> See note in options.js about header

Done.

http://codereview.chromium.org/3681008/diff/1/16#newcode9
chrome/common/extensions/docs/examples/extensions/news/javascript/util.js:9: var
FACEBOOK_BUTTON =
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> Is there a reason these are b64 encoded when there are other icons included as
> part of the extension?  I would like to be consistent about either making all
> icons data URLs or all icons image files unless there is a good reason for the
> split.

Done.

http://codereview.chromium.org/3681008/diff/1/16#newcode61
chrome/common/extensions/docs/examples/extensions/news/javascript/util.js:61:
function getLocalizedMessage(messageId) {
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> This only really saves 3 characters - I would prefer to use the real function
> inline as it would be easier to understand what is happening

Done.

http://codereview.chromium.org/3681008/diff/1/18
File
chrome/common/extensions/docs/examples/extensions/news/views/background.html
(right):

http://codereview.chromium.org/3681008/diff/1/18#newcode1
chrome/common/extensions/docs/examples/extensions/news/views/background.html:1:
<!--
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> I believe that <!DOCTYPE html> needs to be the first line in the document. 
> Could you move this comment below that, and add in the correct open source
> license?  See
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs...
> for an example

Done.

http://codereview.chromium.org/3681008/diff/1/19
File chrome/common/extensions/docs/examples/extensions/news/views/feed.html
(right):

http://codereview.chromium.org/3681008/diff/1/19#newcode26
chrome/common/extensions/docs/examples/extensions/news/views/feed.html:26:
openUrl = 'http://www.facebook.com/sharer.php?u=';
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> These URLs could be constants in feed.js

Reply: Actually these URLs are used by iframe, so i declared them as constants
in iframe script in feed.html.
Done.

http://codereview.chromium.org/3681008/diff/1/20
File chrome/common/extensions/docs/examples/extensions/news/views/options.html
(right):

http://codereview.chromium.org/3681008/diff/1/20#newcode2
chrome/common/extensions/docs/examples/extensions/news/views/options.html:2:
@fileoverview This file serves as the option page of the extension for
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> Please see my note about headers in background.html

Done.

http://codereview.chromium.org/3681008/diff/1/20#newcode10
chrome/common/extensions/docs/examples/extensions/news/views/options.html:10:
<script type = "text/javascript" src = "/javascript/util.js"></script>
On 2010/10/21 03:25:10, kurrik.chromium wrote:
> Minor nit - you don't need type="text/javascript" for HTML5 documents

Done.

[kurrik.chromium, 2010-10-26 23:29:31]

LGTM.  There's a few minor changes you might consider making, but looks pretty
good.

http://codereview.chromium.org/3681008/diff/1/14
File chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js
(right):

http://codereview.chromium.org/3681008/diff/1/14#newcode98
chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js:98:
'http://www.gstatic.com/news/img/logo/iw_il/news.gif') {
Ah, I see.  Thanks for making it a constant :)

On 2010/10/25 10:55:51, navneetg wrote:
> On 2010/10/21 03:25:10, kurrik.chromium wrote:
> > This image should be included in the extension or converted to a data URL in
> > util.js
> 
> Reply: This is just a URL to check condition, not an image.
> Done.

http://codereview.chromium.org/3681008/diff/1/14#newcode200
chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js:200:
// The story body is created as an iframe with a data: URL in order to
I think the concern would be that the feed could embed a call to chrome.* calls
via XSS.  Since you're embedding Google News this is less of a concern, but it
would still be nice to know.

On 2010/10/25 10:55:51, navneetg wrote:
> On 2010/10/21 03:25:10, kurrik.chromium wrote:
> > Does this really work?  Cool :)  
> > 
> > Does this prevent the body from making chrome.* API calls?  That would be my
> > biggest concern here.
> 
> Reply: Actually i am not using any chrome.* API right now inside iframe.

http://codereview.chromium.org/3681008/diff/1/15
File
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js
(right):

http://codereview.chromium.org/3681008/diff/1/15#newcode31
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:31:
}
Interesting, I didn't realize that lint warned on this.  I still prefer to use
semicolons after function declarations, but I guess you could remove them if the
lint warnings bother you.

You should still use semicolons after any assignment, including
myFunction.prototype.myMethod = function() {...};

On 2010/10/25 10:55:51, navneetg wrote:
> On 2010/10/21 03:25:10, kurrik.chromium wrote:
> > Minor style nit - function declarations in JS should end in semicolons - see
> >
>
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...
> > for rationale.  Applies everywhere.
> 
> Reply: I have incoporated this change everywhere, but it shows error while
> running 'gjslint' command.

http://codereview.chromium.org/3681008/diff/1/15#newcode254
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:254:
window.localStorage.setItem('keywords', keywords);
I wouldn't rely on the automatic serialization to behave a certain way.  Perhaps
wrap calls to localStorage with JSON.parse and JSON.stringify methods to be
explicit about what is happening?

e.g.:
  localStorage['foo'] = JSON.stringify(myArray);
  var myArray = JSON.parse(localStorage['foo'] || "[]");



On 2010/10/25 10:55:51, navneetg wrote:
> On 2010/10/21 03:25:10, kurrik.chromium wrote:
> > You read keywords from localstorage and split on ",", but then here you
write
> > the array into localstorage - does this code work the second time around? 
> > Shouldn't you set keywords to keywords.join(',') ?
> 
> Reply: As the array converts into string by default, i thought of using it as
> such. Is it best practice?

http://codereview.chromium.org/3681008/diff/1/15#newcode300
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:300:
}
On 2010/10/25 10:55:51, navneetg wrote:
> On 2010/10/21 03:25:10, kurrik.chromium wrote:
> > Should this function also disable the save button if checkCount is equal to
0?
> 
> Reply: 'enableSaveButton()' function only enables the 'save' button.
> 'manageCheckCount()' function disables 'save' button if checkCount is equal to
> 0.

Done.

[kurrik.chromium, 2010-10-27 00:34:35]

One more note I just noticed - the minutes will go into negative in some cases
(I guess while you're currently in a meeting?) see
http://dl.dropbox.com/u/5556374/img/negative-minutes.png for an example.

~Arne

On 2010/10/26 23:29:31, kurrik.chromium wrote:
> LGTM.  There's a few minor changes you might consider making, but looks pretty
> good.
> 
> http://codereview.chromium.org/3681008/diff/1/14
> File chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js
> (right):
> 
> http://codereview.chromium.org/3681008/diff/1/14#newcode98
> chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js:98:
> 'http://www.gstatic.com/news/img/logo/iw_il/news.gif') {
> Ah, I see.  Thanks for making it a constant :)
> 
> On 2010/10/25 10:55:51, navneetg wrote:
> > On 2010/10/21 03:25:10, kurrik.chromium wrote:
> > > This image should be included in the extension or converted to a data URL
in
> > > util.js
> > 
> > Reply: This is just a URL to check condition, not an image.
> > Done.
> 
> http://codereview.chromium.org/3681008/diff/1/14#newcode200
> chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js:200:
> // The story body is created as an iframe with a data: URL in order to
> I think the concern would be that the feed could embed a call to chrome.*
calls
> via XSS.  Since you're embedding Google News this is less of a concern, but it
> would still be nice to know.
> 
> On 2010/10/25 10:55:51, navneetg wrote:
> > On 2010/10/21 03:25:10, kurrik.chromium wrote:
> > > Does this really work?  Cool :)  
> > > 
> > > Does this prevent the body from making chrome.* API calls?  That would be
my
> > > biggest concern here.
> > 
> > Reply: Actually i am not using any chrome.* API right now inside iframe.
> 
> http://codereview.chromium.org/3681008/diff/1/15
> File
> chrome/common/extensions/docs/examples/extensions/news/javascript/options.js
> (right):
> 
> http://codereview.chromium.org/3681008/diff/1/15#newcode31
>
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:31:
> }
> Interesting, I didn't realize that lint warned on this.  I still prefer to use
> semicolons after function declarations, but I guess you could remove them if
the
> lint warnings bother you.
> 
> You should still use semicolons after any assignment, including
> myFunction.prototype.myMethod = function() {...};
> 
> On 2010/10/25 10:55:51, navneetg wrote:
> > On 2010/10/21 03:25:10, kurrik.chromium wrote:
> > > Minor style nit - function declarations in JS should end in semicolons -
see
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...
> > > for rationale.  Applies everywhere.
> > 
> > Reply: I have incoporated this change everywhere, but it shows error while
> > running 'gjslint' command.
> 
> http://codereview.chromium.org/3681008/diff/1/15#newcode254
>
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:254:
> window.localStorage.setItem('keywords', keywords);
> I wouldn't rely on the automatic serialization to behave a certain way. 
Perhaps
> wrap calls to localStorage with JSON.parse and JSON.stringify methods to be
> explicit about what is happening?
> 
> e.g.:
>   localStorage['foo'] = JSON.stringify(myArray);
>   var myArray = JSON.parse(localStorage['foo'] || "[]");
> 
> 
> 
> On 2010/10/25 10:55:51, navneetg wrote:
> > On 2010/10/21 03:25:10, kurrik.chromium wrote:
> > > You read keywords from localstorage and split on ",", but then here you
> write
> > > the array into localstorage - does this code work the second time around? 
> > > Shouldn't you set keywords to keywords.join(',') ?
> > 
> > Reply: As the array converts into string by default, i thought of using it
as
> > such. Is it best practice?
> 
> http://codereview.chromium.org/3681008/diff/1/15#newcode300
>
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:300:
> }
> On 2010/10/25 10:55:51, navneetg wrote:
> > On 2010/10/21 03:25:10, kurrik.chromium wrote:
> > > Should this function also disable the save button if checkCount is equal
to
> 0?
> > 
> > Reply: 'enableSaveButton()' function only enables the 'save' button.
> > 'manageCheckCount()' function disables 'save' button if checkCount is equal
to
> > 0.
> 
> Done.

[navneetg, 2010-10-27 13:47:44]

Hi,

Please find below my updates on your comments from patch set 5.
Apart from these I have modified the 'custom topics' section and replaced the
content area with text area, So as to hold special characters and white-space
issues.

Thanks & Regards,
Navneet

http://codereview.chromium.org/3681008/diff/1/14
File chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js
(right):

http://codereview.chromium.org/3681008/diff/1/14#newcode200
chrome/common/extensions/docs/examples/extensions/news/javascript/feed.js:200:
// The story body is created as an iframe with a data: URL in order to
On 2010/10/26 23:29:32, kurrik.chromium wrote:
> I think the concern would be that the feed could embed a call to chrome.*
calls
> via XSS.  Since you're embedding Google News this is less of a concern, but it
> would still be nice to know.
> 
> On 2010/10/25 10:55:51, navneetg wrote:
> > On 2010/10/21 03:25:10, kurrik.chromium wrote:
> > > Does this really work?  Cool :)  
> > > 
> > > Does this prevent the body from making chrome.* API calls?  That would be
my
> > > biggest concern here.
> > 
> > Reply: Actually i am not using any chrome.* API right now inside iframe.
> 
> 

ok

http://codereview.chromium.org/3681008/diff/1/15
File
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js
(right):

http://codereview.chromium.org/3681008/diff/1/15#newcode31
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:31:
}
On 2010/10/26 23:29:32, kurrik.chromium wrote:
> Interesting, I didn't realize that lint warned on this.  I still prefer to use
> semicolons after function declarations, but I guess you could remove them if
the
> lint warnings bother you.
> 
> You should still use semicolons after any assignment, including
> myFunction.prototype.myMethod = function() {...};
> 
> On 2010/10/25 10:55:51, navneetg wrote:
> > On 2010/10/21 03:25:10, kurrik.chromium wrote:
> > > Minor style nit - function declarations in JS should end in semicolons -
see
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...
> > > for rationale.  Applies everywhere.
> > 
> > Reply: I have incoporated this change everywhere, but it shows error while
> > running 'gjslint' command.
> 
> 

Reply: I have removed semicolon after every function declaration.

http://codereview.chromium.org/3681008/diff/1/15#newcode254
chrome/common/extensions/docs/examples/extensions/news/javascript/options.js:254:
window.localStorage.setItem('keywords', keywords);
On 2010/10/26 23:29:32, kurrik.chromium wrote:
> I wouldn't rely on the automatic serialization to behave a certain way. 
Perhaps
> wrap calls to localStorage with JSON.parse and JSON.stringify methods to be
> explicit about what is happening?
> 
> e.g.:
>   localStorage['foo'] = JSON.stringify(myArray);
>   var myArray = JSON.parse(localStorage['foo'] || "[]");
> 
> 
> 
> On 2010/10/25 10:55:51, navneetg wrote:
> > On 2010/10/21 03:25:10, kurrik.chromium wrote:
> > > You read keywords from localstorage and split on ",", but then here you
> write
> > > the array into localstorage - does this code work the second time around? 
> > > Shouldn't you set keywords to keywords.join(',') ?
> > 
> > Reply: As the array converts into string by default, i thought of using it
as
> > such. Is it best practice?
> 
> 

Done.