Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1871663002:
Fix for LoginFeedbackTest.Basic broken by recent change. (Closed)
|
Created:
4 years, 8 months ago by miu Modified:
4 years, 8 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix for LoginFeedbackTest.Basic broken by recent change.
https://codereview.chromium.org/1864813002/ introduced a change that
mistakenly attempted to make an AuraWindowCaptureMachine::Capture()
call on the wrong thread. Due to my own negligence, I forgot to test
window screen capture, and so I did not catch the bug. This change
makes the necessary PostTask() call, just like the
WebContentsVideoCaptureDevice implementation, as intended.
BUG=496274
TBR=mcasas@chromium.org
Committed: https://crrev.com/1f5a9d02c8b85e97f4abc5b01b380f8fd96a211c
Cr-Commit-Position: refs/heads/master@{#385874}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 15 (6 generated)
miu@chromium.org changed reviewers: + mcasas@chromium.org
mcasas: This is the fix to the problem you pointed out to me earlier.
Description was changed from ========== Fix for LoginFeedbackTest.Basic broken by recent change. https://codereview.chromium.org/1864813002/ introduced a change that mistakenly attempted to make an AuraWindowCaptureMachine::Capture() call on the wrong thread. Due to my own negligence, I forgot to test window screen capture, and so I did not catch the bug. This change makes the necessary PostTask() call, just like the WebContentsVideoCaptureDevice implementation, as intended. BUG=496274 ========== to ========== Fix for LoginFeedbackTest.Basic broken by recent change. https://codereview.chromium.org/1864813002/ introduced a change that mistakenly attempted to make an AuraWindowCaptureMachine::Capture() call on the wrong thread. Due to my own negligence, I forgot to test window screen capture, and so I did not catch the bug. This change makes the necessary PostTask() call, just like the WebContentsVideoCaptureDevice implementation, as intended. BUG=496274 TBR=mcasas@chromium.org ==========
FYI--Committing with reviewer on TBR to get the tree on the ChromiumOS bots green again as soon as possible.
The CQ bit was checked by miu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1871663002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1871663002/1
LGTM % considering WeakPtr. https://codereview.chromium.org/1871663002/diff/1/content/browser/media/captu... File content/browser/media/capture/aura_window_capture_machine.cc (right): https://codereview.chromium.org/1871663002/diff/1/content/browser/media/captu... content/browser/media/capture/aura_window_capture_machine.cc:132: base::Unretained(this), Actually, since |this| pointer would be used on UI thread, you could use weak_factory_.GetWeakPtr() right? https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...
Thanks for taking a look. Response to your comment: https://codereview.chromium.org/1871663002/diff/1/content/browser/media/captu... File content/browser/media/capture/aura_window_capture_machine.cc (right): https://codereview.chromium.org/1871663002/diff/1/content/browser/media/captu... content/browser/media/capture/aura_window_capture_machine.cc:132: base::Unretained(this), On 2016/04/07 19:12:57, mcasas wrote: > Actually, since |this| pointer would be used > on UI thread, you could use > weak_factory_.GetWeakPtr() > right? tl;dr: It doesn't really matter, but the raw pointer is less overhead. I had considered this. However, we can easily reason that it's not needed: 1. Since we know MaybeCaptureForRefresh() must be called on the same thread as Stop(), and will never be called after Stop(); then we know the tasks posted to run on the UI thread will be sequenced such that the raw pointer will always be valid. 2. If instead we post a task using a WeakPtr, the tasks running on the UI thread will still have the same execution sequence. The weak pointer could never be invalidated in the meantime (because InternalStop() always has to run after the task posted here). So, using the WeakPtr doesn't add any value, but *does* add overhead.
Message was sent while issue was closed.
Description was changed from ========== Fix for LoginFeedbackTest.Basic broken by recent change. https://codereview.chromium.org/1864813002/ introduced a change that mistakenly attempted to make an AuraWindowCaptureMachine::Capture() call on the wrong thread. Due to my own negligence, I forgot to test window screen capture, and so I did not catch the bug. This change makes the necessary PostTask() call, just like the WebContentsVideoCaptureDevice implementation, as intended. BUG=496274 TBR=mcasas@chromium.org ========== to ========== Fix for LoginFeedbackTest.Basic broken by recent change. https://codereview.chromium.org/1864813002/ introduced a change that mistakenly attempted to make an AuraWindowCaptureMachine::Capture() call on the wrong thread. Due to my own negligence, I forgot to test window screen capture, and so I did not catch the bug. This change makes the necessary PostTask() call, just like the WebContentsVideoCaptureDevice implementation, as intended. BUG=496274 TBR=mcasas@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Fix for LoginFeedbackTest.Basic broken by recent change. https://codereview.chromium.org/1864813002/ introduced a change that mistakenly attempted to make an AuraWindowCaptureMachine::Capture() call on the wrong thread. Due to my own negligence, I forgot to test window screen capture, and so I did not catch the bug. This change makes the necessary PostTask() call, just like the WebContentsVideoCaptureDevice implementation, as intended. BUG=496274 TBR=mcasas@chromium.org ========== to ========== Fix for LoginFeedbackTest.Basic broken by recent change. https://codereview.chromium.org/1864813002/ introduced a change that mistakenly attempted to make an AuraWindowCaptureMachine::Capture() call on the wrong thread. Due to my own negligence, I forgot to test window screen capture, and so I did not catch the bug. This change makes the necessary PostTask() call, just like the WebContentsVideoCaptureDevice implementation, as intended. BUG=496274 TBR=mcasas@chromium.org Committed: https://crrev.com/1f5a9d02c8b85e97f4abc5b01b380f8fd96a211c Cr-Commit-Position: refs/heads/master@{#385874} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/1f5a9d02c8b85e97f4abc5b01b380f8fd96a211c Cr-Commit-Position: refs/heads/master@{#385874}
Message was sent while issue was closed.
nodir@chromium.org changed reviewers: + nodir@chromium.org
Message was sent while issue was closed.
Bug number is probably wrong
Message was sent while issue was closed.
On 2016/04/07 22:04:05, nodir wrote: > Bug number is probably wrong Apologies. It should have been bug 486274 (typo). And, bug 601308 reported the breakage. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
