Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15029002:
CrOS Files.app: fix the redraw logic on resize. (Closed)
|
Created:
7 years, 7 months ago by yoshiki Modified:
7 years, 7 months ago Reviewers:
mtomasz CC:
chromium-reviews, rginda+watch_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCrOS Files.app: fix the redraw logic on resize.
Changes:
- If the table is hidden, don;'t update the column widthes.
- Updates the table after normalizing the column.
- Redraw the volume list when back from the image viewer.
This patch fixes both the volume-list issue (http://crbug.com/237921) and the file-list issue (http://crbug.com/237921#c2).
BUG=237921
TEST=manual
R=mtomasz@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198872
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add the volume-list redraw (it fixes the volume list issue). #Patch Set 3 : add the check in FileTableColumnModel.normalizeWidth #
Total comments: 4
Patch Set 4 : addressed comment #Patch Set 5 : rebase #
Messages
Total messages: 19 (0 generated)
@mtomasz: Could you take a look? Thanks
On 2013/05/07 06:34:59, yoshiki wrote: > @mtomasz: Could you take a look? Thanks I don't see any difference after applying this patch. Can you please provide reproducing steps which reveals the bug which is fixed by this patch? Thanks.
On 2013/05/07 06:41:21, mtomasz wrote: > On 2013/05/07 06:34:59, yoshiki wrote: > > @mtomasz: Could you take a look? Thanks > > I don't see any difference after applying this patch. Can you please provide > reproducing steps which reveals the bug which is fixed by this patch? Thanks. This is an occasional issue. It occurs ~10% of the time in my environment. - Open Files.app. - Click on an Image to open the image browser. - Stop ./out/Debug/chrome or ./out/Release/chrome - Re-launch ./out/Debug/chrome or ./out/Release/chrome again. - Press Esc key to quit the image browser. - You may see the the broken list like http://crbug.com/237921#c2. If clientWidth = 0, the 'factor' at file_table.js#49 becomes negative value and all the column width is set to negative value. (Negative width is treated as zero width).
On 2013/05/07 06:51:46, yoshiki wrote: > On 2013/05/07 06:41:21, mtomasz wrote: > > On 2013/05/07 06:34:59, yoshiki wrote: > > > @mtomasz: Could you take a look? Thanks > > > > I don't see any difference after applying this patch. Can you please provide > > reproducing steps which reveals the bug which is fixed by this patch? Thanks. > > This is an occasional issue. It occurs ~10% of the time in my environment. > > - Open Files.app. > - Click on an Image to open the image browser. > - Stop ./out/Debug/chrome or ./out/Release/chrome > - Re-launch ./out/Debug/chrome or ./out/Release/chrome again. > - Press Esc key to quit the image browser. > - You may see the the broken list like http://crbug.com/237921#c2. > > If clientWidth = 0, the 'factor' at file_table.js#49 becomes negative value and > all the column width is set to negative value. (Negative width is treated as > zero width). When I follow your repro steps it fails in 100% with this patch and without this patch.
https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/file_manager.js:1415: if (this.table_.clientWidth > 0) Do we need this? What's the purpose of it? If the list is hidden then it is empty, then normalizing doesn't cost anything.
On 2013/05/07 08:37:52, mtomasz wrote: > https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... > File chrome/browser/resources/file_manager/js/file_manager.js (right): > > https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/file_manager.js:1415: if > (this.table_.clientWidth > 0) > Do we need this? What's the purpose of it? If the list is hidden then it is > empty, then normalizing doesn't cost anything. Ah, I think I see the issue.
https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/file_manager.js:1415: if (this.table_.clientWidth > 0) On 2013/05/07 08:37:53, mtomasz wrote: > Do we need this? What's the purpose of it? If the list is hidden then it is > empty, then normalizing doesn't cost anything. OK, now I see. My repro steps for this are: 1. Open Files.app. 2. Open a picture. 3. Resize. 4. Close the gallery. Result: empty. Expected: not empty. This still happens with this patch. It may be a race with closeFilePopup_'s setTimeout(this.onResize_.bind(this), 0), I am not sure.
On 2013/05/07 08:44:58, mtomasz wrote: > https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... > File chrome/browser/resources/file_manager/js/file_manager.js (right): > > https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/file_manager.js:1415: if > (this.table_.clientWidth > 0) > On 2013/05/07 08:37:53, mtomasz wrote: > > Do we need this? What's the purpose of it? If the list is hidden then it is > > empty, then normalizing doesn't cost anything. > > OK, now I see. My repro steps for this are: > 1. Open Files.app. > 2. Open a picture. > 3. Resize. > 4. Close the gallery. > > Result: empty. > Expected: not empty. > > This still happens with this patch. It may be a race with closeFilePopup_'s > setTimeout(this.onResize_.bind(this), 0), I am not sure. I don't think it's race. When the list element is covered by the image viewer, clientWidth/Height returns zero. As the result of it, 1) column width is set to a negative value. 2) the list is not redrew during clientWidth=0. (1) is already fixed in the patch set #1, but I've just realized (2) is not completely fixed. We should add some forcedly-redraw logic when back from the image viewer.
On 2013/05/07 08:57:18, yoshiki wrote: > On 2013/05/07 08:44:58, mtomasz wrote: > > > https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... > > File chrome/browser/resources/file_manager/js/file_manager.js (right): > > > > > https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... > > chrome/browser/resources/file_manager/js/file_manager.js:1415: if > > (this.table_.clientWidth > 0) > > On 2013/05/07 08:37:53, mtomasz wrote: > > > Do we need this? What's the purpose of it? If the list is hidden then it is > > > empty, then normalizing doesn't cost anything. > > > > OK, now I see. My repro steps for this are: > > 1. Open Files.app. > > 2. Open a picture. > > 3. Resize. > > 4. Close the gallery. > > > > Result: empty. > > Expected: not empty. > > > > This still happens with this patch. It may be a race with closeFilePopup_'s > > setTimeout(this.onResize_.bind(this), 0), I am not sure. > > I don't think it's race. > > When the list element is covered by the image viewer, clientWidth/Height returns > zero. As the result of it, > 1) column width is set to a negative value. > 2) the list is not redrew during clientWidth=0. > > (1) is already fixed in the patch set #1, but I've just realized (2) is not > completely fixed. We should add some forcedly-redraw logic when back from the > image viewer. Aaah! Sorry, I've forgot to patch you CL, so I was checking the trunk :). Give me 5 min to re-review. Sorry!
On 2013/05/07 09:08:16, mtomasz wrote: > On 2013/05/07 08:57:18, yoshiki wrote: > > On 2013/05/07 08:44:58, mtomasz wrote: > > > > > > https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... > > > File chrome/browser/resources/file_manager/js/file_manager.js (right): > > > > > > > > > https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... > > > chrome/browser/resources/file_manager/js/file_manager.js:1415: if > > > (this.table_.clientWidth > 0) > > > On 2013/05/07 08:37:53, mtomasz wrote: > > > > Do we need this? What's the purpose of it? If the list is hidden then it > is > > > > empty, then normalizing doesn't cost anything. > > > > > > OK, now I see. My repro steps for this are: > > > 1. Open Files.app. > > > 2. Open a picture. > > > 3. Resize. > > > 4. Close the gallery. > > > > > > Result: empty. > > > Expected: not empty. > > > > > > This still happens with this patch. It may be a race with closeFilePopup_'s > > > setTimeout(this.onResize_.bind(this), 0), I am not sure. > > > > I don't think it's race. > > > > When the list element is covered by the image viewer, clientWidth/Height > returns > > zero. As the result of it, > > 1) column width is set to a negative value. > > 2) the list is not redrew during clientWidth=0. > > > > (1) is already fixed in the patch set #1, but I've just realized (2) is not > > completely fixed. We should add some forcedly-redraw logic when back from the > > image viewer. > > Aaah! Sorry, I've forgot to patch you CL, so I was checking the trunk :). Give > me 5 min to re-review. Sorry! This patch seem to solve the problem. I think we should remove this clientWidth > 0 check if it is not necessary.
https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/file_manager.js:1415: if (this.table_.clientWidth > 0) This check for clientWidth > 0 seem to me unnecessary.
https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... chrome/browser/resources/file_manager/js/file_manager.js:1415: if (this.table_.clientWidth > 0) On 2013/05/07 09:15:34, mtomasz wrote: > This check for clientWidth > 0 seem to me unnecessary. We need this check. When this.table_.clientWidth=0, in FileTableColumnModel.normalizeWidth(), the 'factor' variable is set to negative value (l.49) and all the column widthes are set to negative value at (l.54).
On 2013/05/07 09:51:04, yoshiki wrote: > https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... > File chrome/browser/resources/file_manager/js/file_manager.js (right): > > https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... > chrome/browser/resources/file_manager/js/file_manager.js:1415: if > (this.table_.clientWidth > 0) > On 2013/05/07 09:15:34, mtomasz wrote: > > This check for clientWidth > 0 seem to me unnecessary. > > We need this check. When this.table_.clientWidth=0, in > FileTableColumnModel.normalizeWidth(), the 'factor' variable is set to negative > value (l.49) and all the column widthes are set to negative value at (l.54). Ah I see. So it will also happen when the file list is narrower than 50 px. Can we just fix the FileTableColumnModel.normalizeWidths then? Eg. by adding a Math.max(0, ...) to cover all cases?
On 2013/05/07 09:58:39, mtomasz wrote: > On 2013/05/07 09:51:04, yoshiki wrote: > > > https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... > > File chrome/browser/resources/file_manager/js/file_manager.js (right): > > > > > https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... > > chrome/browser/resources/file_manager/js/file_manager.js:1415: if > > (this.table_.clientWidth > 0) > > On 2013/05/07 09:15:34, mtomasz wrote: > > > This check for clientWidth > 0 seem to me unnecessary. > > > > We need this check. When this.table_.clientWidth=0, in > > FileTableColumnModel.normalizeWidth(), the 'factor' variable is set to > negative > > value (l.49) and all the column widthes are set to negative value at (l.54). > > Ah I see. So it will also happen when the file list is narrower than 50 px. Can > we just fix the FileTableColumnModel.normalizeWidths then? Eg. by adding a > Math.max(0, ...) to cover all cases? Added. PTAL.
On 2013/05/07 10:52:16, yoshiki wrote: > On 2013/05/07 09:58:39, mtomasz wrote: > > On 2013/05/07 09:51:04, yoshiki wrote: > > > > > > https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... > > > File chrome/browser/resources/file_manager/js/file_manager.js (right): > > > > > > > > > https://codereview.chromium.org/15029002/diff/1/chrome/browser/resources/file... > > > chrome/browser/resources/file_manager/js/file_manager.js:1415: if > > > (this.table_.clientWidth > 0) > > > On 2013/05/07 09:15:34, mtomasz wrote: > > > > This check for clientWidth > 0 seem to me unnecessary. > > > > > > We need this check. When this.table_.clientWidth=0, in > > > FileTableColumnModel.normalizeWidth(), the 'factor' variable is set to > > negative > > > value (l.49) and all the column widthes are set to negative value at (l.54). > > > > Ah I see. So it will also happen when the file list is narrower than 50 px. > Can > > we just fix the FileTableColumnModel.normalizeWidths then? Eg. by adding a > > Math.max(0, ...) to cover all cases? > > Added. PTAL. lgtm, thanks!
lgtm with nits, thanks! https://codereview.chromium.org/15029002/diff/11003/chrome/browser/resources/... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://codereview.chromium.org/15029002/diff/11003/chrome/browser/resources/... chrome/browser/resources/file_manager/js/file_manager.js:1419: this.volumeList_.redraw(); Is this related to this patch? https://codereview.chromium.org/15029002/diff/11003/chrome/browser/resources/... File chrome/browser/resources/file_manager/js/file_table.js (right): https://codereview.chromium.org/15029002/diff/11003/chrome/browser/resources/... chrome/browser/resources/file_manager/js/file_table.js:31: * with some additional constraints. nit 1: sum 100% -> sum 100% if possible.
Thanks! https://codereview.chromium.org/15029002/diff/11003/chrome/browser/resources/... File chrome/browser/resources/file_manager/js/file_manager.js (right): https://codereview.chromium.org/15029002/diff/11003/chrome/browser/resources/... chrome/browser/resources/file_manager/js/file_manager.js:1419: this.volumeList_.redraw(); On 2013/05/08 01:13:26, mtomasz wrote: > Is this related to this patch? This line fixes the left-bar issue which is the original issue of http://crbug.com/237921. https://codereview.chromium.org/15029002/diff/11003/chrome/browser/resources/... File chrome/browser/resources/file_manager/js/file_table.js (right): https://codereview.chromium.org/15029002/diff/11003/chrome/browser/resources/... chrome/browser/resources/file_manager/js/file_table.js:31: * with some additional constraints. On 2013/05/08 01:13:26, mtomasz wrote: > nit 1: sum 100% -> sum 100% if possible. Done.
On 2013/05/08 03:55:25, yoshiki wrote: > Thanks! > > https://codereview.chromium.org/15029002/diff/11003/chrome/browser/resources/... > File chrome/browser/resources/file_manager/js/file_manager.js (right): > > https://codereview.chromium.org/15029002/diff/11003/chrome/browser/resources/... > chrome/browser/resources/file_manager/js/file_manager.js:1419: > this.volumeList_.redraw(); > On 2013/05/08 01:13:26, mtomasz wrote: > > Is this related to this patch? > > This line fixes the left-bar issue which is the original issue of > http://crbug.com/237921. > > https://codereview.chromium.org/15029002/diff/11003/chrome/browser/resources/... > File chrome/browser/resources/file_manager/js/file_table.js (right): > > https://codereview.chromium.org/15029002/diff/11003/chrome/browser/resources/... > chrome/browser/resources/file_manager/js/file_table.js:31: * with some > additional constraints. > On 2013/05/08 01:13:26, mtomasz wrote: > > nit 1: sum 100% -> sum 100% if possible. > > Done. lgtm
Message was sent while issue was closed.
Committed patchset #5 manually as r198872 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
