Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8568031:
Call plugin.onIq() only when onIq is not null. (Closed)
|
Created:
9 years, 1 month ago by Sergey Ulanov Modified:
9 years, 1 month ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, dmaclach+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, ajwong+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCall plugin.onIq() only when onIq is not null.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110896
Patch Set 1 #
Total comments: 4
Patch Set 2 : - #Messages
Total messages: 13 (0 generated)
I saw the following exception thrown on shutdown due to this bug:
[12523:12523:1144244985753:INFO:CONSOLE(198)] "TypeError: Cannot call method 'on
Iq' of null
at [object Object].onIq_ (chrome-extension://acpejakddbkhkjimgifmjdenkfcofck
o/client_session.js:275:17)
at [object Object].onMessage_ (chrome-extension://acpejakddbkhkjimgifmjdenkf
cofcko/wcs.js:110:10)
at [object Object].gk (chrome-extension://acpejakddbkhkjimgifmjdenkfcofcko/w
cs.js:80:40)
at [object Object].Dc (https://talkgadget.google.com/talkgadget/iq?access_to
ken=ya29.AHES6ZTTFB6xGUQrxDULIbpKlKBXHZ3DHGaY4vFk42pnlhQ:199:521)
at [object Object].$i (https://talkgadget.google.com/talkgadget/iq?access_to
ken=ya29.AHES6ZTTFB6xGUQrxDULIbpKlKBXHZ3DHGaY4vFk42pnlhQ:198:375)
at [object Object].handleEvent (https://talkgadget.google.com/talkgadget/iq?
access_token=ya29.AHES6ZTTFB6xGUQrxDULIbpKlKBXHZ3DHGaY4vFk42pnlhQ:36:761)
Jamie: ping
http://codereview.chromium.org/8568031/diff/1/remoting/webapp/me2mom/client_s... File remoting/webapp/me2mom/client_session.js (right): http://codereview.chromium.org/8568031/diff/1/remoting/webapp/me2mom/client_s... remoting/webapp/me2mom/client_session.js:275: if (that.plugin.onIq) Do we want to log something different if there is no onIq(), so we can tell that the message wasn't sent?
http://codereview.chromium.org/8568031/diff/1/remoting/webapp/me2mom/client_s... File remoting/webapp/me2mom/client_session.js (right): http://codereview.chromium.org/8568031/diff/1/remoting/webapp/me2mom/client_s... remoting/webapp/me2mom/client_session.js:275: if (that.plugin.onIq) On 2011/11/19 00:59:14, Wez wrote: > Do we want to log something different if there is no onIq(), so we can tell that > the message wasn't sent? +1 We shouldn't be logging the stanza if we didn't actually send it. Or we should note that we were unable to send the stanza.
http://codereview.chromium.org/8568031/diff/1/remoting/webapp/me2mom/client_s... File remoting/webapp/me2mom/client_session.js (right): http://codereview.chromium.org/8568031/diff/1/remoting/webapp/me2mom/client_s... remoting/webapp/me2mom/client_session.js:275: if (that.plugin.onIq) On 2011/11/19 00:59:14, Wez wrote: > Do we want to log something different if there is no onIq(), so we can tell that > the message wasn't sent? This function handles incoming messages, not outgoing, i.e. it doesn't send anything. onIq isn't set here after the plugin has been shut down after disconnection, so I don't think it's not very important to log this event here. Added log message anyway. http://codereview.chromium.org/8568031/diff/1/remoting/webapp/me2mom/client_s... remoting/webapp/me2mom/client_session.js:275: if (that.plugin.onIq) On 2011/11/19 01:12:49, garykac wrote: > On 2011/11/19 00:59:14, Wez wrote: > > Do we want to log something different if there is no onIq(), so we can tell > that > > the message wasn't sent? > > +1 > We shouldn't be logging the stanza if we didn't actually send it. > Or we should note that we were unable to send the stanza. We don't send anything here, it's for incoming messages.
On 2011/11/19 01:30:52, sergeyu wrote: > http://codereview.chromium.org/8568031/diff/1/remoting/webapp/me2mom/client_s... > File remoting/webapp/me2mom/client_session.js (right): > > http://codereview.chromium.org/8568031/diff/1/remoting/webapp/me2mom/client_s... > remoting/webapp/me2mom/client_session.js:275: if (that.plugin.onIq) > On 2011/11/19 00:59:14, Wez wrote: > > Do we want to log something different if there is no onIq(), so we can tell > that > > the message wasn't sent? > > This function handles incoming messages, not outgoing, i.e. it doesn't send > anything. onIq isn't set here after the plugin has been shut down after > disconnection, so I don't think it's not very important to log this event here. > Added log message anyway. > > http://codereview.chromium.org/8568031/diff/1/remoting/webapp/me2mom/client_s... > remoting/webapp/me2mom/client_session.js:275: if (that.plugin.onIq) > On 2011/11/19 01:12:49, garykac wrote: > > On 2011/11/19 00:59:14, Wez wrote: > > > Do we want to log something different if there is no onIq(), so we can tell > > that > > > the message wasn't sent? > > > > +1 > > We shouldn't be logging the stanza if we didn't actually send it. > > Or we should note that we were unable to send the stanza. > > We don't send anything here, it's for incoming messages. LGTM
Sorry for the delay. LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/8568031/6001
Try job failure for 8568031-6001 (retry) on linux_rel for step "ui_tests". It's a second try, previously, step "ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/8568031/6001
Try job failure for 8568031-6001 (retry) on linux_rel for step "ui_tests". It's a second try, previously, step "ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/8568031/6001
Change committed as 110896 |
