Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
7 years, 9 months ago by vadimt Modified:
7 years, 9 months ago Reviewers:
arv (Not doing code reviews), rgustafson, skare_, sky CC:
chromium-reviews, arv+watch_chromium.org, govind1, stromme Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnabling 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
Patch Set 1 #
Total comments: 12
Patch Set 2 : rgustafson's notes #
Total comments: 7
Patch Set 3 : skare@'s notes #Patch Set 4 : #Patch Set 5 : Rebase #Patch Set 6 : #
Total comments: 2
Patch Set 7 : Removing unnecessary workaround. #
Created: 7 years, 9 months ago
Messages
Total messages: 15 (0 generated)
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?
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.
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?
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.
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.
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.
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).
lgtm great, thanks for removing that block
Please provide owners' approvals: sky@: *.cc; *.grd arv@: *.js NOTE: The CL also fixed a bug in for-in loop.
LGTM
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.
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.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/12508004/11006
Message was sent while issue was closed.
Change committed as 187341 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
