Closed
Bug 1422966
Opened 6 years ago
Closed 6 years ago
[Wayland] - Implement WindowSurfaceWayland
Categories
(Core :: Widget: Gtk, enhancement, P2)
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: stransky, Assigned: stransky)
References
(Blocks 1 open bug)
Details
Attachments
(5 files)
59 bytes,
text/x-review-board-request
|
jhorak
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jhorak
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jhorak
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jhorak
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jhorak
:
review+
|
Details |
We need to implement WindowSurface for Wayland.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8934459 [details] Bug 1422966 - Implement SurfaceWayland for Wayland, https://reviewboard.mozilla.org/r/205394/#review211054 ::: widget/gtk/WindowSurfaceWayland.h:121 (Diff revision 1) > + bool mDelayedCommit; > + bool mFullScreenDamage; > + MessageLoop* mWaylandMessageLoop; > + bool mIsMainThread; minor nit: you're introducing alignment padding here - I suggest you move all bool members to the end to avoid that
Assignee | ||
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8934459 [details] Bug 1422966 - Implement SurfaceWayland for Wayland, https://reviewboard.mozilla.org/r/205394/#review211058 ::: widget/gtk/WindowSurfaceWayland.h:121 (Diff revision 1) > + bool mDelayedCommit; > + bool mFullScreenDamage; > + MessageLoop* mWaylandMessageLoop; > + bool mIsMainThread; Thanks! Feel free to help review those patches.
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8934455 [details] Bug 1422966 - Comment Wayland rendering scheme, https://reviewboard.mozilla.org/r/205196/#review213392
Attachment #8934455 -
Flags: review?(jhorak) → review+
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8934456 [details] Bug 1422966 - Update mDisplay attribute initialization at nsWaylandDisplay::nsWaylandDisplay(), https://reviewboard.mozilla.org/r/205198/#review213396
Attachment #8934456 -
Flags: review?(jhorak) → review+
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8934457 [details] Bug 1422966 - implement WaylandShmPool to manage shared memory for wl_buffer, https://reviewboard.mozilla.org/r/205200/#review213398 Please address commends, otherwise looks good. ::: widget/gtk/WindowSurfaceWayland.cpp:374 (Diff revision 1) > + return fd; > +} > + > +WaylandShmPool::WaylandShmPool(nsWaylandDisplay* aWaylandDisplay, int aSize) > +{ > + mAllocatedSize = aSize; please initialize by : mAllocatedSize(aSize) ::: widget/gtk/WindowSurfaceWayland.cpp:384 (Diff revision 1) > + MOZ_RELEASE_ASSERT(mImageData != MAP_FAILED, > + "Unable to map drawing surface!"); > + > + mShmPool = wl_shm_create_pool(aWaylandDisplay->GetShm(), > + mShmPoolFd, mAllocatedSize); > + wl_proxy_set_queue((struct wl_proxy *)mShmPool, aWaylandDisplay->GetEventQueue()); This call could be briefly commented.
Attachment #8934457 -
Flags: review?(jhorak) → review+
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8934458 [details] Bug 1422966 - implemented WindowBackBuffer to encapsulate wl_buffer, https://reviewboard.mozilla.org/r/205202/#review213404 ::: commit-message-7bfef:7 (Diff revision 1) > + > +wl_buffer is a main Wayland object with graphics data. wl_buffer basically represent one complete window screen. > +When double buffering is involved every window (GdkWindow in our case) utilises two wl_buffers which are cycled. > +One is filed with data by application and one is rendered by compositor. > + > +WindowBackBuffer class manages one wl_buffer. It owns wl_buffer objects, owns WaylandShmPool (which provides shared memory) "It owns wl_buffer object" (only one) ::: widget/gtk/WindowSurfaceWayland.cpp:506 (Diff revision 1) > +WindowBackBuffer::Detach() > +{ > + mAttached = false; > +} > + > +bool WindowBackBuffer::Sync(class WindowBackBuffer* aSourceBuffer) Maybe more appropriate name should be there: like SetImageDataFromBackBuffer. ::: widget/gtk/WindowSurfaceWayland.cpp:509 (Diff revision 1) > +} > + > +bool WindowBackBuffer::Sync(class WindowBackBuffer* aSourceBuffer) > +{ > + bool bufferSizeMatches = MatchSize(aSourceBuffer); > + if (!bufferSizeMatches) { if (!MatchSize(aSourceBuffer)) should be enough. ::: widget/gtk/WindowSurfaceWayland.cpp:513 (Diff revision 1) > + bool bufferSizeMatches = MatchSize(aSourceBuffer); > + if (!bufferSizeMatches) { > + Resize(aSourceBuffer->mWidth, aSourceBuffer->mHeight); > + } > + > + memcpy(mShmPool.GetImageData(), aSourceBuffer->mShmPool.GetImageData(), Since the mShmPool object is modified, could you add WaylandShmPool::(Copy/Set)ImageData(WaylandShmPool* shmPool) which encapsulate the memcpy call?
Attachment #8934458 -
Flags: review?(jhorak) → review-
Assignee | ||
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8934459 [details] Bug 1422966 - Implement SurfaceWayland for Wayland, https://reviewboard.mozilla.org/r/205394/#review213418 ::: widget/gtk/WindowSurfaceWayland.h:123 (Diff revision 1) > + WindowBackBuffer* mBackBuffer; > + wl_callback* mFrameCallback; > + wl_surface* mFrameCallbackSurface; > + bool mDelayedCommit; > + bool mFullScreenDamage; > + MessageLoop* mWaylandMessageLoop; We should call MessageLoop::current() directly.
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8934459 [details] Bug 1422966 - Implement SurfaceWayland for Wayland, https://reviewboard.mozilla.org/r/205394/#review213416 ::: commit-message-5c6df:3 (Diff revision 1) > +Bug 1422966 - Implement SurfaceWayland for Wayland, r?jhorak > + > +WindowSurfaceWayland is Wayland implementation of WindowSurface class. One WindowSurfaceWayland object manages drawing of one nsWindow so those are tied 1:1. It implements base Lock() and Commit() interfaces from WindowSurface. At Wayland side it represents one wl_surface object. nit please wrap at 80 characters ::: widget/gtk/WindowSurfaceWayland.h:115 (Diff revision 1) > + void FrameCallbackHandler(); > + > +private: > + WindowBackBuffer* GetBufferToDraw(int aWidth, int aHeight); > + > + nsWindow* mWidget; Maybe stick to mWindow when not using nsIWidget, mWidget can mean a lot of other stuff. ::: widget/gtk/WindowSurfaceWayland.h:120 (Diff revision 1) > + nsWindow* mWidget; > + nsWaylandDisplay* mWaylandDisplay; > + WindowBackBuffer* mFrontBuffer; > + WindowBackBuffer* mBackBuffer; > + wl_callback* mFrameCallback; > + wl_surface* mFrameCallbackSurface; Hm, does this has something to do with the callback as var name suggests? ::: widget/gtk/WindowSurfaceWayland.cpp:664 (Diff revision 1) > + wl_surface_damage(waylandSurface, r.x, r.y, r.width, r.height); > + } > + } > + > + // Frame callback is always connected to actual wl_surface. When the surface > + // is unmapped/deleted the frame callback is newer called. Unfortunatelly typo: newer - never
Attachment #8934459 -
Flags: review?(jhorak) → review-
Assignee | ||
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8934459 [details] Bug 1422966 - Implement SurfaceWayland for Wayland, https://reviewboard.mozilla.org/r/205394/#review213602 ::: widget/gtk/WindowSurfaceWayland.h:121 (Diff revision 1) > + bool mDelayedCommit; > + bool mFullScreenDamage; > + MessageLoop* mWaylandMessageLoop; > + bool mIsMainThread; fixed
Assignee | ||
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8934459 [details] Bug 1422966 - Implement SurfaceWayland for Wayland, https://reviewboard.mozilla.org/r/205394/#review213604 ::: widget/gtk/WindowSurfaceWayland.h:120 (Diff revision 1) > + nsWindow* mWidget; > + nsWaylandDisplay* mWaylandDisplay; > + WindowBackBuffer* mFrontBuffer; > + WindowBackBuffer* mBackBuffer; > + wl_callback* mFrameCallback; > + wl_surface* mFrameCallbackSurface; Yes, this surface is used at mFrameCallback call only.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8934459 [details] Bug 1422966 - Implement SurfaceWayland for Wayland, https://reviewboard.mozilla.org/r/205394/#review213606 ::: widget/gtk/WindowSurfaceWayland.h:115 (Diff revision 1) > + void FrameCallbackHandler(); > + > +private: > + WindowBackBuffer* GetBufferToDraw(int aWidth, int aHeight); > + > + nsWindow* mWidget; Fixed. I also add TODO for further investigation if we need to hold a reference to nsWindow object.
Comment hidden (mozreview-request) |
Comment 22•6 years ago
|
||
mozreview-review |
Comment on attachment 8934458 [details] Bug 1422966 - implemented WindowBackBuffer to encapsulate wl_buffer, https://reviewboard.mozilla.org/r/205202/#review213614 r+ with nits fixed. ::: widget/gtk/WindowSurfaceWayland.h:77 (Diff revision 2) > + bool IsAttached() { return mAttached; } > + > + bool Resize(int aWidth, int aHeight); > + bool SetImageDataFromBackBuffer(class WindowBackBuffer* aSourceBuffer); > + > + bool MatchSize(int aWidth, int aHeight) Sorry I've overlooked it. This method name is confusing for me, as long as it is testing if sizes are equal, better us IsMatchingSize, IsSameSize or something like this. ::: widget/gtk/WindowSurfaceWayland.h:81 (Diff revision 2) > + > + bool MatchSize(int aWidth, int aHeight) > + { > + return aWidth == mWidth && aHeight == mHeight; > + } > + bool MatchSize(class WindowBackBuffer *aBuffer) see above
Attachment #8934458 -
Flags: review?(jhorak) → review+
Comment 23•6 years ago
|
||
mozreview-review |
Comment on attachment 8934459 [details] Bug 1422966 - Implement SurfaceWayland for Wayland, https://reviewboard.mozilla.org/r/205394/#review213616
Attachment #8934459 -
Flags: review?(jhorak) → review+
Comment hidden (mozreview-request) |
Comment 25•6 years ago
|
||
Pushed by stransky@redhat.com: https://hg.mozilla.org/integration/autoland/rev/3cf5876e6d1c Comment Wayland rendering scheme, r=jhorak https://hg.mozilla.org/integration/autoland/rev/8870736e152c Update mDisplay attribute initialization at nsWaylandDisplay::nsWaylandDisplay(), r=jhorak https://hg.mozilla.org/integration/autoland/rev/9e7204579027 implement WaylandShmPool to manage shared memory for wl_buffer, r=jhorak https://hg.mozilla.org/integration/autoland/rev/4a82861a1399 implemented WindowBackBuffer to encapsulate wl_buffer, r=jhorak https://hg.mozilla.org/integration/autoland/rev/68803e96b4be Implement SurfaceWayland for Wayland, r=jhorak
Comment 26•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3cf5876e6d1c https://hg.mozilla.org/mozilla-central/rev/8870736e152c https://hg.mozilla.org/mozilla-central/rev/9e7204579027 https://hg.mozilla.org/mozilla-central/rev/4a82861a1399 https://hg.mozilla.org/mozilla-central/rev/68803e96b4be
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•