Enabling testing Google Now component extension

Enabling testing Google Now component extension.

Google Now component extension now can be turned on via chrome://flags. The server name needs to be set manually in JS debugger via local memory.

BUG=164227
TEST=Enable the extension via chrome://flags, set the server via local memory, restart, make sure cards appear. Check this on CrOS and Windows.
When this starts working in Canary, set it up to see cards, wait till next day, update Canary to the next version, make sure cards appear on (1) the first start after the update; (2) following starts.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187341

[rgustafson, 2013-03-06 02:51:57]

https://codereview.chromium.org/12508004/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/12508004/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:15043: +      Enable Google Now intergation
integration

https://codereview.chromium.org/12508004/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:15046: +      When enabled, Google Now
notifications will be available in desktop Chrome.
Are you set on this wording?

https://codereview.chromium.org/12508004/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:15049: +      <!-- Toast experiment. -->
Spacing got messed up on this line.

https://codereview.chromium.org/12508004/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/12508004/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1270: kOsDesktop,
Do we want to limit it to the ones with rich notifications (kOsWin | kOsCrOS)?

https://codereview.chromium.org/12508004/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/12508004/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:111: if
(items.activeNotifications.hasOwnProperty(notificationId) &&
What is this doing here? Leak from some other change?

[vadimt, 2013-03-06 21:44:40]

https://codereview.chromium.org/12508004/diff/1/chrome/app/generated_resource...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/12508004/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:15043: +      Enable Google Now intergation
On 2013/03/06 02:51:57, rgustafson wrote:
> integration

Done.

https://codereview.chromium.org/12508004/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:15046: +      When enabled, Google Now
notifications will be available in desktop Chrome.
On 2013/03/06 02:51:57, rgustafson wrote:
> Are you set on this wording?

Done.

https://codereview.chromium.org/12508004/diff/1/chrome/app/generated_resource...
chrome/app/generated_resources.grd:15049: +      <!-- Toast experiment. -->
On 2013/03/06 02:51:57, rgustafson wrote:
> Spacing got messed up on this line.

Done.

https://codereview.chromium.org/12508004/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/12508004/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1270: kOsDesktop,
On 2013/03/06 02:51:57, rgustafson wrote:
> Do we want to limit it to the ones with rich notifications (kOsWin | kOsCrOS)?

Will be happy to chat offline.

https://codereview.chromium.org/12508004/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/12508004/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:111: if
(items.activeNotifications.hasOwnProperty(notificationId) &&
On 2013/03/06 02:51:57, rgustafson wrote:
> What is this doing here? Leak from some other change?

Just fixing a bug. In general, this is a right way to use for-in loop.

[skare_, 2013-03-06 22:08:01]

https://codereview.chromium.org/12508004/diff/6001/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12508004/diff/6001/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1061: //    
switches::kEnableGoogleNowIntegration)) {
can this block just be removed altogether, and file a bug against yourself or
similar for tracking? 
I think checking in commented-out code is generally avoided.

https://codereview.chromium.org/12508004/diff/6001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/12508004/diff/6001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:28: // TODO(vadimt): Consider
processing errors for all storage.set calls.
has this edit already been done in another review or am I experiencing deja vu?

[vadimt, 2013-03-06 22:37:12]

https://codereview.chromium.org/12508004/diff/6001/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12508004/diff/6001/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1061: //    
switches::kEnableGoogleNowIntegration)) {
On 2013/03/06 22:08:01, Travis Skare wrote:
> can this block just be removed altogether, and file a bug against yourself or
> similar for tracking? 
> I think checking in commented-out code is generally avoided.

This text has 2 parts:

(1) TODO item. TODO's are widely used as lightweight replacement bugs that have
to be cleared before release.

(2) Commented out C++ code.
In this case, we are not 100% sure that we won't have switch back to C++, so we
don't want to lose the code.
On the other hand, we don't want the code to be executed.

So, commenting out the code seems quite adequate.

https://codereview.chromium.org/12508004/diff/6001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/12508004/diff/6001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:28: // TODO(vadimt): Consider
processing errors for all storage.set calls.
On 2013/03/06 22:08:01, Travis Skare wrote:
> has this edit already been done in another review or am I experiencing deja
vu?
"Yes" to both :)
The Races review (next in the queue) moves a newline AFAIR.

[rgustafson, 2013-03-06 22:56:00]

lgtm

https://codereview.chromium.org/12508004/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/12508004/diff/1/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:1270: kOsDesktop,
Thanks.
On 2013/03/06 21:44:40, vadimt wrote:
> On 2013/03/06 02:51:57, rgustafson wrote:
> > Do we want to limit it to the ones with rich notifications (kOsWin |
kOsCrOS)?
> 
> Will be happy to chat offline.

https://codereview.chromium.org/12508004/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/12508004/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:111: if
(items.activeNotifications.hasOwnProperty(notificationId) &&
Okay.
On 2013/03/06 21:44:40, vadimt wrote:
> On 2013/03/06 02:51:57, rgustafson wrote:
> > What is this doing here? Leak from some other change?
> 
> Just fixing a bug. In general, this is a right way to use for-in loop.

[skare_, 2013-03-07 00:56:39]

https://codereview.chromium.org/12508004/diff/6001/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12508004/diff/6001/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1061: //    
switches::kEnableGoogleNowIntegration)) {
1) consider an actual bug for this case since we'll want to have it happen
before we release.

2) chrome_browser_main is a fairly communal file -- could we just erase this
whole #if block and rely on source control to get the code later?

On 2013/03/06 22:37:12, vadimt wrote:
> On 2013/03/06 22:08:01, Travis Skare wrote:
> > can this block just be removed altogether, and file a bug against yourself
or
> > similar for tracking? 
> > I think checking in commented-out code is generally avoided.
> 
> This text has 2 parts:
> 
> (1) TODO item. TODO's are widely used as lightweight replacement bugs that
have
> to be cleared before release.
> 
> (2) Commented out C++ code.
> In this case, we are not 100% sure that we won't have switch back to C++, so
we
> don't want to lose the code.
> On the other hand, we don't want the code to be executed.
> 
> So, commenting out the code seems quite adequate.

https://codereview.chromium.org/12508004/diff/6001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/12508004/diff/6001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:28: // TODO(vadimt): Consider
processing errors for all storage.set calls.
ok, great, thanks
On 2013/03/06 22:37:12, vadimt wrote:
> On 2013/03/06 22:08:01, Travis Skare wrote:
> > has this edit already been done in another review or am I experiencing deja
> vu?
> "Yes" to both :)
> The Races review (next in the queue) moves a newline AFAIR.

[vadimt, 2013-03-07 22:33:07]

https://codereview.chromium.org/12508004/diff/6001/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12508004/diff/6001/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1061: //    
switches::kEnableGoogleNowIntegration)) {
On 2013/03/07 00:56:39, Travis Skare wrote:
> 1) consider an actual bug for this case since we'll want to have it happen
> before we release.
> 
> 2) chrome_browser_main is a fairly communal file -- could we just erase this
> whole #if block and rely on source control to get the code later?
> 
> On 2013/03/06 22:37:12, vadimt wrote:
> > On 2013/03/06 22:08:01, Travis Skare wrote:
> > > can this block just be removed altogether, and file a bug against yourself
> or
> > > similar for tracking? 
> > > I think checking in commented-out code is generally avoided.
> > 
> > This text has 2 parts:
> > 
> > (1) TODO item. TODO's are widely used as lightweight replacement bugs that
> have
> > to be cleared before release.
> > 
> > (2) Commented out C++ code.
> > In this case, we are not 100% sure that we won't have switch back to C++, so
> we
> > don't want to lose the code.
> > On the other hand, we don't want the code to be executed.
> > 
> > So, commenting out the code seems quite adequate.
> 

Removed. We have a TODO item in JS code to remove the remaining C++ code (I
don't think we need to convert all TODO items to bugs).

[skare_, 2013-03-07 22:57:16]

lgtm

great, thanks for removing that block

[vadimt, 2013-03-07 23:41:40]

Please provide owners' approvals:
sky@: *.cc; *.grd
arv@: *.js

NOTE: The CL also fixed a bug in for-in loop.

[sky, 2013-03-08 00:44:38]

LGTM

[arv (Not doing code reviews), 2013-03-11 16:04:21]

LGTM with nit

https://codereview.chromium.org/12508004/diff/30001/chrome/browser/resources/...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/12508004/diff/30001/chrome/browser/resources/...
chrome/browser/resources/google_now/background.js:111: if
(items.activeNotifications.hasOwnProperty(notificationId) &&
Does it matter if it is "own" or "in"?

if (notificationId in items.activeNotifications &&

If not prefer "in" since it is easier to read and less error prone.

[vadimt, 2013-03-11 16:51:02]

https://codereview.chromium.org/12508004/diff/30001/chrome/browser/resources/...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/12508004/diff/30001/chrome/browser/resources/...
chrome/browser/resources/google_now/background.js:111: if
(items.activeNotifications.hasOwnProperty(notificationId) &&
On 2013/03/11 16:04:21, arv wrote:
> Does it matter if it is "own" or "in"?
> 
> if (notificationId in items.activeNotifications &&
> 
> If not prefer "in" since it is easier to read and less error prone.

OK, I'm removing this line.

It looks like I've added this as a workaround for a V8 bug that now has gone.
Some time ago, I've started noticing that in this loop, in the first iteration,
notificationId takes value of 'undefined' or the last value assigned before the
loop.
Then I've added this check without full understanding what's happened.

But now the issue has gone, and I don't need this check.

[commit-bot: I haz the power, 2013-03-11 16:51:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vadimt@chromium.org/12508004/11006

[commit-bot: I haz the power, 2013-03-11 19:36:16]

Change committed as 187341