Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
160923002:
Fixes a typo: use the default image if the extension doesn't have icon. (Closed)
|
Created:
6 years, 10 months ago by Jun Mukai Modified:
6 years, 10 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFixes a typo: use the default image if the extension doesn't have icon.
BUG=343115
R=dewittj@chromium.org, atwilson@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251715
Patch Set 1 #Patch Set 2 : re-upload #
Total comments: 2
Patch Set 3 : fix #Patch Set 4 : tests added #Patch Set 5 : re-upload #
Total comments: 2
Patch Set 6 : fix #Patch Set 7 : fix #Patch Set 8 : fix #Patch Set 9 : fix win #
Messages
Total messages: 23 (0 generated)
lgtm https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... chrome/browser/background/background_contents_service.cc:169: if (icon.IsEmpty()) { nit: change this to notification_icon.IsEmpty
On 2014/02/12 21:29:55, dewittj wrote: > lgtm > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > File chrome/browser/background/background_contents_service.cc (right): > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > chrome/browser/background/background_contents_service.cc:169: if > (icon.IsEmpty()) { > nit: change this to notification_icon.IsEmpty + worth adding a test?
https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... chrome/browser/background/background_contents_service.cc:169: if (icon.IsEmpty()) { On 2014/02/12 21:29:55, dewittj wrote: > nit: change this to notification_icon.IsEmpty Done.
On 2014/02/12 21:30:40, dewittj wrote: > On 2014/02/12 21:29:55, dewittj wrote: > > lgtm > > > > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > > File chrome/browser/background/background_contents_service.cc (right): > > > > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > > chrome/browser/background/background_contents_service.cc:169: if > > (icon.IsEmpty()) { > > nit: change this to notification_icon.IsEmpty > > + worth adding a test? I am not sure how to test this... it seems background_contents_service_unittest.cc doesn't contain the test cases for extension crashes. Could it be in another CL?
On 2014/02/12 21:44:09, Jun Mukai wrote: > On 2014/02/12 21:30:40, dewittj wrote: > > On 2014/02/12 21:29:55, dewittj wrote: > > > lgtm > > > > > > > > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > > > File chrome/browser/background/background_contents_service.cc (right): > > > > > > > > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > > > chrome/browser/background/background_contents_service.cc:169: if > > > (icon.IsEmpty()) { > > > nit: change this to notification_icon.IsEmpty > > > > + worth adding a test? > > I am not sure how to test this... it seems > background_contents_service_unittest.cc doesn't contain the test cases for > extension crashes. > Could it be in another CL? Is this fix time-sensitive? Are you planning to merge?
On 2014/02/12 21:47:01, dewittj wrote: > On 2014/02/12 21:44:09, Jun Mukai wrote: > > On 2014/02/12 21:30:40, dewittj wrote: > > > On 2014/02/12 21:29:55, dewittj wrote: > > > > lgtm > > > > > > > > > > > > > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > > > > File chrome/browser/background/background_contents_service.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > > > > chrome/browser/background/background_contents_service.cc:169: if > > > > (icon.IsEmpty()) { > > > > nit: change this to notification_icon.IsEmpty > > > > > > + worth adding a test? > > > > I am not sure how to test this... it seems > > background_contents_service_unittest.cc doesn't contain the test cases for > > extension crashes. > > Could it be in another CL? > > Is this fix time-sensitive? Are you planning to merge? reporter said this is P1/M33, but I don't think this needs to be merged. Okay fixed by the branch point.
On 2014/02/12 21:53:41, Jun Mukai wrote: > On 2014/02/12 21:47:01, dewittj wrote: > > On 2014/02/12 21:44:09, Jun Mukai wrote: > > > On 2014/02/12 21:30:40, dewittj wrote: > > > > On 2014/02/12 21:29:55, dewittj wrote: > > > > > lgtm > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > > > > > File chrome/browser/background/background_contents_service.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > > > > > chrome/browser/background/background_contents_service.cc:169: if > > > > > (icon.IsEmpty()) { > > > > > nit: change this to notification_icon.IsEmpty > > > > > > > > + worth adding a test? > > > > > > I am not sure how to test this... it seems > > > background_contents_service_unittest.cc doesn't contain the test cases for > > > extension crashes. > > > Could it be in another CL? > > > > Is this fix time-sensitive? Are you planning to merge? > > reporter said this is P1/M33, but I don't think this needs to be merged. > Okay fixed by the branch point. note to myself: ExtensionImageLoader needs some BrowserThreads would need to be a browser test.
On 2014/02/13 20:49:43, Jun Mukai wrote: > On 2014/02/12 21:53:41, Jun Mukai wrote: > > On 2014/02/12 21:47:01, dewittj wrote: > > > On 2014/02/12 21:44:09, Jun Mukai wrote: > > > > On 2014/02/12 21:30:40, dewittj wrote: > > > > > On 2014/02/12 21:29:55, dewittj wrote: > > > > > > lgtm > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > > > > > > File chrome/browser/background/background_contents_service.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > > > > > > chrome/browser/background/background_contents_service.cc:169: if > > > > > > (icon.IsEmpty()) { > > > > > > nit: change this to notification_icon.IsEmpty > > > > > > > > > > + worth adding a test? > > > > > > > > I am not sure how to test this... it seems > > > > background_contents_service_unittest.cc doesn't contain the test cases for > > > > extension crashes. > > > > Could it be in another CL? > > > > > > Is this fix time-sensitive? Are you planning to merge? > > > > reporter said this is P1/M33, but I don't think this needs to be merged. > > Okay fixed by the branch point. > > note to myself: ExtensionImageLoader needs some BrowserThreads would > need to be a browser test. aah, not necessarily to be a browser_test fortunately. Uploaded with tests. PTAL.
On 2014/02/15 18:15:29, Jun Mukai wrote: > On 2014/02/13 20:49:43, Jun Mukai wrote: > > On 2014/02/12 21:53:41, Jun Mukai wrote: > > > On 2014/02/12 21:47:01, dewittj wrote: > > > > On 2014/02/12 21:44:09, Jun Mukai wrote: > > > > > On 2014/02/12 21:30:40, dewittj wrote: > > > > > > On 2014/02/12 21:29:55, dewittj wrote: > > > > > > > lgtm > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > > > > > > > File chrome/browser/background/background_contents_service.cc > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > > > > > > > chrome/browser/background/background_contents_service.cc:169: if > > > > > > > (icon.IsEmpty()) { > > > > > > > nit: change this to notification_icon.IsEmpty > > > > > > > > > > > > + worth adding a test? > > > > > > > > > > I am not sure how to test this... it seems > > > > > background_contents_service_unittest.cc doesn't contain the test cases > for > > > > > extension crashes. > > > > > Could it be in another CL? > > > > > > > > Is this fix time-sensitive? Are you planning to merge? > > > > > > reporter said this is P1/M33, but I don't think this needs to be merged. > > > Okay fixed by the branch point. > > > > note to myself: ExtensionImageLoader needs some BrowserThreads would > > need to be a browser test. > > aah, not necessarily to be a browser_test fortunately. > Uploaded with tests. PTAL. Thanks! Lgtm++
On 2014/02/15 18:46:38, dewittj wrote: > On 2014/02/15 18:15:29, Jun Mukai wrote: > > On 2014/02/13 20:49:43, Jun Mukai wrote: > > > On 2014/02/12 21:53:41, Jun Mukai wrote: > > > > On 2014/02/12 21:47:01, dewittj wrote: > > > > > On 2014/02/12 21:44:09, Jun Mukai wrote: > > > > > > On 2014/02/12 21:30:40, dewittj wrote: > > > > > > > On 2014/02/12 21:29:55, dewittj wrote: > > > > > > > > lgtm > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > > > > > > > > File chrome/browser/background/background_contents_service.cc > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > > > > > > > > chrome/browser/background/background_contents_service.cc:169: if > > > > > > > > (icon.IsEmpty()) { > > > > > > > > nit: change this to notification_icon.IsEmpty > > > > > > > > > > > > > > + worth adding a test? > > > > > > > > > > > > I am not sure how to test this... it seems > > > > > > background_contents_service_unittest.cc doesn't contain the test cases > > for > > > > > > extension crashes. > > > > > > Could it be in another CL? > > > > > > > > > > Is this fix time-sensitive? Are you planning to merge? > > > > > > > > reporter said this is P1/M33, but I don't think this needs to be merged. > > > > Okay fixed by the branch point. > > > > > > note to myself: ExtensionImageLoader needs some BrowserThreads would > > > need to be a browser test. > > > > aah, not necessarily to be a browser_test fortunately. > > Uploaded with tests. PTAL. > > Thanks! Lgtm++ May want to land this manually so it makes m34
On 2014/02/15 18:47:07, dewittj wrote: > On 2014/02/15 18:46:38, dewittj wrote: > > On 2014/02/15 18:15:29, Jun Mukai wrote: > > > On 2014/02/13 20:49:43, Jun Mukai wrote: > > > > On 2014/02/12 21:53:41, Jun Mukai wrote: > > > > > On 2014/02/12 21:47:01, dewittj wrote: > > > > > > On 2014/02/12 21:44:09, Jun Mukai wrote: > > > > > > > On 2014/02/12 21:30:40, dewittj wrote: > > > > > > > > On 2014/02/12 21:29:55, dewittj wrote: > > > > > > > > > lgtm > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > > > > > > > > > File chrome/browser/background/background_contents_service.cc > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/160923002/diff/30001/chrome/browser/backgroun... > > > > > > > > > chrome/browser/background/background_contents_service.cc:169: if > > > > > > > > > (icon.IsEmpty()) { > > > > > > > > > nit: change this to notification_icon.IsEmpty > > > > > > > > > > > > > > > > + worth adding a test? > > > > > > > > > > > > > > I am not sure how to test this... it seems > > > > > > > background_contents_service_unittest.cc doesn't contain the test > cases > > > for > > > > > > > extension crashes. > > > > > > > Could it be in another CL? > > > > > > > > > > > > Is this fix time-sensitive? Are you planning to merge? > > > > > > > > > > reporter said this is P1/M33, but I don't think this needs to be merged. > > > > > Okay fixed by the branch point. > > > > > > > > note to myself: ExtensionImageLoader needs some BrowserThreads would > > > > need to be a browser test. > > > > > > aah, not necessarily to be a browser_test fortunately. > > > Uploaded with tests. PTAL. > > > > Thanks! Lgtm++ > > May want to land this manually so it makes m34 We're not the owners of the files. atwilson, could you take a look?
lgtm, with one change I don't really want those two new static functions to be part of the public API of BackgroundContentsService. So either make them private and declare the unittests as friends, or expose them as XXXXXForTest() APIs so they don't start getting called by random code. https://codereview.chromium.org/160923002/diff/190001/chrome/browser/backgrou... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/160923002/diff/190001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:170: if (notification_icon.IsEmpty()) { Why is this changing to use notification_icon instead of icon?
Renamed the methods with 'ForTesting' https://codereview.chromium.org/160923002/diff/190001/chrome/browser/backgrou... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/160923002/diff/190001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:170: if (notification_icon.IsEmpty()) { On 2014/02/17 17:12:32, Andrew T Wilson wrote: > Why is this changing to use notification_icon instead of icon? suggestion from the other reviewer. We created 'notification_icon' from input, and just seeing the created one instead of the input makes sense to me too.
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/160923002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/160923002/520001
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/160923002/390003
Message was sent while issue was closed.
Change committed as 251715 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
