Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1764043002:
Update a webview plugin's WebView's lifecycle immediately after resizing. (Closed)
DescriptionUpdate a webview plugin's WebView's lifecycle immediately after resizing.
Remove the delay timer before updating a webview plugin's geometry, since
the referenced Blink compositing bug has been fixed.
BUG=591174
Committed: https://crrev.com/93e8a2374eae0690244c6c1e8b211d17409e2154
Cr-Commit-Position: refs/heads/master@{#379351}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 1
Patch Set 3 : #
Total comments: 1
Messages
Total messages: 20 (7 generated)
Description was changed from ========== none none BUG= ========== to ========== Update a webview plugin's WebView's lifecycle immediately after resizing. BUG=591174 ==========
Description was changed from ========== Update a webview plugin's WebView's lifecycle immediately after resizing. BUG=591174 ========== to ========== Update a webview plugin's WebView's lifecycle immediately after resizing. Remove the delay timer before updating a webview plugin's geometry, since the referenced Blink compositing bug has been fixed, and we now ensure that the geometry is updating correctly in one shot. BUG=591174 ==========
Description was changed from ========== Update a webview plugin's WebView's lifecycle immediately after resizing. Remove the delay timer before updating a webview plugin's geometry, since the referenced Blink compositing bug has been fixed, and we now ensure that the geometry is updating correctly in one shot. BUG=591174 ========== to ========== Update a webview plugin's WebView's lifecycle immediately after resizing. Remove the delay timer before updating a webview plugin's geometry, since the referenced Blink compositing bug has been fixed. BUG=591174 ==========
chrishtr@chromium.org changed reviewers: + leviw@chromium.org, tommycli@chromium.org
Given the urgency for M50 beta, I have skipped the test. But a good test would be a unittest of WebViewPlugin that supplies a WebView which tries to animate during resize. I can add that later (or Tommy if you are willing). https://codereview.chromium.org/1764043002/diff/1/components/plugins/renderer... File components/plugins/renderer/loadable_plugin_placeholder.cc (left): https://codereview.chromium.org/1764043002/diff/1/components/plugins/renderer... components/plugins/renderer/loadable_plugin_placeholder.cc:41: const int kSizeChangeRecheckDelayMilliseconds = 100; Strictly speaking, the change to this file has to be in the same CL as the other one, since it is not required to fix the referenced bug. But timers like this are bad, especially since they introduce race conditions that are very hard to debug and reproduce.
https://codereview.chromium.org/1764043002/diff/1/components/plugins/renderer... File components/plugins/renderer/loadable_plugin_placeholder.h (left): https://codereview.chromium.org/1764043002/diff/1/components/plugins/renderer... components/plugins/renderer/loadable_plugin_placeholder.h:126: base::OneShotTimer size_update_timer_; LoadablePluginPlaceholder should have this patch reapplied: https://codereview.chromium.org/1491893004/ It basically does the same thing as your modifications, but adds a few more changes.
On 2016/03/04 at 18:32:09, tommycli wrote: > https://codereview.chromium.org/1764043002/diff/1/components/plugins/renderer... > File components/plugins/renderer/loadable_plugin_placeholder.h (left): > > https://codereview.chromium.org/1764043002/diff/1/components/plugins/renderer... > components/plugins/renderer/loadable_plugin_placeholder.h:126: base::OneShotTimer size_update_timer_; > LoadablePluginPlaceholder should have this patch reapplied: > > https://codereview.chromium.org/1491893004/ > > It basically does the same thing as your modifications, but adds a few more changes. Yes. Are you suggesting that be done first? Or can I commit this and then follow up with the rest? Or incorporate into this CL?
On 2016/03/04 18:40:40, chrishtr wrote: > On 2016/03/04 at 18:32:09, tommycli wrote: > > > https://codereview.chromium.org/1764043002/diff/1/components/plugins/renderer... > > File components/plugins/renderer/loadable_plugin_placeholder.h (left): > > > > > https://codereview.chromium.org/1764043002/diff/1/components/plugins/renderer... > > components/plugins/renderer/loadable_plugin_placeholder.h:126: > base::OneShotTimer size_update_timer_; > > LoadablePluginPlaceholder should have this patch reapplied: > > > > https://codereview.chromium.org/1491893004/ > > > > It basically does the same thing as your modifications, but adds a few more > changes. > > Yes. Are you suggesting that be done first? Or can I commit this and then follow > up with the rest? Or incorporate into this CL? Yes having this CL subsume the other one is fine with me.
On 2016/03/04 at 18:47:04, tommycli wrote: > On 2016/03/04 18:40:40, chrishtr wrote: > > On 2016/03/04 at 18:32:09, tommycli wrote: > > > > > https://codereview.chromium.org/1764043002/diff/1/components/plugins/renderer... > > > File components/plugins/renderer/loadable_plugin_placeholder.h (left): > > > > > > > > https://codereview.chromium.org/1764043002/diff/1/components/plugins/renderer... > > > components/plugins/renderer/loadable_plugin_placeholder.h:126: > > base::OneShotTimer size_update_timer_; > > > LoadablePluginPlaceholder should have this patch reapplied: > > > > > > https://codereview.chromium.org/1491893004/ > > > > > > It basically does the same thing as your modifications, but adds a few more > > changes. > > > > Yes. Are you suggesting that be done first? Or can I commit this and then follow > > up with the rest? Or incorporate into this CL? > > Yes having this CL subsume the other one is fine with me. So LGTM as is or not?
On 2016/03/04 18:48:32, chrishtr wrote: > On 2016/03/04 at 18:47:04, tommycli wrote: > > On 2016/03/04 18:40:40, chrishtr wrote: > > > On 2016/03/04 at 18:32:09, tommycli wrote: > > > > > > > > https://codereview.chromium.org/1764043002/diff/1/components/plugins/renderer... > > > > File components/plugins/renderer/loadable_plugin_placeholder.h (left): > > > > > > > > > > > > https://codereview.chromium.org/1764043002/diff/1/components/plugins/renderer... > > > > components/plugins/renderer/loadable_plugin_placeholder.h:126: > > > base::OneShotTimer size_update_timer_; > > > > LoadablePluginPlaceholder should have this patch reapplied: > > > > > > > > https://codereview.chromium.org/1491893004/ > > > > > > > > It basically does the same thing as your modifications, but adds a few > more > > > changes. > > > > > > Yes. Are you suggesting that be done first? Or can I commit this and then > follow > > > up with the rest? Or incorporate into this CL? > > > > Yes having this CL subsume the other one is fine with me. > > So LGTM as is or not? Sorry, I'm not being clear. Not LGTM as is, because the LoadablePluginPlaceholder changes in this patch remove the timer without entirely removing the recheck business. I'd prefer it if either this patch either included the other patch in its entirety, or excluded it entirely. In either case, I think the original patch removes the recheck logic in its entirety.
On 2016/03/04 at 18:50:45, tommycli wrote: > On 2016/03/04 18:48:32, chrishtr wrote: > > On 2016/03/04 at 18:47:04, tommycli wrote: > > > On 2016/03/04 18:40:40, chrishtr wrote: > > > > On 2016/03/04 at 18:32:09, tommycli wrote: > > > > > > > > > > > https://codereview.chromium.org/1764043002/diff/1/components/plugins/renderer... > > > > > File components/plugins/renderer/loadable_plugin_placeholder.h (left): > > > > > > > > > > > > > > > > https://codereview.chromium.org/1764043002/diff/1/components/plugins/renderer... > > > > > components/plugins/renderer/loadable_plugin_placeholder.h:126: > > > > base::OneShotTimer size_update_timer_; > > > > > LoadablePluginPlaceholder should have this patch reapplied: > > > > > > > > > > https://codereview.chromium.org/1491893004/ > > > > > > > > > > It basically does the same thing as your modifications, but adds a few > > more > > > > changes. > > > > > > > > Yes. Are you suggesting that be done first? Or can I commit this and then > > follow > > > > up with the rest? Or incorporate into this CL? > > > > > > Yes having this CL subsume the other one is fine with me. > > > > So LGTM as is or not? > > Sorry, I'm not being clear. Not LGTM as is, because the LoadablePluginPlaceholder changes in this patch remove the timer without entirely removing the recheck business. > > I'd prefer it if either this patch either included the other patch in its entirety, or excluded it entirely. In either case, I think the original patch removes the recheck logic in its entirety. Ok reverted those two files. You can re-commit your CL on top.
lgtm sans below: https://codereview.chromium.org/1764043002/diff/20001/components/plugins/rend... File components/plugins/renderer/webview_plugin.cc (right): https://codereview.chromium.org/1764043002/diff/20001/components/plugins/rend... components/plugins/renderer/webview_plugin.cc:203: base::AutoReset<bool> is_painting( This should be called is_resizing right?
On 2016/03/04 at 19:11:22, tommycli wrote: > lgtm sans below: > > https://codereview.chromium.org/1764043002/diff/20001/components/plugins/rend... > File components/plugins/renderer/webview_plugin.cc (right): > > https://codereview.chromium.org/1764043002/diff/20001/components/plugins/rend... > components/plugins/renderer/webview_plugin.cc:203: base::AutoReset<bool> is_painting( > This should be called is_resizing right? Fixed.
lgtm https://codereview.chromium.org/1764043002/diff/40001/components/plugins/rend... File components/plugins/renderer/webview_plugin.cc (right): https://codereview.chromium.org/1764043002/diff/40001/components/plugins/rend... components/plugins/renderer/webview_plugin.cc:217: web_view_->updateAllLifecyclePhases(); When all you've got is a hammer :p
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/1764043002/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1764043002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1764043002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Update a webview plugin's WebView's lifecycle immediately after resizing. Remove the delay timer before updating a webview plugin's geometry, since the referenced Blink compositing bug has been fixed. BUG=591174 ========== to ========== Update a webview plugin's WebView's lifecycle immediately after resizing. Remove the delay timer before updating a webview plugin's geometry, since the referenced Blink compositing bug has been fixed. BUG=591174 Committed: https://crrev.com/93e8a2374eae0690244c6c1e8b211d17409e2154 Cr-Commit-Position: refs/heads/master@{#379351} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/93e8a2374eae0690244c6c1e8b211d17409e2154 Cr-Commit-Position: refs/heads/master@{#379351} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
