Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2702423002:
Detect webview size change by motion instead of expected size. (Closed)
|
Created:
3 years, 10 months ago by yamaguchi Modified:
3 years, 10 months ago Reviewers:
fukino CC:
chromium-reviews, oka+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yamaguchi+watch_chromium.org, fukino+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDetect webview size change by motion instead of expected size.
This is a workaround to avoid that the webview size never gets to the
expected size, thus it does not issue resizeComplete message at all.
One caveat of this method is that when there is an animation jank
longer than the polling interval (200ms), it'll be detected as the
false end of the animation. In this case, the resizeComplete message
won't be issued after actual end of the animation.
BUG=693416
TEST=Open share dialog in Files.app, enter "." as email address, click "Send" button. Verify the error message is visible.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2702423002
Cr-Commit-Position: refs/heads/master@{#451902}
Committed: https://chromium.googlesource.com/chromium/src/+/479ccd4b4d85396ed50eed8ce07b1b7dd9199109
Patch Set 1 #Patch Set 2 : Use local variable for readability. #
Total comments: 8
Patch Set 3 : Reflect review comments. #
Total comments: 9
Patch Set 4 : Fix comment style. #Patch Set 5 : Remove unused binding. #Patch Set 6 : Remove unused binding. #Messages
Total messages: 37 (23 generated)
Description was changed from ========== Detect webview size change by motion instead of expected size. This is a workaround to avoid that the webview size never gets to the expected size, thus it does not issue resizeComplete message. One caveat of this method is that when there is an animation jank longer than the polling interval (200ms), it'll be detected as the end of the animation. BUG=693416 ========== to ========== Detect webview size change by motion instead of expected size. This is a workaround to avoid that the webview size never gets to the expected size, thus it does not issue resizeComplete message. One caveat of this method is that when there is an animation jank longer than the polling interval (200ms), it'll be detected as the end of the animation. BUG=693416 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Detect webview size change by motion instead of expected size. This is a workaround to avoid that the webview size never gets to the expected size, thus it does not issue resizeComplete message. One caveat of this method is that when there is an animation jank longer than the polling interval (200ms), it'll be detected as the end of the animation. BUG=693416 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Detect webview size change by motion instead of expected size. This is a workaround to avoid that the webview size never gets to the expected size, thus it does not issue resizeComplete message. One caveat of this method is that when there is an animation jank longer than the polling interval (200ms), it'll be detected as the end of the animation. BUG=491102,693416 TEST=Open share dialog in Files.app, click any links, and confirm that no UI components are not clipped. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Detect webview size change by motion instead of expected size. This is a workaround to avoid that the webview size never gets to the expected size, thus it does not issue resizeComplete message. One caveat of this method is that when there is an animation jank longer than the polling interval (200ms), it'll be detected as the end of the animation. BUG=491102,693416 TEST=Open share dialog in Files.app, click any links, and confirm that no UI components are not clipped. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Detect webview size change by motion instead of expected size. This is a workaround to avoid that the webview size never gets to the expected size, thus it does not issue resizeComplete message. One caveat of this method is that when there is an animation jank longer than the polling interval (200ms), it'll be detected as the end of the animation. BUG=491102,693416 TEST=Open share dialog in Files.app, enter "." as email address, click "Send" button. Verify the error message is visible. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
yamaguchi@chromium.org changed reviewers: + fukino@chromium.org
ptal
Description was changed from ========== Detect webview size change by motion instead of expected size. This is a workaround to avoid that the webview size never gets to the expected size, thus it does not issue resizeComplete message. One caveat of this method is that when there is an animation jank longer than the polling interval (200ms), it'll be detected as the end of the animation. BUG=491102,693416 TEST=Open share dialog in Files.app, enter "." as email address, click "Send" button. Verify the error message is visible. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Detect webview size change by motion instead of expected size. This is a workaround to avoid that the webview size never gets to the expected size, thus it does not issue resizeComplete message at all. One caveat of this method is that when there is an animation jank longer than the polling interval (200ms), it'll be detected as the false end of the animation. In this case, the resizeComplete message won't be issued after actual end of the animation. BUG=491102,693416 TEST=Open share dialog in Files.app, enter "." as email address, click "Send" button. Verify the error message is visible. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2702423002/diff/20001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/ui/share_dialog.js (right): https://codereview.chromium.org/2702423002/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:32: * Timeout for detecting the end of resizing animation. Timeout => Polling interval ? https://codereview.chromium.org/2702423002/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:162: // stabilizes. As this is a workaround, please add a TODO comment with crbug.com/XXXX. https://codereview.chromium.org/2702423002/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:163: var checkSize = function(previousWidth, previousHeight) { nit: Add type annotation. https://codereview.chromium.org/2702423002/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:179: setTimeout(checkSize.bind(null, null), how about bind(null, -1, -1)? The first argument is this. For following arguments, let's use the primitive type (number) consistently.
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2702423002/diff/20001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/ui/share_dialog.js (right): https://codereview.chromium.org/2702423002/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:32: * Timeout for detecting the end of resizing animation. On 2017/02/22 02:38:24, fukino wrote: > Timeout => Polling interval ? Done. https://codereview.chromium.org/2702423002/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:162: // stabilizes. On 2017/02/22 02:38:24, fukino wrote: > As this is a workaround, please add a TODO comment with crbug.com/XXXX. Done. https://codereview.chromium.org/2702423002/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:163: var checkSize = function(previousWidth, previousHeight) { On 2017/02/22 02:38:24, fukino wrote: > nit: Add type annotation. Done. https://codereview.chromium.org/2702423002/diff/20001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:179: setTimeout(checkSize.bind(null, null), On 2017/02/22 02:38:24, fukino wrote: > how about bind(null, -1, -1)? > > The first argument is this. > For following arguments, let's use the primitive type (number) consistently. Done.
https://codereview.chromium.org/2702423002/diff/40001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/ui/share_dialog.js (right): https://codereview.chromium.org/2702423002/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:162: // stabilizes. This is a workaround for crbug.com/693416 . nit: space between 'crbug.com/693416' and '.' is not needed. https://codereview.chromium.org/2702423002/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:185: setTimeout(checkSize.bind(this, -1, -1), checkSize.bind(null, -1, -1). In checkSize function, the ShareDialog object is not referred, so we don't need to bind this.
https://codereview.chromium.org/2702423002/diff/40001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/ui/share_dialog.js (right): https://codereview.chromium.org/2702423002/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:164: // it from frame drops. four spaces before 'it' is not needed.
https://codereview.chromium.org/2702423002/diff/40001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/ui/share_dialog.js (right): https://codereview.chromium.org/2702423002/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:162: // stabilizes. This is a workaround for crbug.com/693416 . On 2017/02/22 03:34:47, fukino wrote: > nit: space between 'crbug.com/693416' and '.' is not needed. Done. https://codereview.chromium.org/2702423002/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:164: // it from frame drops. On 2017/02/22 03:47:23, fukino wrote: > four spaces before 'it' is not needed. Done.
https://codereview.chromium.org/2702423002/diff/40001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/ui/share_dialog.js (right): https://codereview.chromium.org/2702423002/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:185: setTimeout(checkSize.bind(this, -1, -1), On 2017/02/22 03:34:47, fukino wrote: > checkSize.bind(null, -1, -1). > In checkSize function, the ShareDialog object is not referred, so we don't need > to bind this. How about this?
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2702423002/diff/40001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/ui/share_dialog.js (right): https://codereview.chromium.org/2702423002/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:185: setTimeout(checkSize.bind(this, -1, -1), On 2017/02/22 04:13:51, fukino wrote: > On 2017/02/22 03:34:47, fukino wrote: > > checkSize.bind(null, -1, -1). > > In checkSize function, the ShareDialog object is not referred, so we don't > need > > to bind this. > > How about this? Done.
lgtm
The CQ bit was unchecked by yamaguchi@chromium.org
The CQ bit was checked by yamaguchi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry, lgtm WITH A NIT. https://codereview.chromium.org/2702423002/diff/40001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/ui/share_dialog.js (right): https://codereview.chromium.org/2702423002/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:179: setTimeout(checkSize.bind(this, newWidth, newHeight), checkSize.bind(null, newWidth, newHeight)
The CQ bit was unchecked by fukino@chromium.org
https://codereview.chromium.org/2702423002/diff/40001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/ui/share_dialog.js (right): https://codereview.chromium.org/2702423002/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:179: setTimeout(checkSize.bind(this, newWidth, newHeight), On 2017/02/22 04:31:05, fukino wrote: > checkSize.bind(null, newWidth, newHeight) Done.
The CQ bit was checked by yamaguchi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fukino@chromium.org Link to the patchset: https://codereview.chromium.org/2702423002/#ps100001 (title: "Remove unused binding.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Detect webview size change by motion instead of expected size. This is a workaround to avoid that the webview size never gets to the expected size, thus it does not issue resizeComplete message at all. One caveat of this method is that when there is an animation jank longer than the polling interval (200ms), it'll be detected as the false end of the animation. In this case, the resizeComplete message won't be issued after actual end of the animation. BUG=491102,693416 TEST=Open share dialog in Files.app, enter "." as email address, click "Send" button. Verify the error message is visible. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Detect webview size change by motion instead of expected size. This is a workaround to avoid that the webview size never gets to the expected size, thus it does not issue resizeComplete message at all. One caveat of this method is that when there is an animation jank longer than the polling interval (200ms), it'll be detected as the false end of the animation. In this case, the resizeComplete message won't be issued after actual end of the animation. BUG=693416 TEST=Open share dialog in Files.app, enter "." as email address, click "Send" button. Verify the error message is visible. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487738195022350,
"parent_rev": "a313bd2dbe1576252dff87250f0d2b20434a3b9b", "commit_rev":
"479ccd4b4d85396ed50eed8ce07b1b7dd9199109"}
Message was sent while issue was closed.
Description was changed from ========== Detect webview size change by motion instead of expected size. This is a workaround to avoid that the webview size never gets to the expected size, thus it does not issue resizeComplete message at all. One caveat of this method is that when there is an animation jank longer than the polling interval (200ms), it'll be detected as the false end of the animation. In this case, the resizeComplete message won't be issued after actual end of the animation. BUG=693416 TEST=Open share dialog in Files.app, enter "." as email address, click "Send" button. Verify the error message is visible. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Detect webview size change by motion instead of expected size. This is a workaround to avoid that the webview size never gets to the expected size, thus it does not issue resizeComplete message at all. One caveat of this method is that when there is an animation jank longer than the polling interval (200ms), it'll be detected as the false end of the animation. In this case, the resizeComplete message won't be issued after actual end of the animation. BUG=693416 TEST=Open share dialog in Files.app, enter "." as email address, click "Send" button. Verify the error message is visible. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2702423002 Cr-Commit-Position: refs/heads/master@{#451902} Committed: https://chromium.googlesource.com/chromium/src/+/479ccd4b4d85396ed50eed8ce07b... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/479ccd4b4d85396ed50eed8ce07b... |
