Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1167643003:
Correctly invoke the exported cast functions. (Closed)
DescriptionCorrectly invoke the exported cast functions.
The previous invocation did not work as expected, since the exported
symbols reside in the global namespace instead of on the backgroundSetup
object.
BUG=489929
Committed: https://crrev.com/119a842066a0d0cfc8ecf90c91ba9e39add61516
Cr-Commit-Position: refs/heads/master@{#333583}
Patch Set 1 #Patch Set 2 : Add comments #
Total comments: 1
Patch Set 3 : Different comment #Patch Set 4 : #
Total comments: 2
Patch Set 5 : Remove unneeded invocation #
Total comments: 6
Messages
Total messages: 20 (2 generated)
jdufault@chromium.org changed reviewers: + achuith@chromium.org, oshima@chromium.org
On 2015/06/05 22:56:41, jdufault wrote: > mailto:jdufault@chromium.org changed reviewers: > + mailto:achuith@chromium.org, mailto:oshima@chromium.org Would you mind taking a look? Thanks!
i'll stamp once achuithb@ is happy.
Needs comments
On 2015/06/05 23:26:54, achuithb wrote: > Needs comments Done.
https://codereview.chromium.org/1167643003/diff/20001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1167643003/diff/20001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:134: // more expected backgroundSetup.getMirrorCapable...()). Hmm, how about something more along the lines of: // The methods in backgroundSetup are renamed during minification, so we have to bind the exported global API methods to backgroundSetup using call(). The purpose of the comment is simply to explain why we have to use weird syntax. We can probably just duplicate the comment since it's not too long.
On 2015/06/05 23:46:49, achuithb wrote: > https://codereview.chromium.org/1167643003/diff/20001/chrome/browser/ui/ash/c... > File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): > > https://codereview.chromium.org/1167643003/diff/20001/chrome/browser/ui/ash/c... > chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:134: // more expected > backgroundSetup.getMirrorCapable...()). > Hmm, how about something more along the lines of: > > // The methods in backgroundSetup are renamed during minification, so we have to > bind the exported global API methods to backgroundSetup using call(). > > The purpose of the comment is simply to explain why we have to use weird syntax. > We can probably just duplicate the comment since it's not too long. That's a much better comment; thanks. Done.
https://codereview.chromium.org/1167643003/diff/60001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1167643003/diff/60001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:155: ExecuteJavaScript("backgroundSetup.Qu('user-stop');"); Is this still necessary? Does the call() fix take care of this?
https://codereview.chromium.org/1167643003/diff/60001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1167643003/diff/60001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:155: ExecuteJavaScript("backgroundSetup.Qu('user-stop');"); On 2015/06/08 18:51:51, achuithb wrote: > Is this still necessary? Does the call() fix take care of this? Thinking about it, it should be fine to eliminate it now since stopCastMirroring was only present in developer builds of the extension.
https://codereview.chromium.org/1167643003/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1167643003/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:154: ExecuteJavaScript("backgroundSetup.Qu('user-stop');"); Do you need this? This is a work-around for minification, right?
https://codereview.chromium.org/1167643003/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1167643003/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:154: ExecuteJavaScript("backgroundSetup.Qu('user-stop');"); On 2015/06/08 19:09:42, achuithb wrote: > Do you need this? This is a work-around for minification, right? Or was this necessary because the symbol was not yet exported?
https://codereview.chromium.org/1167643003/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1167643003/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:154: ExecuteJavaScript("backgroundSetup.Qu('user-stop');"); On 2015/06/08 19:10:38, achuithb wrote: > On 2015/06/08 19:09:42, achuithb wrote: > > Do you need this? This is a work-around for minification, right? > > Or was this necessary because the symbol was not yet exported? Right, the symbol was not exported yet. So we workaround the minification by calling the minified symbol. This will go away in a week or two when the release version of the cast extension is updated.
https://codereview.chromium.org/1167643003/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1167643003/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:154: ExecuteJavaScript("backgroundSetup.Qu('user-stop');"); On 2015/06/08 19:16:19, jdufault wrote: > On 2015/06/08 19:10:38, achuithb wrote: > > On 2015/06/08 19:09:42, achuithb wrote: > > > Do you need this? This is a work-around for minification, right? > > > > Or was this necessary because the symbol was not yet exported? > > Right, the symbol was not exported yet. So we workaround the minification by > calling the minified symbol. This will go away in a week or two when the release > version of the cast extension is updated. Could we file a bug so we can track this? And reference it here? Just to make sure we don't accumulate cruft.
https://codereview.chromium.org/1167643003/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1167643003/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:154: ExecuteJavaScript("backgroundSetup.Qu('user-stop');"); On 2015/06/08 19:18:47, achuithb wrote: > On 2015/06/08 19:16:19, jdufault wrote: > > On 2015/06/08 19:10:38, achuithb wrote: > > > On 2015/06/08 19:09:42, achuithb wrote: > > > > Do you need this? This is a work-around for minification, right? > > > > > > Or was this necessary because the symbol was not yet exported? > > > > Right, the symbol was not exported yet. So we workaround the minification by > > calling the minified symbol. This will go away in a week or two when the > release > > version of the cast extension is updated. > > Could we file a bug so we can track this? And reference it here? Just to make > sure we don't accumulate cruft. Yep, see the comment above. The bug is at crbug.com/489929
lgtm https://codereview.chromium.org/1167643003/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/cast_config_delegate_chromeos.cc (right): https://codereview.chromium.org/1167643003/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/cast_config_delegate_chromeos.cc:154: ExecuteJavaScript("backgroundSetup.Qu('user-stop');"); On 2015/06/08 19:29:49, jdufault wrote: > On 2015/06/08 19:18:47, achuithb wrote: > > On 2015/06/08 19:16:19, jdufault wrote: > > > On 2015/06/08 19:10:38, achuithb wrote: > > > > On 2015/06/08 19:09:42, achuithb wrote: > > > > > Do you need this? This is a work-around for minification, right? > > > > > > > > Or was this necessary because the symbol was not yet exported? > > > > > > Right, the symbol was not exported yet. So we workaround the minification by > > > calling the minified symbol. This will go away in a week or two when the > > release > > > version of the cast extension is updated. > > > > Could we file a bug so we can track this? And reference it here? Just to make > > sure we don't accumulate cruft. > > Yep, see the comment above. The bug is at crbug.com/489929 Ah, oops. you're right.
lgtm
The CQ bit was checked by jdufault@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1167643003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/119a842066a0d0cfc8ecf90c91ba9e39add61516 Cr-Commit-Position: refs/heads/master@{#333583} |
