Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1412083002:
Indicate in the Window menu which Chrome window has an active sound playing (Closed)
DescriptionIndicate in the Window menu which Chrome window has an active sound playing
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/70d337daa152a75db530fa04214caba48ff39bd6
Cr-Commit-Position: refs/heads/master@{#357917}
Patch Set 1 #Patch Set 2 : Indicate in the Window menu which Chrome window has an active sound playing #Patch Set 3 : To show which Chromium window has an active/muted sound playing #
Total comments: 17
Patch Set 4 : Applied changes based on the CR, and also refactor the logic #
Total comments: 15
Patch Set 5 : Modified based on the code reviews #
Total comments: 10
Patch Set 6 : add unit tests #Patch Set 7 : put Emojis in generated_resources.grd #
Total comments: 9
Patch Set 8 : Applied some changes based on comments #
Total comments: 3
Patch Set 9 : Did some final nits #Patch Set 10 : Removed emojis from generated_resources.grd #Messages
Total messages: 34 (6 generated)
Description was changed from ========== Indicate in the Window menu which Chrome window has an active sound playing 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. ========== to ========== Indicate in the Window menu which Chrome window has an active sound playing 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. ==========
shahriar.rostami@gmail.com changed reviewers: + miu@chromium.org, pinkerton@chromium.org, pkasting@google.com, rsesek@chromium.org
On 2015/10/19 19:38:48, shahriar wrote: > mailto:shahriar.rostami@gmail.com changed reviewers: > + mailto:miu@chromium.org, mailto:pinkerton@chromium.org, mailto:pkasting@google.com, > mailto:rsesek@chromium.org Please take a look at the changes. I use Emojis to show the sound is playing or it is playing but is mute. First I put these Emojis in generated_resources.grd but couldn't upload it (Rietveld annoying limitation for large files). We can replace these Emojis with icons/images, though for the prototype emojis were much easier to concatenate with window title. Looking forward your feedbacks.
PTAL.
pkasting@chromium.org changed reviewers: - pkasting@google.com
When you have multiple reviewers, always say exactly what you want each person to review. Generally each part of your change needs one reviewer, or, if the best reviewer is not an OWNER, a reviewer and an OWNER. Multiple reviewers are normally only necessary when no one reviewer or pair of reviewers is best for the whole change, or when several people all need to provide input on a controversial change. In this case, for example, I'm not a good reviewer since I don't speak ObjC, so removing myself.
On 2015/10/19 20:10:58, Peter Kasting wrote: > When you have multiple reviewers, always say exactly what you want each person > to review. > > Generally each part of your change needs one reviewer, or, if the best reviewer > is not an OWNER, a reviewer and an OWNER. Multiple reviewers are normally only > necessary when no one reviewer or pair of reviewers is best for the whole > change, or when several people all need to provide input on a controversial > change. > > In this case, for example, I'm not a good reviewer since I don't speak ObjC, so > removing myself. I added Peter and others based on the people commented/added on the bug associated with this CR, to ask for feedbacks. Anyways thanks Peter for letting me know about the process for adding reviewers.
Thanks for the patch! https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_window.h:97: virtual void UpdateMediaState(TabMediaState media_state, This needs a comment describing what it does. https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_window.h:98: content::WebContents* contents) = 0; This is pure virtual, so what about the other platforms? https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_cocoa.h (right): https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.h:197: TabMediaState media_state_; Why do you need to track any additional data other than |media_state_| ? https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:91: const char* const kWhiteSpaceCharacter = " "; This doesn't need to be a string, it can be a single char (single quotes). https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:301: NSString* newTitle = nit: over-indented by one space. https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:307: if (media_state_ == TAB_MEDIA_STATE_AUDIO_PLAYING){ nit: extra space after == https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:308: [titleWithMediaState appendFormat:@"%s", kWhiteSpaceCharacter]; … and then use %c instead of %s. https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:310: // [titleWithMediaState appendString:l10n_util::GetNSString( Don't check-in commented code. https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:313: else if (media_state_ == TAB_MEDIA_STATE_AUDIO_MUTING){ nit: this goes on the same line as } https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:328: if (media_state_contents_ == nil) Why only update if |media_state_contents_| is NULL (also, nil is only for ObjC objects, use nullptr or implicit boolean conversion for C++ objects)? https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:332: media_state == TAB_MEDIA_STATE_AUDIO_MUTING) nit: this needs {} since the body spans multiple lines https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:336: if (media_state == TAB_MEDIA_STATE_NONE) nit: if any branch of an if/else has {}, all do. https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller.h (right): https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.h:374: - (void)setMediaState:(TabMediaState)media_state This needs a comment. https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.h:374: - (void)setMediaState:(TabMediaState)media_state naming: In Objective-C, use camelCase, so mediaState. https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.h:375: on:(content::WebContents*)contents; nit: align the colon after on: with the one after setMediaState https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:255: on:(content::WebContents*)changed; nit: align colons https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:1631: TabMediaState media_state = chrome::GetTabMediaStateForContents(contents); naming: mediaState
On 2015/10/20 13:39:48, Robert Sesek wrote: > Thanks for the patch! > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/brows... > File chrome/browser/ui/browser_window.h (right): > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/brows... > chrome/browser/ui/browser_window.h:97: virtual void > UpdateMediaState(TabMediaState media_state, > This needs a comment describing what it does. > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/brows... > chrome/browser/ui/browser_window.h:98: content::WebContents* contents) = 0; > This is pure virtual, so what about the other platforms? > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/browser_window_cocoa.h (right): > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.h:197: TabMediaState media_state_; > Why do you need to track any additional data other than |media_state_| ? > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:91: const char* const > kWhiteSpaceCharacter = " "; > This doesn't need to be a string, it can be a single char (single quotes). > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:301: NSString* newTitle = > nit: over-indented by one space. > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:307: if (media_state_ == > TAB_MEDIA_STATE_AUDIO_PLAYING){ > nit: extra space after == > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:308: [titleWithMediaState > appendFormat:@"%s", kWhiteSpaceCharacter]; > … and then use %c instead of %s. > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:310: // [titleWithMediaState > appendString:l10n_util::GetNSString( > Don't check-in commented code. > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:313: else if (media_state_ == > TAB_MEDIA_STATE_AUDIO_MUTING){ > nit: this goes on the same line as } > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:328: if (media_state_contents_ > == nil) > Why only update if |media_state_contents_| is NULL (also, nil is only for ObjC > objects, use nullptr or implicit boolean conversion for C++ objects)? > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:332: media_state == > TAB_MEDIA_STATE_AUDIO_MUTING) > nit: this needs {} since the body spans multiple lines > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:336: if (media_state == > TAB_MEDIA_STATE_NONE) > nit: if any branch of an if/else has {}, all do. > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/browser_window_controller.h (right): > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_controller.h:374: - > (void)setMediaState:(TabMediaState)media_state > This needs a comment. > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_controller.h:374: - > (void)setMediaState:(TabMediaState)media_state > naming: In Objective-C, use camelCase, so mediaState. > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_controller.h:375: > on:(content::WebContents*)contents; > nit: align the colon after on: with the one after setMediaState > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:255: > on:(content::WebContents*)changed; > nit: align colons > > https://codereview.chromium.org/1412083002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:1631: TabMediaState > media_state = chrome::GetTabMediaStateForContents(contents); > naming: mediaState Thanks for the feedbacks. Robert, please take another look. Regarding Emojis, I tried to put them in generated_resources.grd, but Rietveld annoying limitation didn't let me to push my changes (because of file size). I paste the diff of generated_resources.grd, so can somebody please update it for me? (or please give me a solution how to upload large files): diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd index ccee420..7917b4f 100644 --- a/chrome/app/generated_resources.grd +++ b/chrome/app/generated_resources.grd @@ -12907,6 +12907,12 @@ Some features may be unavailable. Please check that the profile exists and you <message name="IDS_SHOW_EXTENSIONS_MAC" desc="The Mac menu item to show extensions in the window menu."> Extensions </message> + <message name="IDS_WINDOW_AUDIO_PLAYING_MAC" desc="The emoji to append to the title of a window showing audio is playing."> + <U+1F50A> + </message> + <message name="IDS_WINDOW_AUDIO_MUTE_MAC" desc="The emoji to append to the title of a window showing audio is muted."> + <U+1F507> + </message> <message name="IDS_ALL_WINDOWS_FRONT_MAC" desc="The Mac menu item for bring all to front the window menu."> Bring All to Front </message>
Overall approach looks good to me. Though, I have some comments about interfacing, and minor other things: https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_window.h:100: virtual void UpdateMediaState(TabMediaState media_state) = 0; I'd ask an OWNER (e.g., sky@ or pkasting@) about this, but IMO I wouldn't declare the virtual method in this header file. Instead, just declare the method in the BrowserWindowCocoa class (and then see my comment below). https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:308: [titleWithMediaState appendFormat:@"%c", kWhiteSpaceCharacter]; Can the whitespace char be included in the string resource? Perhaps it should be a non-breaking space instead of a plain ASCII space char? Though, the Cocoa experts probably know better than me here. ;-) https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:309: [titleWithMediaState appendString:@"🔊"]; Was this moved to generated_resources.prd? If so, please upload the updated patch. https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.mm:1899: [self browserWindow]->UpdateMediaState(mediaState); Rather than declare the UpdateMediaState() method in the cross-platform BrowserWindow interface class (see comment above), you could just: static_cast<BrowserWindowCocoa*>([self browserWindow])->UpdateMediaState(mediaState); https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2345: - (void)updateWindowMediaState:(TabMediaState)mediaState Please document this method and indicate what the window-level media state will be in each of the possible cases. It'd be a lot easier for future maintainers of this code (so they don't have to spend several minutes eval'ing this logic). https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2367: - (BOOL)isAnyOtherTab:(content::WebContents*)selected ditto: Please add a short method-level comment summarizing what this does. https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2371: content::WebContents* currentContents = Just after this statement, add the following so the rest of the block doesn't need to be needlessly executed: if (selected == currentContents) continue; https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2375: if (selected != currentContents && (see above comment)...then you don't need the first half of this expression.
https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:304: NSMutableString* titleWithMediaState = [NSMutableString string]; Use [NSMutableString stringWithString:] to avoid unnecessary copying/resizing. https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:309: [titleWithMediaState appendString:@"🔊"]; Two appends shouldn't be necessary. Just [title appendFormat:@"%c <emoji>", kWhiteSpaceCharacter] https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:311: else if (media_state_ == TAB_MEDIA_STATE_AUDIO_MUTING) { nit: this goes on line 310. https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2346: on:(content::WebContents*)selected { nit: align the colons here, too. https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2349: [BrowserWindowController browserWindowControllerForWindow:window]; nit: this is over-indented by two spaces https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2353: ![self isAnyOtherTab:selected withState:TAB_MEDIA_STATE_AUDIO_MUTING]) nit: both the if and else bodies need {} https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2368: withState:(TabMediaState)state{ nit: align the colons
On 2015/10/22 00:01:52, Robert Sesek wrote: > https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): > > https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:304: NSMutableString* > titleWithMediaState = [NSMutableString string]; > Use [NSMutableString stringWithString:] to avoid unnecessary copying/resizing. > > https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:309: [titleWithMediaState > appendString:@"🔊"]; > Two appends shouldn't be necessary. Just [title appendFormat:@"%c <emoji>", > kWhiteSpaceCharacter] > > https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:311: else if (media_state_ == > TAB_MEDIA_STATE_AUDIO_MUTING) { > nit: this goes on line 310. > > https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): > > https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2346: > on:(content::WebContents*)selected { > nit: align the colons here, too. > > https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2349: > [BrowserWindowController browserWindowControllerForWindow:window]; > nit: this is over-indented by two spaces > > https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2353: ![self > isAnyOtherTab:selected withState:TAB_MEDIA_STATE_AUDIO_MUTING]) > nit: both the if and else bodies need {} > > https://codereview.chromium.org/1412083002/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2368: > withState:(TabMediaState)state{ > nit: align the colons Please take another look, Robert and Miu. I also paste the generated_resource.grd diff, maybe one can upload it for me (There is a size limitation for non @chromium and @google accounts IMHO). diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd index ccee420..7917b4f 100644 --- a/chrome/app/generated_resources.grd +++ b/chrome/app/generated_resources.grd @@ -12907,6 +12907,12 @@ Some features may be unavailable. Please check that the profile exists and you <message name="IDS_SHOW_EXTENSIONS_MAC" desc="The Mac menu item to show extensions in the window menu."> Extensions </message> + <message name="IDS_WINDOW_AUDIO_PLAYING_MAC" desc="The emoji to append to the title of a window showing audio is playing."> + <U+1F50A> + </message> + <message name="IDS_WINDOW_AUDIO_MUTE_MAC" desc="The emoji to append to the title of a window showing audio is muted."> + <U+1F507> + </message> <message name="IDS_ALL_WINDOWS_FRONT_MAC" desc="The Mac menu item for bring all to front the window menu."> Bring All to Front </message>
I'm not sure why you're having issues with the GRD file. It's too big for me to upload, too, so `git cl` just falls back on uploading the diff :/. Also, if you want an easier way to resolve all the stylistic nits, just run `git cl format` and it will address them automatically. https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_window.h:18: #include "chrome/browser/ui/tabs/tab_utils.h" Still needed? https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_cocoa.h (right): https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.h:183: void UpdateMediaState(TabMediaState media_state); Copy the comment that was lost. https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.h:197: TabMediaState media_state_; This should also have a comment. https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:305: stringWithString:newTitle]; There's no reason to not keep |newTitle| around at all now. You can just do, NSString* newTitle = [NSMutableString stringWithString: base::SysUTF16ToNSString(browser_->GetWindowTitleForCurrentTab()]; https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:308: [titleWithMediaState appendFormat:@"%c 🔊", kWhiteSpaceCharacter]; There's now the %c whitespace character that's being formatted, as well as an additional space in the format string. Is that intentional? https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_cocoa.mm:309: else if (media_state_ == TAB_MEDIA_STATE_AUDIO_MUTING) nit: extra space after == https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2360: withState:TAB_MEDIA_STATE_AUDIO_PLAYING] && nit: align colons https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2364: withState:TAB_MEDIA_STATE_AUDIO_MUTING]) { nit: align colons https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2383: tabStripModel_->GetWebContentsAt(i); nit: over-indented. Only indent 4 spaces from the previous line https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2387: chrome::GetTabMediaStateForContents(currentContents); nit: same comment about over-indented
On 2015/10/23 14:26:37, Robert Sesek wrote: > I'm not sure why you're having issues with the GRD file. It's too big for me to > upload, too, so `git cl` just falls back on uploading the diff :/. > > Also, if you want an easier way to resolve all the stylistic nits, just run `git > cl format` and it will address them automatically. > > https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/brows... > File chrome/browser/ui/browser_window.h (right): > > https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/brows... > chrome/browser/ui/browser_window.h:18: #include > "chrome/browser/ui/tabs/tab_utils.h" > Still needed? > > https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/browser_window_cocoa.h (right): > > https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.h:183: void > UpdateMediaState(TabMediaState media_state); > Copy the comment that was lost. > > https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.h:197: TabMediaState media_state_; > This should also have a comment. > > https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): > > https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:305: stringWithString:newTitle]; > There's no reason to not keep |newTitle| around at all now. You can just do, > > NSString* newTitle = [NSMutableString stringWithString: > base::SysUTF16ToNSString(browser_->GetWindowTitleForCurrentTab()]; > > https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:308: [titleWithMediaState > appendFormat:@"%c 🔊", kWhiteSpaceCharacter]; > There's now the %c whitespace character that's being formatted, as well as an > additional space in the format string. Is that intentional? > > https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:309: else if (media_state_ == > TAB_MEDIA_STATE_AUDIO_MUTING) > nit: extra space after == > > https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): > > https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2360: > withState:TAB_MEDIA_STATE_AUDIO_PLAYING] && > nit: align colons > > https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2364: > withState:TAB_MEDIA_STATE_AUDIO_MUTING]) { > nit: align colons > > https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2383: > tabStripModel_->GetWebContentsAt(i); > nit: over-indented. Only indent 4 spaces from the previous line > > https://codereview.chromium.org/1412083002/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:2387: > chrome::GetTabMediaStateForContents(currentContents); > nit: same comment about over-indented Thanks for the feedback. Regarding GRD file, I ask a question on #chromium IRC channel, and somebody told me a commiter should modify that file for you, just send the reviewer or owner a generated diff. That's why I am a bit confused. He told me it's Rietveld annoying limitation for large files. But I will try that again to see if I can upload it. For the 'git cl format' thank you very much for telling me this, I am always ashamed of my faults for these formatting issues.
Also, it'd be good to have unittests for this if possible, so that this feature doesn't regress.
On 2015/10/23 15:16:11, Robert Sesek wrote: > Also, it'd be good to have unittests for this if possible, so that this feature > doesn't regress. Ok, I will write unit test/s for it. Thanks.
lgtm (feature, approach, and general code structure). I'll leave it to rsesek@ to review the deeper details and the unit test code (to be written).
On 2015/10/26 20:51:28, miu wrote: > lgtm (feature, approach, and general code structure). I'll leave it to rsesek@ > to review the deeper details and the unit test code (to be written). rsesek@ please take another look. You might ask why I use contents_media_state_maps_ in tab_strip_controller_unittest.mm. I searched a lot to find how I can use setMediaState in tab_controller.mm. This method didn't update things to be reflected by tab_utils.cc GetTabMediaStateForContents. tab_utils.cc uses two methods to determine whether audio is playing or muted: contents->WasRecentlyAudible() contents->IsAudioMuted() So, basically I tried to circumvent difficulties I encountered to update content's media state by overriding TabStripController and implementing my own state management mechanism for media state. I only used this derived controller for this specific unit test. Other unit tests are still using non-derived TabStripController.
Great job on the tests! Just a few more comments. https://codereview.chromium.org/1412083002/diff/120001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1412083002/diff/120001/chrome/app/generated_r... chrome/app/generated_resources.grd:12908: + 🔊 Glad to see you were able to upload the GRD. I'm wondering how this should work for RTL languages. I think we may want to instead format the string using a placeholder, so that the icon doesn't always appear at the end of the string. This would be something like: <message …> <ph name="TAB_TITLE">$1<ex>The Title of the Tab</ex></ph> 🔊 </message> https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_cocoa.mm:319: if (media_state_ == TAB_MEDIA_STATE_AUDIO_PLAYING) nit: needs braces {} around both if and else if. https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_cocoa.mm:321: appendFormat:@"%c%@", kWhiteSpaceCharacter, … continuing comment from the GRD file. Then this would be: l10n_util::GetNSStringF(IDS_WINDOW_AUDIO_PLAYING_MAC, browser_->GetWindowTitleForCurrentTab()) https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm (right): https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:56: NSString* playingEmoji = l10n_util::GetNSString(IDS_WINDOW_AUDIO_PLAYING_MAC); naming: use under_scores (see other test file for rationale). https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller.h (right): https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.h:379: - (TabMediaState)getMediaState; naming: Getters in Cocoa don't use the word "get", so just "mediaState". https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.h (right): https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/tabs/tab_strip_controller.h:271: - (void)updateWindowMediaState:(TabMediaState)mediaState Comment these two new methods, too. https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/tabs/tab_strip_controller.h:274: - (TabMediaState)getMediaStateForContents:(content::WebContents*)contents; naming: No, "get" here, either. https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/tabs/tab_strip_controller_unittest.mm (right): https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/tabs/tab_strip_controller_unittest.mm:34: static std::map<content::WebContents*, TabMediaState> Rather than keeping this in a static, store this in a property on TabStripControllerForMediaTesting and access just hold a pointer to it in the test. https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/tabs/tab_strip_controller_unittest.mm:384: BrowserWindowController* windowController = naming: Use under_scores for variable names, except in Objective-C methods. This includes tests, so this should be window_controller. And Line 393 should be contents_at_tab1. Etc.
On 2015/10/28 18:45:58, Robert Sesek wrote: > Great job on the tests! Just a few more comments. > > https://codereview.chromium.org/1412083002/diff/120001/chrome/app/generated_r... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/1412083002/diff/120001/chrome/app/generated_r... > chrome/app/generated_resources.grd:12908: + 🔊 > Glad to see you were able to upload the GRD. > > I'm wondering how this should work for RTL languages. I think we may want to > instead format the string using a placeholder, so that the icon doesn't always > appear at the end of the string. This would be something like: > > <message …> > <ph name="TAB_TITLE">$1<ex>The Title of the Tab</ex></ph> 🔊 > </message> > > https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... > File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): > > https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:319: if (media_state_ == > TAB_MEDIA_STATE_AUDIO_PLAYING) > nit: needs braces {} around both if and else if. > > https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:321: appendFormat:@"%c%@", > kWhiteSpaceCharacter, > … continuing comment from the GRD file. Then this would be: > > l10n_util::GetNSStringF(IDS_WINDOW_AUDIO_PLAYING_MAC, > browser_->GetWindowTitleForCurrentTab()) > > https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... > File chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm (right): > > https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:56: NSString* > playingEmoji = l10n_util::GetNSString(IDS_WINDOW_AUDIO_PLAYING_MAC); > naming: use under_scores (see other test file for rationale). > > https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... > File chrome/browser/ui/cocoa/browser_window_controller.h (right): > > https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/browser_window_controller.h:379: - > (TabMediaState)getMediaState; > naming: Getters in Cocoa don't use the word "get", so just "mediaState". > > https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... > File chrome/browser/ui/cocoa/tabs/tab_strip_controller.h (right): > > https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.h:271: - > (void)updateWindowMediaState:(TabMediaState)mediaState > Comment these two new methods, too. > > https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/tabs/tab_strip_controller.h:274: - > (TabMediaState)getMediaStateForContents:(content::WebContents*)contents; > naming: No, "get" here, either. > > https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... > File chrome/browser/ui/cocoa/tabs/tab_strip_controller_unittest.mm (right): > > https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/tabs/tab_strip_controller_unittest.mm:34: static > std::map<content::WebContents*, TabMediaState> > Rather than keeping this in a static, store this in a property on > TabStripControllerForMediaTesting and access just hold a pointer to it in the > test. > > https://codereview.chromium.org/1412083002/diff/120001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/tabs/tab_strip_controller_unittest.mm:384: > BrowserWindowController* windowController = > naming: Use under_scores for variable names, except in Objective-C methods. This > includes tests, so this should be window_controller. And Line 393 should be > contents_at_tab1. Etc. Thanks for your kind feedback. rsesek@ PTAL.
Last few nits, sorry for not noticing these earlier! https://codereview.chromium.org/1412083002/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_cocoa.h (right): https://codereview.chromium.org/1412083002/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_cocoa.h:186: void UpdateMediaState(TabMediaState media_state); nit: order this above the trivial getters (also re-order the .mm file) https://codereview.chromium.org/1412083002/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_cocoa.h:190: TabMediaState mediaState(); naming: media_state(), and you can inline the body of this method (since it's a trivial getter) https://codereview.chromium.org/1412083002/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_cocoa.h:193: NSString* windowTitle(); naming: WindowTitle(), and reorder above media_state()
On 2015/11/02 22:36:43, Robert Sesek wrote: > Last few nits, sorry for not noticing these earlier! > > https://codereview.chromium.org/1412083002/diff/140001/chrome/browser/ui/coco... > File chrome/browser/ui/cocoa/browser_window_cocoa.h (right): > > https://codereview.chromium.org/1412083002/diff/140001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/browser_window_cocoa.h:186: void > UpdateMediaState(TabMediaState media_state); > nit: order this above the trivial getters (also re-order the .mm file) > > https://codereview.chromium.org/1412083002/diff/140001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/browser_window_cocoa.h:190: TabMediaState mediaState(); > naming: media_state(), and you can inline the body of this method (since it's a > trivial getter) > > https://codereview.chromium.org/1412083002/diff/140001/chrome/browser/ui/coco... > chrome/browser/ui/cocoa/browser_window_cocoa.h:193: NSString* windowTitle(); > naming: WindowTitle(), and reorder above media_state() Please take another look.
LGTM
The CQ bit was checked by shahriar.rostami@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org Link to the patchset: https://codereview.chromium.org/1412083002/#ps160001 (title: "Did some final nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412083002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412083002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/70d337daa152a75db530fa04214caba48ff39bd6 Cr-Commit-Position: refs/heads/master@{#357917}
Message was sent while issue was closed.
newt@chromium.org changed reviewers: + newt@chromium.org
Message was sent while issue was closed.
Unfortunately, emoji aren't (currently) supported by Chrome's translation process. It might be possible to translate these, but since these emoji won't vary by language, a simpler option would be to use $1 in the grd file, and replace that with the emoji character at runtime. Could you make this change very soon (in the next 24 hours), since this is blocking us from running translations currently? If not, I can land a change to remove the emoji, and let you follow up when you get a chance. Thanks, and sorry about the extra difficulty!
Message was sent while issue was closed.
On 2015/11/05 02:21:15, newt wrote: > Unfortunately, emoji aren't (currently) supported by Chrome's translation > process. > > It might be possible to translate these, but since these emoji won't vary by > language, a simpler option would be to use $1 in the grd file, and replace that > with the emoji character at runtime. > > Could you make this change very soon (in the next 24 hours), since this is > blocking us from running translations currently? If not, I can land a change to > remove the emoji, and let you follow up when you get a chance. > > Thanks, and sorry about the extra difficulty! I removed Emojis from generated_resources.grd. PTAL, thanks.
Message was sent while issue was closed.
generated_resources.grd looks good. Someone else will need to take a look at the Objective C changes. Thanks :)
Message was sent while issue was closed.
This should be done on a new CL, actually. This CL has already been closed/landed. Usually we only reuse CLs when something has been reverted and will be re-landed, but that's somewhat contentious (there was a recent discussion on chromium-dev about this).
Message was sent while issue was closed.
On 2015/11/05 15:27:09, Robert Sesek wrote: > This should be done on a new CL, actually. This CL has already been > closed/landed. Usually we only reuse CLs when something has been reverted and > will be re-landed, but that's somewhat contentious (there was a recent > discussion on chromium-dev about this). OK, thanks for the explanation. I will submit a new CL soon. |
