Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10534174:
platform apps: use app name for WebRTC balloons (Closed)
|
Created:
8 years, 6 months ago by Evan Stade Modified:
8 years, 4 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionplatform apps: use app name for WebRTC balloons
BUG=132846
TEST=manual
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=150443
Patch Set 1 #Patch Set 2 : remove bold product name #
Total comments: 11
Patch Set 3 : review #
Total comments: 7
Patch Set 4 : don't leak #
Messages
Total messages: 13 (0 generated)
I'll admit I'm not super familiar w/ this code so adding xians as well http://codereview.chromium.org/10534174/diff/2001/chrome/browser/media/media_... File chrome/browser/media/media_stream_capture_indicator.cc (right): http://codereview.chromium.org/10534174/diff/2001/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:46: !profile ? NULL : profile->GetExtensionService(); this tertiary-operator-ness is hurting my eyes -- how about a more conventional early return approach? if (!profile) return NULL; ExtensionService* extensions = profile->GetExtensionService(); if (!extensions) return NULL; return extensions->extensions()->GetXXX(...); http://codereview.chromium.org/10534174/diff/2001/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:297: if (index != image_load_index_) is this to prevent displaying older images that may have loaded?
it looks really good, some nits and questions. http://codereview.chromium.org/10534174/diff/2001/chrome/browser/media/media_... File chrome/browser/media/media_stream_capture_indicator.cc (right): http://codereview.chromium.org/10534174/diff/2001/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:273: pending_extension_name_ = extension->name(); can we simply store the 10n_util::GetStringFUTF16(message_id, UTF8ToUTF16(extension->name()))) ? http://codereview.chromium.org/10534174/diff/2001/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:285: image_load_index_++; ++image_load_index_; http://codereview.chromium.org/10534174/diff/2001/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:295: const std::string& extension_id, What is the difference between extension_id and extension->name()? http://codereview.chromium.org/10534174/diff/2001/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:297: if (index != image_load_index_) are we skipping showing the balloon when the new ShowBalloonForExtension comes before OnImageLoaded? Or shall we show the ballon anyway, can we use extension_id for this case?
http://codereview.chromium.org/10534174/diff/2001/chrome/browser/media/media_... File chrome/browser/media/media_stream_capture_indicator.cc (right): http://codereview.chromium.org/10534174/diff/2001/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:46: !profile ? NULL : profile->GetExtensionService(); On 2012/07/31 02:14:12, scherkus wrote: > this tertiary-operator-ness is hurting my eyes -- how about a more conventional > early return approach? > > if (!profile) > return NULL; > ExtensionService* extensions = profile->GetExtensionService(); > if (!extensions) > return NULL; > return extensions->extensions()->GetXXX(...); Done. http://codereview.chromium.org/10534174/diff/2001/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:273: pending_extension_name_ = extension->name(); On 2012/07/31 08:24:37, xians1 wrote: > can we simply store the 10n_util::GetStringFUTF16(message_id, > UTF8ToUTF16(extension->name()))) ? Done. http://codereview.chromium.org/10534174/diff/2001/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:285: image_load_index_++; On 2012/07/31 08:24:37, xians1 wrote: > ++image_load_index_; Done. http://codereview.chromium.org/10534174/diff/2001/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:295: const std::string& extension_id, On 2012/07/31 08:24:37, xians1 wrote: > What is the difference between extension_id and extension->name()? name is a display string, extension_id() is a hash http://codereview.chromium.org/10534174/diff/2001/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:297: if (index != image_load_index_) On 2012/07/31 08:24:37, xians1 wrote: > are we skipping showing the balloon when the new ShowBalloonForExtension comes > before OnImageLoaded? yes > Or shall we show the ballon anyway, can we use extension_id for this case? well, we can't use extension id without knowing what type of message to show. The same extension could request video, then audio. Instead I've created a message map. http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_... File chrome/browser/media/media_stream_capture_indicator.cc (right): http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:296: message.swap(pending_messages_[index]); this leaks... the vector never gets smaller. But it only holds empty strings, so it seems not too terrible.
http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_... File chrome/browser/media/media_stream_capture_indicator.cc (right): http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:296: message.swap(pending_messages_[index]); On 2012/07/31 23:19:29, Evan Stade wrote: > this leaks... the vector never gets smaller. But it only holds empty strings, so > it seems not too terrible. I'm not super familiar with ImageLoadingTracker but will it execute OnImageLoaded() out of order? For example if pending_messages_ contains ["audio", "audio", "video"] am I guaranteed that OIL() is called in exact response to correspond with those? If it is guaranteed, then wouldn't a deque work here? you would push_back() when calling LoadImage() then pop_front() inside OIL(). (caveat: I'm kind of sleepy today so I apologize if this makes no sense or doesn't work at all!)
On 2012/08/01 00:18:17, scherkus wrote: > http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_... > File chrome/browser/media/media_stream_capture_indicator.cc (right): > > http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_... > chrome/browser/media/media_stream_capture_indicator.cc:296: > message.swap(pending_messages_[index]); > On 2012/07/31 23:19:29, Evan Stade wrote: > > this leaks... the vector never gets smaller. But it only holds empty strings, > so > > it seems not too terrible. > > I'm not super familiar with ImageLoadingTracker but will it execute > OnImageLoaded() out of order? For example if pending_messages_ contains > ["audio", "audio", "video"] am I guaranteed that OIL() is called in exact > response to correspond with those? no, and that is why index exists. > > If it is guaranteed, then wouldn't a deque work here? you would push_back() when > calling LoadImage() then pop_front() inside OIL(). > > (caveat: I'm kind of sleepy today so I apologize if this makes no sense or > doesn't work at all!)
Gotcha. lgtm! On Jul 31, 2012 6:24 PM, <estade@chromium.org> wrote: > On 2012/08/01 00:18:17, scherkus wrote: > > http://codereview.chromium.**org/10534174/diff/4003/chrome/** > browser/media/media_stream_**capture_indicator.cc<http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_stream_capture_indicator.cc> > >> File chrome/browser/media/media_**stream_capture_indicator.cc (right): >> > > > http://codereview.chromium.**org/10534174/diff/4003/chrome/** > browser/media/media_stream_**capture_indicator.cc#**newcode296<http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_stream_capture_indicator.cc#newcode296> > >> chrome/browser/media/media_**stream_capture_indicator.cc:**296: >> message.swap(pending_messages_**[index]); >> On 2012/07/31 23:19:29, Evan Stade wrote: >> > this leaks... the vector never gets smaller. But it only holds empty >> > strings, > >> so >> > it seems not too terrible. >> > > I'm not super familiar with ImageLoadingTracker but will it execute >> OnImageLoaded() out of order? For example if pending_messages_ contains >> ["audio", "audio", "video"] am I guaranteed that OIL() is called in exact >> response to correspond with those? >> > > no, and that is why index exists. > > > If it is guaranteed, then wouldn't a deque work here? you would >> push_back() >> > when > >> calling LoadImage() then pop_front() inside OIL(). >> > > (caveat: I'm kind of sleepy today so I apologize if this makes no sense or >> doesn't work at all!) >> > > http://codereview.chromium.**org/10534174/<http://codereview.chromium.org/105... >
LGTM http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_... File chrome/browser/media/media_stream_capture_indicator.cc (right): http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:48: ExtensionService* extensions = profile->GetExtensionService(); Nit: can you call this extensions_service, so that you don't end up with the slightly confusing extensions->extensions() repetition on the next line?
one nit, lgtm if you address it. http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_... File chrome/browser/media/media_stream_capture_indicator.cc (right): http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:296: message.swap(pending_messages_[index]); should we just use erase to completely delete the item?
http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_... File chrome/browser/media/media_stream_capture_indicator.cc (right): http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:296: message.swap(pending_messages_[index]); On 2012/08/01 07:54:14, xians1 wrote: > should we just use erase to completely delete the item? erase will move the rest of the elements in the vector over by one, which breaks the indexing. If you want I can fix this leak in another way (I guess the best way is to convert it to an int->string map), but I thought the extra code complexity might not be worth it.
http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_... File chrome/browser/media/media_stream_capture_indicator.cc (right): http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:296: message.swap(pending_messages_[index]); On 2012/08/01 22:25:15, Evan Stade wrote: > On 2012/08/01 07:54:14, xians1 wrote: > > should we just use erase to completely delete the item? > > erase will move the rest of the elements in the vector over by one, which breaks > the indexing. If you want I can fix this leak in another way (I guess the best > way is to convert it to an int->string map), but I thought the extra code > complexity might not be worth it. I was thinking ImageLoadingTracker::Observer could be replaced with base::Closure -- that way you could (for example) bind the string16 message into the closure that gets executed when the image is loaded! ...but that's a change for a different CL :)
http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_... File chrome/browser/media/media_stream_capture_indicator.cc (right): http://codereview.chromium.org/10534174/diff/4003/chrome/browser/media/media_... chrome/browser/media/media_stream_capture_indicator.cc:48: ExtensionService* extensions = profile->GetExtensionService(); On 2012/08/01 05:12:37, Mihai Parparita wrote: > Nit: can you call this extensions_service, so that you don't end up with the > slightly confusing extensions->extensions() repetition on the next line? Done.
On Wed, Aug 1, 2012 at 3:40 PM, <scherkus@chromium.org> wrote: > I was thinking ImageLoadingTracker::Observer could be replaced with > base::Closure -- that way you could (for example) bind the string16 > message into the closure that gets executed when the image is loaded! > > ...but that's a change for a different CL :) > BTW, filed http://crbug.com/141673 about this. Mihai |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
