Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1432803002:
Removed emojis from generated_resources.grd and update the code. (Closed)
|
Created:
5 years, 1 month ago by shahriar Modified:
5 years, 1 month ago Reviewers:
Robert Sesek CC:
chromium-reviews, asanka, benjhayden+dwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIndicate in the Window menu which Chrome window has an active sound playing
Continuing https://codereview.chromium.org/1412083002 but:
Removed emojis from generated_resources.grd and update the code.
Now we are keeping Emojis in the code rather than resource files.
BUG=498288
TEST=Follow these steps (taken from bug report):
1. Open multiple Chrome windows (optionally with multiple tabs)
2. Play sound clip (e.g via youtube) on a given tab
3. Drop down the Window menu
4. It is also possible to mute the tab and see what will happen but
chrome://flags/#enable-tab-audio-muting should be enable.
Committed: https://crrev.com/eed68eca8150449bb6749bd7900320b2c0594f45
Cr-Commit-Position: refs/heads/master@{#361238}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Inlined emojis and remove accessors #
Total comments: 5
Patch Set 3 : Reflect recent review comments #Patch Set 4 : Fixed the unit test build failure by casting NSNotFound #
Total comments: 3
Patch Set 5 : Applied some fixes based on the recent nits #
Total comments: 1
Patch Set 6 : Prevent leaking strings in the unit test #Messages
Total messages: 33 (9 generated)
Description was changed from ========== Removed emojis from generated_resources.grd and update the code. Now we are keeping Emojis in the code rather than resource files. BUG=498288 ========== to ========== Removed emojis from generated_resources.grd and update the code. Now we are keeping Emojis in the code rather than resource files. BUG=498288 ==========
shahriar.rostami@gmail.com changed reviewers: + rsesek@chromium.org
PTAL.
Description was changed from ========== Removed emojis from generated_resources.grd and update the code. Now we are keeping Emojis in the code rather than resource files. BUG=498288 ========== to ========== Removed emojis from generated_resources.grd and update the code. Now we are keeping Emojis in the code rather than resource files. BUG=498288 Continuing https://codereview.chromium.org/1412083002 ==========
https://codereview.chromium.org/1432803002/diff/1/chrome/browser/ui/cocoa/bro... File chrome/browser/ui/cocoa/browser_window_cocoa.h (right): https://codereview.chromium.org/1432803002/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_cocoa.h:183: void UpdateMediaState(TabMediaState media_state); This part of your CL has already landed. You should sync your tree and rebase this change on top of that, so that you're only including new code. https://codereview.chromium.org/1432803002/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_cocoa.h:196: NSString* media_playing_emoji() { return @"🔊"; } I don't think these need accessors at all. Just place them inline in the code. Alternatively, just make them constants rather than members.
On 2015/11/06 15:23:14, Robert Sesek wrote: > https://codereview.chromium.org/1432803002/diff/1/chrome/browser/ui/cocoa/bro... > File chrome/browser/ui/cocoa/browser_window_cocoa.h (right): > > https://codereview.chromium.org/1432803002/diff/1/chrome/browser/ui/cocoa/bro... > chrome/browser/ui/cocoa/browser_window_cocoa.h:183: void > UpdateMediaState(TabMediaState media_state); > This part of your CL has already landed. You should sync your tree and rebase > this change on top of that, so that you're only including new code. > > https://codereview.chromium.org/1432803002/diff/1/chrome/browser/ui/cocoa/bro... > chrome/browser/ui/cocoa/browser_window_cocoa.h:196: NSString* > media_playing_emoji() { return @"🔊"; } > I don't think these need accessors at all. Just place them inline in the code. > Alternatively, just make them constants rather than members. In terms of rebasing, you are right, I will do that, thanks. But is this revert https://codereview.chromium.org/1412803012/ means my commit did not apply to the tree and I have to commit the whole thing with the fixes?
On 2015/11/09 04:01:03, shahriar wrote: > On 2015/11/06 15:23:14, Robert Sesek wrote: > > > https://codereview.chromium.org/1432803002/diff/1/chrome/browser/ui/cocoa/bro... > > File chrome/browser/ui/cocoa/browser_window_cocoa.h (right): > > > > > https://codereview.chromium.org/1432803002/diff/1/chrome/browser/ui/cocoa/bro... > > chrome/browser/ui/cocoa/browser_window_cocoa.h:183: void > > UpdateMediaState(TabMediaState media_state); > > This part of your CL has already landed. You should sync your tree and rebase > > this change on top of that, so that you're only including new code. > > > > > https://codereview.chromium.org/1432803002/diff/1/chrome/browser/ui/cocoa/bro... > > chrome/browser/ui/cocoa/browser_window_cocoa.h:196: NSString* > > media_playing_emoji() { return @"🔊"; } > > I don't think these need accessors at all. Just place them inline in the code. > > Alternatively, just make them constants rather than members. > > In terms of rebasing, you are right, I will do that, thanks. > But is this revert https://codereview.chromium.org/1412803012/ means my commit > did not apply to the tree > and I have to commit the whole thing with the fixes? Ah, I wasn't added to that review for some reason. In that case, let's just continue on this review.
On 2015/11/09 20:52:20, Robert Sesek (OOO til 23.11) wrote: > On 2015/11/09 04:01:03, shahriar wrote: > > On 2015/11/06 15:23:14, Robert Sesek wrote: > > > > > > https://codereview.chromium.org/1432803002/diff/1/chrome/browser/ui/cocoa/bro... > > > File chrome/browser/ui/cocoa/browser_window_cocoa.h (right): > > > > > > > > > https://codereview.chromium.org/1432803002/diff/1/chrome/browser/ui/cocoa/bro... > > > chrome/browser/ui/cocoa/browser_window_cocoa.h:183: void > > > UpdateMediaState(TabMediaState media_state); > > > This part of your CL has already landed. You should sync your tree and > rebase > > > this change on top of that, so that you're only including new code. > > > > > > > > > https://codereview.chromium.org/1432803002/diff/1/chrome/browser/ui/cocoa/bro... > > > chrome/browser/ui/cocoa/browser_window_cocoa.h:196: NSString* > > > media_playing_emoji() { return @"🔊"; } > > > I don't think these need accessors at all. Just place them inline in the > code. > > > Alternatively, just make them constants rather than members. > > > > In terms of rebasing, you are right, I will do that, thanks. > > But is this revert https://codereview.chromium.org/1412803012/ means my commit > > did not apply to the tree > > and I have to commit the whole thing with the fixes? > > Ah, I wasn't added to that review for some reason. In that case, let's just > continue on this review. Please take another look.
On 2015/11/09 23:31:18, shahriar wrote: > On 2015/11/09 20:52:20, Robert Sesek (OOO til 23.11) wrote: > > On 2015/11/09 04:01:03, shahriar wrote: > > > On 2015/11/06 15:23:14, Robert Sesek wrote: > > > > > > > > > > https://codereview.chromium.org/1432803002/diff/1/chrome/browser/ui/cocoa/bro... > > > > File chrome/browser/ui/cocoa/browser_window_cocoa.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1432803002/diff/1/chrome/browser/ui/cocoa/bro... > > > > chrome/browser/ui/cocoa/browser_window_cocoa.h:183: void > > > > UpdateMediaState(TabMediaState media_state); > > > > This part of your CL has already landed. You should sync your tree and > > rebase > > > > this change on top of that, so that you're only including new code. > > > > > > > > > > > > > > https://codereview.chromium.org/1432803002/diff/1/chrome/browser/ui/cocoa/bro... > > > > chrome/browser/ui/cocoa/browser_window_cocoa.h:196: NSString* > > > > media_playing_emoji() { return @"🔊"; } > > > > I don't think these need accessors at all. Just place them inline in the > > code. > > > > Alternatively, just make them constants rather than members. > > > > > > In terms of rebasing, you are right, I will do that, thanks. > > > But is this revert https://codereview.chromium.org/1412803012/ means my > commit > > > did not apply to the tree > > > and I have to commit the whole thing with the fixes? > > > > Ah, I wasn't added to that review for some reason. In that case, let's just > > continue on this review. > > Please take another look. For the build bot failure (last commit), should I find a workaround?
On 2015/11/09 23:33:43, shahriar wrote: > For the build bot failure (last commit), should I find a workaround? Yes. The unsatisfying fix we came up with is https://codereview.chromium.org/1398013003/diff/1/ui/gfx/range/range_mac_unit.... Make that a constant in your test and use that to compare against? https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:309: NSString* playing_emoji = [[NSString alloc] initWithString:@"🔊"]; This is leaked (alloc with no release). You can just use @"" literals to SysNSStringToUTF16. https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.h (right): https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.h:280: on:(content::WebContents*)changed; naming: instead of just "on:", this would be more descriptive as "forWebContents:" https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:253: - (BOOL)isAnyOtherTab:(content::WebContents*)selected Similarly, "doesAnyOtherWebContents:haveMediaState:" would be more descriptive https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:1632: nit: remove blank line https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_strip_controller_unittest.mm (right): https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller_unittest.mm:35: std::map<content::WebContents*, TabMediaState> contents_media_state_maps; naming: contentsMediaStateMaps_
On 2015/11/09 23:41:25, Robert Sesek (OOO til 23.11) wrote: > On 2015/11/09 23:33:43, shahriar wrote: > > For the build bot failure (last commit), should I find a workaround? > > Yes. The unsatisfying fix we came up with is > https://codereview.chromium.org/1398013003/diff/1/ui/gfx/range/range_mac_unit.... > > Make that a constant in your test and use that to compare against? > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:309: NSString* playing_emoji = > [[NSString alloc] initWithString:@"🔊"]; > This is leaked (alloc with no release). You can just use @"" literals to > SysNSStringToUTF16. > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/tabs/tab_strip_controller.h (right): > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.h:280: > on:(content::WebContents*)changed; > naming: instead of just "on:", this would be more descriptive as > "forWebContents:" > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:253: - > (BOOL)isAnyOtherTab:(content::WebContents*)selected > Similarly, "doesAnyOtherWebContents:haveMediaState:" would be more descriptive > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:1632: > nit: remove blank line > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/tabs/tab_strip_controller_unittest.mm (right): > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_controller_unittest.mm:35: > std::map<content::WebContents*, TabMediaState> contents_media_state_maps; > naming: contentsMediaStateMaps_ I tried to have something like this: static const NSUInteger NSNotFound = NSIntegerMax; and use this instead, however got symbol(s) not found for architecture x86_64 which is obvious. And also cannot use NSInteger because of : comparison of integers of different signs: 'const unsigned long' and 'const long'. Any hint, please?
On 2015/11/10 00:47:56, shahriar wrote: > On 2015/11/09 23:41:25, Robert Sesek (OOO til 23.11) wrote: > > On 2015/11/09 23:33:43, shahriar wrote: > > > For the build bot failure (last commit), should I find a workaround? > > > > Yes. The unsatisfying fix we came up with is > > > https://codereview.chromium.org/1398013003/diff/1/ui/gfx/range/range_mac_unit.... > > > > Make that a constant in your test and use that to compare against? > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > chrome/browser/ui/cocoa/browser_window_cocoa.mm:309: NSString* playing_emoji = > > [[NSString alloc] initWithString:@"🔊"]; > > This is leaked (alloc with no release). You can just use @"" literals to > > SysNSStringToUTF16. > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > File chrome/browser/ui/cocoa/tabs/tab_strip_controller.h (right): > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > chrome/browser/ui/cocoa/tabs/tab_strip_controller.h:280: > > on:(content::WebContents*)changed; > > naming: instead of just "on:", this would be more descriptive as > > "forWebContents:" > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:253: - > > (BOOL)isAnyOtherTab:(content::WebContents*)selected > > Similarly, "doesAnyOtherWebContents:haveMediaState:" would be more descriptive > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:1632: > > nit: remove blank line > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > File chrome/browser/ui/cocoa/tabs/tab_strip_controller_unittest.mm (right): > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > chrome/browser/ui/cocoa/tabs/tab_strip_controller_unittest.mm:35: > > std::map<content::WebContents*, TabMediaState> contents_media_state_maps; > > naming: contentsMediaStateMaps_ > > I tried to have something like this: static const NSUInteger NSNotFound = > NSIntegerMax; and use this instead, > however got symbol(s) not found for architecture x86_64 which is obvious. > And also cannot use NSInteger because of : > comparison of integers of different signs: 'const unsigned long' and 'const > long'. > Any hint, please? static const NSUInteger kNotFound = static_cast<NSUInteger>(NSNotFound);
On 2015/11/11 22:33:12, Robert Sesek (OOO til 23.11) wrote: > On 2015/11/10 00:47:56, shahriar wrote: > > On 2015/11/09 23:41:25, Robert Sesek (OOO til 23.11) wrote: > > > On 2015/11/09 23:33:43, shahriar wrote: > > > > For the build bot failure (last commit), should I find a workaround? > > > > > > Yes. The unsatisfying fix we came up with is > > > > > > https://codereview.chromium.org/1398013003/diff/1/ui/gfx/range/range_mac_unit.... > > > > > > Make that a constant in your test and use that to compare against? > > > > > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > > File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): > > > > > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > > chrome/browser/ui/cocoa/browser_window_cocoa.mm:309: NSString* playing_emoji > = > > > [[NSString alloc] initWithString:@"🔊"]; > > > This is leaked (alloc with no release). You can just use @"" literals to > > > SysNSStringToUTF16. > > > > > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > > File chrome/browser/ui/cocoa/tabs/tab_strip_controller.h (right): > > > > > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > > chrome/browser/ui/cocoa/tabs/tab_strip_controller.h:280: > > > on:(content::WebContents*)changed; > > > naming: instead of just "on:", this would be more descriptive as > > > "forWebContents:" > > > > > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > > File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): > > > > > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:253: - > > > (BOOL)isAnyOtherTab:(content::WebContents*)selected > > > Similarly, "doesAnyOtherWebContents:haveMediaState:" would be more > descriptive > > > > > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:1632: > > > nit: remove blank line > > > > > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > > File chrome/browser/ui/cocoa/tabs/tab_strip_controller_unittest.mm (right): > > > > > > > > > https://codereview.chromium.org/1432803002/diff/20001/chrome/browser/ui/cocoa... > > > chrome/browser/ui/cocoa/tabs/tab_strip_controller_unittest.mm:35: > > > std::map<content::WebContents*, TabMediaState> contents_media_state_maps; > > > naming: contentsMediaStateMaps_ > > > > I tried to have something like this: static const NSUInteger NSNotFound = > > NSIntegerMax; and use this instead, > > however got symbol(s) not found for architecture x86_64 which is obvious. > > And also cannot use NSInteger because of : > > comparison of integers of different signs: 'const unsigned long' and 'const > > long'. > > Any hint, please? > > static const NSUInteger kNotFound = static_cast<NSUInteger>(NSNotFound); I changed the order of arguments because of this review: https://codereview.chromium.org/1411043009/diff/1/chrome/browser/ui/cocoa/bro... I applied the review sdefresne@ got on his CL, should I mention this somewhere to be ethical? PTAL.
I don't think you need to credit sdefresne@ since it's such a minor contribution, though thank you for the consideration. Also, sorry for the delay. I was out of the country for two weeks. https://codereview.chromium.org/1432803002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): https://codereview.chromium.org/1432803002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:309: // NSString* playing_emoji = [[NSString alloc] initWithString:@"🔊"]; Remove commented-out code. https://codereview.chromium.org/1432803002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/1432803002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:255: nit: remove blank line https://codereview.chromium.org/1432803002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2323: haveMediaState:TAB_MEDIA_STATE_AUDIO_PLAYING]) nit: needs {}
On 2015/11/21 03:39:01, Robert Sesek wrote: > I don't think you need to credit sdefresne@ since it's such a minor > contribution, though thank you for the consideration. > > Also, sorry for the delay. I was out of the country for two weeks. > > https://codereview.chromium.org/1432803002/diff/60001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): > > https://codereview.chromium.org/1432803002/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:309: // NSString* playing_emoji > = [[NSString alloc] initWithString:@"🔊"]; > Remove commented-out code. > > https://codereview.chromium.org/1432803002/diff/60001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): > > https://codereview.chromium.org/1432803002/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:255: > nit: remove blank line > > https://codereview.chromium.org/1432803002/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2323: > haveMediaState:TAB_MEDIA_STATE_AUDIO_PLAYING]) > nit: needs {} Thanks Robert, I learned many things from you in this CL. PTAL.
https://codereview.chromium.org/1432803002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm (right): https://codereview.chromium.org/1432803002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:56: NSString* playing_emoji = [[NSString alloc] initWithString:@"🔊"]; This is now leaked, since this is using alloc/init. You can just use NSString* playing_emoji = @"🔊"
On 2015/11/23 16:25:17, Robert Sesek wrote: > https://codereview.chromium.org/1432803002/diff/80001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm (right): > > https://codereview.chromium.org/1432803002/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:56: NSString* > playing_emoji = [[NSString alloc] initWithString:@"🔊"]; > This is now leaked, since this is using alloc/init. You can just use NSString* > playing_emoji = @"🔊" PTAL.
LGTM
The CQ bit was checked by shahriar.rostami@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1432803002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432803002/100001
Description was changed from ========== Removed emojis from generated_resources.grd and update the code. Now we are keeping Emojis in the code rather than resource files. BUG=498288 Continuing https://codereview.chromium.org/1412083002 ========== to ========== Indicate in the Window menu which Chrome window has an active sound playing Continuing https://codereview.chromium.org/1412083002 but: Removed emojis from generated_resources.grd and update the code. Now we are keeping Emojis in the code rather than resource files. BUG=498288 TEST=Follow these steps (taken from bug report): 1. Open multiple Chrome windows (optionally with multiple tabs) 2. Play sound clip (e.g via youtube) on a given tab 3. Drop down the Window menu 4. It is also possible to mute the tab and see what will happen but chrome://flags/#enable-tab-audio-muting should be enable. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shahriar.rostami@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1432803002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432803002/100001
On 2015/11/23 18:44:38, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) I don't understand why "DownloadNotificationTest.CancelDownload" should be failed. It's not related to the changes that we made. Do you have any idea rsesek@?
On 2015/11/23 21:08:21, shahriar wrote: > On 2015/11/23 18:44:38, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > I don't understand why "DownloadNotificationTest.CancelDownload" should be > failed. > It's not related to the changes that we made. Do you have any idea rsesek@? I got that failure earlier today on a different CL. It's not related, so just try submitting again.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shahriar.rostami@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1432803002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432803002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/eed68eca8150449bb6749bd7900320b2c0594f45 Cr-Commit-Position: refs/heads/master@{#361238} |
