Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1903223005:
Fix for 593676 - Minimizes/eliminates momentary flash of Windows frame (Closed)
DescriptionThis will reduce or eliminate the momentary display of the window frame when showing/hiding the window. It does this by adding/removing the frame extending into the client upon showing or hiding using DwmExtendFrameIntoClientArea, respectively.
BUG=593676
Committed: https://crrev.com/03e3572cecc4f206b260dbeddf40172d46217617
Cr-Commit-Position: refs/heads/master@{#403046}
Patch Set 1 #Patch Set 2 : Revert previous change. Modified code actually causing the issue #
Total comments: 1
Patch Set 3 : Better solution to minimize/eliminate 'ghost' frame when showing/hiding borderless window #
Total comments: 1
Patch Set 4 : nit-removal #
Total comments: 1
Patch Set 5 : nit; formatting #Messages
Total messages: 31 (12 generated)
Description was changed from ========== This CL does fix the issue related to the referenced bug. However it may leave an artifact that is unfavorable; There is no highlight shadow around the outside of the window which may cause the window to blend into the background. Ideally, this window should not be painting the frame and allow for the OS frame. This will make the window feel more "natural" on the platform without losing its whole intent. BUG=593676 ========== to ========== This CL does fix the issue related to the referenced bug. However it may leave an artifact that is unfavorable; There is no highlight shadow around the outside of the window which may cause the window to blend into the background. Ideally, this window should not be painting the frame and allow for the OS frame. This will make the window feel more "natural" on the platform without losing its whole intent. Once I figure out how the Feedback dialog works, I will submit a separate CL which will use the OS frame. BUG=593676 ==========
kylixrd@chromium.org changed reviewers: + benwells@chromium.org
If this change is removing shadows for all chrome app frameless windows, I don't think it is acceptable. Shadowless windows feel quite broken. Note: the first line of the description is the subject and should be a self contained summary of the change.
Description was changed from ========== This CL does fix the issue related to the referenced bug. However it may leave an artifact that is unfavorable; There is no highlight shadow around the outside of the window which may cause the window to blend into the background. Ideally, this window should not be painting the frame and allow for the OS frame. This will make the window feel more "natural" on the platform without losing its whole intent. Once I figure out how the Feedback dialog works, I will submit a separate CL which will use the OS frame. BUG=593676 ========== to ========== This CL does fix the issue related to the referenced bug. However it may leave an artifact that is unfavorable; There is no highlight shadow around the outside of the window which may cause the window to blend into the background. Ideally, this window should not be painting the frame and allow for the OS frame. This will make the window feel more "natural" on the platform without losing its whole intent. Once I figure out how the Feedback dialog works, I will submit a separate CL which will use the OS frame. BUG=593676 ==========
Description was changed from ========== This CL does fix the issue related to the referenced bug. However it may leave an artifact that is unfavorable; There is no highlight shadow around the outside of the window which may cause the window to blend into the background. Ideally, this window should not be painting the frame and allow for the OS frame. This will make the window feel more "natural" on the platform without losing its whole intent. Once I figure out how the Feedback dialog works, I will submit a separate CL which will use the OS frame. BUG=593676 ========== to ========== This CL does fix the issue related to the referenced bug. However it may leave an artifact that is unfavorable; There is no highlight shadow around the outside of the window which may cause the window to blend into the background. Ideally, this window should not be painting the frame and allow for the OS frame. This will make the window feel more "natural" on the platform without losing its whole intent. Once I figure out how the Feedback dialog works, I will submit a separate CL which will use the OS frame. BUG=593676 ==========
On 2016/04/22 01:50:43, benwells wrote: > If this change is removing shadows for all chrome app frameless windows, I don't > think it is acceptable. Shadowless windows feel quite broken. That was my concern as well, which is why I brought it up. The next question is this; is it so awful to use the platform's frame for the Feedback dialog? Right now the whole window, including the caption bar, is being rendered with HTML & CSS. While it looks the same on every platform, it also looks out of place on every platform. The only downside to that approach right now is that there is no "Apps" api to set the caption of the Window and it seems there are no plans to support it according to this issue: https://bugs.chromium.org/p/chromium/issues/detail?id=225773 A private API could be added (feedback_private_api.cc) to support this. Short of the caption title issue, I've got a working prototype using the native platform frame and it behaves correctly. It is correctly animated for showing and hiding (fade in/out) and has the highlight shadow. > Note: the first line of the description is the subject and should be a self > contained summary of the change. Fixed.
https://codereview.chromium.org/1903223005/diff/20001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/1903223005/diff/20001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2329: MARGINS m = {1, 1, 1, 1}; I've uploaded this change for discussion purposes. Using blame, I traced this code back to ben@chromium.org. This is the code that is responsible for producing the "ghosted" frame effect. I have no idea how those magic values were chosen, and seem arbitrary. I can reduce it by changing to the above margins, however it is still visible. Changing the above to 0, will eliminate the ghosting, but then be back to no active window highlight/shadow. Changing to -1 for all dimensions enables "sheet-of-glass" mode, which then causes the ghost effect to encompass the whole window. This is even more visibly apparent. I suspect the actual issue is being caused by the delay between creating and showing the top-level window and the construction and rendering of the child window for the content. When closing the window, the child window appears to be hidden prior to destroying the top-level window. If the child window can be made to follow the visibility of the parent window, I suspect that will solve the issue.
kylixrd@chromium.org changed reviewers: + sky@chromium.org
sky@chromium.org changed reviewers: + ben@chromium.org - sky@chromium.org
sky->ben
After sitting idle for a couple of months, I'd thought of another possible solution to this problem. This patch doesn't fully eliminate the "momentary flashing border", but it significantly reduces it. It now only shows the thin 'drop-shadow' around the border as the window is being shown. It never shows when hiding since the code removes the frame client extension prior to being hidden. The problem is caused by the asynchronous manner in which the content of the window is painted. Windows relies on content being rendered into the HDC passed along with the WM_PRINT or WM_PRINTCLIENT messages in order to animate in/out (fade in/out) the window onto or from the screen. A solution would be to pass the HDC down to the compositor from which a GL/DX context is created. The window content is then rendered to this context synchronously. I realize this is likely to be very difficult without adding a whole lot more plumbing. This patch is a compromise to that more extreme solution.
On 2016/06/28 19:27:19, kylix_rd wrote: > After sitting idle for a couple of months, I'd thought of another possible > solution to this problem. This patch doesn't fully eliminate the "momentary > flashing border", but it significantly reduces it. It now only shows the thin > 'drop-shadow' around the border as the window is being shown. It never shows > when hiding since the code removes the frame client extension prior to being > hidden. > > The problem is caused by the asynchronous manner in which the content of the > window is painted. Windows relies on content being rendered into the HDC passed > along with the WM_PRINT or WM_PRINTCLIENT messages in order to animate in/out > (fade in/out) the window onto or from the screen. > > A solution would be to pass the HDC down to the compositor from which a GL/DX > context is created. The window content is then rendered to this context > synchronously. I realize this is likely to be very difficult without adding a > whole lot more plumbing. > > This patch is a compromise to that more extreme solution. Seems legit, but you might want to find a different reviewer. I personally don't have the expertise to review this.
On 2016/06/28 23:32:05, benwells wrote: > Seems legit, but you might want to find a different reviewer. I personally don't > have the expertise to review this. Any suggestions? sky@ has already deferred. sadrul@ is in the OWNERS file in src/ui/views, but I think this person is mainly on ChromeOS and Linux. thakis@? ananta@?
On 2016/06/29 22:05:11, kylix_rd wrote: > On 2016/06/28 23:32:05, benwells wrote: > > Seems legit, but you might want to find a different reviewer. I personally > don't > > have the expertise to review this. > > Any suggestions? sky@ has already deferred. sadrul@ is in the OWNERS file in > src/ui/views, but I think this person is mainly on ChromeOS and Linux. thakis@? > ananta@? Hmmm, maybe we need a few new owners then? People that come to mind are ananta and scottmg.
On 2016/06/29 22:18:55, benwells wrote: > On 2016/06/29 22:05:11, kylix_rd wrote: > > On 2016/06/28 23:32:05, benwells wrote: > > > Seems legit, but you might want to find a different reviewer. I personally > > don't > > > have the expertise to review this. > > > > Any suggestions? sky@ has already deferred. sadrul@ is in the OWNERS file in > > src/ui/views, but I think this person is mainly on ChromeOS and Linux. > thakis@? > > ananta@? > > Hmmm, maybe we need a few new owners then? People that come to mind are ananta > and scottmg. I'll ask them. ananta@ & scottmg@ are right behind me :)... Well "virtual" scottmg@ is. There's the GVC here in the office.
scottmg@chromium.org changed reviewers: + ananta@chromium.org, scottmg@chromium.org
lgtm, but ananta is hwnd_message_handler owner even though he's not listed there. Please update the CL to say what it does (remove extension before hiding, re-add after showing, etc.) https://codereview.chromium.org/1903223005/diff/40001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/1903223005/diff/40001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:319: // DwmExtendFrameIntoClientArea nit; . at end of sentence.
On 2016/06/29 22:35:38, scottmg wrote: > lgtm, but ananta is hwnd_message_handler owner even though he's not listed > there. > > Please update the CL to say what it does (remove extension before hiding, re-add > after showing, etc.) * "... update the CL description ..." > > https://codereview.chromium.org/1903223005/diff/40001/ui/views/win/hwnd_messa... > File ui/views/win/hwnd_message_handler.h (right): > > https://codereview.chromium.org/1903223005/diff/40001/ui/views/win/hwnd_messa... > ui/views/win/hwnd_message_handler.h:319: // DwmExtendFrameIntoClientArea > nit; . at end of sentence.
On 2016/06/29 22:35:38, scottmg wrote: > lgtm, but ananta is hwnd_message_handler owner even though he's not listed > there. Should an OWNERS file be added to the src/ui/view/win directory? Right now, folks in src/ui/views/OWNERS are getting all the Windows specific stuff.
Description was changed from ========== This CL does fix the issue related to the referenced bug. However it may leave an artifact that is unfavorable; There is no highlight shadow around the outside of the window which may cause the window to blend into the background. Ideally, this window should not be painting the frame and allow for the OS frame. This will make the window feel more "natural" on the platform without losing its whole intent. Once I figure out how the Feedback dialog works, I will submit a separate CL which will use the OS frame. BUG=593676 ========== to ========== This will reduce or eliminate the momentary display of the window frame when showing/hiding the window. It does this by adding/removing the frame extending into the client upon showing or hiding using DwmExtendFrameIntoClientArea, respectively. BUG=593676 ==========
I added ben@ to the review as I believe he authored the lines you're changing. ping ben@ On Wed, Jun 29, 2016 at 3:38 PM, <kylixrd@chromium.org> wrote: > On 2016/06/29 22:35:38, scottmg wrote: >> lgtm, but ananta is hwnd_message_handler owner even though he's not listed >> there. > > Should an OWNERS file be added to the src/ui/view/win directory? Right now, > folks in src/ui/views/OWNERS are getting all the Windows specific stuff. > > > https://codereview.chromium.org/1903223005/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm https://codereview.chromium.org/1903223005/diff/60001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/1903223005/diff/60001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:214: enum class DwmFrameState {OFF, ON}; nit: space between { & O and N & }
The CQ bit was checked by kylixrd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org, ben@chromium.org Link to the patchset: https://codereview.chromium.org/1903223005/#ps80001 (title: "nit; formatting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== This will reduce or eliminate the momentary display of the window frame when showing/hiding the window. It does this by adding/removing the frame extending into the client upon showing or hiding using DwmExtendFrameIntoClientArea, respectively. BUG=593676 ========== to ========== This will reduce or eliminate the momentary display of the window frame when showing/hiding the window. It does this by adding/removing the frame extending into the client upon showing or hiding using DwmExtendFrameIntoClientArea, respectively. BUG=593676 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== This will reduce or eliminate the momentary display of the window frame when showing/hiding the window. It does this by adding/removing the frame extending into the client upon showing or hiding using DwmExtendFrameIntoClientArea, respectively. BUG=593676 ========== to ========== This will reduce or eliminate the momentary display of the window frame when showing/hiding the window. It does this by adding/removing the frame extending into the client upon showing or hiding using DwmExtendFrameIntoClientArea, respectively. BUG=593676 Committed: https://crrev.com/03e3572cecc4f206b260dbeddf40172d46217617 Cr-Commit-Position: refs/heads/master@{#403046} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/03e3572cecc4f206b260dbeddf40172d46217617 Cr-Commit-Position: refs/heads/master@{#403046} |
