Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16075005:
Everybody nulls sometimes (Closed)
|
Created:
7 years, 6 months ago by please use gerrit instead Modified:
7 years, 6 months ago CC:
chromium-reviews, groby+spellwatch_chromium.org, rpetterson, rouslan+spellwatch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionCheck for NULL spellcheck service when periodically receiving a list of markers
This CL alters SpellcheckMessageFilter to check for NULL spellcheck service when
it periodically receives a list of document markers from the renderer.
BUG=244521
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203054
Patch Set 1 #
Total comments: 1
Patch Set 2 : Update comment #
Total comments: 2
Patch Set 3 : Fix typo in comment #Patch Set 4 : Update comment #Messages
Total messages: 18 (0 generated)
Groby: PTAL (http://youtu.be/ijZRCIrTgQc)
LGTM with comment nit. Also, https://plus.google.com/107226275692313566931/posts/DreQhUtXCuT https://codereview.chromium.org/16075005/diff/1/chrome/browser/spellchecker/s... File chrome/browser/spellchecker/spellcheck_message_filter.cc (right): https://codereview.chromium.org/16075005/diff/1/chrome/browser/spellchecker/s... chrome/browser/spellchecker/spellcheck_message_filter.cc:104: // Spellcheck service may not be available for each renderer process ID. Spellcheck service _must_ be available for each process, or the ServiceFactory will create it. However, there is not necessarily a process for each ID (if the process is shutting down).
Taking back half of my comment nit - I forgot about incognito. (Which _does_ give a NULL service back. So what we're seeing in the field might be incognito spellchecking attempts)
On 2013/05/29 01:35:43, groby wrote: > Taking back half of my comment nit - I forgot about incognito. (Which _does_ > give a NULL service back. So what we're seeing in the field might be incognito > spellchecking attempts) Not sure about this. Spellcheck seems to works in incognito mode... I can see the service not being available for a process that is shutting down, however. I will add that comment.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/16075005/18001
Groby: Should we always NULL-check results from SpellcheckServiceFactory::GetForRenderProcessId(int) because of the NULL value on teardown? I can submit a CL for that, too.
drive-by typo... https://codereview.chromium.org/16075005/diff/18001/chrome/browser/spellcheck... File chrome/browser/spellchecker/spellcheck_message_filter.cc (right): https://codereview.chromium.org/16075005/diff/18001/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_message_filter.cc:104: // Spellcheck service may not be available a renderer that is shutting down. for/in a renderer?
https://codereview.chromium.org/16075005/diff/18001/chrome/browser/spellcheck... File chrome/browser/spellchecker/spellcheck_message_filter.cc (right): https://codereview.chromium.org/16075005/diff/18001/chrome/browser/spellcheck... chrome/browser/spellchecker/spellcheck_message_filter.cc:104: // Spellcheck service may not be available a renderer that is shutting down. On 2013/05/29 17:39:42, rpetterson wrote: > for/in a renderer? Done. Thank you.
On 2013/05/29 18:36:59, Rouslan Solomakhin wrote: > https://codereview.chromium.org/16075005/diff/18001/chrome/browser/spellcheck... > File chrome/browser/spellchecker/spellcheck_message_filter.cc (right): > > https://codereview.chromium.org/16075005/diff/18001/chrome/browser/spellcheck... > chrome/browser/spellchecker/spellcheck_message_filter.cc:104: // Spellcheck > service may not be available a renderer that is shutting down. > On 2013/05/29 17:39:42, rpetterson wrote: > > for/in a renderer? > > Done. Thank you. Scratch that. It's already in the queue. Maybe some other time =)
List of reviewers changed. rlp@chromium.org did a drive-by without LGTM'ing!
On 2013/05/29 19:14:32, I haz the power (commit-bot) wrote: > List of reviewers changed. mailto:rlp@chromium.org did a drive-by without LGTM'ing! Rachel: It's your lucky day ;-)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/16075005/39001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/16075005/39002
Hehe :) lgtm On May 29, 2013 12:21 PM, <rouslan@chromium.org> wrote: > On 2013/05/29 19:14:32, I haz the power (commit-bot) wrote: > >> List of reviewers changed. mailto:rlp@chromium.org did a drive-by without >> > LGTM'ing! > > Rachel: It's your lucky day ;-) > > https://codereview.chromium.**org/16075005/<https://codereview.chromium.org/1... >
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=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/16075005/39002
Message was sent while issue was closed.
Change committed as 203054
Message was sent while issue was closed.
NECESITO MI CORRECTOR ORTOGRAFICO |
