Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
27223006:
Requesting cards on push messages (Closed)
|
Created:
7 years, 2 months ago by vadimt Modified:
7 years, 2 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRequesting cards on push messages.
When a new push message arrives, making a server request for all groups.
BUG=164227
TEST=Now, there is no reasonable way to test it; stay tuned!
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229311
Patch Set 1 #
Total comments: 6
Patch Set 2 : rebase and robliao comments #
Total comments: 18
Patch Set 3 : More robliao@ comments #Patch Set 4 : rgustafson's comments #Patch Set 5 : Fixing unit tests #Patch Set 6 : More rgustafson's comments #
Messages
Total messages: 24 (0 generated)
https://codereview.chromium.org/27223006/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/27223006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:1191: console.log(JSON.stringify(message)); This should either be removed or annotated with what this message is. https://codereview.chromium.org/27223006/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/27223006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/manifest.json:2: //chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh How often do we expect this to change from here? https://codereview.chromium.org/27223006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/manifest.json:18: "identity", Alphabetize
https://codereview.chromium.org/27223006/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/27223006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:1191: console.log(JSON.stringify(message)); This should either be removed or annotated with what this message is. https://codereview.chromium.org/27223006/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/27223006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/manifest.json:2: //chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh How often do we expect this to change from here? https://codereview.chromium.org/27223006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/manifest.json:18: "identity", Alphabetize
https://codereview.chromium.org/27223006/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/27223006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:1191: console.log(JSON.stringify(message)); On 2013/10/15 00:04:55, Robert Liao wrote: > This should either be removed or annotated with what this message is. Done. https://codereview.chromium.org/27223006/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/27223006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/manifest.json:2: //chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh On 2013/10/15 00:04:55, Robert Liao wrote: > How often do we expect this to change from here? This one should be last. The old one was retrieved in a wrong (arbitrary) way. This one - in a right way. More details - stop by. https://codereview.chromium.org/27223006/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/manifest.json:18: "identity", On 2013/10/15 00:04:55, Robert Liao wrote: > Alphabetize Done.
https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/manifest.json:2: //chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh One more change required in utility.js. https://code.google.com/p/chromium/codesearch#search/&q=pmofbkohncoogjjhahejj...
https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/manifest.json:2: //chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh On 2013/10/15 16:57:05, Robert Liao wrote: > One more change required in utility.js. > > https://code.google.com/p/chromium/codesearch#search/&q=pmofbkohncoogjjhahejj... No, since that place is just an example of a format of a frame.
https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/manifest.json:2: //chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh Then the comment should be more like [Extension ID] rather than referencing this specific extension. A question that may come up in the future: What is pmofbkohncoogjjhahejjfbppikbjigm? It won't be easily found in the code search. On 2013/10/15 17:40:26, vadimt wrote: > On 2013/10/15 16:57:05, Robert Liao wrote: > > One more change required in utility.js. > > > > > https://code.google.com/p/chromium/codesearch#search/&q=pmofbkohncoogjjhahejj... > > No, since that place is just an example of a format of a frame.
https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/manifest.json:2: //chrome-extension://pafkbggdmjlpgkdkcbjmhmfcdpncadgh On 2013/10/15 17:44:47, Robert Liao wrote: > Then the comment should be more like [Extension ID] rather than referencing this > specific extension. > > A question that may come up in the future: > What is pmofbkohncoogjjhahejjfbppikbjigm? > It won't be easily found in the code search. > > On 2013/10/15 17:40:26, vadimt wrote: > > On 2013/10/15 16:57:05, Robert Liao wrote: > > > One more change required in utility.js. > > > > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=pmofbkohncoogjjhahejj... > > > > No, since that place is just an example of a format of a frame. > Done.
https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:1186: if (items && items.lastPollNowPayload != message.payload) { What are you expecting in the payload and why does it need to be different? https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:1189: authenticationManager.isSignedIn(function(token) { You can receive push messages without being signed in? I thought that was already a prereq. It also already calls setAuthorization soon after this. Is this necessary?
lgtm
https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:1186: if (items && items.lastPollNowPayload != message.payload) { On 2013/10/15 17:57:51, rgustafson wrote: > What are you expecting in the payload and why does it need to be different? When the extension start for the first time, you get a message with an empty payload. If you sign out and sign in, you get a message with the latest payload. To filter out this noise and to identify real messages, the server should send timestamps as a payload, and the client is supposed to check that the payload is not empty, and is different from last received one. https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:1189: authenticationManager.isSignedIn(function(token) { On 2013/10/15 17:57:51, rgustafson wrote: > You can receive push messages without being signed in? I thought that was > already a prereq. It also already calls setAuthorization soon after this. Is > this necessary? It's more for readability and solidity of the code. At the end, we'll poll if NC checkbox is on AND we are signed in. It will be strange if one of these conditions is missing.
https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:563: setAuthorization(request, function(success) { But the signed-in condition isn't missing, it's right here. What am I missing? Making another auth request doesn't change the behavior. https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:581: var requestParameters = '?timeZoneOffsetMs=' + Remove this. It was moved. https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:1186: if (items && items.lastPollNowPayload != message.payload) { On 2013/10/15 19:41:43, vadimt wrote: > On 2013/10/15 17:57:51, rgustafson wrote: > > What are you expecting in the payload and why does it need to be different? > > When the extension start for the first time, you get a message with an empty > payload. If you sign out and sign in, you get a message with the latest payload. > To filter out this noise and to identify real messages, the server should send > timestamps as a payload, and the client is supposed to check that the payload is > not empty, and is different from last received one. Some comments of that explanation, at least what you're expecting as the value of the payload, would go a very long way to let readers of this code understand.
https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:563: setAuthorization(request, function(success) { On 2013/10/15 20:56:54, rgustafson wrote: > But the signed-in condition isn't missing, it's right here. What am I missing? > Making another auth request doesn't change the behavior. I'm not saying I'm changing the behavior. The check there is for readability and solidity of the code. https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:581: var requestParameters = '?timeZoneOffsetMs=' + On 2013/10/15 20:56:54, rgustafson wrote: > Remove this. It was moved. Done. https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:1186: if (items && items.lastPollNowPayload != message.payload) { On 2013/10/15 20:56:54, rgustafson wrote: > On 2013/10/15 19:41:43, vadimt wrote: > > On 2013/10/15 17:57:51, rgustafson wrote: > > > What are you expecting in the payload and why does it need to be different? > > > > When the extension start for the first time, you get a message with an empty > > payload. If you sign out and sign in, you get a message with the latest > payload. > > To filter out this noise and to identify real messages, the server should send > > timestamps as a payload, and the client is supposed to check that the payload > is > > not empty, and is different from last received one. > > Some comments of that explanation, at least what you're expecting as the value > of the payload, would go a very long way to let readers of this code understand. Done.
https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:563: setAuthorization(request, function(success) { On 2013/10/15 21:14:09, vadimt wrote: > On 2013/10/15 20:56:54, rgustafson wrote: > > But the signed-in condition isn't missing, it's right here. What am I missing? > > Making another auth request doesn't change the behavior. > > I'm not saying I'm changing the behavior. The check there is for readability and > solidity of the code. There are two problems here: (1) Duplicate code between polling entry points if your goal is to also add NC checkbox code to there and another place for normally starting polling. See below comment. (2) Adding code that doesn't change any behavior -- you shouldn't be adding that code until it does change something. The behavior DOES change if you use it to enable bg mode, etc as below. https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:1191: requestNotificationGroups([]); Why isn't this (and the surrounding sign in check) onStateChange instead? In this case, it should be checking that all of the prereqs for polling are fulfilled and turning on the background permission to get into the same state as starting normal polling, right?
https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:563: setAuthorization(request, function(success) { On 2013/10/16 17:14:32, rgustafson wrote: > On 2013/10/15 21:14:09, vadimt wrote: > > On 2013/10/15 20:56:54, rgustafson wrote: > > > But the signed-in condition isn't missing, it's right here. What am I > missing? > > > Making another auth request doesn't change the behavior. > > > > I'm not saying I'm changing the behavior. The check there is for readability > and > > solidity of the code. > > There are two problems here: > (1) Duplicate code between polling entry points if your goal is to also add NC > checkbox code to there and another place for normally starting polling. See > below comment. > (2) Adding code that doesn't change any behavior -- you shouldn't be adding that > code until it does change something. The behavior DOES change if you use it to > enable bg mode, etc as below. I've changed implementation to use updateCardsAttempts.isRunning(). Thanks for the comments. This matches updateNotificationsCards(). updateCardsAttempts.isRunning() is our standard indication that we are in a polling state. Note that both are used in almost identical situations: (1) alarm for poll gets fired; (2) signal for poll arrives from the server. Checking for updateCardsAttempts.isRunning() may be currently doesn't change the behavior, but will, once we start taking into account the NC flag. https://codereview.chromium.org/27223006/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:1191: requestNotificationGroups([]); On 2013/10/16 17:14:32, rgustafson wrote: > Why isn't this (and the surrounding sign in check) onStateChange instead? In > this case, it should be checking that all of the prereqs for polling are > fulfilled and turning on the background permission to get into the same state as > starting normal polling, right? The fact that we received a push notification doesn't indicate changing of the state. This is similar to the fact that firing a poll alarm doesn't indicate a state change. The state remains the same. OnStateChange will be called once 'googleNowDisabled' in server responses changes.
lgtm
mpcomplete, please provide OWNER's approval for files in chrome\common\extensions\api\ The reason for changing the extension ID is that for push messaging, both server (sending messages) and client have to be associated with the same account. The old extension ID was not associated with the account that we'll use for the server, so we need to change it to a one created in a right way.
You could add the new ID and leave the old one in there if you need backwards compat. LGTM
Thanks! Since this is a component extension, and the old ID gets removed from Chrome sources, there is no need for backward compatibility.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/27223006/33001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/27223006/33001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/27223006/33001
Message was sent while issue was closed.
Change committed as 229311 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
